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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
