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

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

Em Mon, 14 Oct 2024 17:06:28 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> 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@xs4all.nl>
> ---
>  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);
>  			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);

Looks OK to me.

> @@ -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)

This hunk looks suspicious on my eyes.

Why this is not needed?

Had you test with all vb2 callers including the DVB one?

If this is fixing a pre-existing condition, which seems the
case, please place it on a separate patch, clearly stating
why we need to drop an old code.

An alternative would be, for now, apply the same change as you
did at the first hunk, e. g.:

	if (q->ops->wait_finish)
		call_void_qop(q, wait_finish, q);
	else if (q->lock)
		mutex_lock(q->lock);

	...
	if (q->ops->wait_prepare)
		call_void_qop(q, wait_prepare, q);
	else if (q->lock)
		mutex_unlock(q->lock);

And only drop those when we're certain that none of the devices
would break with such change.

Once patch 1 is fixed, feel free to add my:

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

for patches 2-10.


Thanks,
Mauro