On Tue, Jan 20, 2026 at 03:57:56PM +0200, Jani Nikula wrote: > On Tue, 20 Jan 2026, Jani Nikula <[email protected]> wrote: > > On Fri, 16 Jan 2026, Suraj Kandpal <[email protected]> wrote: > >> From: Mika Kahola <[email protected]> > >> > >> Move intel_pps_on() to intel_dpll_mgr PLL enabling > >> .enable function hook to enable panel power earlier. > >> We need to do this to make sure we are following the > >> modeset sequences of Bspec. This had changed when we > >> moved the PLL PHY enablement for CX0 from .enable_clock > >> to dpll.enable hook > > > > So I really hate this. > > > > Yeah, maybe it follows the spec now, but what connection does the DPLL > > manager have with the panel power sequencing? > > > > Absolutely nothing. > > > > The DPLL manager has no business calling PPS functions. > > > > Currently only the g4x and DDI encoder code does PPS power calls, and > > they're the only ones who should manage PPS. > > > >> > >> Signed-off-by: Mika Kahola <[email protected]> > >> Signed-off-by: Suraj Kandpal <[email protected]> > >> --- > >> > >> v2 -> v3: > >> - Rather than splitting the PHY enablement sequence, enable PPS > >> earlier (Imre) > > > > Please point me at the review comment. I couldn't find anything that > > would suggest moving the PPS calls to the DPLL manager. > > > > Please let's not do this. > > Cc: Imre
Yes, moving the intel_pps_on() call from the DDI code to PLL code is not correct and it's not what I suggested (offline). Based on Suraj's and Mika's testing the ordering of intel_pps_on() vs. enabling the PLL did make a difference, avoiding a PLL enabling timeout. This rationale for the change should've been also added to the commit log, as I requested in an earlier version of the patchset addressing the same issue in: https://lore.kernel.org/all/aWeybp1JaC99Rf8L@ideak-desk If bspec is actually correct and PPS must be enabled before enabling the PLL, then the correct way to do that would be moving intel_pps_on() to intel_ddi_pre_pll_enable(). But I have doubt that bspec is correct and this ordering must be followed. As I pointed out in my earlier review comment above, I'm not sure what happens then if the PLL must be in an enabled state already (due to CMTG) before a DDI output using the same PLL is enabled. So I'd prefer more evidence that following the bspec order is actually a requirement (for instance a confirmation about that from HW folks). > > BR, > > Jani. > > > > > >> > >> drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++++-- > >> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 5 +++++ > >> 2 files changed, 9 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > >> b/drivers/gpu/drm/i915/display/intel_ddi.c > >> index cb91d07cdaa6..1784fa687c03 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > >> @@ -2653,8 +2653,10 @@ static void mtl_ddi_pre_enable_dp(struct > >> intel_atomic_state *state, > >> /* 3. Select Thunderbolt */ > >> mtl_port_buf_ctl_io_selection(encoder); > >> > >> - /* 4. Enable Panel Power if PPS is required */ > >> - intel_pps_on(intel_dp); > >> + /* > >> + * 4. Enable Panel Power if PPS is required > >> + * moved to intel_dpll_mgr .enable hook > >> + */ > >> > >> /* 5. Enable the port PLL */ > >> intel_ddi_enable_clock(encoder, crtc_state); > >> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> index 9aa84a430f09..b5655c734c53 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> @@ -40,6 +40,7 @@ > >> #include "intel_hti.h" > >> #include "intel_mg_phy_regs.h" > >> #include "intel_pch_refclk.h" > >> +#include "intel_pps.h" > >> #include "intel_step.h" > >> #include "intel_tc.h" > >> > >> @@ -4401,6 +4402,10 @@ static void mtl_pll_enable(struct intel_display > >> *display, > >> if (drm_WARN_ON(display->drm, !encoder)) > >> return; > >> > >> + /* Enable Panel Power if PPS is required */ > >> + if (intel_encoder_is_dp(encoder)) > >> + intel_pps_on(enc_to_intel_dp(encoder)); > >> + > >> intel_mtl_pll_enable(encoder, pll, dpll_hw_state); > >> } > > -- > Jani Nikula, Intel
