Re:Re: [PATCH v3 1/4] media: wave5: handle remaining decoder runtime resume failures

[jiale yao]

Hi Brandon,

At 2026-10-22 23:29:23, "Brandon Brnich" <b-brnich@xxxxxx> wrote:
>Hi Jiale
>
>Thanks for the patch.
>
>On 10/21/26 10:15, Jiale Yao wrote:
>> Several decoder callbacks continue into firmware commands after
>> pm_runtime_resume_and_get() fails. The runtime resume callback can fail
>> while enabling the VPU clocks, so those commands may access registers
>> while the device is unavailable. The matching runtime PM puts can also
>> be issued without a reference.
>> 
>> Check the remaining resume results. Propagate errors from callbacks that
>> can return them, complete buffers or jobs from void callbacks, and avoid
>> firmware access and unmatched puts on failure.
>> 
>> Fixes: 2092b3833487 ("media: chips-media: wave5: Support runtime suspend/resume")
>> Fixes: cbb9c0d50e47 ("media: chips-media: wave5: Fix SError of kernel panic when closed")
>> Fixes: a52e6f7923c1 ("media: chips-media: wave5: Resume device before setting EOS flag")
>> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
>> ---
>>   .../chips-media/wave5/wave5-vpu-dec.c         | 47 +++++++++++++++++--
>>   1 file changed, 42 insertions(+), 5 deletions(-)
>> 
>> 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 86bc36ee17..320b40581b 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> @@ -829,7 +829,10 @@ static int wave5_vpu_dec_stop(struct vpu_instance *inst)
>>   		 * accesses VPU registers via send_firmware_command(), so the
>>   		 * device must be resumed first to avoid an asynchronous SError.
>>   		 */
>> -		pm_runtime_resume_and_get(inst->dev->dev);
>> +		ret = pm_runtime_resume_and_get(inst->dev->dev);
>> +		if (ret < 0)
>> +			return ret;
>> +
>>   		ret = wave5_vpu_dec_set_eos_on_firmware(inst);
>>   		pm_runtime_put_autosuspend(inst->dev->dev);
>>   		if (ret)
>> @@ -1392,7 +1395,11 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>>   	int ret = 0;
>>   
>>   	dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
>> -	pm_runtime_resume_and_get(inst->dev->dev);
>> +	ret = pm_runtime_resume_and_get(inst->dev->dev);
>> +	if (ret < 0) {
>> +		wave5_return_bufs(q, VB2_BUF_STATE_QUEUED);
>> +		return ret;
>> +	}
>>   
>>   	v4l2_m2m_update_start_streaming_state(m2m_ctx, q);
>>   
>> @@ -1556,9 +1563,28 @@ static void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
>>   	struct vpu_instance *inst = vb2_get_drv_priv(q);
>>   	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
>>   	unsigned long timeout;
>> +	int ret;
>>   
>>   	dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
>> -	pm_runtime_resume_and_get(inst->dev->dev);
>> +	ret = pm_runtime_resume_and_get(inst->dev->dev);
>> +	if (ret < 0) {
>> +		if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>> +			struct vpu_src_buffer *vpu_buf;
>> +
>> +			inst->retry = false;
>> +			inst->queuing_num = 0;
>> +			while ((vpu_buf = inst_src_buf_remove(inst)) != NULL)
>> +				;
>> +			inst->eos = false;
>> +		}
>> +
>> +		v4l2_m2m_update_stop_streaming_state(m2m_ctx, q);
>> +		wave5_return_bufs(q, VB2_BUF_STATE_ERROR);
>> +		inst->empty_queue = false;
>> +		inst->sent_eos = false;
>> +		return;
>> +	}
>> +
>>   	inst->empty_queue = true;
>>   
>>   	timeout = jiffies + msecs_to_jiffies(VPU_DEC_STOP_TIMEOUT);
>> @@ -1675,7 +1701,13 @@ static void wave5_vpu_dec_device_run(void *priv)
>>   	bool cmd_issued = false;
>>   
>>   	dev_dbg(inst->dev->dev, "%s: Fill the ring buffer with new bitstream data", __func__);
>> -	pm_runtime_resume_and_get(inst->dev->dev);
>> +	ret = pm_runtime_resume_and_get(inst->dev->dev);
>> +	if (ret < 0) {
>> +		dev_err(inst->dev->dev, "Failed to resume VPU: %d\n", ret);
>> +		v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
>> +		return;
>> +	}
>> +
>
>Wouldn't this lead to an infinite loop in the m2m scheduler? looking 
>around other drivers (rockchip and hantro) they handle this case with 
>failing the buffers and then marking the job as finished. Similar to 
>what you did in the encoder path. Maybe you can make a helper so both 
>device runs can use?

Thanks for your review, how about this one

https://lore.kernel.org/all/20261023092804.673980-1-yaojiale02@xxxxxxx/

>
>Best,
>Brandon
>
>>   	if (!inst->retry) {
>>   		ret = fill_ringbuffer(inst);
>>   		if (ret < 0) {
>> @@ -1818,7 +1850,11 @@ static void wave5_vpu_dec_job_abort(void *priv)
>>   	 * device must be resumed first; otherwise the register access faults
>>   	 * with an asynchronous SError.
>>   	 */
>> -	pm_runtime_resume_and_get(inst->dev->dev);
>> +	ret = pm_runtime_resume_and_get(inst->dev->dev);
>> +	if (ret < 0) {
>> +		dev_warn(inst->dev->dev, "Failed to resume VPU: %d\n", ret);
>> +		goto finish_job;
>> +	}
>>   
>>   	ret = wave5_vpu_dec_set_eos_on_firmware(inst);
>>   	if (ret)
>> @@ -1827,6 +1863,7 @@ static void wave5_vpu_dec_job_abort(void *priv)
>>   
>>   	pm_runtime_put_autosuspend(inst->dev->dev);
>>   
>> +finish_job:
>>   	v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
>>   }
>>   

Jiale