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

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

[Stanimir Varbanov]

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@linaro.org>
---
 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;
+
 	/*
 	 * Allocate memory for all planes in this buffer
 	 * NOTE: mmapped areas should be page aligned
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index cb97c224be73..0b6e88e1aa79 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -427,6 +427,16 @@ struct vb2_buf_ops {
  * @dev:	device to use for the default allocation context if the driver
  *		doesn't fill in the @alloc_devs array.
  * @dma_attrs:	DMA attributes to use for the DMA.
+ * @bidirectional: when this flag is set the DMA direction for the buffers of
+ *		this queue will be overridden with DMA_BIDIRECTIONAL direction.
+ *		This is useful in cases where the hardware (firmware) writes to
+ *		a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
+ *		buffer which is mapped for write (DMA_FROM_DEVICE) in order
+ *		to satisfy some internal hardware restrictions or adds a padding
+ *		needed by the processing algorithm. In case the DMA mapping is
+ *		not bidirectional but the hardware (firmware) trying to access
+ *		the buffer (in the opposite direction) this could lead to an
+ *		IOMMU protection faults.
  * @fileio_read_once:		report EOF after reading the first buffer
  * @fileio_write_immediately:	queue buffer after each write() call
  * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the driver
@@ -495,6 +505,7 @@ struct vb2_queue {
 	unsigned int			io_modes;
 	struct device			*dev;
 	unsigned long			dma_attrs;
+	unsigned			bidirectional:1;
 	unsigned			fileio_read_once:1;
 	unsigned			fileio_write_immediately:1;
 	unsigned			allow_zero_bytesused:1;
-- 
2.11.0

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

[Sakari Ailus]

On Mon, Aug 14, 2017 at 11:41:55AM +0300, 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@linaro.org>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> ---
>  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;
> +
>  	/*
>  	 * Allocate memory for all planes in this buffer
>  	 * NOTE: mmapped areas should be page aligned
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..0b6e88e1aa79 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -427,6 +427,16 @@ struct vb2_buf_ops {
>   * @dev:	device to use for the default allocation context if the driver
>   *		doesn't fill in the @alloc_devs array.
>   * @dma_attrs:	DMA attributes to use for the DMA.
> + * @bidirectional: when this flag is set the DMA direction for the buffers of
> + *		this queue will be overridden with DMA_BIDIRECTIONAL direction.
> + *		This is useful in cases where the hardware (firmware) writes to
> + *		a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
> + *		buffer which is mapped for write (DMA_FROM_DEVICE) in order
> + *		to satisfy some internal hardware restrictions or adds a padding
> + *		needed by the processing algorithm. In case the DMA mapping is
> + *		not bidirectional but the hardware (firmware) trying to access
> + *		the buffer (in the opposite direction) this could lead to an
> + *		IOMMU protection faults.
>   * @fileio_read_once:		report EOF after reading the first buffer
>   * @fileio_write_immediately:	queue buffer after each write() call
>   * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the driver
> @@ -495,6 +505,7 @@ struct vb2_queue {
>  	unsigned int			io_modes;
>  	struct device			*dev;
>  	unsigned long			dma_attrs;
> +	unsigned			bidirectional:1;
>  	unsigned			fileio_read_once:1;
>  	unsigned			fileio_write_immediately:1;
>  	unsigned			allow_zero_bytesused:1;
> -- 
> 2.11.0
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Hans Verkuil]

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@linaro.org>
> ---
>  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.

I don't know enough of the low-level handling to be able to tell whether that is
a problem or not, but even if it is not then a comment somewhere to explain that
it is OK is probably a good idea.

Regards,

	Hans

>  	/*
>  	 * Allocate memory for all planes in this buffer
>  	 * NOTE: mmapped areas should be page aligned
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..0b6e88e1aa79 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -427,6 +427,16 @@ struct vb2_buf_ops {
>   * @dev:	device to use for the default allocation context if the driver
>   *		doesn't fill in the @alloc_devs array.
>   * @dma_attrs:	DMA attributes to use for the DMA.
> + * @bidirectional: when this flag is set the DMA direction for the buffers of
> + *		this queue will be overridden with DMA_BIDIRECTIONAL direction.
> + *		This is useful in cases where the hardware (firmware) writes to
> + *		a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
> + *		buffer which is mapped for write (DMA_FROM_DEVICE) in order
> + *		to satisfy some internal hardware restrictions or adds a padding
> + *		needed by the processing algorithm. In case the DMA mapping is
> + *		not bidirectional but the hardware (firmware) trying to access
> + *		the buffer (in the opposite direction) this could lead to an
> + *		IOMMU protection faults.
>   * @fileio_read_once:		report EOF after reading the first buffer
>   * @fileio_write_immediately:	queue buffer after each write() call
>   * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the driver
> @@ -495,6 +505,7 @@ struct vb2_queue {
>  	unsigned int			io_modes;
>  	struct device			*dev;
>  	unsigned long			dma_attrs;
> +	unsigned			bidirectional:1;
>  	unsigned			fileio_read_once:1;
>  	unsigned			fileio_write_immediately:1;
>  	unsigned			allow_zero_bytesused:1;
> 

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@linaro.org>
>> ---
>>  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

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

[Laurent Pinchart]

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@linaro.org>
> >> ---
> >> 
> >>  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.

> +}
> +
>  /**
>   * __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,

Laurent Pinchart

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@linaro.org>
>>>> ---
>>>>
>>>>  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

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@linaro.org>:
> 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@linaro.org>
>>>>> ---
>>>>>
>>>>>  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