Hi Hans,
On Mon, Nov 19, 2018 at 8:09 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> .../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
