[9/9] media: vb2: use lock if wait_prepare/finish are NULL
Commit Message
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(-)
Comments
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;
>
@@ -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;