Re: [RFC PATCH] media: vb2: add bidirectional flag in vb2_queue

[Stanimir Varbanov]

Hi Laurent,

On 08/16/2017 03:28 PM, Laurent Pinchart wrote:
> Hi Stan,
> 
> On Wednesday 16 Aug 2017 14:46:50 Stanimir Varbanov wrote:
>> On 08/15/2017 01:04 PM, Hans Verkuil wrote:
>>> On 08/14/17 10:41, Stanimir Varbanov wrote:
>>>> Hi,
>>>>
>>>> This RFC patch is intended to give to the drivers a choice to change
>>>> the default behavior of the v4l2-core DMA mapping direction from
>>>> DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or OUTPUT)
>>>> to DMA_BIDIRECTIONAL during queue_init time.
>>>>
>>>> Initially the issue with DMA mapping direction has been found in
>>>> Venus encoder driver where the firmware side of the driver adds few
>>>> lines padding on bottom of the image buffer, and the consequence was
>>>> triggering of IOMMU protection faults.
>>>>
>>>> Probably other drivers could also has a benefit of this feature (hint)
>>>> in the future.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
>>>> ---
>>>>
>>>>  drivers/media/v4l2-core/videobuf2-core.c |  3 +++
>>>>  include/media/videobuf2-core.h           | 11 +++++++++++
>>>>  2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>>> b/drivers/media/v4l2-core/videobuf2-core.c index
>>>> 14f83cecfa92..17d07fda4cdc 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>> @@ -200,6 +200,9 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>>>>
>>>>  	int plane;
>>>>  	int ret = -ENOMEM;
>>>>
>>>> +	if (q->bidirectional)
>>>> +		dma_dir = DMA_BIDIRECTIONAL;
>>>> +
>>>
>>> Does this only have to be used in mem_alloc? In the __prepare_*() it is
>>> still using DMA_TO/FROM_DEVICE.
>>
>> Yes, it looks like the DMA direction should be covered in the
>> __prepare_* too. Thus the patch should look like below:
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index 14f83cecfa92..0089e7dac7dd 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -188,14 +188,21 @@ module_param(debug, int, 0644);
>>  static void __vb2_queue_cancel(struct vb2_queue *q);
>>  static void __enqueue_in_driver(struct vb2_buffer *vb);
>>
>> +static enum dma_data_direction __get_dma_dir(struct vb2_queue *q)
>> +{
>> +	if (q->bidirectional)
>> +		return DMA_BIDIRECTIONAL;
>> +
>> +	return q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> 
> We could also compute the DMA direction once only and store it in the queue. I 
> have no big preference at the moment.

Yes, I like the idea. I'll cook a regular patch where the DMA direction
will be computed in vb2_core_queue_init().

-- 
regards,
Stan