Re: [PATCH v2 4/7] media: chips-media: wave5: Use spinlock whenever statue is changed

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le mardi 27 mai 2025 à 05:02 +0000, jackson.lee a écrit :
> Hi Nicolas
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Sent: Saturday, May 24, 2025 2:41 AM
> > To: jackson.lee <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> > hverkuil-cisco@xs4all.nl; sebastian.fricke@collabora.com;
> > bob.beckett@collabora.com; dafna.hirschfeld@collabora.com
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; lafley.kim
> > <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; hverkuil@xs4all.nl; Nas
> > Chung <nas.chung@chipsnmedia.com>
> > Subject: Re: [PATCH v2 4/7] media: chips-media: wave5: Use spinlock
> > whenever statue is changed
> > 
> > Hi,
> > 
> > Le jeudi 22 mai 2025 à 16:26 +0900, Jackson.lee a écrit :
> > > From: Jackson Lee <jackson.lee@chipsnmedia.com>
> > > 
> > > The device_run and finish_decode is not any more synchronized, so lock
> > > was needed in the device_run whenever state was changed.
> > 
> > Can you try to introduce the locking ahead of the patches, otherwise this
> > break bisectability as the in-between become racy.
> 
> 
> Do you want to introduce this patch ahead of the performance patch?

I'm sure you can find the right anwser if you understand why I'm asking this. The way
patchset should be layout is so that at any step applying the set, the driver
should remain stable. If one patch above breaks something, and you fix it in the
next patch, this is not a bisectable set.

git bisect does not know about "sets" and shouldn't need to know about it either.

regards,
Nicolas

> 
> Thanks
> Jackson
> 
> > 
> > Nicolas
> > 
> > > 
> > > Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
> > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > > ---
> > >  drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > 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
> > > 42981c3b49bc..719c5527eb7f 100644
> > > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > @@ -1577,6 +1577,7 @@ static void wave5_vpu_dec_device_run(void *priv)
> > >  	struct queue_status_info q_status;
> > >  	u32 fail_res = 0;
> > >  	int ret = 0;
> > > +	unsigned long flags;
> > > 
> > >  	dev_dbg(inst->dev->dev, "%s: Fill the ring buffer with new
> > bitstream data", __func__);
> > >  	pm_runtime_resume_and_get(inst->dev->dev);
> > > @@ -1617,7 +1618,9 @@ static void wave5_vpu_dec_device_run(void *priv)
> > >  			}
> > >  			spin_unlock_irqrestore(&inst->state_spinlock, flags);
> > >  		} else {
> > > +			spin_lock_irqsave(&inst->state_spinlock, flags);
> > >  			switch_state(inst, VPU_INST_STATE_INIT_SEQ);
> > > +			spin_unlock_irqrestore(&inst->state_spinlock, flags);
> > >  		}
> > > 
> > >  		break;
> > > @@ -1628,8 +1631,9 @@ static void wave5_vpu_dec_device_run(void *priv)
> > >  		 * we had a chance to switch, which leads to an invalid state
> > >  		 * change.
> > >  		 */
> > > +		spin_lock_irqsave(&inst->state_spinlock, flags);
> > >  		switch_state(inst, VPU_INST_STATE_PIC_RUN);
> > > -
> > > +		spin_unlock_irqrestore(&inst->state_spinlock, flags);
> > >  		/*
> > >  		 * During DRC, the picture decoding remains pending, so just
> > leave the job
> > >  		 * active until this decode operation completes.
> > > @@ -1643,7 +1647,9 @@ static void wave5_vpu_dec_device_run(void *priv)
> > >  		ret = wave5_prepare_fb(inst);
> > >  		if (ret) {
> > >  			dev_warn(inst->dev->dev, "Framebuffer preparation,
> > fail: %d\n",
> > > ret);
> > > +			spin_lock_irqsave(&inst->state_spinlock, flags);
> > >  			switch_state(inst, VPU_INST_STATE_STOP);
> > > +			spin_unlock_irqrestore(&inst->state_spinlock, flags);
> > >  			break;
> > >  		}
> > >