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@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Clint > > > > Taylor > > > > Sent: Thursday, 24 October 2024 0.47 > > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx > > > > 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@xxxxxxxxx> > > > > > > > Signed-off-by: Clint Taylor <clinton.a.taylor@xxxxxxxxx> > > > > --- > > > > 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
