diff mbox series

[9/9] media: vb2: use lock if wait_prepare/finish are NULL

Message ID 29c41a6823d813e8b00e18f9397470a42347f1b3.1725285495.git.hverkuil-cisco@xs4all.nl
State Accepted
Commit 88785982a19daa765e30ab3a605680202cfaee4e
Headers show
Series media: vb2: prepare for vb2_ops_wait_prepare/finish removal | expand

Commit Message

Hans Verkuil Sept. 2, 2024, 2:04 p.m. UTC
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

Hans Verkuil Sept. 10, 2024, 11:12 a.m. UTC | #1
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 mbox series

Patch

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;