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

[Stanimir Varbanov]

Hi Hans,

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;
+}
+
 /**
  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
  */
 static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
-	enum dma_data_direction dma_dir =
-		q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	enum dma_data_direction dma_dir = __get_dma_dir(q);
 	void *mem_priv;
 	int plane;
 	int ret = -ENOMEM;
@@ -978,8 +985,7 @@ static int __prepare_userptr(struct vb2_buffer *vb,
const void *pb)
 	void *mem_priv;
 	unsigned int plane;
 	int ret = 0;
-	enum dma_data_direction dma_dir =
-		q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	enum dma_data_direction dma_dir = __get_dma_dir(q);
 	bool reacquired = vb->planes[0].mem_priv == NULL;

 	memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1096,8 +1102,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb,
const void *pb)
 	void *mem_priv;
 	unsigned int plane;
 	int ret = 0;
-	enum dma_data_direction dma_dir =
-		q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	enum dma_data_direction dma_dir = __get_dma_dir(q);
 	bool reacquired = vb->planes[0].mem_priv == NULL;

 	memset(planes, 0, sizeof(planes[0]) * vb->num_planes);


-- 
regards,
Stan