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] [day] [month] [year] [list]
Message-ID: <20250613140441.GE1002387@ragnatech.se>
Date: Fri, 13 Jun 2025 16:04:41 +0200
From: Niklas Söderlund <niklas.soderlund@...natech.se>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	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

On 2025-06-12 20:05:57 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Jun 12, 2025 at 07:55:57PM +0200, Niklas Söderlund wrote:
> > On 2025-06-12 12:20:13 +0200, Jacopo Mondi wrote:
> > > 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 not up to snuff on the diff between the two sets. But from our
> > discussion on IRC today I dropped all 3 patches from my testing branch
> > and that broke my stress tests (after a few runs). I will keep these
> > three patches in my ISP branch for now.
> 
> Oh! That wasn't expected
> 
> The patch you were carring in your branch before this set
> (commit 93aab3f241dbff9af838af0b46f5a31fa6240b68 in your tree)
> didn't reset the FCPX for VSPX as far as I understand (and I have
> verified it by testing iirc).
> 
> The FCPX reset was guarded (in that patch) by
> 
>         if (pipe->lif)
> 
> and the VSPX pipe doesn't have a LIF, no reset was ever performed..

Indeed. I spent the day running more tests as and found it the fail I 
mentioned was in my tests. They timed out the yavta process did not 
terminate quickly enough after the last buffer was captured.

Running with this (or the old set) reduces this stop time, why I don't 
know. But what is clear is running without these patches is OK and 
everything functions as expected. Sorry for the noise.

> 
> 
> >
> > Just to be clear the stress tests where fine with the old version too,
> > only dropping all 3 reset patches had an negative effect.
> >
> > >
> > > 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
> >
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund