From patchwork Tue Aug 2 08:15:02 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Guennadi Liakhovetski X-Patchwork-Id: 1028292 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p728FQ8f014019 for ; Tue, 2 Aug 2011 08:15:26 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752318Ab1HBIPL (ORCPT ); Tue, 2 Aug 2011 04:15:11 -0400 Received: from moutng.kundenserver.de ([212.227.126.171]:51957 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752174Ab1HBIPJ (ORCPT ); Tue, 2 Aug 2011 04:15:09 -0400 Received: from axis700.grange (dslb-178-001-141-173.pools.arcor-ip.net [178.1.141.173]) by mrelayeu.kundenserver.de (node=mrbap4) with ESMTP (Nemesis) id 0LpeYo-1RH9tJ0DI6-00fSsW; Tue, 02 Aug 2011 10:15:03 +0200 Received: by axis700.grange (Postfix, from userid 1000) id 98CB5189B6E; Tue, 2 Aug 2011 10:15:02 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by axis700.grange (Postfix) with ESMTP id 8DB2E189B6D; Tue, 2 Aug 2011 10:15:02 +0200 (CEST) Date: Tue, 2 Aug 2011 10:15:02 +0200 (CEST) From: Guennadi Liakhovetski X-X-Sender: lyakh@axis700.grange To: Hans Verkuil cc: Pawel Osciak , Linux Media Mailing List , Sakari Ailus , Sakari Ailus , Laurent Pinchart , Mauro Carvalho Chehab Subject: Re: [PATCH v3] V4L: add two new ioctl()s for multi-size videobuffer management In-Reply-To: <201107280856.55731.hverkuil@xs4all.nl> Message-ID: References: <201107280856.55731.hverkuil@xs4all.nl> MIME-Version: 1.0 X-Provags-ID: V02:K0:8ASR9AuSP/BRHaJ7ai8sL1OcmuaL9ok9HHJnr9a7zG0 9ufhptmD7oq3AdALUowcg222nzxIYPS61KIOfW6+aDRBmuTMuV rAmFQf/mx9CZI0VFO82IJ3rety66lrHezsbNmofoRWN9ddHaZe JRZy9vfMrjFsFQoHwXtfz6xp2WaXvvn6sicVruMjEYpMwDaI2i 8/Y3cmOT3v0FPbzHrRchmAHDZMHERSBL0pi/EZ/hhg= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 02 Aug 2011 08:15:30 +0000 (UTC) On Thu, 28 Jul 2011, Hans Verkuil wrote: > On Thursday, July 28, 2011 06:11:38 Pawel Osciak wrote: > > Hi Guennadi, > > > > On Wed, Jul 20, 2011 at 01:43, Guennadi Liakhovetski > > wrote: > > > A possibility to preallocate and initialise buffers of different sizes > > > in V4L2 is required for an efficient implementation of asnapshot mode. > > > This patch adds two new ioctl()s: VIDIOC_CREATE_BUFS and > > > VIDIOC_PREPARE_BUF and defines respective data structures. > > > > > > Signed-off-by: Guennadi Liakhovetski > > > --- > > > > > > > > > This looks nicer, I like how we got rid of destroy and gave up on > > making holes, it would've given us a lot of headaches. I'm thinking > > about some issues though and also have some comments/questions further > > below. > > > > Already mentioned by others mixing of REQBUFS and CREATE_BUFS. > > Personally I'd like to allow mixing, including REQBUFS for non-zero, > > because I think it would be easy to do. I think it could work in the > > same way as REQBUFS for !=0 works currently (at least in vb2), if we > > already have some buffers allocated and they are not in use, we free > > them and a new set is allocated. So I guess it could just stay this > > way. REQBUFS(0) would of course free everything. > > > > Passing format to CREATE_BUFS will make vb2 a bit format-aware, as it > > would have to pass it forward to the driver somehow. The obvious way > > would be just vb2 calling the driver's s_fmt handler, but that won't > > work, as you can't pass indexes to s_fmt. So we'd have to implement a > > new driver callback for setting formats per index. I guess there is no > > way around it, unless we actually take the format struct out of > > CREATE_BUFS and somehow do it via S_FMT. The single-planar structure > > is full already though, the only way would be to use > > v4l2_pix_format_mplane instead with plane count = 1 (or more if > > needed). > > I just got an idea for this: use TRY_FMT. That will do exactly what > you want. In fact, perhaps we should remove the format struct from > CREATE_BUFS and use __u32 sizes[VIDEO_MAX_PLANES] instead. Let the > application call TRY_FMT and initialize the sizes array instead of > putting that into vb2. We may need a num_planes field as well. If the > sizes are all 0 (or num_planes is 0), then the driver can use the current > format, just as it does with REQBUFS. > > Or am I missing something? ...After more thinking and looking at the vb2 code, this began to feel wrong to me. This introduces an asymmetry, which doesn't necessarily look good to me. At present we have the TRY_FMT and S_FMT ioctl()s, which among other tasks calculate sizeimage and bytesperline - either per plane or total. Besides we also have the REQBUFS call, that internally calls the .queue_setup() queue method. In that method the _driver_ has a chance to calculate for the _current format_ the number of planes (again?...) and buffer sizes for each plane. This suggests, that the latter calculation can be different from the former. Now you're suggesting to use TRY_FMT to calculate the number of planes and per-plane sizeofimage, and then use _only_ this information to set up the buffers from the CREATE_BUFS ioctl(). So, are we now claiming, that this information alone (per-plane-sizeofimage) should be dufficient to set up buffers? OTOH, Pawel's above question has a simple answer: vb2 is not becoming format aware. It just passes the format on to the driver with the (modified) .queue_setup() method: (of course, we would first add a new method, migrate all drivers, then remove the old one). Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index f87472a..f5a7d92 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -210,9 +216,10 @@ struct vb2_buffer { * the buffer back by calling vb2_buffer_done() function */ struct vb2_ops { - int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, - unsigned int *num_planes, unsigned long sizes[], - void *alloc_ctxs[]); + int (*queue_setup)(struct vb2_queue *q, + struct v4l2_create_buffers *create, + unsigned int *num_buffers, unsigned int *num_planes, + unsigned long sizes[], void *alloc_ctxs[]); void (*wait_prepare)(struct vb2_queue *q); void (*wait_finish)(struct vb2_queue *q);