Re: [RFCv3 PATCH 05/10] vb2: fix buf_init/buf_cleanup call sequences

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 13, 2014 at 6:40 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> Ensure that these ops are properly balanced.
>
> There two scenarios:

s/There two/There are two/

>
> 1) for MMAP buf_init is called when the buffers are created and buf_cleanup
>    must be called when the queue is finally freed. This scenario was always
>    working.
>
> 2) for USERPTR and DMABUF it is more complicated. When a buffer is queued
>    the code checks if all planes of this buffer have been acquired before.
>    If that's the case, then only buf_prepare has to be called. Otherwise
>    buf_clean needs to be called if the buffer was acquired before, then,

s/buf_clean/buf_cleanup/

>    once all changed planes have been (re)acquired, buf_init has to be
>    called again followed by buf_prepare. Should buf_prepare fail, then

s/again//

I know what you mean, but there is only one call to buf_init in this
particular sequence.

>    buf_cleanup must be called again because all planes will be released

s/again because all planes will be released/on the newly acquired
planes to release them/

>    in case of an error.
>
> Finally, in __vb2_queue_free we have to check if the buffer was actually
> acquired before calling buf_cleanup. While that it always true for MMAP
> mode, it is not necessarily true for the other modes. E.g. if you just
> call REQBUFS and close the filehandle, then buffers were ever queued and

s/ever/never/
s/filehandle/file handle/

> so no buf_init was ever called.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 100 +++++++++++++++++++++----------
>  1 file changed, 67 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 3756378..7766bf5 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -373,8 +373,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>         /* Call driver-provided cleanup function for each buffer, if provided */
>         for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
>              ++buffer) {
> -               if (q->bufs[buffer])
> -                       call_vb_qop(q->bufs[buffer], buf_cleanup, q->bufs[buffer]);
> +               struct vb2_buffer *vb = q->bufs[buffer];
> +
> +               if (vb && vb->planes[0].mem_priv)
> +                       call_vb_qop(vb, buf_cleanup, vb);
>         }
>
>         /* Release video buffer memory */
> @@ -1161,6 +1163,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>         unsigned int plane;
>         int ret;
>         int write = !V4L2_TYPE_IS_OUTPUT(q->type);
> +       bool reacquired = vb->planes[0].mem_priv == NULL;

This requires a comment I feel. In general, I'm not especially happy
with the fact that we are making mem_priv != NULL equivalent to
buf_init() called and succeeded. It is true right now, but it'll be
very hard to make the association without previously seeing this very
patch I feel.

I don't see a perfect way to do this, but I'm strongly leaning towards
having a bool inited in the vb2_buffer. Really.

