Re: [PATCH 01/10] media: videobuf2-core: update vb2_thread if wait_finish/prepare are NULL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
>>  	}
>>
> 





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux