Re: [PATCH v3 00/24] i.MX Media Driver

[Steve Longerbeam <slongerbeam@gmail.com>]

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;
}

>