>
>         /* Copy relevant information provided by the userspace */
>         __fill_vb2_buffer(vb, b, planes);
> @@ -1186,12 +1189,16 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                 }
>
>                 /* Release previously acquired memory if present */
> -               if (vb->planes[plane].mem_priv)
> +               if (vb->planes[plane].mem_priv) {
> +                       if (!reacquired) {
> +                               reacquired = true;
> +                               call_vb_qop(vb, buf_cleanup, vb);
> +                       }
>                         call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> +               }
>
>                 vb->planes[plane].mem_priv = NULL;
> -               vb->v4l2_planes[plane].m.userptr = 0;
> -               vb->v4l2_planes[plane].length = 0;
> +               memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
>
>                 /* Acquire each plane's memory */
>                 mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
> @@ -1208,23 +1215,34 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>         }
>
>         /*
> -        * Call driver-specific initialization on the newly acquired buffer,
> -        * if provided.
> -        */
> -       ret = call_vb_qop(vb, buf_init, vb);
> -       if (ret) {
> -               dprintk(1, "qbuf: buffer initialization failed\n");
> -               fail_vb_qop(vb, buf_init);
> -               goto err;
> -       }
> -
> -       /*
>          * Now that everything is in order, copy relevant information
>          * provided by userspace.
>          */
>         for (plane = 0; plane < vb->num_planes; ++plane)
>                 vb->v4l2_planes[plane] = planes[plane];
>
> +       if (reacquired) {l
> +               /*
> +                * One or more planes changed, so we must call buf_init to do
> +                * the driver-specific initialization on the newly acquired
> +                * buffer, if provided.
> +                */
> +               ret = call_vb_qop(vb, buf_init, vb);
> +               if (ret) {
> +                       dprintk(1, "qbuf: buffer initialization failed\n");
> +                       fail_vb_qop(vb, buf_init);
> +                       goto err;
> +               }
> +       }
> +
> +       ret = call_vb_qop(vb, buf_prepare, vb);
> +       if (ret) {
> +               dprintk(1, "qbuf: buffer preparation failed\n");
> +               fail_vb_qop(vb, buf_prepare);
> +               call_vb_qop(vb, buf_cleanup, vb);
> +               goto err;
> +       }
> +
>         return 0;
>  err:
>         /* In case of errors, release planes that were already acquired */
> @@ -1244,8 +1262,13 @@ err:
>   */
>  static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>  {
> +       int ret;
> +
>         __fill_vb2_buffer(vb, b, vb->v4l2_planes);
> -       return 0;
> +       ret = call_vb_qop(vb, buf_prepare, vb);
> +       if (ret)
> +               fail_vb_qop(vb, buf_prepare);
> +       return ret;
>  }
>
>  /**
> @@ -1259,6 +1282,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>         unsigned int plane;
>         int ret;
>         int write = !V4L2_TYPE_IS_OUTPUT(q->type);
> +       bool reacquired = vb->planes[0].mem_priv == NULL;
>
>         /* Copy relevant information provided by the userspace */
>         __fill_vb2_buffer(vb, b, planes);
> @@ -1294,6 +1318,11 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>
>                 dprintk(1, "qbuf: buffer for plane %d changed\n", plane);
>
> +               if (!reacquired) {
> +                       reacquired = true;
> +                       call_vb_qop(vb, buf_cleanup, vb);
> +               }
> +
>                 /* Release previously acquired memory if present */
>                 __vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
>                 memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
> @@ -1329,23 +1358,33 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>         }
>
>         /*
> -        * Call driver-specific initialization on the newly acquired buffer,
> -        * if provided.
> -        */
> -       ret = call_vb_qop(vb, buf_init, vb);
> -       if (ret) {
> -               dprintk(1, "qbuf: buffer initialization failed\n");
> -               fail_vb_qop(vb, buf_init);
> -               goto err;
> -       }
> -
> -       /*
>          * Now that everything is in order, copy relevant information
>          * provided by userspace.
>          */
>         for (plane = 0; plane < vb->num_planes; ++plane)
>                 vb->v4l2_planes[plane] = planes[plane];
>
> +       if (reacquired) {

This sequence until 'return 0" is an exact same code as in
__qbuf_userptr. Could we extract this?
I'm thinking that we could do this:

- rename __qbuf_userptr and __qbuf_dmabuf to __acquire_userptr/dmabuf
and have them return whether reacquired
- extract the sequence above and call buf_prepare from __buf_prepare
for userptr and dmabuf after calling __acquire_*
- get rid of qbuf_dmabuf, instead just call buf_prepare from
__buf_prepare directly like for useptr and dmabuf

> +               /*
> +                * Call driver-specific initialization on the newly acquired buffer,
> +                * if provided.
> +                */
> +               ret = call_vb_qop(vb, buf_init, vb);
> +               if (ret) {
> +                       dprintk(1, "qbuf: buffer initialization failed\n");
> +                       fail_vb_qop(vb, buf_init);
> +                       goto err;
> +               }
> +       }
> +
> +       ret = call_vb_qop(vb, buf_prepare, vb);
> +       if (ret) {
> +               dprintk(1, "qbuf: buffer preparation failed\n");
> +               fail_vb_qop(vb, buf_prepare);
> +               call_vb_qop(vb, buf_cleanup, vb);

Should we explicitly set mem_priv to NULL here? This is one the issues
I have with using mem_priv, it's hard to reason if it's ok to do
buf_cleanup and expect mem_priv to be NULL here always.
Could we please add that inited bool into vb2_buffer to make it simpler?

> +               goto err;
> +       }
> +
>         return 0;
>  err:
>         /* In case of errors, release planes that were already acquired */
> @@ -1420,11 +1459,6 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>                 ret = -EINVAL;
>         }
>
> -       if (!ret) {
> -               ret = call_vb_qop(vb, buf_prepare, vb);
> -               if (ret)
> -                       fail_vb_qop(vb, buf_prepare);
> -       }
>         if (ret)
>                 dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
>         vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
> --
> 1.8.4.rc3
>



-- 
Best regards,
Pawel Osciak
--
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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux