Re: [PATCHv2 3/4] vb2 core: add new queue_setup_lock/unlock ops

[Tomasz Figa <tfiga@chromium.org>]

Hi Hans,

On Mon, Nov 19, 2018 at 8:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> If queue->lock is different from the video_device lock, then
> you need to serialize queue_setup with VIDIOC_S_FMT, and this
> should be done by the driver.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 51 +++++++++++++------
>  include/media/videobuf2-core.h                | 19 +++++++
>  2 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index f7e7e633bcd7..269485920beb 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -465,7 +465,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>          */
>         if (q->num_buffers) {
>                 bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
> -                                 q->cnt_wait_prepare != q->cnt_wait_finish;
> +                                 q->cnt_wait_prepare != q->cnt_wait_finish ||
> +                                 q->cnt_queue_setup_lock != q->cnt_queue_setup_unlock;
>
>                 if (unbalanced || debug) {
>                         pr_info("counters for queue %p:%s\n", q,
> @@ -473,10 +474,14 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>                         pr_info("     setup: %u start_streaming: %u stop_streaming: %u\n",
>                                 q->cnt_queue_setup, q->cnt_start_streaming,
>                                 q->cnt_stop_streaming);
> +                       pr_info("     queue_setup_lock: %u queue_setup_unlock: %u\n",
> +                               q->cnt_queue_setup_lock, q->cnt_queue_setup_unlock);
>                         pr_info("     wait_prepare: %u wait_finish: %u\n",
>                                 q->cnt_wait_prepare, q->cnt_wait_finish);
>                 }
>                 q->cnt_queue_setup = 0;
> +               q->cnt_queue_setup_lock = 0;
> +               q->cnt_queue_setup_unlock = 0;
>                 q->cnt_wait_prepare = 0;
>                 q->cnt_wait_finish = 0;
>                 q->cnt_start_streaming = 0;
> @@ -717,6 +722,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>         num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
>         memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>         q->memory = memory;
> +       call_void_qop(q, queue_setup_lock, q);
>
>         /*
>          * Ask the driver how many buffers and planes per buffer it requires.
> @@ -725,22 +731,27 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>         ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes,
>                        plane_sizes, q->alloc_devs);
>         if (ret)
> -               return ret;
> +               goto unlock;
>
>         /* Check that driver has set sane values */
> -       if (WARN_ON(!num_planes))
> -               return -EINVAL;
> +       if (WARN_ON(!num_planes)) {
> +               ret = -EINVAL;
> +               goto unlock;
> +       }
>
>         for (i = 0; i < num_planes; i++)
> -               if (WARN_ON(!plane_sizes[i]))
> -                       return -EINVAL;
> +               if (WARN_ON(!plane_sizes[i])) {
> +                       ret = -EINVAL;
> +                       goto unlock;
> +               }
>
>         /* Finally, allocate buffers and video memory */
>         allocated_buffers =
>                 __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
>         if (allocated_buffers == 0) {
>                 dprintk(1, "memory allocation failed\n");
> -               return -ENOMEM;
> +               ret = -ENOMEM;
> +               goto unlock;
>         }
>
>         /*
> @@ -775,19 +786,19 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>                  */
>         }
>
> -       mutex_lock(&q->mmap_lock);
>         q->num_buffers = allocated_buffers;

Why is it safe to stop acquiring mmap_lock here? Looking at other
code, I feel like it's assumed that q->num_buffers is actually
protected by it.

> +       call_void_qop(q, queue_setup_unlock, q);
>
>         if (ret < 0) {
>                 /*
>                  * Note: __vb2_queue_free() will subtract 'allocated_buffers'
>                  * from q->num_buffers.
>                  */
> +               mutex_lock(&q->mmap_lock);
>                 __vb2_queue_free(q, allocated_buffers);
>                 mutex_unlock(&q->mmap_lock);
>                 return ret;
>         }
> -       mutex_unlock(&q->mmap_lock);
>
>         /*
>          * Return the number of successfully allocated buffers
> @@ -795,8 +806,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>          */
>         *count = allocated_buffers;
>         q->waiting_for_buffers = !q->is_output;
> -
>         return 0;
> +
> +unlock:
> +       call_void_qop(q, queue_setup_unlock, q);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>
> @@ -813,10 +827,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>                 return -ENOBUFS;
>         }
>
> +       call_void_qop(q, queue_setup_lock, q);
>         if (!q->num_buffers) {
>                 if (q->waiting_in_dqbuf && *count) {
>                         dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> -                       return -EBUSY;
> +                       ret = -EBUSY;
> +                       goto unlock;
>                 }
>                 memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>                 q->memory = memory;
> @@ -837,14 +853,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>         ret = call_qop(q, queue_setup, q, &num_buffers,
>                        &num_planes, plane_sizes, q->alloc_devs);
>         if (ret)
> -               return ret;
> +               goto unlock;
>
>         /* Finally, allocate buffers and video memory */
>         allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
>                                 num_planes, plane_sizes);
>         if (allocated_buffers == 0) {
>                 dprintk(1, "memory allocation failed\n");
> -               return -ENOMEM;
> +               ret = -ENOMEM;
> +               goto unlock;
>         }
>
>         /*
> @@ -869,19 +886,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>                  */
>         }
>
> -       mutex_lock(&q->mmap_lock);
>         q->num_buffers += allocated_buffers;

