Hi Hans,
Thank you for the patch.
On Friday, 22 September 2017 15:29:22 EEST Hans Verkuil wrote:
> Some controls (ROTATE) change the buffer layout. This means that they cannot
> be set when buffers are allocated. This can be achieved by calling
> v4l2_ctrl_grab(ctrl, true) when the queue becomes busy (i.e. the first
> buffer is allocated) and v4l2_ctrl_grab(ctrl, false) when the queue becomes
> idle (i.e. the last buffer is released).
>
> However, we do not have such queue ops at the moment. This patch adds a
> queue_busy and a queue_idle op.
I wonder if this is the best approach. From a quick look at the patch, and
without seeing drivers using it, I'm worried those new callbacks will be prone
to race conditions with control operations. I think an in-depth analysis of
the issue is needed first, and I'd rather avoid new users of the rotation
control until we come up with a good way to handle it.
> Note that currently queue_busy returns a void, so it assumes that it can't
> return an error.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> Jacob, this has only been compile-tested. It's best if you add this patch to
> your rockchip-rga patch series and test it there.
> ---
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 14f83cecfa92..e8ad0e9f78d2
> 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -665,6 +665,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory, }
>
> if (*count == 0 || q->num_buffers != 0 || q->memory != memory) {
> + num_buffers = q->num_buffers;
> /*
> * We already have buffers allocated, so first check if they
> * are not in use and can be freed.
> @@ -686,6 +687,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory, mutex_unlock(&q->mmap_lock);
> if (ret)
> return ret;
> + if (num_buffers)
> + call_void_qop(q, queue_idle, q);
>
> /*
> * In case of REQBUFS(0) return immediately without calling
> @@ -752,6 +755,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory, */
> }
>
> + call_void_qop(q, queue_busy, q);
> mutex_lock(&q->mmap_lock);
> q->num_buffers = allocated_buffers;
>
> @@ -762,6 +766,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory, */
> __vb2_queue_free(q, allocated_buffers);
> mutex_unlock(&q->mmap_lock);
> + call_void_qop(q, queue_idle, q);
> return ret;
> }
> mutex_unlock(&q->mmap_lock);
> @@ -842,6 +847,8 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum
> vb2_memory memory, */
> }
>
> + if (!q->num_buffers)
> + call_void_qop(q, queue_busy, q);
> mutex_lock(&q->mmap_lock);
> q->num_buffers += allocated_buffers;
>
> @@ -852,6 +859,8 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum
> vb2_memory memory, */
> __vb2_queue_free(q, allocated_buffers);
> mutex_unlock(&q->mmap_lock);
> + if (!q->num_buffers)
> + call_void_qop(q, queue_idle, q);
> return -ENOMEM;
> }
> mutex_unlock(&q->mmap_lock);
> @@ -2011,11 +2020,15 @@ static int __vb2_init_fileio(struct vb2_queue *q,
> int read); static int __vb2_cleanup_fileio(struct vb2_queue *q);
> void vb2_core_queue_release(struct vb2_queue *q)
> {
> + bool was_busy = vb2_is_busy(q);
> +
> __vb2_cleanup_fileio(q);
> __vb2_queue_cancel(q);
> mutex_lock(&q->mmap_lock);
> __vb2_queue_free(q, q->num_buffers);
> mutex_unlock(&q->mmap_lock);
> + if (was_busy)
> + call_void_qop(q, queue_idle, q);
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..3aa039fd29ce 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -315,6 +315,10 @@ 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_busy: called when the queue becomes busy (i.e. at least one
> + * buffer has been allocated).
> + * @queue_idle: called when the last buffer from the queue is released
> + * and the queue is no longer marked busy.
> * @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
> @@ -380,6 +384,8 @@ 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_busy)(struct vb2_queue *q);
> + void (*queue_idle)(struct vb2_queue *q);
>
> void (*wait_prepare)(struct vb2_queue *q);
> void (*wait_finish)(struct vb2_queue *q);
> @@ -547,6 +553,8 @@ struct vb2_queue {
> * called. Used to check for unbalanced ops.
> */
> u32 cnt_queue_setup;
> + u32 cnt_queue_busy;
> + u32 cnt_queue_idle;
> u32 cnt_wait_prepare;
> u32 cnt_wait_finish;
> u32 cnt_start_streaming;
--
Regards,
Laurent Pinchart
