On Thu, Nov 06, 2025 at 03:12:34PM +0200, Ville Syrjälä wrote: > On Thu, Nov 06, 2025 at 12:03:28PM +0000, Kandpal, Suraj wrote: > > > Subject: Re: [PATCH 5/7] drm/i915/ltphy: Nuke bogus weird timeouts > > > > > > On Wed, 05 Nov 2025, Ville Syrjala <[email protected]> wrote: > > > > From: Ville Syrjälä <[email protected]> > > > > > > > > The LT PHY code is abusing intel_de_wait_custom() in all kinds of > > > > weird ways. Get rid of the weird slow timeouts. If these are actually > > > > needed then the fast timeouts should really be specified as the > > > > default 2 microscond or something. > > > > > > > > This will let us eventually nuke intel_de_wait_custom() and convert > > > > over to poll_timeout_us(). > > > > > > > > Signed-off-by: Ville Syrjälä <[email protected]> > > > > > > Suraj, any input here? > > > > > > Reviewed-by: Jani Nikula <[email protected]> > > > > > > > --- > > > > drivers/gpu/drm/i915/display/intel_lt_phy.c | 11 +++++------ > > > > drivers/gpu/drm/i915/display/intel_lt_phy_regs.h | 1 - > > > > 2 files changed, 5 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.c > > > > b/drivers/gpu/drm/i915/display/intel_lt_phy.c > > > > index 6fb68157b322..cc1d6b7a7de4 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_lt_phy.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_lt_phy.c > > > > @@ -1178,10 +1178,9 @@ intel_lt_phy_lane_reset(struct intel_encoder > > > *encoder, > > > > if (intel_de_wait_custom(display, XELPDP_PORT_CLOCK_CTL(display, > > > port), > > > > XELPDP_LANE_PCLK_PLL_ACK(0), > > > > XELPDP_LANE_PCLK_PLL_ACK(0), > > > > - XE3PLPD_MACCLK_TURNON_LATENCY_US, > > > > - XE3PLPD_MACCLK_TURNON_LATENCY_MS, > > > NULL)) > > > > + XE3PLPD_MACCLK_TURNON_LATENCY_US, 0, > > > NULL)) > > > > drm_warn(display->drm, "PHY %c PLL MacCLK assertion Ack > > > not done after %dus.\n", > > > > - phy_name(phy), > > > XE3PLPD_MACCLK_TURNON_LATENCY_MS * 1000); > > > > + phy_name(phy), > > > XE3PLPD_MACCLK_TURNON_LATENCY_US); > > > > According to Bspec: 74499 > > Latency can be either 21us for 1ms depending on what port is connected. > > > > > > > > > > intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, port), > > > > XELPDP_FORWARD_CLOCK_UNGATE, > > > > @@ -1192,7 +1191,7 @@ intel_lt_phy_lane_reset(struct intel_encoder > > > > *encoder, > > > > > > > > if (intel_de_wait_custom(display, XELPDP_PORT_BUF_CTL2(display, > > > port), > > > > lane_phy_current_status, 0, > > > > - XE3PLPD_RESET_END_LATENCY_US, 2, NULL)) > > > > + XE3PLPD_RESET_END_LATENCY_US, 0, NULL)) > > > > Bspec : 74499 > > Says 200us but 2ms (1.5ms to be precise) was the actual time we found the > > this to work properly > > > > > > > > drm_warn(display->drm, > > > > "PHY %c failed to bring out of Lane reset after > > > %dus.\n", > > > > phy_name(phy), > > > XE3PLPD_RESET_END_LATENCY_US); @@ -1674,7 +1673,7 > > > > @@ void intel_lt_phy_pll_enable(struct intel_encoder *encoder, > > > > if (intel_de_wait_custom(display, > > > XELPDP_PORT_CLOCK_CTL(display, port), > > > > XELPDP_LANE_PCLK_PLL_ACK(0), > > > > XELPDP_LANE_PCLK_PLL_ACK(0), > > > > - > > > XE3PLPD_MACCLK_TURNON_LATENCY_US, 2, NULL)) > > > > + > > > XE3PLPD_MACCLK_TURNON_LATENCY_US, 0, NULL)) > > > > Ditto here. > > > > > > drm_warn(display->drm, "PHY %c PLL MacCLK Ack > > > assertion Timeout after %dus.\n", > > > > phy_name(phy), > > > XE3PLPD_MACCLK_TURNON_LATENCY_US); > > > > > > > > @@ -1702,7 +1701,7 @@ void intel_lt_phy_pll_enable(struct intel_encoder > > > *encoder, > > > > /* 16. Poll for PORT_BUF_CTL2 register PHY Pulse Status > > > > = 1 > > > for Owned PHY Lanes. */ > > > > if (intel_de_wait_custom(display, > > > XELPDP_PORT_BUF_CTL2(display, port), > > > > lane_phy_pulse_status, > > > lane_phy_pulse_status, > > > > - > > > XE3PLPD_RATE_CALIB_DONE_LATENCY_US, 2, NULL)) > > > > + > > > XE3PLPD_RATE_CALIB_DONE_LATENCY_US, 0, NULL)) > > > > Ditto here. > > I would suggest giving this a CI run on NVLS before merging this. > > Since you have some idea why these magic numbers were chosen > please redo all of these, and make sure to: > - don't use intel_de_wait_custom() unless absolutely necessary > - if you need to use intel_de_wait_custom() then either > use the default '2,<whatever ms>' or '<whatever us>,0' timeouts > - document all the used timeouts. This is especially important > when they are not directly specified in bspec > > > > > Regards, > > Suraj Kandpal > > > > > > drm_warn(display->drm, "PHY %c PLL rate not > > > changed after %dus.\n", > > > > phy_name(phy), > > > XE3PLPD_RATE_CALIB_DONE_LATENCY_US); > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy_regs.h > > > > b/drivers/gpu/drm/i915/display/intel_lt_phy_regs.h > > > > index 9223487d764e..36abc2bdbd9b 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_lt_phy_regs.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_lt_phy_regs.h > > > > @@ -7,7 +7,6 @@ > > > > #define __INTEL_LT_PHY_REGS_H__ > > > > > > > > #define XE3PLPD_MSGBUS_TIMEOUT_FAST_US 500 > > > > -#define XE3PLPD_MACCLK_TURNON_LATENCY_MS 1 > > > > #define XE3PLPD_MACCLK_TURNON_LATENCY_US 21 > > > > #define XE3PLPD_MACCLK_TURNOFF_LATENCY_US 1 > > > > #define XE3PLPD_RATE_CALIB_DONE_LATENCY_US 50
Oh,and these do not belong in the regs.h file. If you feel the need to have names for these (eg. if they are used multiple times) then just put them in the .c file. -- Ville Syrjälä Intel
