Re: [PATCH 2/2] drm/i915/display: Allow display PHYs to reset power state

[Jani Nikula <jani.nikula@linux.intel.com>]

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;

-- 
Jani Nikula, Intel