Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444432 > unrolled thread

[PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Back to article view | Back to linux.kernel

Contents

  [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf Javier Martinez Canillas <javier@osg.samsung.com> - 2016-07-15 18:30 +0200
    Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Shuah Khan <shuahkh@osg.samsung.com> - 2016-07-15 21:50 +0200
      Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Javier Martinez Canillas <javier@osg.samsung.com> - 2016-07-16 00:00 +0200
    Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead  of vidioc_qbuf Luis de Bethencourt <luisbg@osg.samsung.com> - 2016-07-16 14:20 +0200
    Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Michael Olbrich <m.olbrich@pengutronix.de> - 2016-07-18 10:10 +0200
    Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Hans Verkuil <hverkuil@xs4all.nl> - 2016-07-18 10:40 +0200
      Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Nicolas Dufresne <nicolas@ndufresne.ca> - 2016-07-18 15:40 +0200
      Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Javier Martinez Canillas <javier@osg.samsung.com> - 2016-07-18 16:20 +0200
    Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead  of vidioc_qbuf Marek Szyprowski <m.szyprowski@samsung.com> - 2016-07-18 12:30 +0200
    Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Sakari Ailus <sakari.ailus@iki.fi> - 2016-07-20 15:30 +0200
      Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Javier Martinez Canillas <javier@osg.samsung.com> - 2016-07-20 16:10 +0200
        Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Javier Martinez Canillas <javier@osg.samsung.com> - 2016-07-20 16:20 +0200
        Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue  instead of vidioc_qbuf Hans Verkuil <hverkuil@xs4all.nl> - 2016-07-20 16:20 +0200

#1444432 — [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

The buffer planes' dma-buf are currently mapped when buffers are queued
from userspace but it's more appropriate to do the mapping when buffers
are queued in the driver since that's when the actual DMA operation are
going to happen.

Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

Hello,

A side effect of this change is that if the dmabuf map fails for some
reasons (i.e: a driver using the DMA contig memory allocator but CMA
not being enabled), the fail will no longer happen on VIDIOC_QBUF but
later (i.e: in VIDIOC_STREAMON).

I don't know if that's an issue though but I think is worth mentioning.

Best regards,
Javier

 drivers/media/v4l2-core/videobuf2-core.c | 88 ++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 34 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index ca8ffeb56d72..3fdf882bf279 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -186,7 +186,7 @@ module_param(debug, int, 0644);
 })
 
 static void __vb2_queue_cancel(struct vb2_queue *q);
-static void __enqueue_in_driver(struct vb2_buffer *vb);
+static int __enqueue_in_driver(struct vb2_buffer *vb);
 
 /**
  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
@@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
 		vb->planes[plane].mem_priv = mem_priv;
 	}
 
-	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
-	 * really we want to do this just before the DMA, not while queueing
-	 * the buffer(s)..
-	 */
-	for (plane = 0; plane < vb->num_planes; ++plane) {
-		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
-		if (ret) {
-			dprintk(1, "failed to map dmabuf for plane %d\n",
-				plane);
-			goto err;
-		}
-		vb->planes[plane].dbuf_mapped = 1;
-	}
-
 	/*
 	 * Now that everything is in order, copy relevant information
 	 * provided by userspace.
@@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
 		vb->planes[plane].data_offset = planes[plane].data_offset;
 	}
 
-	if (reacquired) {
-		/*
-		 * Call driver-specific initialization on the newly acquired buffer,
-		 * if provided.
-		 */
-		ret = call_vb_qop(vb, buf_init, vb);
+	return 0;
+err:
+	/* In case of errors, release planes that were already acquired */
+	__vb2_buf_dmabuf_put(vb);
+
+	return ret;
+}
+
+/**
+ * __buf_map_dmabuf() - map dmabuf for buffer planes
+ */
+static int __buf_map_dmabuf(struct vb2_buffer *vb)
+{
+	int ret;
+	unsigned int plane;
+
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
 		if (ret) {
-			dprintk(1, "buffer initialization failed\n");
-			goto err;
+			dprintk(1, "failed to map dmabuf for plane %d\n",
+				plane);
+			return ret;
 		}
+		vb->planes[plane].dbuf_mapped = 1;
+	}
+
+	/*
+	 * Call driver-specific initialization on the newly
+	 * acquired buffer, if provided.
+	 */
+	ret = call_vb_qop(vb, buf_init, vb);
+	if (ret) {
+		dprintk(1, "buffer initialization failed\n");
+		return ret;
 	}
 
 	ret = call_vb_qop(vb, buf_prepare, vb);
 	if (ret) {
 		dprintk(1, "buffer preparation failed\n");
 		call_void_vb_qop(vb, buf_cleanup, vb);
-		goto err;
+		return ret;
 	}
 
 	return 0;
-err:
-	/* In case of errors, release planes that were already acquired */
-	__vb2_buf_dmabuf_put(vb);
-
-	return ret;
 }
 
 /**
  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
  */
-static void __enqueue_in_driver(struct vb2_buffer *vb)
+static int __enqueue_in_driver(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 	unsigned int plane;
+	int ret;
 
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->owned_by_drv_count);
 
 	trace_vb2_buf_queue(q, vb);
 
+	if (q->memory == VB2_MEMORY_DMABUF) {
+		ret = __buf_map_dmabuf(vb);
+		if (ret)
+			return ret;
+	}
+
 	/* sync buffers */
 	for (plane = 0; plane < vb->num_planes; ++plane)
 		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
 
 	call_void_vb_qop(vb, buf_queue, vb);
+
+	return 0;
 }
 
 static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
@@ -1438,8 +1452,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	 * If any buffers were queued before streamon,
 	 * we can now pass them to driver for processing.
 	 */
-	list_for_each_entry(vb, &q->queued_list, queued_entry)
-		__enqueue_in_driver(vb);
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		ret = __enqueue_in_driver(vb);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Tell the driver to start streaming */
 	q->start_streaming_called = 1;
@@ -1540,8 +1557,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	 * If already streaming, give the buffer to driver for processing.
 	 * If not, the buffer will be given to driver on next streamon.
 	 */
-	if (q->start_streaming_called)
-		__enqueue_in_driver(vb);
+	if (q->start_streaming_called) {
+		ret = __enqueue_in_driver(vb);
+		if (ret)
+			return ret;
+	}
 
 	/* Fill buffer information for the userspace */
 	if (pb)
-- 
2.5.5

[toc] | [next] | [standalone]

#1444539 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

On 07/15/2016 10:26 AM, Javier Martinez Canillas wrote:
> The buffer planes' dma-buf are currently mapped when buffers are queued
> from userspace but it's more appropriate to do the mapping when buffers
> are queued in the driver since that's when the actual DMA operation are
> going to happen.
> 
> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
> Hello,
> 
> A side effect of this change is that if the dmabuf map fails for some
> reasons (i.e: a driver using the DMA contig memory allocator but CMA
> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> later (i.e: in VIDIOC_STREAMON).
> 
> I don't know if that's an issue though but I think is worth mentioning.

How does this change impact the user applications.? This changes
the behavior and user applications now get dmabuf map error at a
later stage in the call sequence.

The change itself looks consistent with the change described.

-- Shuah

> 
> Best regards,
> Javier
> 
>  drivers/media/v4l2-core/videobuf2-core.c | 88 ++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ca8ffeb56d72..3fdf882bf279 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
>  })
>  
>  static void __vb2_queue_cancel(struct vb2_queue *q);
> -static void __enqueue_in_driver(struct vb2_buffer *vb);
> +static int __enqueue_in_driver(struct vb2_buffer *vb);
>  
>  /**
>   * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
>  		vb->planes[plane].mem_priv = mem_priv;
>  	}
>  
> -	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
> -	 * really we want to do this just before the DMA, not while queueing
> -	 * the buffer(s)..
> -	 */
> -	for (plane = 0; plane < vb->num_planes; ++plane) {
> -		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> -		if (ret) {
> -			dprintk(1, "failed to map dmabuf for plane %d\n",
> -				plane);
> -			goto err;
> -		}
> -		vb->planes[plane].dbuf_mapped = 1;
> -	}
> -
>  	/*
>  	 * Now that everything is in order, copy relevant information
>  	 * provided by userspace.
> @@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
>  		vb->planes[plane].data_offset = planes[plane].data_offset;
>  	}
>  
> -	if (reacquired) {
> -		/*
> -		 * Call driver-specific initialization on the newly acquired buffer,
> -		 * if provided.
> -		 */
> -		ret = call_vb_qop(vb, buf_init, vb);
> +	return 0;
> +err:
> +	/* In case of errors, release planes that were already acquired */
> +	__vb2_buf_dmabuf_put(vb);
> +
> +	return ret;
> +}
> +
> +/**
> + * __buf_map_dmabuf() - map dmabuf for buffer planes
> + */
> +static int __buf_map_dmabuf(struct vb2_buffer *vb)
> +{
> +	int ret;
> +	unsigned int plane;
> +
> +	for (plane = 0; plane < vb->num_planes; ++plane) {
> +		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>  		if (ret) {
> -			dprintk(1, "buffer initialization failed\n");
> -			goto err;
> +			dprintk(1, "failed to map dmabuf for plane %d\n",
> +				plane);
> +			return ret;
>  		}
> +		vb->planes[plane].dbuf_mapped = 1;
> +	}
> +
> +	/*
> +	 * Call driver-specific initialization on the newly
> +	 * acquired buffer, if provided.
> +	 */
> +	ret = call_vb_qop(vb, buf_init, vb);
> +	if (ret) {
> +		dprintk(1, "buffer initialization failed\n");
> +		return ret;
>  	}
>  
>  	ret = call_vb_qop(vb, buf_prepare, vb);
>  	if (ret) {
>  		dprintk(1, "buffer preparation failed\n");
>  		call_void_vb_qop(vb, buf_cleanup, vb);
> -		goto err;
> +		return ret;
>  	}
>  
>  	return 0;
> -err:
> -	/* In case of errors, release planes that were already acquired */
> -	__vb2_buf_dmabuf_put(vb);
> -
> -	return ret;
>  }
>  
>  /**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
>  	unsigned int plane;
> +	int ret;
>  
>  	vb->state = VB2_BUF_STATE_ACTIVE;
>  	atomic_inc(&q->owned_by_drv_count);
>  
>  	trace_vb2_buf_queue(q, vb);
>  
> +	if (q->memory == VB2_MEMORY_DMABUF) {
> +		ret = __buf_map_dmabuf(vb);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* sync buffers */
>  	for (plane = 0; plane < vb->num_planes; ++plane)
>  		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>  
>  	call_void_vb_qop(vb, buf_queue, vb);
> +
> +	return 0;
>  }
>  
>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> @@ -1438,8 +1452,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> -		__enqueue_in_driver(vb);
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1540,8 +1557,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	 * If already streaming, give the buffer to driver for processing.
>  	 * If not, the buffer will be given to driver on next streamon.
>  	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
> +	if (q->start_streaming_called) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Fill buffer information for the userspace */
>  	if (pb)
> 

