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

[Jacob Chen]

Hi,

2017-08-17 20:11 GMT+08:00 Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>:
> 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.
>>>>>

Just butt in.....
some codec hardware also need it, which will use
capture buffers as reference to decode other buffers.

>>>>> 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