[PATCH v2 07/12] drm/i915/cx0: Remove bus reset after every c10 transaction

[Kahola, Mika]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Friday, 25 October 2024 1.21
> To: Taylor, Clinton A <clinton.a.taylor at intel.com>
> Cc: Kahola, Mika <mika.kahola at intel.com>; intel-xe at lists.freedesktop.org; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [PATCH v2 07/12] drm/i915/cx0: Remove bus reset after every c10
> transaction
> 
> On Thu, Oct 24, 2024 at 10:15:11PM +0000, Taylor, Clinton A wrote:
> > On Thu, 2024-10-24 at 12:18 -0700, Matt Roper wrote:
> > > On Thu, Oct 24, 2024 at 06:08:46AM +0000, Kahola, Mika wrote:
> > > > > -----Original Message-----
> > > > > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On
> > > > > Behalf Of Clint Taylor
> > > > > Sent: Thursday, 24 October 2024 0.47
> > > > > To: intel-gfx at lists.freedesktop.org;
> > > > > intel-xe at lists.freedesktop.org
> > > > > Subject: [PATCH v2 07/12] drm/i915/cx0: Remove bus reset after
> > > > > every c10 transaction
> > > > >
> > > > > C10 phy timeouts occur on xe3lpd if the c10 bus is reset every transaction.
> > > > > Starting with xe3lpd this is bus reset not necessary
> > > > >
> > > >
> > > > This C10/C20 bus reset was originally placed as a workaround to prevent
> bus timeouts.
> > > > These timeouts were fixed elsewhere and therefore these are unnecessary
> lines.
> > >
> > > I'm a bit confused by the patch / explanation here.  Before this
> > > patch we did the reset on all platforms, unconditionally.  The code
> > > change below is removing the reset from the existing platforms
> > > (MTL/ARL and
> > > Xe2) but keeping it only on the new Xe3 platforms.
> > >
> > > If the timeout mystery was solved and these resets are no longer
> > > needed, shouldn't we be removing the line completely rather than
> > > making it conditional to the new platforms?  Or do we have now have
> > > new, unexplained failures specifically on Xe3 that requires that we
> > > bring back this hack at the same time we're removing it from the
> > > older platforms?
> > >
> > I reversed the conditional when splitting the c10 patches. Will
> > correct and send a new series.
> 
> Okay, even if the condition got reversed by accident, I'm still unclear on whether
> we truly still need this on pre-Xe3 platforms or not.  Based on Mika's explanation
> is sounds like maybe these lines should simply be getting removed completely, and
> that that's independent of the Xe3 work going on?  We can see what he thinks
> tomorrow.

We could simply remove these intel_cx0_bus_reset() from read/write operations. These were originally added as a workaround as we had read failures from the bus as well as write failures to the bus.

https://patchwork.freedesktop.org/patch/562869/?series=124602&rev=5

The read/write sequence doesn't require bus reset after every read/write operation either.

-Mika-
> 
> 
> Matt
> 
> >
> > -Clint
> >
> > >
> > > Matt
> > >
> > > > Reviewed-by: Mika Kahola <mika.kahola at intel.com>
> > > >
> > > > > Signed-off-by: Clint Taylor <clinton.a.taylor at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > > index c1357bdb8a3b..a8966a7a9927 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > > @@ -224,7 +224,8 @@ static int __intel_cx0_read_once(struct
> > > > > intel_encoder *encoder,
> > > > >  	 * down and let the message bus to end up
> > > > >  	 * in a known state
> > > > >  	 */
> > > > > -	intel_cx0_bus_reset(encoder, lane);
> > > > > +	if ((DISPLAY_VER(i915) >= 30))
> > > > > +		intel_cx0_bus_reset(encoder, lane);
> > > > >
> > > > >  	return REG_FIELD_GET(XELPDP_PORT_P2M_DATA_MASK, val);  } @@ -
> > > > > 313,7 +314,8 @@ static int __intel_cx0_write_once(struct
> > > > > intel_encoder *encoder,
> > > > >  	 * down and let the message bus to end up
> > > > >  	 * in a known state
> > > > >  	 */
> > > > > -	intel_cx0_bus_reset(encoder, lane);
> > > > > +	if ((DISPLAY_VER(i915) >= 30))
> > > > > +		intel_cx0_bus_reset(encoder, lane);
> > > > >
> > > > >  	return 0;
> > > > >  }
> > > > > --
> > > > > 2.25.1
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation