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 <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> ---
>
> 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)
>
