From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lb1-smtp-cloud2.xs4all.net ([194.109.24.21]:45856 "EHLO lb1-smtp-cloud2.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751958AbdBCOcC (ORCPT ); Fri, 3 Feb 2017 09:32:02 -0500 Subject: Re: vb2 queue_setup documentation clarification (was "Re: [PATCH v3 00/24] i.MX Media Driver") To: Laurent Pinchart , Steve Longerbeam References: <1483755102-24785-1-git-send-email-steve_longerbeam@mentor.com> <20170202185826.GV27312@n2100.armlinux.org.uk> <2e1cf096-ecb8-ba3d-a554-f4cc6999ed4e@gmail.com> <1600727.suTjUfaXG8@avalon> Cc: Russell King - ARM Linux , Steve Longerbeam , linux-media@vger.kernel.org, p.zabel@pengutronix.de, Sakari Ailus From: Hans Verkuil Message-ID: <9d1a185a-f614-9c16-298a-2596444569b0@xs4all.nl> Date: Fri, 3 Feb 2017 15:31:59 +0100 MIME-Version: 1.0 In-Reply-To: <1600727.suTjUfaXG8@avalon> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 03/02/17 15:21, Laurent Pinchart wrote: > Hi Steve, > > (stripping the CC list a bit and adding Sakari Ailus) > > On Thursday 02 Feb 2017 11:12:41 Steve Longerbeam wrote: >> On 02/02/2017 10:58 AM, Russell King - ARM Linux wrote: > > [snip] > >>> It seems to me that if you don't take account of the existing queue >>> size, your camif_queue_setup() has the side effect that each time >>> either of these are called. Hence, the vb2 queue increases by the >>> same amount each time, which is probably what you don't want. >>> >>> The documentation on queue_setup() leaves much to be desired: >>> * @queue_setup: called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS() >>> * handlers before memory allocation. It can be called >>> * twice: if the original number of requested buffers >>> * could not be allocated, then it will be called a >>> * second time with the actually allocated number of >>> * buffers to verify if that is OK. >>> * The driver should return the required number of buffers >>> * in \*num_buffers, the required number of planes per >>> * buffer in \*num_planes, the size of each plane should be >>> * set in the sizes\[\] array and optional per-plane >>> * allocator specific device in the alloc_devs\[\] array. >>> * When called from VIDIOC_REQBUFS,() \*num_planes == 0, >>> * the driver has to use the currently configured format to >>> * determine the plane sizes and \*num_buffers is the total >>> * number of buffers that are being allocated. When called >>> * from VIDIOC_CREATE_BUFS,() \*num_planes != 0 and it >>> * describes the requested number of planes and sizes\[\] >>> * contains the requested plane sizes. If either >>> * \*num_planes or the requested sizes are invalid callback >>> * must return %-EINVAL. In this case \*num_buffers are >>> * being allocated additionally to q->num_buffers. >>> >>> That's really really ambiguous, because the "In this case" part doesn't >>> really tell you which case it's talking about - but it seems to me looking >>> at the code that it's referring to the VIDIOC_CREATE_BUFS case. >> >> Yes, I caught this when adding fixes from v4l2-compliance testing, which >> is not part of the version 3 driver. I agree it is a confusing API. When >> called from VIDIOC_CREATE_BUFS (indicated by *num_planes != 0), >> *num_buffers is supposed to be requested buffers _in addition_ to >> already requested q->num_buffers, which is important info and >> should be emphasized a little more than the "oh by the way" fashion >> in the prototype description, IMHO. > > Hans, Sakari, any opinion ? This certainly could be improved. The total number of buffers is q->num_buffers + *num_buffer where q->num_buffers is 0 when called from VIDIOC_REQBUFS and can be > 0 when called from VIDIOC_CREATEBUFS. So if you want to ensure that a certain minimum number of buffers is allocated, then you would code that as: if (q->num_buffers + *num_buffers < 3) *num_buffers = 3 - q->num_buffers; Regards, Hans