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

["Taylor, Clinton A" <clinton.a.taylor@intel.com>]

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@lists.freedesktop.org> On Behalf Of Clint
> > > Taylor
> > > Sent: Thursday, 24 October 2024 0.47
> > > To: intel-gfx@lists.freedesktop.org; intel-xe@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.

-Clint

> 
> Matt
> 
> > Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> > 
> > > Signed-off-by: Clint Taylor <clinton.a.taylor@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