[PATCH v3 00/24] i.MX Media Driver
Steve Longerbeam
slongerbeam at gmail.com
Thu Feb 2 19:12:41 UTC 2017
On 02/02/2017 10:58 AM, Russell King - ARM Linux wrote:
> On Thu, Feb 02, 2017 at 10:26:55AM -0800, Steve Longerbeam wrote:
>> On 02/02/2017 09:56 AM, Russell King - ARM Linux wrote:
>>> and for whatever reason we end up falling out through free_ring. This
>>> is VERY bad news, because it means that the ring which SMFC took a copy
>>> of is now freed beneath its feet.
>> Yes, that is bad. That was a bug, if imx_media_dma_buf_queue_from_vb()
>> returned error, the ring should not have been freed, it should have only
>> returned the error. And further bad stuff happens from that point on.
>>
>> But all of this is gone in version 4.
> I think there's an error in how you think the queue_setup() works.
>
> camif_queue_setup() always returns the number of buffers between
> IMX_MEDIA_MIN_RING_BUFS and IMX_MEDIA_MAX_RING_BUFS. However, it seems
> that, looking through the videobuf2-core.c code, that the value is
> passed to __vb2_queue_alloc() to allocate the specified number of
> _additional_ buffers over and on-top of the existing q->num_buffers:
>
> static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> unsigned int num_buffers, unsigned int num_planes,
> const unsigned plane_sizes[VB2_MAX_PLANES])
> {
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> ...
> vb->index = q->num_buffers + buffer;
>
> and
>
> int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> unsigned int *count)
> {
> unsigned int num_buffers, allocated_buffers, num_planes = 0;
> ...
> num_buffers = min_t(unsigned int, *count, VB2_MAX_FRAME);
> num_buffers = max_t(unsigned int, num_buffers, q->min_buffers_needed);
> ...
> /*
> * Ask the driver how many buffers and planes per buffer it requires.
> * Driver also sets the size and allocator context for each plane.
> */
> ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes,
> plane_sizes, q->alloc_devs);
> if (ret)
> return ret;
>
> /* Finally, allocate buffers and video memory */
> allocated_buffers =
> __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
>
> or:
>
> int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> unsigned int *count, unsigned requested_planes,
> const unsigned requested_sizes[])
> {
> unsigned int num_planes = 0, num_buffers, allocated_buffers;
> ...
> num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes;
> ...
> /*
> * Ask the driver, whether the requested number of buffers, planes per
> * buffer and their sizes are acceptable
> */
> ret = call_qop(q, queue_setup, q, &num_buffers,
> &num_planes, plane_sizes, q->alloc_devs);
> if (ret)
> return ret;
>
> /* Finally, allocate buffers and video memory */
> allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
> num_planes, plane_sizes);
>
>
> 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.
>
> As you support both .vidioc_create_bufs and .vidioc_reqbufs, it seems
> to me that you're not handling the VIDIOC_CREATE_BUFS case correctly.
>
> Can you please make sure that your next version resolves that?
Here is the current .queue_setup() op in imx-media-capture.c:
static int capture_queue_setup(struct vb2_queue *vq,
unsigned int *nbuffers,
unsigned int *nplanes,
unsigned int sizes[],
struct device *alloc_devs[])
{
struct capture_priv *priv = vb2_get_drv_priv(vq);
struct v4l2_pix_format *pix = &priv->vdev.fmt.fmt.pix;
unsigned int count = *nbuffers;
if (vq->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
if (*nplanes) {
if (*nplanes != 1 || sizes[0] < pix->sizeimage)
return -EINVAL;
count += vq->num_buffers;
}
while (pix->sizeimage * count > VID_MEM_LIMIT)
count--;
if (*nplanes)
*nbuffers = (count < vq->num_buffers) ? 0 :
count - vq->num_buffers;
else
*nbuffers = count;
*nplanes = 1;
sizes[0] = pix->sizeimage;
return 0;
}
>
More information about the devel
mailing list