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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Thu, 17 Oct 2024 17:09:23 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> The vb2_thread is used for DVB support. This will queue and dequeue buffers
> automatically.
> 
> 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 just check if the callback is available, and if not, use
> q->lock, just as __vb2_wait_for_done_vb() does.
> 
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
> I'm just updating this patch, not the others in this series.
> 
> Changes since v1:
> - move the locking code inside the 'if (!threadio->stop)'
> - do the same for vb2_core_qbuf()
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 26 ++++++++++++++-----
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index d064e0664851..d2275c878ea9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -3218,10 +3218,17 @@ static int vb2_thread(void *data)
>  				continue;
>  			prequeue--;
>  		} else {
> -			call_void_qop(q, wait_finish, q);
> -			if (!threadio->stop)
> +			if (!threadio->stop) {
> +				if (q->ops->wait_finish)
> +					call_void_qop(q, wait_finish, q);
> +				else if (q->lock)
> +					mutex_lock(q->lock);
>  				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 +3240,19 @@ 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)
> +		if (!threadio->stop) {
> +			if (q->ops->wait_finish)
> +				call_void_qop(q, wait_finish, q);
> +			else if (q->lock)
> +				mutex_lock(q->lock);
>  			ret = vb2_core_qbuf(q, vb, NULL, NULL);
> -		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);
> +		}
>  		if (ret || threadio->stop)
>  			break;
>  	}



Thanks,
Mauro