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

Imre Deak
2026-01-22T18:02:43Z

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