[toc] | [prev] | [next] | [standalone]

#1444635 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hello Shuah,

On 07/15/2016 03:42 PM, Shuah Khan wrote:
> On 07/15/2016 10:26 AM, Javier Martinez Canillas wrote:
>> The buffer planes' dma-buf are currently mapped when buffers are queued
>> from userspace but it's more appropriate to do the mapping when buffers
>> are queued in the driver since that's when the actual DMA operation are
>> going to happen.
>>
>> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>>
>> Hello,
>>
>> A side effect of this change is that if the dmabuf map fails for some
>> reasons (i.e: a driver using the DMA contig memory allocator but CMA
>> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
>> later (i.e: in VIDIOC_STREAMON).
>>
>> I don't know if that's an issue though but I think is worth mentioning.
> 
> How does this change impact the user applications.? This changes

One thing that Nicolas mentioned is that for example GStreamer uses QBUF
to detect if a dma-buf is compatible, and fallbacks to a slow path if
that's not the case. For example if VIDIOC_QBUF fails, gst can attempt
to do another VIDIOC_REQBUFS with a different streaming I/O method as a
fallback.

If now QBUF doesn't fail, then gst will believe that it's OK and drop the
buffer so won't be able to recover from an error later and do a fallback.

Now, I don't know if that is the correct thing to expect since the v4l2
doc for VIDIOC_QBUF doesn't say that the ioctl should be used for this.

The question is if validating that the exported dma-buf can be imported
is something that could be done without attempting to do the mapping.

> the behavior and user applications now get dmabuf map error at a
> later stage in the call sequence.
>
> The change itself looks consistent with the change described.
> 
> -- Shuah
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1444823 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

On 15/07/16 17:26, Javier Martinez Canillas wrote:
> The buffer planes' dma-buf are currently mapped when buffers are queued
> from userspace but it's more appropriate to do the mapping when buffers
> are queued in the driver since that's when the actual DMA operation are
> going to happen.
> 
> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
> Hello,
> 
> A side effect of this change is that if the dmabuf map fails for some
> reasons (i.e: a driver using the DMA contig memory allocator but CMA
> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> later (i.e: in VIDIOC_STREAMON).
> 
> I don't know if that's an issue though but I think is worth mentioning.
> 
> Best regards,
> Javier
> 

Just run this path on the ODROID using GStreamer and the vivid driver.
It worked nicely.

Tested-by: Luis de Bethencourt <luisbg@osg.samsung.com>

Thanks Javier,
Luis

[toc] | [prev] | [next] | [standalone]

#1445289 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hi,

On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas wrote:
> The buffer planes' dma-buf are currently mapped when buffers are queued
> from userspace but it's more appropriate to do the mapping when buffers
> are queued in the driver since that's when the actual DMA operation are
> going to happen.
> 
> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
> Hello,
> 
> A side effect of this change is that if the dmabuf map fails for some
> reasons (i.e: a driver using the DMA contig memory allocator but CMA
> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> later (i.e: in VIDIOC_STREAMON).
> 
> I don't know if that's an issue though but I think is worth mentioning.

And for mem2mem devices? Does this mean that the second VIDIOC_STREAMON
will fail? That would make it impossible detect if the buffers on the
capture or the output side are incorrect.

It's already quite difficult to handle these issues gracefully and without
loosing any data. This would make it even worse.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[toc] | [prev] | [next] | [standalone]

#1445299 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

On 07/15/2016 06:26 PM, Javier Martinez Canillas wrote:
> The buffer planes' dma-buf are currently mapped when buffers are queued
> from userspace but it's more appropriate to do the mapping when buffers
> are queued in the driver since that's when the actual DMA operation are
> going to happen.

Does this solve anything? Once the DMA has started the behavior is the same
as before (QBUF maps the dmabuf), only while the DMA engine hasn't started
yet are the QBUF calls just accepted and the mapping takes place when the
DMA is kickstarted. This makes QBUF behave inconsistently.

You don't describe here WHY this change is needed.

I'm not sure I agree with the TODO, and even if I did, I'm not sure I agree
with this solution. Since queuing the buffer to the driver is not the same
as 'just before the DMA', since there may be many buffers queued up in the
driver and you don't know in vb2 when the buffer is at the 'just before the DMA'
stage.

Regards,

	Hans

> 
> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
> Hello,
> 
> A side effect of this change is that if the dmabuf map fails for some
> reasons (i.e: a driver using the DMA contig memory allocator but CMA
> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> later (i.e: in VIDIOC_STREAMON).
> 
> I don't know if that's an issue though but I think is worth mentioning.
> 
> Best regards,
> Javier
> 
>  drivers/media/v4l2-core/videobuf2-core.c | 88 ++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ca8ffeb56d72..3fdf882bf279 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
>  })
>  
>  static void __vb2_queue_cancel(struct vb2_queue *q);
> -static void __enqueue_in_driver(struct vb2_buffer *vb);
> +static int __enqueue_in_driver(struct vb2_buffer *vb);
>  
>  /**
>   * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
>  		vb->planes[plane].mem_priv = mem_priv;
>  	}
>  
> -	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
> -	 * really we want to do this just before the DMA, not while queueing
> -	 * the buffer(s)..
> -	 */
> -	for (plane = 0; plane < vb->num_planes; ++plane) {
> -		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> -		if (ret) {
> -			dprintk(1, "failed to map dmabuf for plane %d\n",
> -				plane);
> -			goto err;
> -		}
> -		vb->planes[plane].dbuf_mapped = 1;
> -	}
> -
>  	/*
>  	 * Now that everything is in order, copy relevant information
>  	 * provided by userspace.
> @@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
>  		vb->planes[plane].data_offset = planes[plane].data_offset;
>  	}
>  
> -	if (reacquired) {
> -		/*
> -		 * Call driver-specific initialization on the newly acquired buffer,
> -		 * if provided.
> -		 */
> -		ret = call_vb_qop(vb, buf_init, vb);
> +	return 0;
> +err:
> +	/* In case of errors, release planes that were already acquired */
> +	__vb2_buf_dmabuf_put(vb);
> +
> +	return ret;
> +}
> +
> +/**
> + * __buf_map_dmabuf() - map dmabuf for buffer planes
> + */
> +static int __buf_map_dmabuf(struct vb2_buffer *vb)
> +{
> +	int ret;
> +	unsigned int plane;
> +
> +	for (plane = 0; plane < vb->num_planes; ++plane) {
> +		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>  		if (ret) {
> -			dprintk(1, "buffer initialization failed\n");
> -			goto err;
> +			dprintk(1, "failed to map dmabuf for plane %d\n",
> +				plane);
> +			return ret;
>  		}
> +		vb->planes[plane].dbuf_mapped = 1;
> +	}
> +
> +	/*
> +	 * Call driver-specific initialization on the newly
> +	 * acquired buffer, if provided.
> +	 */
> +	ret = call_vb_qop(vb, buf_init, vb);
> +	if (ret) {
> +		dprintk(1, "buffer initialization failed\n");
> +		return ret;
>  	}
>  
>  	ret = call_vb_qop(vb, buf_prepare, vb);
>  	if (ret) {
>  		dprintk(1, "buffer preparation failed\n");
>  		call_void_vb_qop(vb, buf_cleanup, vb);
> -		goto err;
> +		return ret;
>  	}
>  
>  	return 0;
> -err:
> -	/* In case of errors, release planes that were already acquired */
> -	__vb2_buf_dmabuf_put(vb);
> -
> -	return ret;
>  }
>  
>  /**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
>  	unsigned int plane;
> +	int ret;
>  
>  	vb->state = VB2_BUF_STATE_ACTIVE;
>  	atomic_inc(&q->owned_by_drv_count);
>  
>  	trace_vb2_buf_queue(q, vb);
>  
> +	if (q->memory == VB2_MEMORY_DMABUF) {
> +		ret = __buf_map_dmabuf(vb);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* sync buffers */
>  	for (plane = 0; plane < vb->num_planes; ++plane)
>  		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>  
>  	call_void_vb_qop(vb, buf_queue, vb);
> +
> +	return 0;
>  }
>  
>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> @@ -1438,8 +1452,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> -		__enqueue_in_driver(vb);
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1540,8 +1557,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	 * If already streaming, give the buffer to driver for processing.
>  	 * If not, the buffer will be given to driver on next streamon.
>  	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
> +	if (q->start_streaming_called) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Fill buffer information for the userspace */
>  	if (pb)
> 

[toc] | [prev] | [next] | [standalone]

#1445488 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

[Multipart message — attachments visible in raw view] — view raw

Le lundi 18 juillet 2016 à 10:34 +0200, Hans Verkuil a écrit :
> On 07/15/2016 06:26 PM, Javier Martinez Canillas wrote:
> > The buffer planes' dma-buf are currently mapped when buffers are queued
> > from userspace but it's more appropriate to do the mapping when buffers
> > are queued in the driver since that's when the actual DMA operation are
> > going to happen.
> 
> Does this solve anything? Once the DMA has started the behavior is the same
> as before (QBUF maps the dmabuf), only while the DMA engine hasn't started
> yet are the QBUF calls just accepted and the mapping takes place when the
> DMA is kickstarted. This makes QBUF behave inconsistently.

The expected behaviour would have been to ensure that DMABuf mapping
only happen when the driver need the buffer (as late as possible). As
you describes it, the goal is not met.

> 
> You don't describe here WHY this change is needed.

It should have been explained (just like this patch should have been
marked as RFC). The is numerous reason why you don't want to spend
userspace time mapping a buffers.

First the context, mapping a DMA-Buf is a costy operation. With the
venu of DMA-Buf fences, (in implicit mode) this operation becomes even
more expensive in term of time you block userspace. By mapping in QBUF,
you do block the userspace process for a certain duration.

By delaying the mapping later, the time spent mapping is now in the
driver thread, without blocking the userspace. This allow running with
much lower latency, as the userspace can (if already available) fetch
the following buffer and put it in the queue without further delays. As
the buffers are available earlier, the streaming can be started sooner
and no time is lost.

