Re: [PATCH v18 04/13] media: mediatek: jpeg: Fix buffer completion on multi-core streaming stop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: deadlock in stop_streaming due to vb2_wait_for_all_buffers() before draining software queues
--

commit 6489658be7c3e86842a167c26d82109d2bba7681
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: Fix buffer completion on multi-core streaming stop

This commit calls vb2_wait_for_all_buffers() during stop_streaming to
ensure queued buffers are processed before stopping in multi-core
environments.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 7856a556ce68e..d0fb68bc884b4 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -850,8 +850,12 @@ static struct vb2_v4l2_buffer *mtk_jpeg_buf_remove(struct mtk_jpeg_ctx *ctx,
>  static void mtk_jpeg_enc_stop_streaming(struct vb2_queue *q)
>  {
>  	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  	struct vb2_v4l2_buffer *vb;
>  
> +	if (jpeg->variant->multi_core)
> +		vb2_wait_for_all_buffers(q);
> +

[Severity: High]
Does this blocking wait introduce a deadlock when buffers are only queued
to one side?

In the V4L2 M2M framework, if a userspace application queues a buffer to
only the output or capture queue, the hardware worker is never scheduled
to run. The buffer remains in the driver's software queue.

Because vb2_wait_for_all_buffers() is called unconditionally before the
loop that manually drains the software queue in
mtk_jpeg_enc_stop_streaming(), could it block indefinitely waiting for the
hardware worker to process buffers it cannot reach?

>  	while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
>  		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
>  }
> @@ -859,6 +863,7 @@ static void mtk_jpeg_enc_stop_streaming(struct vb2_queue *q)
>  static void mtk_jpeg_dec_stop_streaming(struct vb2_queue *q)
>  {
>  	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  	struct vb2_v4l2_buffer *vb;
>  
>  	/*
> @@ -866,6 +871,9 @@ static void mtk_jpeg_dec_stop_streaming(struct vb2_queue *q)
>  	 * Before STREAMOFF, we still have to return the old resolution and
>  	 * subsampling. Update capture queue when the stream is off.
>  	 */
> +	if (jpeg->variant->multi_core)
> +		vb2_wait_for_all_buffers(q);
> +

[Severity: High]
This appears to have the exact same deadlock potential in
mtk_jpeg_dec_stop_streaming() as in the encoder.

Since vb2_wait_for_all_buffers() blocks until the owned driver count reaches
zero, won't this block indefinitely when streamoff is called if there are
unprocessed buffers trapped in the software queue?

>  	if (ctx->state == MTK_JPEG_SOURCE_CHANGE &&
>  	    V4L2_TYPE_IS_CAPTURE(q->type)) {
>  		struct mtk_jpeg_src_buf *src_buf;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791614327.git.kyrie.wu@mediatek.com?part=4