| Message ID | 29c41a6823d813e8b00e18f9397470a42347f1b3.1725285495.git.hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: vb2: prepare for vb2_ops_wait_prepare/finish removal | expand |
Hi Hans, Thank you for the patch. On Mon, Sep 02, 2024 at 04:04:55PM +0200, Hans Verkuil wrote: > If the wait_prepare or wait_finish callback is set, then call it. > If it is NULL and the queue lock pointer is not NULL, then just > unlock/lock that mutex. > > This allows simplifying drivers by dropping the wait_prepare and > wait_finish ops (and eventually the vb2_ops_wait_prepare/finish helpers). > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 6335ac7b771a..d064e0664851 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -2035,7 +2035,10 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) > * become ready or for streamoff. Driver's lock is released to > * allow streamoff or qbuf to be called while waiting. > */ > - 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); How about calling vb2_ops_wait_prepare() here ? I think that would be more explicit. You would likely need to move the function to videobuf2-core.c. Same below for wait_finish. > > /* > * All locks have been released, it is safe to sleep now. > @@ -2045,12 +2048,16 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) > !list_empty(&q->done_list) || !q->streaming || > q->error); > > + if (q->ops->wait_finish) > + call_void_qop(q, wait_finish, q); > + else if (q->lock) > + mutex_lock(q->lock); > + > + q->waiting_in_dqbuf = 0; > /* > * We need to reevaluate both conditions again after reacquiring > * the locks or return an error if one occurred. > */ > - call_void_qop(q, wait_finish, q); > - q->waiting_in_dqbuf = 0; > if (ret) { > dprintk(q, 1, "sleep was interrupted\n"); > return ret;
On 09/09/2024 17:05, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Mon, Sep 02, 2024 at 04:04:55PM +0200, Hans Verkuil wrote: >> If the wait_prepare or wait_finish callback is set, then call it. >> If it is NULL and the queue lock pointer is not NULL, then just >> unlock/lock that mutex. >> >> This allows simplifying drivers by dropping the wait_prepare and >> wait_finish ops (and eventually the vb2_ops_wait_prepare/finish helpers). >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/common/videobuf2/videobuf2-core.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 6335ac7b771a..d064e0664851 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -2035,7 +2035,10 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) >> * become ready or for streamoff. Driver's lock is released to >> * allow streamoff or qbuf to be called while waiting. >> */ >> - 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); > > How about calling vb2_ops_wait_prepare() here ? I think that would be > more explicit. You would likely need to move the function to > videobuf2-core.c. Same below for wait_finish. Why would I call a function that does exactly this? It is just harder to see what it does since you need to look up that function. Just in case it wasn't clear: once this series is in I can start removing vb2_ops_wait_prepare/finish from all media drivers, and eventually remove those two helpers from vb2. Regards, Hans > >> >> /* >> * All locks have been released, it is safe to sleep now. >> @@ -2045,12 +2048,16 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) >> !list_empty(&q->done_list) || !q->streaming || >> q->error); >> >> + if (q->ops->wait_finish) >> + call_void_qop(q, wait_finish, q); >> + else if (q->lock) >> + mutex_lock(q->lock); >> + >> + q->waiting_in_dqbuf = 0; >> /* >> * We need to reevaluate both conditions again after reacquiring >> * the locks or return an error if one occurred. >> */ >> - call_void_qop(q, wait_finish, q); >> - q->waiting_in_dqbuf = 0; >> if (ret) { >> dprintk(q, 1, "sleep was interrupted\n"); >> return ret; >
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 6335ac7b771a..d064e0664851 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -2035,7 +2035,10 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) * become ready or for streamoff. Driver's lock is released to * allow streamoff or qbuf to be called while waiting. */ - 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); /* * All locks have been released, it is safe to sleep now. @@ -2045,12 +2048,16 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) !list_empty(&q->done_list) || !q->streaming || q->error); + if (q->ops->wait_finish) + call_void_qop(q, wait_finish, q); + else if (q->lock) + mutex_lock(q->lock); + + q->waiting_in_dqbuf = 0; /* * We need to reevaluate both conditions again after reacquiring * the locks or return an error if one occurred. */ - call_void_qop(q, wait_finish, q); - q->waiting_in_dqbuf = 0; if (ret) { dprintk(q, 1, "sleep was interrupted\n"); return ret;
If the wait_prepare or wait_finish callback is set, then call it. If it is NULL and the queue lock pointer is not NULL, then just unlock/lock that mutex. This allows simplifying drivers by dropping the wait_prepare and wait_finish ops (and eventually the vb2_ops_wait_prepare/finish helpers). Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/common/videobuf2/videobuf2-core.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)