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

[Matt Roper]

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.


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