| Message ID | 20241023214701.963830-8-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/xe3lpd: ptl display patches | expand |
> -----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. 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
On Wed, 23 Oct 2024, Clint Taylor <clinton.a.taylor@intel.com> wrote: > C10 phy timeouts occur on xe3lpd if the c10 bus is reset every > transaction. Starting with xe3lpd this is bus reset not necessary > > 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)) Drop the extra braces. > + 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)) Ditto. BR, Jani. > + intel_cx0_bus_reset(encoder, lane); > > return 0; > }
On Thu, 24 Oct 2024, "Kahola, Mika" <mika.kahola@intel.com> 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. > > Reviewed-by: Mika Kahola <mika.kahola@intel.com> Why no mention of the extra braces in there? BR, Jani. > >> 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 >
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? 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 >
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
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@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. 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@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
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Friday, 25 October 2024 1.21 > To: Taylor, Clinton A <clinton.a.taylor@intel.com> > Cc: Kahola, Mika <mika.kahola@intel.com>; intel-xe@lists.freedesktop.org; intel- > gfx@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@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. > > 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@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 > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
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; }
C10 phy timeouts occur on xe3lpd if the c10 bus is reset every transaction. Starting with xe3lpd this is bus reset not necessary 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(-)