Re: [PATCH v3 1/3] drm/i915/pps: Enable panel power earlier

[Imre Deak <imre.deak@intel.com>]

On Tue, Jan 20, 2026 at 03:57:56PM +0200, Jani Nikula wrote:
> On Tue, 20 Jan 2026, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Fri, 16 Jan 2026, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> >> From: Mika Kahola <mika.kahola@intel.com>
> >>
> >> 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 <mika.kahola@intel.com>
> >> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> ---
> >>
> >> 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