From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752106AbcGRK15 (ORCPT ); Mon, 18 Jul 2016 06:27:57 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:43218 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751441AbcGRK1y (ORCPT ); Mon, 18 Jul 2016 06:27:54 -0400 X-AuditID: cbfec7f5-f792a6d000001302-a8-578caf25b1c2 Subject: Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf To: Javier Martinez Canillas , linux-kernel@vger.kernel.org References: <1468599966-31988-1-git-send-email-javier@osg.samsung.com> Cc: Mauro Carvalho Chehab , Kyungmin Park , Pawel Osciak , linux-media@vger.kernel.org, Hans Verkuil , Shuah Khan , Nicolas Dufresne , Luis de Bethencourt From: Marek Szyprowski Message-id: <1f87017c-5d5f-bcdc-3df3-e04962cbc978@samsung.com> Date: Mon, 18 Jul 2016 12:27:48 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-version: 1.0 In-reply-to: <1468599966-31988-1-git-send-email-javier@osg.samsung.com> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrKLMWRmVeSWpSXmKPExsVy+t/xK7qq63vCDWausLI4NfkZk8Wbt2uY LM42vWG3uLxrDptFz4atrBZH501jtLi/pNBi15d7bBZT3v5kt5j65QOLA5fHjrtLGD0e/3rJ 5rGl/y67x6ZPd1k9+rasYvT4vEnO49TXz+wB7FFcNimpOZllqUX6dglcGc+bvrMXNBlUPLuy kbmBsVG1i5GTQ0LARGLpjwZ2CFtM4sK99WxdjFwcQgJLGSWm/Wxjh3CeM0o8adjDBFIlLJAg ceTOBFYQW0QgVOLfxduMILaQgJvEyXXTWUEamAUuMknMvrObBSTBJmAo0fW2iw3E5hWwk5gx 8wfYIBYBVYnrhx+CxUUFYiQabx9mh6gRlPgx+R5YL6eAu8ThhT/B6pkFzCS+vDzMCmHLS2xe 85Z5AqPALCQts5CUzUJStoCReRWjaGppckFxUnqukV5xYm5xaV66XnJ+7iZGSGR83cG49JjV IUYBDkYlHt4ba7vDhVgTy4orcw8xSnAwK4nwRq7tCRfiTUmsrEotyo8vKs1JLT7EKM3BoiTO O3PX+xAhgfTEktTs1NSC1CKYLBMHp1QDY1ZpnMhPZUvZ2BNh/Lvur2ye+9I5/EnPO3sB+ejs i02lkhWX9Pb9N0uSuFveeaXBWu/vBp/eyxN2yc0562t43OCPktDTBRJOry4EPbpToLjQsEFL 6+cl0Y19vOx/Y66v0JTOnZLf07rPb/Mk9Y0N/rxGcwtjJeoSi60Xs9gJb5UX2i7/s/63Ektx RqKhFnNRcSIA48Dn9YgCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Javier Martinez Canillas 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