Hi Hans,
Thank you for the patch.
On Mon, Oct 14, 2024 at 05:06:28PM +0200, Hans Verkuil wrote:
> For read/write support the vb2_thread is used. This will queue and
> dequeue buffers automatically to provide the read() or write() feature.
>
> It calls wait_finish/prepare around vb2_core_dqbuf() and vb2_core_qbuf(),
> but that assumes all drivers have these ops set. But that will change
> due to commit 88785982a19d ("media: vb2: use lock if wait_prepare/finish
> are NULL").
>
> So instead check if the callback is available, and if not, use q->lock,
> just as __vb2_wait_for_done_vb() does.
>
> It was also used around vb2_core_qbuf(), but VIDIOC_QBUF doesn't
> need this since it doesn't do a blocking wait, so just drop the
> wait_finish/prepare callbacks around vb2_core_qbuf().
>
> Signed-off-by: Hans Verkuil <hverkuil@xxxxxxxxx>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index d064e0664851b26b2da71e0a374c49a2d2c5e217..e9c1d9e3222323be50b3039eb463384a3d558239 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -3218,10 +3218,16 @@ static int vb2_thread(void *data)
> continue;
> prequeue--;
> } else {
> - call_void_qop(q, wait_finish, q);
> + if (q->ops->wait_finish)
> + call_void_qop(q, wait_finish, q);
> + else if (q->lock)
> + mutex_lock(q->lock);
I would still prefer moving vb2_ops_wait_prepare() and
vb2_ops_wait_finish() to videobuf2-core.c and calling the functions
here, instead of locking the mutex directly. I think it would make the
code more readable. I won't block the patch for this, but I think it
would be better.
Also, should we check at queue init time that drivers either set a queue
lock or provide the .wait_prepare() and .wait_finish() operations ?
> if (!threadio->stop)
> ret = vb2_core_dqbuf(q, &index, NULL, 0);
> - call_void_qop(q, wait_prepare, q);
> + if (q->ops->wait_prepare)
> + call_void_qop(q, wait_prepare, q);
> + else if (q->lock)
> + mutex_unlock(q->lock);
> dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret);
> if (!ret)
> vb = vb2_get_buffer(q, index);
> @@ -3233,12 +3239,10 @@ static int vb2_thread(void *data)
> if (vb->state != VB2_BUF_STATE_ERROR)
> if (threadio->fnc(vb, threadio->priv))
> break;
> - call_void_qop(q, wait_finish, q);
> if (copy_timestamp)
> vb->timestamp = ktime_get_ns();
> if (!threadio->stop)
> ret = vb2_core_qbuf(q, vb, NULL, NULL);
> - call_void_qop(q, wait_prepare, q);
> if (ret || threadio->stop)
> break;
> }
>
--
Regards,
Laurent Pinchart
