> -----Original Message----- > From: Kahola, Mika <[email protected]> > Sent: Thursday, June 11, 2026 12:59 PM > To: Kandpal, Suraj <[email protected]>; intel- > [email protected]; [email protected] > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe dpll_hw_state > compare on LT PHY > > > -----Original Message----- > > From: Kandpal, Suraj <[email protected]> > > Sent: Tuesday, 9 June 2026 13.04 > > To: Kahola, Mika <[email protected]>; > > [email protected]; [email protected] > > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe dpll_hw_state > > compare on LT PHY > > > > > > > > > -----Original Message----- > > > From: Kahola, Mika <[email protected]> > > > Sent: Tuesday, June 9, 2026 2:40 PM > > > To: Kandpal, Suraj <[email protected]>; > > > [email protected]; [email protected] > > > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe > > > dpll_hw_state compare on LT PHY > > > > > > > -----Original Message----- > > > > From: Kandpal, Suraj <[email protected]> > > > > Sent: Tuesday, 9 June 2026 11.33 > > > > To: Kahola, Mika <[email protected]>; > > > > [email protected]; [email protected] > > > > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe > > > > dpll_hw_state compare on LT PHY > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Kahola, Mika <[email protected]> > > > > > Sent: Tuesday, June 9, 2026 1:38 PM > > > > > To: Kandpal, Suraj <[email protected]>; intel- > > > > > [email protected]; [email protected] > > > > > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe > > > > > dpll_hw_state compare on LT PHY > > > > > > > > > > > -----Original Message----- > > > > > > From: Kandpal, Suraj <[email protected]> > > > > > > Sent: Tuesday, 9 June 2026 10.51 > > > > > > To: Kahola, Mika <[email protected]>; > > > > > > [email protected]; > > > > > > [email protected] > > > > > > Cc: Kahola, Mika <[email protected]> > > > > > > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe > > > > > > dpll_hw_state compare on LT PHY > > > > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Kandpal, Suraj > > > > > > > Sent: Tuesday, June 9, 2026 1:13 PM > > > > > > > To: 'Mika Kahola' <[email protected]>; > > > > > > > [email protected]; > > > > > > > [email protected] > > > > > > > Cc: Kahola, Mika <[email protected]> > > > > > > > Subject: RE: [PATCH] drm/i915/display: Skip generic pipe > > > > > > > dpll_hw_state compare on LT PHY > > > > > > > > > > > > > > > Subject: [PATCH] drm/i915/display: Skip generic pipe > > > > > > > > dpll_hw_state compare on LT PHY > > > > > > > > > > > > > > > > LT PHY PLL readout is only partially reliable, and the LT > > > > > > > > PHY code already documents that only a subset of the state > > > > > > > > can be read back reliably after power gating. > > > > > > > > > > > > > > > > The generic pipe-state verification compares dpll_hw_state > > > > > > > > as part of intel_pipe_config_compare(), which can trigger > > > > > > > > false-positive "pipe state doesn't match!" warnings on LT > > > > > > > > PHY > > > platforms. > > > > > > > > DPLL-specific verification already exists via > intel_dpll_state_verify(). > > > > > > > > > > > > > > > > Skip the generic dpll_hw_state pipe-state compare on LT > > > > > > > > PHY platforms and rely on the dedicated DPLL verification path > instead. > > > > > > > > > > > > > > > > Signed-off-by: Mika Kahola <[email protected]> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 ++++++-- > > > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > index 2fa10f858279..85ad2bc4963d 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > @@ -5374,8 +5374,12 @@ intel_pipe_config_compare(const > > > > > > > > struct intel_crtc_state *current_config, > > > > > > > > if (display->dpll.mgr) > > > > > > > > PIPE_CONF_CHECK_P(intel_dpll); > > > > > > > > > > > > > > > > - /* FIXME convert everything over the dpll_mgr */ > > > > > > > > - if (display->dpll.mgr || HAS_GMCH(display)) > > > > > > > > + /* > > > > > > > > + * LT PHY PLL readout is only partially reliable and > > > > > > > > the PLL > state > > > > > > > > + * is already verified via intel_dpll_state_verify(). > > > > > > > > Avoid false > > > > > > > > + * positives from the generic pipe state comparison. > > > > > > > > + */ > > > > > > > > + if ((display->dpll.mgr || HAS_GMCH(display)) &&. > > > > > > > > !HAS_LT_PHY(display)) > > > > > > > > PIPE_CONF_CHECK_PLL(dpll_hw_state); > > > > > > > > > > > > > > intel_lt_phy_pll_compare_hw_state only checks the reliable > > > > > > > state hence we don’t want to add this here config 0 and > > > > > > > config 2 are expected to be reliable > > > > > > > > > > > > > > > > > > > If you are seeing pipe state mismatch on either of these VDR > > > > > > registers then it’s a issue where PHY is not giving use > > > > > > correct value since these two > > > > > register must absolutely be retained by LT PHY. > > > > > > > > > > This error showed up with TBT monitor which yields empty states > > > > > for found and expected states. Since we check in > > > > > verify_single_dpll_state() function PLL state with > > > > > .compare_hw_state hook I think we wouldn't need to check the PLL > state here. > > > > > > > > So .compare_hw_state would call intel_lt_phy_pll_compare_hw_state > > > > Which means for tbt mode it would return early anyways. So we > > > > again wont > > > require this check. > > > > > > > > We already have this bit of code as I mentioned above in the > > > > function > > > > > > > > if (a->tbt_mode || b->tbt_mode) > > > > return true; > > > > > > > It looks like we may be verifying PLL state twice in > intel_modeset_verify_crtc(). > > > > > > verify_crtc_state() -> intel_pipe_config_compare() does > > > PIPE_CONF_CHECK_PLL(dpll_hw_state), which is a generic compare and > > > does not seem to account for the LT PHY/TBT special case. > > > > So PIPE_CONF_CHECK_PLL has the following code #define > > PIPE_CONF_CHECK_PLL(name) do { \ > > if (!intel_dpll_compare_hw_state(display, ¤t_config->name, \ > > &pipe_config->name)) { \ > > ... > > } \ > > } while (0) > > > > bool intel_dpll_compare_hw_state(struct intel_display *display, > > const struct intel_dpll_hw_state *a, > > const struct intel_dpll_hw_state *b) > > { > > if (display->dpll.mgr) > > return display->dpll.mgr->compare_hw_state(a, b); > > ... > > } > > > > And compare_hw_state calls > > > > static bool xe3plpd_compare_hw_state(const struct intel_dpll_hw_state > *_a, > > const struct intel_dpll_hw_state > > *_b) { > > return intel_lt_phy_pll_compare_hw_state(&_a->ltpll, &_b->ltpll); > > } > > > > Hence it is not generic code it gets directed to LT PHY portion of code. > > If you are still seeing a mismatch means someone is not setting the > > tbt_mode. That would be the root cause of any mismatch we see which is > > why removing the CONF_CHECK_PLL() isn’t the correct way to go > > > > Regards, > > Suraj Kandpal > > > > We end up calling intel_lt_phy_pll_compare_hw_state() from both > verify_crtc_state() and intel_dpll_state_verify(). The only difference that I > spotted was that in verify_single_dpll_state() we readout the HW state which > sets the tbt_mode. Maybe that gives a hint why the bug was fixed by this > proposed patch. > > intel_modeset_verify_crtc > ├── verify_crtc_state > │ └── intel_pipe_config_compare > │ └── PIPE_CONF_CHECK_PLL(dpll_hw_state) > │ └── intel_dpll_compare_hw_state > │ └── display->dpll.mgr->compare_hw_state > │ └── xe3plpd_compare_hw_state > │ └── intel_lt_phy_pll_compare_hw_state > └── intel_dpll_state_verify > └── verify_single_dpll_state > └── dpll_mgr->compare_hw_state > └── xe3plpd_compare_hw_state > └── intel_lt_phy_pll_compare_hw_state
Right so I think the issue is we are not using the correct hw state to check tbt_mode so you need to find out how we make sure we are using the correct pll state for verification Rather than just bypassing verify compare state altogether Regards, Suraj Kandpal > > -Mika- > > > > > > > After that, intel_modeset_verify_crtc() also calls > > > intel_dpll_state_verify(), which for LT PHY goes through the > > > .compare_hw_state hook and therefore already applies the platform- > specific handling. > > > > > > Because of that, I wonder if the generic PLL state compare is really > > > needed in this case. At the very least, it would seem that > > > PIPE_CONF_CHECK_PLL() should also handle the TBT case explicitly and > > > return early in that case, in the same way as the LT PHY-specific > .compare_hw_state path does. > > > > > > -Mika- > > > > > > > Regards, > > > > Suraj Kandpal > > > > > > > > > > > > > > -Mika- > > > > > > > > > > > > > > > > > Regards, > > > > > > Suraj Kandpal > > > > > > > > > > > > > Regards, > > > > > > > Suraj Kandpal > > > > > > > > > > > > > > > > > > > > > > > PIPE_CONF_CHECK_X(dsi_pll.ctrl); > > > > > > > > -- > > > > > > > > 2.43.0
