| Message ID | 20250129130105.198817-3-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: Allow display PHYs to reset power state | expand |
On Wed, 29 Jan 2025, Mika Kahola <mika.kahola@intel.com> wrote: > The dedicated display PHYs reset to a power state that blocks S0ix, > increasing idle system power. After a system reset (cold boot, > S3/4/5, warm reset) if a dedicated PHY is not being brought up > shortly, use these steps to move the PHY to the lowest power state > to save power. > > 1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62 GHz. > This brings lanes out of reset and enables the PLL to allow powerdown to be moved > to the Disable state. > 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state and disables the PLL. > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 35 +++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_cx0_phy.h | 1 + > .../drm/i915/display/intel_display_reset.c | 2 ++ > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 ++ > 4 files changed, 40 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > index bffe3d4bbe8b..0bd74e899221 100644 > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > @@ -3559,3 +3559,38 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state, > else > intel_c20pll_state_verify(new_crtc_state, crtc, encoder, &mpll_hw_state.c20); > } > + > +/* > + * WA 14022081154 > + * The dedicated display PHYs reset to a power state that blocks S0ix, increasing idle > + * system power. After a system reset (cold boot, S3/4/5, warm reset) if a dedicated > + * PHY is not being brought up shortly, use these steps to move the PHY to the lowest > + * power state to save power. > + * > + * 1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62 GHz. > + * This brings lanes out of reset and enables the PLL to allow powerdown to be moved > + * to the Disable state. > + * 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state and disables the PLL. > + */ > +void wa_14022081154(struct intel_display *display) Please do not name functions like this. There's zero indication what this is about. You're adding a long comment what's going on, surely you can name the function accordingly? > +{ > + struct intel_encoder *encoder; > + u32 val; > + > + if (DISPLAY_VER(display) < 30) > + return; > + > + for_each_intel_encoder(display->drm, encoder) { > + if (encoder->port == PORT_A || encoder->port == PORT_B) { > + val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port)); > + > + if (REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val) == XELPDP_DDI_CLOCK_SELECT_NONE) { > + struct intel_cx0pll_state pll_state = {}; > + int port_clock = 162000; > + intel_c10pll_calc_state_from_table(encoder, mtl_c10_edp_tables, port_clock, true, &pll_state); > + __intel_cx0pll_enable(encoder, &pll_state, true, port_clock, 4); > + intel_cx0pll_disable(encoder); > + } > + } > + } > +} > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > index 573fa7d3e88f..abebe7fd2cf2 100644 > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > @@ -42,5 +42,6 @@ bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a, > void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state); > int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder); > +void wa_14022081154(struct intel_display *display); > > #endif /* __INTEL_CX0_PHY_H__ */ > diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.c b/drivers/gpu/drm/i915/display/intel_display_reset.c > index 093b386c95e8..93b2697a0876 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_reset.c > +++ b/drivers/gpu/drm/i915/display/intel_display_reset.c > @@ -7,6 +7,7 @@ > > #include "i915_drv.h" > #include "intel_clock_gating.h" > +#include "intel_cx0_phy.h" > #include "intel_display_driver.h" > #include "intel_display_reset.h" > #include "intel_display_types.h" > @@ -116,6 +117,7 @@ void intel_display_reset_finish(struct drm_i915_private *i915) > intel_pps_unlock_regs_wa(display); > intel_display_driver_init_hw(display); > intel_clock_gating_init(i915); > + wa_14022081154(display); Contrast with intel_pps_unlock_regs_wa() call above. > intel_hpd_init(i915); > > ret = __intel_display_driver_resume(display, state, ctx); > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index b8fa04d3cd5c..76394411dd7a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -27,6 +27,7 @@ > #include "bxt_dpio_phy_regs.h" > #include "i915_drv.h" > #include "i915_reg.h" > +#include "intel_cx0_phy.h" > #include "intel_de.h" > #include "intel_display_types.h" > #include "intel_dkl_phy.h" > @@ -4552,6 +4553,7 @@ static void sanitize_dpll_state(struct drm_i915_private *i915, > return; > > adlp_cmtg_clock_gating_wa(i915, pll); > + wa_14022081154(&i915->display); Contrast with adlp_cmtg_clock_gating_wa() above. Imagine all of these were wa_32148191827(), wa_650914334(), wa_134174341(), etc. It's fine for compilers, not for humans. > > if (pll->active_mask) > return;
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Wednesday, 29 January 2025 16.45 > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org; intel- > xe@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Kahola, Mika <mika.kahola@intel.com> > Subject: Re: [PATCH 2/2] drm/i915/display: Allow display PHYs to reset power > state > > On Wed, 29 Jan 2025, Mika Kahola <mika.kahola@intel.com> wrote: > > The dedicated display PHYs reset to a power state that blocks S0ix, > > increasing idle system power. After a system reset (cold boot, S3/4/5, > > warm reset) if a dedicated PHY is not being brought up shortly, use > > these steps to move the PHY to the lowest power state to save power. > > > > 1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62 > GHz. > > This brings lanes out of reset and enables the PLL to allow powerdown to be > moved > > to the Disable state. > > 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state > and disables the PLL. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 35 > > +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cx0_phy.h | 1 + > > .../drm/i915/display/intel_display_reset.c | 2 ++ > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 ++ > > 4 files changed, 40 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > index bffe3d4bbe8b..0bd74e899221 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > @@ -3559,3 +3559,38 @@ void intel_cx0pll_state_verify(struct > intel_atomic_state *state, > > else > > intel_c20pll_state_verify(new_crtc_state, crtc, encoder, > > &mpll_hw_state.c20); } > > + > > +/* > > + * WA 14022081154 > > + * The dedicated display PHYs reset to a power state that blocks > > +S0ix, increasing idle > > + * system power. After a system reset (cold boot, S3/4/5, warm reset) > > +if a dedicated > > + * PHY is not being brought up shortly, use these steps to move the > > +PHY to the lowest > > + * power state to save power. > > + * > > + * 1. Follow the PLL Enable Sequence, using any valid frequency such as DP > 1.62 GHz. > > + * This brings lanes out of reset and enables the PLL to allow powerdown to > be moved > > + * to the Disable state. > > + * 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state > and disables the PLL. > > + */ > > +void wa_14022081154(struct intel_display *display) > > Please do not name functions like this. There's zero indication what this is about. > You're adding a long comment what's going on, surely you can name the function > accordingly? Ok, I will change the name. I wasn't really sure how we should name these wa's. I've seen both ways to be used. It's true that this is not very descriptive name for a function. > > > +{ > > + struct intel_encoder *encoder; > > + u32 val; > > + > > + if (DISPLAY_VER(display) < 30) > > + return; > > + > > + for_each_intel_encoder(display->drm, encoder) { > > + if (encoder->port == PORT_A || encoder->port == PORT_B) { > > + val = intel_de_read(display, > XELPDP_PORT_CLOCK_CTL(display, > > +encoder->port)); > > + > > + if (REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, > val) == XELPDP_DDI_CLOCK_SELECT_NONE) { > > + struct intel_cx0pll_state pll_state = {}; > > + int port_clock = 162000; > > + intel_c10pll_calc_state_from_table(encoder, > mtl_c10_edp_tables, port_clock, true, &pll_state); > > + __intel_cx0pll_enable(encoder, &pll_state, true, > port_clock, 4); > > + intel_cx0pll_disable(encoder); > > + } > > + } > > + } > > +} > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > index 573fa7d3e88f..abebe7fd2cf2 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > @@ -42,5 +42,6 @@ bool intel_cx0pll_compare_hw_state(const struct > > intel_cx0pll_state *a, void intel_cx0_phy_set_signal_levels(struct > intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state); int > > intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder); > > +void wa_14022081154(struct intel_display *display); > > > > #endif /* __INTEL_CX0_PHY_H__ */ > > diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.c > > b/drivers/gpu/drm/i915/display/intel_display_reset.c > > index 093b386c95e8..93b2697a0876 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_reset.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_reset.c > > @@ -7,6 +7,7 @@ > > > > #include "i915_drv.h" > > #include "intel_clock_gating.h" > > +#include "intel_cx0_phy.h" > > #include "intel_display_driver.h" > > #include "intel_display_reset.h" > > #include "intel_display_types.h" > > @@ -116,6 +117,7 @@ void intel_display_reset_finish(struct drm_i915_private > *i915) > > intel_pps_unlock_regs_wa(display); > > intel_display_driver_init_hw(display); > > intel_clock_gating_init(i915); > > + wa_14022081154(display); > > Contrast with intel_pps_unlock_regs_wa() call above. Got it! > > > intel_hpd_init(i915); > > > > ret = __intel_display_driver_resume(display, state, ctx); diff > > --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > index b8fa04d3cd5c..76394411dd7a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > @@ -27,6 +27,7 @@ > > #include "bxt_dpio_phy_regs.h" > > #include "i915_drv.h" > > #include "i915_reg.h" > > +#include "intel_cx0_phy.h" > > #include "intel_de.h" > > #include "intel_display_types.h" > > #include "intel_dkl_phy.h" > > @@ -4552,6 +4553,7 @@ static void sanitize_dpll_state(struct > drm_i915_private *i915, > > return; > > > > adlp_cmtg_clock_gating_wa(i915, pll); > > + wa_14022081154(&i915->display); > > Contrast with adlp_cmtg_clock_gating_wa() above. Got it! > > Imagine all of these were wa_32148191827(), wa_650914334(), > wa_134174341(), etc. It's fine for compilers, not for humans. I do agree. I will fix the naming. Thanks for the review and comments! -Mika- > > > > > if (pll->active_mask) > > return; > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c index bffe3d4bbe8b..0bd74e899221 100644 --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c @@ -3559,3 +3559,38 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state, else intel_c20pll_state_verify(new_crtc_state, crtc, encoder, &mpll_hw_state.c20); } + +/* + * WA 14022081154 + * The dedicated display PHYs reset to a power state that blocks S0ix, increasing idle + * system power. After a system reset (cold boot, S3/4/5, warm reset) if a dedicated + * PHY is not being brought up shortly, use these steps to move the PHY to the lowest + * power state to save power. + * + * 1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62 GHz. + * This brings lanes out of reset and enables the PLL to allow powerdown to be moved + * to the Disable state. + * 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state and disables the PLL. + */ +void wa_14022081154(struct intel_display *display) +{ + struct intel_encoder *encoder; + u32 val; + + if (DISPLAY_VER(display) < 30) + return; + + for_each_intel_encoder(display->drm, encoder) { + if (encoder->port == PORT_A || encoder->port == PORT_B) { + val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port)); + + if (REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val) == XELPDP_DDI_CLOCK_SELECT_NONE) { + struct intel_cx0pll_state pll_state = {}; + int port_clock = 162000; + intel_c10pll_calc_state_from_table(encoder, mtl_c10_edp_tables, port_clock, true, &pll_state); + __intel_cx0pll_enable(encoder, &pll_state, true, port_clock, 4); + intel_cx0pll_disable(encoder); + } + } + } +} diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h index 573fa7d3e88f..abebe7fd2cf2 100644 --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h @@ -42,5 +42,6 @@ bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a, void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state); int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder); +void wa_14022081154(struct intel_display *display); #endif /* __INTEL_CX0_PHY_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.c b/drivers/gpu/drm/i915/display/intel_display_reset.c index 093b386c95e8..93b2697a0876 100644 --- a/drivers/gpu/drm/i915/display/intel_display_reset.c +++ b/drivers/gpu/drm/i915/display/intel_display_reset.c @@ -7,6 +7,7 @@ #include "i915_drv.h" #include "intel_clock_gating.h" +#include "intel_cx0_phy.h" #include "intel_display_driver.h" #include "intel_display_reset.h" #include "intel_display_types.h" @@ -116,6 +117,7 @@ void intel_display_reset_finish(struct drm_i915_private *i915) intel_pps_unlock_regs_wa(display); intel_display_driver_init_hw(display); intel_clock_gating_init(i915); + wa_14022081154(display); intel_hpd_init(i915); ret = __intel_display_driver_resume(display, state, ctx); diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index b8fa04d3cd5c..76394411dd7a 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -27,6 +27,7 @@ #include "bxt_dpio_phy_regs.h" #include "i915_drv.h" #include "i915_reg.h" +#include "intel_cx0_phy.h" #include "intel_de.h" #include "intel_display_types.h" #include "intel_dkl_phy.h" @@ -4552,6 +4553,7 @@ static void sanitize_dpll_state(struct drm_i915_private *i915, return; adlp_cmtg_clock_gating_wa(i915, pll); + wa_14022081154(&i915->display); if (pll->active_mask) return;
The dedicated display PHYs reset to a power state that blocks S0ix, increasing idle system power. After a system reset (cold boot, S3/4/5, warm reset) if a dedicated PHY is not being brought up shortly, use these steps to move the PHY to the lowest power state to save power. 1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62 GHz. This brings lanes out of reset and enables the PLL to allow powerdown to be moved to the Disable state. 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state and disables the PLL. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/display/intel_cx0_phy.c | 35 +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cx0_phy.h | 1 + .../drm/i915/display/intel_display_reset.c | 2 ++ drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 ++ 4 files changed, 40 insertions(+)