Ditto.

> +       call_void_qop(q, queue_setup_unlock, q);
>
>         if (ret < 0) {
>                 /*
>                  * Note: __vb2_queue_free() will subtract 'allocated_buffers'
>                  * from q->num_buffers.
>                  */
> +               mutex_lock(&q->mmap_lock);
>                 __vb2_queue_free(q, allocated_buffers);
>                 mutex_unlock(&q->mmap_lock);
>                 return -ENOMEM;
>         }
> -       mutex_unlock(&q->mmap_lock);
>
>         /*
>          * Return the number of successfully allocated buffers
> @@ -890,6 +907,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>         *count = allocated_buffers;
>
>         return 0;
> +
> +unlock:
> +       call_void_qop(q, queue_setup_unlock, q);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 613f22910174..92861b6fe7f8 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -333,6 +333,20 @@ struct vb2_buffer {
>   *                     \*num_buffers are being allocated additionally to
>   *                     q->num_buffers. If either \*num_planes or the requested
>   *                     sizes are invalid callback must return %-EINVAL.
> + * @queue_setup_lock:  called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
> + *                     to serialize @queue_setup with ioctls like
> + *                     VIDIOC_S_FMT() that change the buffer size. Only
> + *                     required if queue->lock differs from the mutex that is
> + *                     used to serialize the ioctls that change the buffer
> + *                     size. This callback should lock the ioctl serialization
> + *                     mutex.
> + * @queue_setup_unlock:        called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
> + *                     to serialize @queue_setup with ioctls like
> + *                     VIDIOC_S_FMT() that change the buffer size. Only
> + *                     required if queue->lock differs from the mutex that is
> + *                     used to serialize the ioctls that change the buffer
> + *                     size. This callback should unlock the ioctl
> + *                     serialization mutex.
>   * @wait_prepare:      release any locks taken while calling vb2 functions;
>   *                     it is called before an ioctl needs to wait for a new
>   *                     buffer to arrive; required to avoid a deadlock in
> @@ -403,10 +417,13 @@ struct vb2_ops {
>         int (*queue_setup)(struct vb2_queue *q,
>                            unsigned int *num_buffers, unsigned int *num_planes,
>                            unsigned int sizes[], struct device *alloc_devs[]);
> +       void (*queue_setup_lock)(struct vb2_queue *q);
> +       void (*queue_setup_unlock)(struct vb2_queue *q);
>
>         void (*wait_prepare)(struct vb2_queue *q);
>         void (*wait_finish)(struct vb2_queue *q);
>
> +

Stray blank line?

Best regards,
Tomasz