| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <57893C98.6040804@osg.samsung.com>
Date: Fri, 15 Jul 2016 13:42:16 -0600
From: Shuah Khan <shuahkh@....samsung.com>
To: Javier Martinez Canillas <javier@....samsung.com>,
linux-kernel@...r.kernel.org
Cc: Marek Szyprowski <m.szyprowski@...sung.com>,
Mauro Carvalho Chehab <mchehab@...pensource.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Pawel Osciak <pawel@...iak.com>, linux-media@...r.kernel.org,
Hans Verkuil <hverkuil@...all.nl>,
Nicolas Dufresne <nicolas.dufresne@...labora.com>,
Luis de Bethencourt <luisbg@....samsung.com>,
Shuah Khan <shuahkh@....samsung.com>
Subject: 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@...labora.com>
> Signed-off-by: Javier Martinez Canillas <javier@....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)
>
Powered by blists - more mailing lists