> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx> > Sent: Friday, 25 October 2024 1.21 > To: Taylor, Clinton A <clinton.a.taylor@xxxxxxxxx> > Cc: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > 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@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. 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@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
