RE: [PATCH v1 7/7] media: chips-media: wave5: Resume device before setting EOS flag

[jackson . lee]

Hi Nicolas

> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> Sent: Thursday, July 16, 2026 11:19 AM
> To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> hverkuil-cisco@xxxxxxxxx; bob.beckett@xxxxxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; lafley.kim
> <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx; hverkuil@xxxxxxxxx; Nas
> Chung <nas.chung@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 7/7] media: chips-media: wave5: Resume device
> before setting EOS flag
> 
> Hi,
> 
> Le vendredi 26 juin 2026 à 10:22 +0900, Jackson.lee a écrit :
> > From: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
> >
> > Setting the EOS flag talks to the firmware via
> > send_firmware_command(), which accesses VPU registers. Both the
> > STREAMOFF path
> > (wave5_vpu_dec_job_abort()) and the V4L2_DEC_CMD_STOP path
> > (wave5_vpu_dec_stop()) can run while the device is runtime suspended,
> > so those register accesses hit powered-down hardware and the SoC
> > raises an asynchronous SError, panicking the kernel:
> >
> >   SError Interrupt on CPU3, code 0x00000000bf000000 -- SError
> >    send_firmware_command+0x2c/0x160 [wave5]
> >    wave5_vpu_dec_set_bitstream_flag+0x6c/0x80 [wave5]
> >    wave5_vpu_dec_update_bitstream_buffer+0x80/0xec [wave5]
> >    wave5_vpu_dec_job_abort+0x44/0xa0 [wave5]
> >    v4l2_m2m_cancel_job+0x110/0x19c [v4l2_mem2mem]
> >    v4l2_m2m_streamoff+0x24/0x140 [v4l2_mem2mem]
> >
> > Resume the device with pm_runtime_resume_and_get() around the EOS
> > firmware command and release it with pm_runtime_put_autosuspend(),
> > matching the runtime PM handling already done in
> > wave5_vpu_dec_device_run().
> >
> > Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
> > Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
> > ---
> >  .../chips-media/wave5/wave5-vpu-dec.c         | 19
> > +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > 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 1817b83c5884..6564cf3ec739 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > @@ -823,7 +823,15 @@ static int wave5_vpu_dec_stop(struct vpu_instance
> *inst)
> >  		 * calls do not block on a mutex while inside this spinlock.
> >  		 */
> >  		spin_unlock_irqrestore(&inst->state_spinlock, flags);
> > +		/*
> > +		 * V4L2_DEC_CMD_STOP can arrive while the device is runtime
> > +		 * suspended (e.g. on pipeline teardown). Setting the EOS
> > flag
> > +		 * 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);
> 
> I'll take this one, as it improves things a lot, but please follow-up with
> fixing all the missing return value heck for pm_runtime_resume_and_get().
> 

Thanks for your review.
I will follow up with a separate patch that adds the missing return value for all pm_runtime_resume_and_get() calls.

Thanks
Jackson

> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>



> 
> >  		ret = wave5_vpu_dec_set_eos_on_firmware(inst);
> > +		pm_runtime_put_autosuspend(inst->dev->dev);
> >  		if (ret)
> >  			return ret;
> >
> > @@ -1797,11 +1805,22 @@ static void wave5_vpu_dec_job_abort(void *priv)
> >  	if (ret)
> >  		return;
> >
> > +	/*
> > +	 * job_abort() runs from the STREAMOFF path and may be called while
> > the
> > +	 * device is runtime suspended. Setting the EOS flag talks to the
> > +	 * firmware (send_firmware_command() accesses VPU registers), so
> the
> > +	 * device must be resumed first; otherwise the register access
> faults
> > +	 * with an asynchronous SError.
> > +	 */
> > +	pm_runtime_resume_and_get(inst->dev->dev);
> > +
> >  	ret = wave5_vpu_dec_set_eos_on_firmware(inst);
> >  	if (ret)
> >  		dev_warn(inst->dev->dev,
> >  			 "Setting EOS for the bitstream, fail: %d\n", ret);
> >
> > +	pm_runtime_put_autosuspend(inst->dev->dev);
> > +
> >  	v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
> >  }
> >