Another reason, which was not part of our discussion, is that if you
have a display at lower framerate, it would allow appropriate frame
skipping to be implemented. Mapping all the frames, even the one that
won't be displayed would be inefficient.

So basically, what we are saying, is that the currently implemented
mechanism is a was of userspace time, reduce the benefit of using
fences and increase latency.
 
> 
> I'm not sure I agree with the TODO, and even if I did, I'm not sure I agree
> with this solution. Since queuing the buffer to the driver is not the same
> as 'just before the DMA', since there may be many buffers queued up in the
> driver and you don't know in vb2 when the buffer is at the 'just before the DMA'
> stage.

Unfortunate, but it's just software ;-P An idea would be to introduce
some new state for preparing a buffers, so the driver don't endup
waiting at an unfortunate moment. Again, this is all only needed if we
can provide the same level of buffer validation we had at QBUF. As we
don't expose to userspace the information needed to validate if a DMA-
Buf is compatible, we started (not yet merged) implementing fallback at
QBuf. Failing asynchronously would leave userspace with absolutely no
way to handle the case of incompatible DMA-Buf.

Regards,
Nicolas

p.s. I'll be away for the rest of the summer, see you in September.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> > 
> > ---
> > 
> > Hello,
> > 
> > A side effect of this change is that if the dmabuf map fails for some
> > reasons (i.e: a driver using the DMA contig memory allocator but CMA
> > not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> > later (i.e: in VIDIOC_STREAMON).
> > 
> > I don't know if that's an issue though but I think is worth mentioning.
> > 
> > Best regards,
> > Javier
> > 
> >  drivers/media/v4l2-core/videobuf2-core.c | 88 ++++++++++++++++++++------------
> >  1 file changed, 54 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index ca8ffeb56d72..3fdf882bf279 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
> >  })
> >  
> >  static void __vb2_queue_cancel(struct vb2_queue *q);
> > -static void __enqueue_in_driver(struct vb2_buffer *vb);
> > +static int __enqueue_in_driver(struct vb2_buffer *vb);
> >  
> >  /**
> >   * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> > @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
> >  		vb->planes[plane].mem_priv = mem_priv;
> >  	}
> >  
> > -	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
> > -	 * really we want to do this just before the DMA, not while queueing
> > -	 * the buffer(s)..
> > -	 */
> > -	for (plane = 0; plane < vb->num_planes; ++plane) {
> > -		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> > -		if (ret) {
> > -			dprintk(1, "failed to map dmabuf for plane %d\n",
> > -				plane);
> > -			goto err;
> > -		}
> > -		vb->planes[plane].dbuf_mapped = 1;
> > -	}
> > -
> >  	/*
> >  	 * Now that everything is in order, copy relevant information
> >  	 * provided by userspace.
> > @@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
> >  		vb->planes[plane].data_offset = planes[plane].data_offset;
> >  	}
> >  
> > -	if (reacquired) {
> > -		/*
> > -		 * Call driver-specific initialization on the newly acquired buffer,
> > -		 * if provided.
> > -		 */
> > -		ret = call_vb_qop(vb, buf_init, vb);
> > +	return 0;
> > +err:
> > +	/* In case of errors, release planes that were already acquired */
> > +	__vb2_buf_dmabuf_put(vb);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * __buf_map_dmabuf() - map dmabuf for buffer planes
> > + */
> > +static int __buf_map_dmabuf(struct vb2_buffer *vb)
> > +{
> > +	int ret;
> > +	unsigned int plane;
> > +
> > +	for (plane = 0; plane < vb->num_planes; ++plane) {
> > +		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> >  		if (ret) {
> > -			dprintk(1, "buffer initialization failed\n");
> > -			goto err;
> > +			dprintk(1, "failed to map dmabuf for plane %d\n",
> > +				plane);
> > +			return ret;
> >  		}
> > +		vb->planes[plane].dbuf_mapped = 1;
> > +	}
> > +
> > +	/*
> > +	 * Call driver-specific initialization on the newly
> > +	 * acquired buffer, if provided.
> > +	 */
> > +	ret = call_vb_qop(vb, buf_init, vb);
> > +	if (ret) {
> > +		dprintk(1, "buffer initialization failed\n");
> > +		return ret;
> >  	}
> >  
> >  	ret = call_vb_qop(vb, buf_prepare, vb);
> >  	if (ret) {
> >  		dprintk(1, "buffer preparation failed\n");
> >  		call_void_vb_qop(vb, buf_cleanup, vb);
> > -		goto err;
> > +		return ret;
> >  	}
> >  
> >  	return 0;
> > -err:
> > -	/* In case of errors, release planes that were already acquired */
> > -	__vb2_buf_dmabuf_put(vb);
> > -
> > -	return ret;
> >  }
> >  
> >  /**
> >   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
> >   */
> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> >  	struct vb2_queue *q = vb->vb2_queue;
> >  	unsigned int plane;
> > +	int ret;
> >  
> >  	vb->state = VB2_BUF_STATE_ACTIVE;
> >  	atomic_inc(&q->owned_by_drv_count);
> >  
> >  	trace_vb2_buf_queue(q, vb);
> >  
> > +	if (q->memory == VB2_MEMORY_DMABUF) {
> > +		ret = __buf_map_dmabuf(vb);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	/* sync buffers */
> >  	for (plane = 0; plane < vb->num_planes; ++plane)
> >  		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
> >  
> >  	call_void_vb_qop(vb, buf_queue, vb);
> > +
> > +	return 0;
> >  }
> >  
> >  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> > @@ -1438,8 +1452,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	 * If any buffers were queued before streamon,
> >  	 * we can now pass them to driver for processing.
> >  	 */
> > -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> > -		__enqueue_in_driver(vb);
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		ret = __enqueue_in_driver(vb);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >  
> >  	/* Tell the driver to start streaming */
> >  	q->start_streaming_called = 1;
> > @@ -1540,8 +1557,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	 * If already streaming, give the buffer to driver for processing.
> >  	 * If not, the buffer will be given to driver on next streamon.
> >  	 */
> > -	if (q->start_streaming_called)
> > -		__enqueue_in_driver(vb);
> > +	if (q->start_streaming_called) {
> > +		ret = __enqueue_in_driver(vb);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	/* Fill buffer information for the userspace */
> >  	if (pb)
> > 

[toc] | [prev] | [next] | [standalone]

#1445508 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hello Hans, Michael and Marek,

Thanks a lot for your feedback.

On 07/18/2016 04:34 AM, Hans Verkuil wrote:
> On 07/15/2016 06:26 PM, Javier Martinez Canillas wrote:
>> The buffer planes' dma-buf are currently mapped when buffers are queued
>> from userspace but it's more appropriate to do the mapping when buffers
>> are queued in the driver since that's when the actual DMA operation are
>> going to happen.
> 
> Does this solve anything? Once the DMA has started the behavior is the same
> as before (QBUF maps the dmabuf), only while the DMA engine hasn't started
> yet are the QBUF calls just accepted and the mapping takes place when the
> DMA is kickstarted. This makes QBUF behave inconsistently.
> 
> You don't describe here WHY this change is needed.
>

Nicolas pointed me to the TODO and suggested me the patch for the reasons
he explained in his latest email. And yes, this should had been tagged as
a RFC and just to know what you think about it. Sorry for missing that.

> I'm not sure I agree with the TODO, and even if I did, I'm not sure I agree
> with this solution. Since queuing the buffer to the driver is not the same
> as 'just before the DMA', since there may be many buffers queued up in the
> driver and you don't know in vb2 when the buffer is at the 'just before the DMA'
> stage.
>

Right, I meant "as closer as possible to when the actual DMA is going to happen"
rather than "just before the DMA".

> Regards,
> 
> 	Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1445364 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hello,


On 2016-07-15 18:26, Javier Martinez Canillas wrote:
> The buffer planes' dma-buf are currently mapped when buffers are queued
> from userspace but it's more appropriate to do the mapping when buffers
> are queued in the driver since that's when the actual DMA operation are
> going to happen.
>
> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

Sorry, but I don't get why such change is really needed. If possible it is
better to report errors to userspace as early as possible, not postpone them
to the moment, when they cannot be easily debugged.

>
> ---
>
> Hello,
>
> A side effect of this change is that if the dmabuf map fails for some
> reasons (i.e: a driver using the DMA contig memory allocator but CMA
> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> later (i.e: in VIDIOC_STREAMON).
>
> I don't know if that's an issue though but I think is worth mentioning.
>
> Best regards,
> Javier
>
>   drivers/media/v4l2-core/videobuf2-core.c | 88 ++++++++++++++++++++------------
>   1 file changed, 54 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ca8ffeb56d72..3fdf882bf279 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
>   })
>   
>   static void __vb2_queue_cancel(struct vb2_queue *q);
> -static void __enqueue_in_driver(struct vb2_buffer *vb);
> +static int __enqueue_in_driver(struct vb2_buffer *vb);
>   
>   /**
>    * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
>   		vb->planes[plane].mem_priv = mem_priv;
>   	}
>   
> -	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
> -	 * really we want to do this just before the DMA, not while queueing
> -	 * the buffer(s)..
> -	 */
> -	for (plane = 0; plane < vb->num_planes; ++plane) {
> -		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> -		if (ret) {
> -			dprintk(1, "failed to map dmabuf for plane %d\n",
> -				plane);
> -			goto err;
> -		}
> -		vb->planes[plane].dbuf_mapped = 1;
> -	}
> -
>   	/*
>   	 * Now that everything is in order, copy relevant information
>   	 * provided by userspace.
> @@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const void *pb)
>   		vb->planes[plane].data_offset = planes[plane].data_offset;
>   	}
>   
> -	if (reacquired) {
> -		/*
> -		 * Call driver-specific initialization on the newly acquired buffer,
> -		 * if provided.
> -		 */
> -		ret = call_vb_qop(vb, buf_init, vb);
> +	return 0;
> +err:
> +	/* In case of errors, release planes that were already acquired */
> +	__vb2_buf_dmabuf_put(vb);
> +
> +	return ret;
> +}
> +
> +/**
> + * __buf_map_dmabuf() - map dmabuf for buffer planes
> + */
> +static int __buf_map_dmabuf(struct vb2_buffer *vb)
> +{
> +	int ret;
> +	unsigned int plane;
> +
> +	for (plane = 0; plane < vb->num_planes; ++plane) {
> +		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>   		if (ret) {
> -			dprintk(1, "buffer initialization failed\n");
> -			goto err;
> +			dprintk(1, "failed to map dmabuf for plane %d\n",
> +				plane);
> +			return ret;
>   		}
> +		vb->planes[plane].dbuf_mapped = 1;
> +	}
> +
> +	/*
> +	 * Call driver-specific initialization on the newly
> +	 * acquired buffer, if provided.
> +	 */
> +	ret = call_vb_qop(vb, buf_init, vb);
> +	if (ret) {
> +		dprintk(1, "buffer initialization failed\n");
> +		return ret;
>   	}
>   
>   	ret = call_vb_qop(vb, buf_prepare, vb);
>   	if (ret) {
>   		dprintk(1, "buffer preparation failed\n");
>   		call_void_vb_qop(vb, buf_cleanup, vb);
> -		goto err;
> +		return ret;
>   	}
>   
>   	return 0;
> -err:
> -	/* In case of errors, release planes that were already acquired */
> -	__vb2_buf_dmabuf_put(vb);
> -
> -	return ret;
>   }
>   
>   /**
>    * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>    */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
>   {
>   	struct vb2_queue *q = vb->vb2_queue;
>   	unsigned int plane;
> +	int ret;
>   
>   	vb->state = VB2_BUF_STATE_ACTIVE;
>   	atomic_inc(&q->owned_by_drv_count);
>   
>   	trace_vb2_buf_queue(q, vb);
>   
> +	if (q->memory == VB2_MEMORY_DMABUF) {
> +		ret = __buf_map_dmabuf(vb);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	/* sync buffers */
>   	for (plane = 0; plane < vb->num_planes; ++plane)
>   		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>   
>   	call_void_vb_qop(vb, buf_queue, vb);
> +
> +	return 0;
>   }
>   
>   static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> @@ -1438,8 +1452,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
>   	 * If any buffers were queued before streamon,
>   	 * we can now pass them to driver for processing.
>   	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> -		__enqueue_in_driver(vb);
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	/* Tell the driver to start streaming */
>   	q->start_streaming_called = 1;
> @@ -1540,8 +1557,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>   	 * If already streaming, give the buffer to driver for processing.
>   	 * If not, the buffer will be given to driver on next streamon.
>   	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
> +	if (q->start_streaming_called) {
> +		ret = __enqueue_in_driver(vb);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	/* Fill buffer information for the userspace */
>   	if (pb)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [prev] | [next] | [standalone]

#1447230 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hi Javier,

On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas wrote:
> The buffer planes' dma-buf are currently mapped when buffers are queued
> from userspace but it's more appropriate to do the mapping when buffers
> are queued in the driver since that's when the actual DMA operation are
> going to happen.
> 
> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
> Hello,
> 
> A side effect of this change is that if the dmabuf map fails for some
> reasons (i.e: a driver using the DMA contig memory allocator but CMA
> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> later (i.e: in VIDIOC_STREAMON).
> 
> I don't know if that's an issue though but I think is worth mentioning.

I have the same question has Hans --- why?

I rather think we should keep the buffers mapped all the time. That'd
require a bit of extra from the DMA-BUF framework I suppose, to support
streaming mappings.

The reason for that is performance. If you're passing the buffer between a
couple of hardware devices, there's no need to map and unmap it every time
the buffer is accessed by the said devices. That'd avoid an unnecessary
cache flush as well, something that tends to be quite expensive. On a PC
with resolutions typically used on webcams that might not really matter. But
if you have an embedded system with a relatively modest 10 MP camera sensor,
it's one of the first things you'll notice if you check where the CPU time
is being spent.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1447247 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hello Sakari,

On 07/20/2016 09:20 AM, Sakari Ailus wrote:
> Hi Javier,
> 
> On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas wrote:
>> The buffer planes' dma-buf are currently mapped when buffers are queued
>> from userspace but it's more appropriate to do the mapping when buffers
>> are queued in the driver since that's when the actual DMA operation are
>> going to happen.
>>
>> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>>
>> Hello,
>>
>> A side effect of this change is that if the dmabuf map fails for some
>> reasons (i.e: a driver using the DMA contig memory allocator but CMA
>> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
>> later (i.e: in VIDIOC_STREAMON).
>>
>> I don't know if that's an issue though but I think is worth mentioning.
> 
> I have the same question has Hans --- why?
>

Yes, sorry for missing this information. Nicolas already explained a little
bit but the context is that I want to add dma-buf fence support to videobuf2,
and currently the dma-buf is unmapped in VIDIOC_DQBUF.

But with dma-buf fence, the idea is to be able to dequeue a buffer even when
the driver has not yet finished processing the buffer. So the dma-buf needs to
be mapped until vb2_buffer_done() when the driver is done processing the vb2,
and is able to signal the pending fence.

Since the unmapping was going to be delayed to vb2_buffer_done(), I thought
it would make sense to also move the mapping closer to when is really going
to be used and that's why I moved it to __enqueue_in_driver() in this patch.

But I didn't know that user-space was using the dma-buf map as a way to know
if the dma-buf will be compatible and fallback to a different streaming I/O
method if that's not the case. So $SUBJECT is wrong if it prevents user-space
to recover gracefully from a dma-buf mapping failure.

In any case, only delaying the unmapping is needed to support fence and doing
the map early in VIDIOC_QBUF is not an issue.
 
> I rather think we should keep the buffers mapped all the time. That'd
> require a bit of extra from the DMA-BUF framework I suppose, to support
> streaming mappings.
> 

Interesting, I can take a look to this possibility after adding the dma-buf
fence support.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1447252 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

Hello Hans,

On 07/20/2016 10:12 AM, Hans Verkuil wrote:
> On 07/20/2016 04:06 PM, Javier Martinez Canillas wrote:
>> Hello Sakari,
>>
>> On 07/20/2016 09:20 AM, Sakari Ailus wrote:
>>> Hi Javier,
>>>
>>> On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas wrote:
>>>> The buffer planes' dma-buf are currently mapped when buffers are queued
>>>> from userspace but it's more appropriate to do the mapping when buffers
>>>> are queued in the driver since that's when the actual DMA operation are
>>>> going to happen.
>>>>
>>>> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> A side effect of this change is that if the dmabuf map fails for some
>>>> reasons (i.e: a driver using the DMA contig memory allocator but CMA
>>>> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
>>>> later (i.e: in VIDIOC_STREAMON).
>>>>
>>>> I don't know if that's an issue though but I think is worth mentioning.
>>>
>>> I have the same question has Hans --- why?
>>>
>>
>> Yes, sorry for missing this information. Nicolas already explained a little
>> bit but the context is that I want to add dma-buf fence support to videobuf2,
>> and currently the dma-buf is unmapped in VIDIOC_DQBUF.
>>
>> But with dma-buf fence, the idea is to be able to dequeue a buffer even when
>> the driver has not yet finished processing the buffer. So the dma-buf needs to
>> be mapped until vb2_buffer_done() when the driver is done processing the vb2,
>> and is able to signal the pending fence.
>>
>> Since the unmapping was going to be delayed to vb2_buffer_done(), I thought
>> it would make sense to also move the mapping closer to when is really going
>> to be used and that's why I moved it to __enqueue_in_driver() in this patch.
>>
>> But I didn't know that user-space was using the dma-buf map as a way to know
>> if the dma-buf will be compatible and fallback to a different streaming I/O
>> method if that's not the case. So $SUBJECT is wrong if it prevents user-space
>> to recover gracefully from a dma-buf mapping failure.
>>
>> In any case, only delaying the unmapping is needed to support fence and doing
>> the map early in VIDIOC_QBUF is not an issue.
> 
> OK. I've rejected this patch. I understand the DQBUF part and I happily accept

Ok, thanks.

> a patch for that. But the other side should be left as-is. The TODO comment
> should probably be dropped, now that I think about it.
>

I can post such a patch, do you want me to also add a comment about why is done
in QBUF instead of when the buffer is queued in the driver (e.g: that user-space
is able to recover in QBUF but no in STREAMON) or just removing it and mention
that in the commit message is enough?
 
> Regards,
> 
> 	Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1447256 — Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

On 07/20/2016 04:06 PM, Javier Martinez Canillas wrote:
> Hello Sakari,
> 
> On 07/20/2016 09:20 AM, Sakari Ailus wrote:
>> Hi Javier,
>>
>> On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas wrote:
>>> The buffer planes' dma-buf are currently mapped when buffers are queued
>>> from userspace but it's more appropriate to do the mapping when buffers
>>> are queued in the driver since that's when the actual DMA operation are
>>> going to happen.
>>>
>>> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>
>>> ---
>>>
>>> Hello,
>>>
>>> A side effect of this change is that if the dmabuf map fails for some
>>> reasons (i.e: a driver using the DMA contig memory allocator but CMA
>>> not being enabled), the fail will no longer happen on VIDIOC_QBUF but
>>> later (i.e: in VIDIOC_STREAMON).
>>>
>>> I don't know if that's an issue though but I think is worth mentioning.
>>
>> I have the same question has Hans --- why?
>>
> 
> Yes, sorry for missing this information. Nicolas already explained a little
> bit but the context is that I want to add dma-buf fence support to videobuf2,
> and currently the dma-buf is unmapped in VIDIOC_DQBUF.
> 
> But with dma-buf fence, the idea is to be able to dequeue a buffer even when
> the driver has not yet finished processing the buffer. So the dma-buf needs to
> be mapped until vb2_buffer_done() when the driver is done processing the vb2,
> and is able to signal the pending fence.
> 
> Since the unmapping was going to be delayed to vb2_buffer_done(), I thought
> it would make sense to also move the mapping closer to when is really going
> to be used and that's why I moved it to __enqueue_in_driver() in this patch.
> 
> But I didn't know that user-space was using the dma-buf map as a way to know
> if the dma-buf will be compatible and fallback to a different streaming I/O
> method if that's not the case. So $SUBJECT is wrong if it prevents user-space
> to recover gracefully from a dma-buf mapping failure.
> 
> In any case, only delaying the unmapping is needed to support fence and doing
> the map early in VIDIOC_QBUF is not an issue.

OK. I've rejected this patch. I understand the DQBUF part and I happily accept
a patch for that. But the other side should be left as-is. The TODO comment
should probably be dropped, now that I think about it.

Regards,

	Hans

>> I rather think we should keep the buffers mapped all the time. That'd
>> require a bit of extra from the DMA-BUF framework I suppose, to support
>> streaming mappings.
>>
> 
> Interesting, I can take a look to this possibility after adding the dma-buf
> fence support.
> 
> Best regards,
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web