On 14/10/2024 21:15, Laurent Pinchart wrote: > 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. The whole point of this series is to prepare for the removal of the wait_finish/prepare callbacks. So this patch is just a temporary change. Eventually this code will change to just a mutex_lock. > > Also, should we check at queue init time that drivers either set a queue > lock or provide the .wait_prepare() and .wait_finish() operations ? It does that already, from videobuf2-core.c, vb2_core_queue_init(): /* Warn if q->lock is NULL and no custom wait_prepare is provided */ if (WARN_ON(!q->lock && !q->ops->wait_prepare)) return -EINVAL; Regards, Hans > >> 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; >> } >> >