Re: [PATCH v0 3/4] media: chips-media: wave5: avoid skipping device_run while VPU has work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Invalid comparison between VPU hardware command count and V4L2 software source buffer count causes decoding stalls for multi-frame bitstream buffers.
- [Medium] Bypassing the queue-empty check allows `queuing_num` to underflow, permanently breaking the driver's job scheduling optimization.
- [Low] Variable shadowing of `q_status` in `wave5_vpu_dec_device_run`.

Pre-existing issues:
- [High] If the hardware ringbuffer lacks space for an incoming source buffer, the buffer is permanently dropped from the feed list.
--

commit 483963c314a78ddc15cfbde8b0ce5a96c14f95c1
Author: Jackson Lee <jackson.lee@chipsnmedia.com>

media: chips-media: wave5: avoid skipping device_run while VPU has work
    
This patch fixes an issue where the decoder stalls because empty_queue is
set to true even when the m2m context has pending commands. It queries the
VPU queue status before deciding whether to skip device_run, ensuring that
commands continue to be issued if the VPU is actively processing.

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> index 01d1368b29656..b7fa19ea2aacb 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -1663,9 +1663,15 @@ static void wave5_vpu_dec_device_run(void *priv)
>  		} else if (!inst->eos &&
>  				inst->queuing_num == 0 &&
>  				inst->state == VPU_INST_STATE_PIC_RUN) {
> -			dev_dbg(inst->dev->dev, "%s: no bitstream for feeding, so skip ", __func__);
> -			inst->empty_queue = true;
> -			goto finish_job_and_return;
> +			struct queue_status_info q_status;

[Severity: Low]
Is this declaration of q_status shadowing an existing variable?

Looking at the top of wave5_vpu_dec_device_run(), q_status is already
declared. Could we reuse the existing variable instead of declaring a new
one here?

> +
> +			wave5_vpu_dec_give_command(inst, DEC_GET_QUEUE_STATUS, &q_status);
> +			if (q_status.instance_queue_count == v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {

[Severity: High]
Does this comparison accurately reflect the remaining work if a single source
buffer contains multiple encoded frames?

The instance_queue_count tracks the number of active commands in the
hardware, while v4l2_m2m_num_src_bufs_ready counts unconsumed userspace
bitstream buffers. If a user queues a single source buffer holding three
frames, num_src_bufs_ready is 1. When the hardware starts processing the
first frame, instance_queue_count becomes 1.

If they are equal, this code sets empty_queue to true and stops scheduling.
When the hardware finishes the first frame, instance_queue_count drops to 0,
but num_src_bufs_ready remains 1 because the buffer is only partially
consumed.

Can this cause the remaining frames in the buffer to permanently stall since
the worker might refuse to schedule until empty_queue is cleared by
another buffer?

[Severity: Medium]
What happens to the queuing_num counter if this condition falls through?

If instance_queue_count is not equal to num_src_bufs_ready, the block falls
through and the PIC_RUN state handling unconditionally calls start_decode().
If the VPU accepts the command, the routine executes:

	if (fail_res == WAVE5_SYSERR_QUEUEING_FAIL) {
		...
	} else {
		inst->retry = false;
		if (!inst->eos)
			inst->queuing_num--;
	}

Since we only entered this block because inst->queuing_num was 0, would this
decrement cause queuing_num to underflow to -1?

Because it is signed, it would remain negative indefinitely, potentially
breaking the idle tracking logic and causing the driver to repeatedly issue
start_decode() commands without bitstream data.

> +				dev_dbg(inst->dev->dev, "%s: no bitstream, skip\n",
> +					__func__);
> +				inst->empty_queue = true;
> +				goto finish_job_and_return;
> +			}
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the fill_ringbuffer()
routine leak source buffers when the ringbuffer is full?

Looking at fill_ringbuffer() in the same file:

	while ((vpu_buf = inst_src_buf_remove(inst)) != NULL) {
		...
		if (remain_size < src_size) {
			dev_dbg(inst->dev->dev,
				"%s: remaining size: %zu < source size: %zu for src buf (%u)\n",
				__func__, remain_size, src_size, vbuf->vb2_buf.index);
			break;
		}
		...
	}

The function inst_src_buf_remove() takes the buffer off the available list.
When remain_size is less than src_size, the loop breaks out early, but the
buffer doesn't seem to be added back to the available list.

Will this permanently drop the buffer from the feed list, leading to lost
bitstream chunks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604020155.77-1-jackson.lee@chipsnmedia.com?part=3