linux-kernel - Re: [PATCH 3/3] media: vsp1: Reset FCP for VSPX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <rkihvnry5yybz7bjcbanth4yq7svywicmbhksz4ity6buw52ff@tmxefdhvpgaf>
Date: Thu, 12 Jun 2025 12:20:13 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>, 
	Niklas Söderlund <niklas.soderlund@...natech.se>, linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] media: vsp1: Reset FCP for VSPX

Hi Laurent

On Thu, Jun 12, 2025 at 01:12:15PM +0300, Laurent Pinchart wrote:
> On Thu, Jun 12, 2025 at 11:48:49AM +0200, Jacopo Mondi wrote:
> > On Thu, Jun 12, 2025 at 02:36:11AM +0300, Laurent Pinchart wrote:
> > > On Mon, Jun 09, 2025 at 09:01:44PM +0200, Jacopo Mondi wrote:
> > > > According section "62.3.7.1 "Operation Control Setting
> > >
> > > "According to"
> > >
> > > > IP set VSPX+FCPVX" of the R-Car Gen4 Hardware Manual, FCPX has to
> > > > be reset when stopping the image processing.
> > >
> > > That's only when stopping "image process of VSPX+FCPVX immediately".
> > > Note the "immediately", which involves resetting the VSP too. The code
> > > below waits for the pipeline to stop at the end of the frame. Resetting
> > > the FCP doesn't seem to be required in that case.
> >
> > True, we certainly don't
> >
> > 2. Set value 1 to VSPX register VI6_SRESET.SRST0. VSPX will invoke
> >    termination process immediately.
> >
> > but rather wait for the last frame to complete before stopping the
> > pipeline.
> >
> > Do you think this patch should be dropped ?
>
> I would say so, I don't think there's a need to reset the FCP when
> waiting for the pipeline to stop normally. Or have you noticed issues
> that this patch would solve ?
>

Not really, this comes straight from the upporting of the FCPD reset.

We've been running with an older of this patch that wasn't actually
resetting the FCP and we got no issues. At the same time Niklas has
confirmed running with this version that actually resets the FCP
doesn't introduce regressions.

I'm fine dropping this patch indeed

> > > > Softawre reset the FCPX after the vsp1 pipe has stopped.
> > >
> > > s/Softawre/Software/
> > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>
> > > > ---
> > > >  drivers/media/platform/renesas/vsp1/vsp1_pipe.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > > > index a6e5e10f3ef275c1b081c3d957e6cf356332afce..c6f2417aabc479384012ab8ab99556029ede1f44 100644
> > > > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > > > @@ -499,6 +499,7 @@ bool vsp1_pipeline_stopped(struct vsp1_pipeline *pipe)
> > > >  int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
> > > >  {
> > > >  	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
> > > > +	u32 version = vsp1->version & VI6_IP_VERSION_MODEL_MASK;
> > > >  	struct vsp1_entity *entity;
> > > >  	unsigned long flags;
> > > >  	int ret;
> > > > @@ -515,8 +516,7 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
> > > >  			spin_unlock_irqrestore(&pipe->irqlock, flags);
> > > >  		}
> > > >
> > > > -		if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
> > > > -		    VI6_IP_VERSION_MODEL_VSPD_GEN3)
> > > > +		if (version == VI6_IP_VERSION_MODEL_VSPD_GEN3)
> > > >  			ret |= rcar_fcp_soft_reset(vsp1->fcp);
> > > >
> > > >  	} else {
> > > > @@ -529,6 +529,9 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
> > > >  		ret = wait_event_timeout(pipe->wq, vsp1_pipeline_stopped(pipe),
> > > >  					 msecs_to_jiffies(500));
> > > >  		ret = ret == 0 ? -ETIMEDOUT : 0;
> > > > +
> > > > +		if (version == VI6_IP_VERSION_MODEL_VSPX_GEN4)
> > > > +			ret |= rcar_fcp_soft_reset(vsp1->fcp);
> > > >  	}
> > > >
> > > >  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
>
> --
> Regards,
>
> Laurent Pinchart