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

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

[Mika Kahola]

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.

Before doing this, let's refactor the pll enabling in such a way that
the crtc_state structure is no longer needed.

Mika Kahola (2):
  drm/i915/display: Drop crtc_state from C10/C20 pll programming
  drm/i915/display: Allow display PHYs to reset power state

 drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 144 ++++++++++++------
 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, 104 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH 1/2] drm/i915/display: Drop crtc_state from C10/C20 pll programming

[Mika Kahola]

For PLL programming for C10 and C20 we don't need to
carry crtc_state but instead use only necessary parts
of the crtc_state i.e. pll_state.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 109 +++++++++++--------
 1 file changed, 64 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 48b0b9755b2b..bffe3d4bbe8b 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2021,13 +2021,12 @@ intel_c10pll_tables_get(struct intel_crtc_state *crtc_state,
 	return NULL;
 }
 
-static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
-				    struct intel_encoder *encoder)
+static void intel_cx0pll_update_ssc(struct intel_encoder *encoder,
+				    struct intel_cx0pll_state *pll_state, bool is_dp)
 {
 	struct intel_display *display = to_intel_display(encoder);
-	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
 
-	if (intel_crtc_has_dp_encoder(crtc_state)) {
+	if (is_dp) {
 		if (intel_panel_use_ssc(display)) {
 			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 			pll_state->ssc_enabled =
@@ -2036,11 +2035,10 @@ static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
 	}
 }
 
-static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
-				    struct intel_encoder *encoder)
+static void intel_c10pll_update_pll(struct intel_encoder *encoder,
+				    struct intel_cx0pll_state *pll_state, bool is_dp)
 {
 	struct intel_display *display = to_intel_display(encoder);
-	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
 	int i;
 
 	if (pll_state->ssc_enabled)
@@ -2051,32 +2049,49 @@ static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
 		pll_state->c10.pll[i] = 0;
 }
 
+static int intel_c10pll_calc_state_from_table(struct intel_encoder *encoder,
+					      const struct intel_c10pll_state * const *tables, int port_clock, bool is_dp,
+					      struct intel_cx0pll_state *pll_state)
+{
+	int i;
+
+	for (i = 0; tables[i]; i++) {
+		if (port_clock == tables[i]->clock) {
+			pll_state->c10 = *tables[i];
+			intel_cx0pll_update_ssc(encoder, pll_state, is_dp);
+			intel_c10pll_update_pll(encoder, pll_state, is_dp);
+			pll_state->use_c10 = true;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state,
 				   struct intel_encoder *encoder)
 {
 	const struct intel_c10pll_state * const *tables;
-	int i;
+	int val;
 
 	tables = intel_c10pll_tables_get(crtc_state, encoder);
 	if (!tables)
 		return -EINVAL;
 
-	for (i = 0; tables[i]; i++) {
-		if (crtc_state->port_clock == tables[i]->clock) {
-			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
-			intel_cx0pll_update_ssc(crtc_state, encoder);
-			intel_c10pll_update_pll(crtc_state, encoder);
-			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
-
-			return 0;
-		}
-	}
+	val = intel_c10pll_calc_state_from_table(encoder, tables,
+						 crtc_state->port_clock, intel_crtc_has_dp_encoder(crtc_state),
+						 &crtc_state->dpll_hw_state.cx0pll);
+	if (val == 0)
+		return 0;
 
 	/* For HDMI PLLs try SNPS PHY algorithm, if there are no precomputed tables */
 	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
 		intel_snps_hdmi_pll_compute_c10pll(&crtc_state->dpll_hw_state.cx0pll.c10,
 						   crtc_state->port_clock);
-		intel_c10pll_update_pll(crtc_state, encoder);
+		intel_c10pll_update_pll(encoder,
+					&crtc_state->dpll_hw_state.cx0pll,
+					intel_crtc_has_dp_encoder(crtc_state));
 		crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
 
 		return 0;
@@ -2112,10 +2127,9 @@ static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
 }
 
 static void intel_c10_pll_program(struct intel_display *display,
-				  const struct intel_crtc_state *crtc_state,
-				  struct intel_encoder *encoder)
+				  struct intel_encoder *encoder,
+				  const struct intel_c10pll_state *pll_state)
 {
-	const struct intel_c10pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c10;
 	int i;
 
 	intel_cx0_rmw(encoder, INTEL_CX0_BOTH_LANES, PHY_C10_VDR_CONTROL(1),
@@ -2334,7 +2348,9 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
 	for (i = 0; tables[i]; i++) {
 		if (crtc_state->port_clock == tables[i]->clock) {
 			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
-			intel_cx0pll_update_ssc(crtc_state, encoder);
+			intel_cx0pll_update_ssc(encoder,
+						&crtc_state->dpll_hw_state.cx0pll,
+						intel_crtc_has_dp_encoder(crtc_state));
 			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
 			return 0;
 		}
@@ -2600,19 +2616,14 @@ static int intel_get_c20_custom_width(u32 clock, bool dp)
 }
 
 static void intel_c20_pll_program(struct intel_display *display,
-				  const struct intel_crtc_state *crtc_state,
-				  struct intel_encoder *encoder)
+				  struct intel_encoder *encoder,
+				  const struct intel_c20pll_state *pll_state,
+				  int clock, bool dp)
 {
-	const struct intel_c20pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c20;
-	bool dp = false;
 	u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(encoder);
-	u32 clock = crtc_state->port_clock;
 	bool cntx;
 	int i;
 
-	if (intel_crtc_has_dp_encoder(crtc_state))
-		dp = true;
-
 	/* 1. Read current context selection */
 	cntx = intel_cx0_read(encoder, INTEL_CX0_LANE0, PHY_C20_VDR_CUSTOM_SERDES_RATE) & BIT(0);
 
@@ -2736,7 +2747,8 @@ static int intel_c10pll_calc_port_clock(struct intel_encoder *encoder,
 }
 
 static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
-					 const struct intel_crtc_state *crtc_state,
+					 const struct intel_cx0pll_state *pll_state,
+					 bool is_dp, int port_clock,
 					 bool lane_reversal)
 {
 	struct intel_display *display = to_intel_display(encoder);
@@ -2751,18 +2763,17 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
 
 	val |= XELPDP_FORWARD_CLOCK_UNGATE;
 
-	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) &&
-	    is_hdmi_frl(crtc_state->port_clock))
+	if (!is_dp && is_hdmi_frl(port_clock))
 		val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_DIV18CLK);
 	else
 		val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_MAXPCLK);
 
 	/* TODO: HDMI FRL */
 	/* DP2.0 10G and 20G rates enable MPLLA*/
-	if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == 2000000)
-		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
+	if (port_clock == 1000000 || port_clock == 2000000)
+		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
 	else
-		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
+		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
 
 	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
 		     XELPDP_LANE1_PHY_CLOCK_SELECT | XELPDP_FORWARD_CLOCK_UNGATE |
@@ -2992,8 +3003,9 @@ static u32 intel_cx0_get_pclk_pll_ack(u8 lane_mask)
 	return val;
 }
 
-static void intel_cx0pll_enable(struct intel_encoder *encoder,
-				const struct intel_crtc_state *crtc_state)
+static void __intel_cx0pll_enable(struct intel_encoder *encoder,
+				  const struct intel_cx0pll_state *pll_state,
+				  bool is_dp, int port_clock, int lane_count)
 {
 	struct intel_display *display = to_intel_display(encoder);
 	enum phy phy = intel_encoder_to_phy(encoder);
@@ -3007,7 +3019,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 	 * 1. Program PORT_CLOCK_CTL REGISTER to configure
 	 * clock muxes, gating and SSC
 	 */
-	intel_program_port_clock_ctl(encoder, crtc_state, lane_reversal);
+	intel_program_port_clock_ctl(encoder, pll_state, is_dp, port_clock, lane_reversal);
 
 	/* 2. Bring PHY out of reset. */
 	intel_cx0_phy_lane_reset(encoder, lane_reversal);
@@ -3027,15 +3039,15 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 
 	/* 5. Program PHY internal PLL internal registers. */
 	if (intel_encoder_is_c10phy(encoder))
-		intel_c10_pll_program(display, crtc_state, encoder);
+		intel_c10_pll_program(display, encoder, &pll_state->c10);
 	else
-		intel_c20_pll_program(display, crtc_state, encoder);
+		intel_c20_pll_program(display, encoder, &pll_state->c20, port_clock, is_dp);
 
 	/*
 	 * 6. Program the enabled and disabled owned PHY lane
 	 * transmitters over message bus
 	 */
-	intel_cx0_program_phy_lane(encoder, crtc_state->lane_count, lane_reversal);
+	intel_cx0_program_phy_lane(encoder, lane_count, lane_reversal);
 
 	/*
 	 * 7. Follow the Display Voltage Frequency Switching - Sequence
@@ -3046,8 +3058,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 	 * 8. Program DDI_CLK_VALFREQ to match intended DDI
 	 * clock frequency.
 	 */
-	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
-		       crtc_state->port_clock);
+	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
 
 	/*
 	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
@@ -3074,6 +3085,14 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 	intel_cx0_phy_transaction_end(encoder, wakeref);
 }
 
+static void intel_cx0pll_enable(struct intel_encoder *encoder,
+				const struct intel_crtc_state *crtc_state)
+{
+	__intel_cx0pll_enable(encoder, &crtc_state->dpll_hw_state.cx0pll,
+			      intel_crtc_has_dp_encoder(crtc_state), crtc_state->port_clock, crtc_state->lane_count);
+
+}
+
 int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
 {
 	struct intel_display *display = to_intel_display(encoder);
-- 
2.43.0

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

[Mika Kahola]

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)
+{
+	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;
-- 
2.43.0

Re: [PATCH 1/2] drm/i915/display: Drop crtc_state from C10/C20 pll programming

[Jani Nikula]

On Wed, 29 Jan 2025, Mika Kahola <mika.kahola@intel.com> wrote:
> For PLL programming for C10 and C20 we don't need to
> carry crtc_state but instead use only necessary parts
> of the crtc_state i.e. pll_state.

This is not a good enough justification alone. Usually we pass
crtc_state around because we're going to need more stuff from there
anyway. In that case, someone's going to have to reverse this, or pass a
bunch more parameters than just "is_dp".

I see that you're doing this because you actually need this in a context
that doens't have crtc_state. Then *that* needs to be the rationale.

Even so, there's a good chance this will bite us later.


BR,
Jani.


>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 109 +++++++++++--------
>  1 file changed, 64 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 48b0b9755b2b..bffe3d4bbe8b 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2021,13 +2021,12 @@ intel_c10pll_tables_get(struct intel_crtc_state *crtc_state,
>  	return NULL;
>  }
>  
> -static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
> -				    struct intel_encoder *encoder)
> +static void intel_cx0pll_update_ssc(struct intel_encoder *encoder,
> +				    struct intel_cx0pll_state *pll_state, bool is_dp)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
> -	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
>  
> -	if (intel_crtc_has_dp_encoder(crtc_state)) {
> +	if (is_dp) {
>  		if (intel_panel_use_ssc(display)) {
>  			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  			pll_state->ssc_enabled =
> @@ -2036,11 +2035,10 @@ static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
>  	}
>  }
>  
> -static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
> -				    struct intel_encoder *encoder)
> +static void intel_c10pll_update_pll(struct intel_encoder *encoder,
> +				    struct intel_cx0pll_state *pll_state, bool is_dp)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
> -	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
>  	int i;
>  
>  	if (pll_state->ssc_enabled)
> @@ -2051,32 +2049,49 @@ static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
>  		pll_state->c10.pll[i] = 0;
>  }
>  
> +static int intel_c10pll_calc_state_from_table(struct intel_encoder *encoder,
> +					      const struct intel_c10pll_state * const *tables, int port_clock, bool is_dp,
> +					      struct intel_cx0pll_state *pll_state)
> +{
> +	int i;
> +
> +	for (i = 0; tables[i]; i++) {
> +		if (port_clock == tables[i]->clock) {
> +			pll_state->c10 = *tables[i];
> +			intel_cx0pll_update_ssc(encoder, pll_state, is_dp);
> +			intel_c10pll_update_pll(encoder, pll_state, is_dp);
> +			pll_state->use_c10 = true;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state,
>  				   struct intel_encoder *encoder)
>  {
>  	const struct intel_c10pll_state * const *tables;
> -	int i;
> +	int val;
>  
>  	tables = intel_c10pll_tables_get(crtc_state, encoder);
>  	if (!tables)
>  		return -EINVAL;
>  
> -	for (i = 0; tables[i]; i++) {
> -		if (crtc_state->port_clock == tables[i]->clock) {
> -			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
> -			intel_cx0pll_update_ssc(crtc_state, encoder);
> -			intel_c10pll_update_pll(crtc_state, encoder);
> -			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> -
> -			return 0;
> -		}
> -	}
> +	val = intel_c10pll_calc_state_from_table(encoder, tables,
> +						 crtc_state->port_clock, intel_crtc_has_dp_encoder(crtc_state),
> +						 &crtc_state->dpll_hw_state.cx0pll);
> +	if (val == 0)
> +		return 0;
>  
>  	/* For HDMI PLLs try SNPS PHY algorithm, if there are no precomputed tables */
>  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>  		intel_snps_hdmi_pll_compute_c10pll(&crtc_state->dpll_hw_state.cx0pll.c10,
>  						   crtc_state->port_clock);
> -		intel_c10pll_update_pll(crtc_state, encoder);
> +		intel_c10pll_update_pll(encoder,
> +					&crtc_state->dpll_hw_state.cx0pll,
> +					intel_crtc_has_dp_encoder(crtc_state));
>  		crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
>  
>  		return 0;
> @@ -2112,10 +2127,9 @@ static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
>  }
>  
>  static void intel_c10_pll_program(struct intel_display *display,
> -				  const struct intel_crtc_state *crtc_state,
> -				  struct intel_encoder *encoder)
> +				  struct intel_encoder *encoder,
> +				  const struct intel_c10pll_state *pll_state)
>  {
> -	const struct intel_c10pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c10;
>  	int i;
>  
>  	intel_cx0_rmw(encoder, INTEL_CX0_BOTH_LANES, PHY_C10_VDR_CONTROL(1),
> @@ -2334,7 +2348,9 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>  	for (i = 0; tables[i]; i++) {
>  		if (crtc_state->port_clock == tables[i]->clock) {
>  			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
> -			intel_cx0pll_update_ssc(crtc_state, encoder);
> +			intel_cx0pll_update_ssc(encoder,
> +						&crtc_state->dpll_hw_state.cx0pll,
> +						intel_crtc_has_dp_encoder(crtc_state));
>  			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
>  			return 0;
>  		}
> @@ -2600,19 +2616,14 @@ static int intel_get_c20_custom_width(u32 clock, bool dp)
>  }
>  
>  static void intel_c20_pll_program(struct intel_display *display,
> -				  const struct intel_crtc_state *crtc_state,
> -				  struct intel_encoder *encoder)
> +				  struct intel_encoder *encoder,
> +				  const struct intel_c20pll_state *pll_state,
> +				  int clock, bool dp)
>  {
> -	const struct intel_c20pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c20;
> -	bool dp = false;
>  	u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(encoder);
> -	u32 clock = crtc_state->port_clock;
>  	bool cntx;
>  	int i;
>  
> -	if (intel_crtc_has_dp_encoder(crtc_state))
> -		dp = true;
> -
>  	/* 1. Read current context selection */
>  	cntx = intel_cx0_read(encoder, INTEL_CX0_LANE0, PHY_C20_VDR_CUSTOM_SERDES_RATE) & BIT(0);
>  
> @@ -2736,7 +2747,8 @@ static int intel_c10pll_calc_port_clock(struct intel_encoder *encoder,
>  }
>  
>  static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
> -					 const struct intel_crtc_state *crtc_state,
> +					 const struct intel_cx0pll_state *pll_state,
> +					 bool is_dp, int port_clock,
>  					 bool lane_reversal)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
> @@ -2751,18 +2763,17 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
>  
>  	val |= XELPDP_FORWARD_CLOCK_UNGATE;
>  
> -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) &&
> -	    is_hdmi_frl(crtc_state->port_clock))
> +	if (!is_dp && is_hdmi_frl(port_clock))
>  		val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_DIV18CLK);
>  	else
>  		val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_MAXPCLK);
>  
>  	/* TODO: HDMI FRL */
>  	/* DP2.0 10G and 20G rates enable MPLLA*/
> -	if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == 2000000)
> -		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
> +	if (port_clock == 1000000 || port_clock == 2000000)
> +		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
>  	else
> -		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
> +		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
>  
>  	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>  		     XELPDP_LANE1_PHY_CLOCK_SELECT | XELPDP_FORWARD_CLOCK_UNGATE |
> @@ -2992,8 +3003,9 @@ static u32 intel_cx0_get_pclk_pll_ack(u8 lane_mask)
>  	return val;
>  }
>  
> -static void intel_cx0pll_enable(struct intel_encoder *encoder,
> -				const struct intel_crtc_state *crtc_state)
> +static void __intel_cx0pll_enable(struct intel_encoder *encoder,
> +				  const struct intel_cx0pll_state *pll_state,
> +				  bool is_dp, int port_clock, int lane_count)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
>  	enum phy phy = intel_encoder_to_phy(encoder);
> @@ -3007,7 +3019,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>  	 * 1. Program PORT_CLOCK_CTL REGISTER to configure
>  	 * clock muxes, gating and SSC
>  	 */
> -	intel_program_port_clock_ctl(encoder, crtc_state, lane_reversal);
> +	intel_program_port_clock_ctl(encoder, pll_state, is_dp, port_clock, lane_reversal);
>  
>  	/* 2. Bring PHY out of reset. */
>  	intel_cx0_phy_lane_reset(encoder, lane_reversal);
> @@ -3027,15 +3039,15 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>  
>  	/* 5. Program PHY internal PLL internal registers. */
>  	if (intel_encoder_is_c10phy(encoder))
> -		intel_c10_pll_program(display, crtc_state, encoder);
> +		intel_c10_pll_program(display, encoder, &pll_state->c10);
>  	else
> -		intel_c20_pll_program(display, crtc_state, encoder);
> +		intel_c20_pll_program(display, encoder, &pll_state->c20, port_clock, is_dp);
>  
>  	/*
>  	 * 6. Program the enabled and disabled owned PHY lane
>  	 * transmitters over message bus
>  	 */
> -	intel_cx0_program_phy_lane(encoder, crtc_state->lane_count, lane_reversal);
> +	intel_cx0_program_phy_lane(encoder, lane_count, lane_reversal);
>  
>  	/*
>  	 * 7. Follow the Display Voltage Frequency Switching - Sequence
> @@ -3046,8 +3058,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>  	 * 8. Program DDI_CLK_VALFREQ to match intended DDI
>  	 * clock frequency.
>  	 */
> -	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> -		       crtc_state->port_clock);
> +	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
>  
>  	/*
>  	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> @@ -3074,6 +3085,14 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>  	intel_cx0_phy_transaction_end(encoder, wakeref);
>  }
>  
> +static void intel_cx0pll_enable(struct intel_encoder *encoder,
> +				const struct intel_crtc_state *crtc_state)
> +{
> +	__intel_cx0pll_enable(encoder, &crtc_state->dpll_hw_state.cx0pll,
> +			      intel_crtc_has_dp_encoder(crtc_state), crtc_state->port_clock, crtc_state->lane_count);
> +
> +}
> +
>  int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
>  {
>  	struct intel_display *display = to_intel_display(encoder);

-- 
Jani Nikula, Intel

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

[Jani Nikula]

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: Allow display PHYs to reset power state

[Patchwork]

== Series Details ==

Series: drm/i915/display: Allow display PHYs to reset power state
URL   : https://patchwork.freedesktop.org/series/144102/
State : warning

== Summary ==

Error: dim checkpatch failed
74a393b566c5 drm/i915/display: Drop crtc_state from C10/C20 pll programming
-:53: WARNING:LONG_LINE: line length of 122 exceeds 100 columns
#53: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:2053:
+					      const struct intel_c10pll_state * const *tables, int port_clock, bool is_dp,

-:94: WARNING:LONG_LINE: line length of 111 exceeds 100 columns
#94: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:2083:
+						 crtc_state->port_clock, intel_crtc_has_dp_encoder(crtc_state),

-:248: WARNING:LONG_LINE: line length of 117 exceeds 100 columns
#248: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:3092:
+			      intel_crtc_has_dp_encoder(crtc_state), crtc_state->port_clock, crtc_state->lane_count);

-:250: CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#250: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:3094:
+
+}

total: 0 errors, 3 warnings, 1 checks, 225 lines checked
04f9a9dc3240 drm/i915/display: Allow display PHYs to reset power state
-:12: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#12: 
1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62 GHz.

-:52: WARNING:LONG_LINE: line length of 111 exceeds 100 columns
#52: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:3587:
+			if (REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val) == XELPDP_DDI_CLOCK_SELECT_NONE) {

-:55: WARNING:LONG_LINE: line length of 126 exceeds 100 columns
#55: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:3590:
+				intel_c10pll_calc_state_from_table(encoder, mtl_c10_edp_tables, port_clock, true, &pll_state);

-:55: WARNING:LINE_SPACING: Missing a blank line after declarations
#55: FILE: drivers/gpu/drm/i915/display/intel_cx0_phy.c:3590:
+				int port_clock = 162000;
+				intel_c10pll_calc_state_from_table(encoder, mtl_c10_edp_tables, port_clock, true, &pll_state);

total: 0 errors, 4 warnings, 0 checks, 72 lines checked

✗ i915.CI.BAT: failure for drm/i915/display: Allow display PHYs to reset power state

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5147 bytes --]

== Series Details ==

Series: drm/i915/display: Allow display PHYs to reset power state
URL   : https://patchwork.freedesktop.org/series/144102/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_16039 -> Patchwork_144102v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_144102v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_144102v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/index.html

Participating hosts (44 -> 43)
------------------------------

  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_144102v1:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
    - bat-apl-1:          [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-apl-1/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-apl-1/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html

  
Known issues
------------

  Here are the changes found in Patchwork_144102v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@dmabuf@all-tests:
    - fi-pnv-d510:        NOTRUN -> [INCOMPLETE][3] ([i915#12904]) +1 other test incomplete
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/fi-pnv-d510/igt@dmabuf@all-tests.html
    - bat-apl-1:          [PASS][4] -> [INCOMPLETE][5] ([i915#12904]) +1 other test incomplete
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-apl-1/igt@dmabuf@all-tests.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-apl-1/igt@dmabuf@all-tests.html

  * igt@i915_selftest@live@gt_mocs:
    - bat-twl-2:          NOTRUN -> [ABORT][6] ([i915#12919]) +1 other test abort
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-twl-2/igt@i915_selftest@live@gt_mocs.html

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-6:         [PASS][7] -> [DMESG-FAIL][8] ([i915#12061]) +1 other test dmesg-fail
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-mtlp-6/igt@i915_selftest@live@workarounds.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         [PASS][9] -> [SKIP][10] ([i915#9197]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [DMESG-FAIL][11] ([i915#12061]) -> [PASS][12] +1 other test pass
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-arlh-3/igt@i915_selftest@live@workarounds.html
    - bat-arls-5:         [DMESG-FAIL][13] ([i915#12061]) -> [PASS][14] +1 other test pass
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-arls-5/igt@i915_selftest@live@workarounds.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-arls-5/igt@i915_selftest@live@workarounds.html
    - {bat-mtlp-9}:       [DMESG-FAIL][15] ([i915#12061]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16039/bat-mtlp-9/igt@i915_selftest@live@workarounds.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/bat-mtlp-9/igt@i915_selftest@live@workarounds.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12904]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904
  [i915#12919]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12919
  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197


Build changes
-------------

  * Linux: CI_DRM_16039 -> Patchwork_144102v1

  CI-20190529: 20190529
  CI_DRM_16039: bdfc862a5c719d934efbdedb050ef891b9feef15 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8214: 7a8a3744466fbb89127201077f030033c72df948 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_144102v1: bdfc862a5c719d934efbdedb050ef891b9feef15 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_144102v1/index.html

[-- Attachment #2: Type: text/html, Size: 6210 bytes --]

RE: [PATCH 1/2] drm/i915/display: Drop crtc_state from C10/C20 pll programming

[Kahola, Mika]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Wednesday, 29 January 2025 16.40
> 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 1/2] drm/i915/display: Drop crtc_state from C10/C20 pll
> programming
> 
> On Wed, 29 Jan 2025, Mika Kahola <mika.kahola@intel.com> wrote:
> > For PLL programming for C10 and C20 we don't need to carry crtc_state
> > but instead use only necessary parts of the crtc_state i.e. pll_state.
> 
> This is not a good enough justification alone. Usually we pass crtc_state around
> because we're going to need more stuff from there anyway. In that case,
> someone's going to have to reverse this, or pass a bunch more parameters than
> just "is_dp".
> 
> I see that you're doing this because you actually need this in a context that
> doens't have crtc_state. Then *that* needs to be the rationale.
> 
> Even so, there's a good chance this will bite us later.

We only need from crtc_state the pll_state when enabling/disabling pll's and hence this change. What was not mentioned in the commit message is that this will partially prepares the work where all pll handling is moved over to a better location i.e. intel_dpll_mgr.c file. As MTL+ with PICA chips being only platform(s) outside this file. But this change could be left for later stage and when this refactoring work is ready for review round.

Thanks!
Mika 
> 
> 
> BR,
> Jani.
> 
> 
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 109
> > +++++++++++--------
> >  1 file changed, 64 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index 48b0b9755b2b..bffe3d4bbe8b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2021,13 +2021,12 @@ intel_c10pll_tables_get(struct intel_crtc_state
> *crtc_state,
> >  	return NULL;
> >  }
> >
> > -static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
> > -				    struct intel_encoder *encoder)
> > +static void intel_cx0pll_update_ssc(struct intel_encoder *encoder,
> > +				    struct intel_cx0pll_state *pll_state, bool
> is_dp)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> > -	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
> >
> > -	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > +	if (is_dp) {
> >  		if (intel_panel_use_ssc(display)) {
> >  			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  			pll_state->ssc_enabled =
> > @@ -2036,11 +2035,10 @@ static void intel_cx0pll_update_ssc(struct
> intel_crtc_state *crtc_state,
> >  	}
> >  }
> >
> > -static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
> > -				    struct intel_encoder *encoder)
> > +static void intel_c10pll_update_pll(struct intel_encoder *encoder,
> > +				    struct intel_cx0pll_state *pll_state, bool
> is_dp)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> > -	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
> >  	int i;
> >
> >  	if (pll_state->ssc_enabled)
> > @@ -2051,32 +2049,49 @@ static void intel_c10pll_update_pll(struct
> intel_crtc_state *crtc_state,
> >  		pll_state->c10.pll[i] = 0;
> >  }
> >
> > +static int intel_c10pll_calc_state_from_table(struct intel_encoder *encoder,
> > +					      const struct intel_c10pll_state *
> const *tables, int port_clock, bool is_dp,
> > +					      struct intel_cx0pll_state *pll_state) {
> > +	int i;
> > +
> > +	for (i = 0; tables[i]; i++) {
> > +		if (port_clock == tables[i]->clock) {
> > +			pll_state->c10 = *tables[i];
> > +			intel_cx0pll_update_ssc(encoder, pll_state, is_dp);
> > +			intel_c10pll_update_pll(encoder, pll_state, is_dp);
> > +			pll_state->use_c10 = true;
> > +
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state,
> >  				   struct intel_encoder *encoder)  {
> >  	const struct intel_c10pll_state * const *tables;
> > -	int i;
> > +	int val;
> >
> >  	tables = intel_c10pll_tables_get(crtc_state, encoder);
> >  	if (!tables)
> >  		return -EINVAL;
> >
> > -	for (i = 0; tables[i]; i++) {
> > -		if (crtc_state->port_clock == tables[i]->clock) {
> > -			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
> > -			intel_cx0pll_update_ssc(crtc_state, encoder);
> > -			intel_c10pll_update_pll(crtc_state, encoder);
> > -			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> > -
> > -			return 0;
> > -		}
> > -	}
> > +	val = intel_c10pll_calc_state_from_table(encoder, tables,
> > +						 crtc_state->port_clock,
> intel_crtc_has_dp_encoder(crtc_state),
> > +						 &crtc_state-
> >dpll_hw_state.cx0pll);
> > +	if (val == 0)
> > +		return 0;
> >
> >  	/* For HDMI PLLs try SNPS PHY algorithm, if there are no precomputed
> tables */
> >  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> >  		intel_snps_hdmi_pll_compute_c10pll(&crtc_state-
> >dpll_hw_state.cx0pll.c10,
> >  						   crtc_state->port_clock);
> > -		intel_c10pll_update_pll(crtc_state, encoder);
> > +		intel_c10pll_update_pll(encoder,
> > +					&crtc_state->dpll_hw_state.cx0pll,
> > +
> 	intel_crtc_has_dp_encoder(crtc_state));
> >  		crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> >
> >  		return 0;
> > @@ -2112,10 +2127,9 @@ static void
> > intel_c10pll_readout_hw_state(struct intel_encoder *encoder,  }
> >
> >  static void intel_c10_pll_program(struct intel_display *display,
> > -				  const struct intel_crtc_state *crtc_state,
> > -				  struct intel_encoder *encoder)
> > +				  struct intel_encoder *encoder,
> > +				  const struct intel_c10pll_state *pll_state)
> >  {
> > -	const struct intel_c10pll_state *pll_state = &crtc_state-
> >dpll_hw_state.cx0pll.c10;
> >  	int i;
> >
> >  	intel_cx0_rmw(encoder, INTEL_CX0_BOTH_LANES,
> PHY_C10_VDR_CONTROL(1),
> > @@ -2334,7 +2348,9 @@ static int intel_c20pll_calc_state(struct
> intel_crtc_state *crtc_state,
> >  	for (i = 0; tables[i]; i++) {
> >  		if (crtc_state->port_clock == tables[i]->clock) {
> >  			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
> > -			intel_cx0pll_update_ssc(crtc_state, encoder);
> > +			intel_cx0pll_update_ssc(encoder,
> > +						&crtc_state-
> >dpll_hw_state.cx0pll,
> > +
> 	intel_crtc_has_dp_encoder(crtc_state));
> >  			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
> >  			return 0;
> >  		}
> > @@ -2600,19 +2616,14 @@ static int intel_get_c20_custom_width(u32
> > clock, bool dp)  }
> >
> >  static void intel_c20_pll_program(struct intel_display *display,
> > -				  const struct intel_crtc_state *crtc_state,
> > -				  struct intel_encoder *encoder)
> > +				  struct intel_encoder *encoder,
> > +				  const struct intel_c20pll_state *pll_state,
> > +				  int clock, bool dp)
> >  {
> > -	const struct intel_c20pll_state *pll_state = &crtc_state-
> >dpll_hw_state.cx0pll.c20;
> > -	bool dp = false;
> >  	u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(encoder);
> > -	u32 clock = crtc_state->port_clock;
> >  	bool cntx;
> >  	int i;
> >
> > -	if (intel_crtc_has_dp_encoder(crtc_state))
> > -		dp = true;
> > -
> >  	/* 1. Read current context selection */
> >  	cntx = intel_cx0_read(encoder, INTEL_CX0_LANE0,
> > PHY_C20_VDR_CUSTOM_SERDES_RATE) & BIT(0);
> >
> > @@ -2736,7 +2747,8 @@ static int intel_c10pll_calc_port_clock(struct
> > intel_encoder *encoder,  }
> >
> >  static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
> > -					 const struct intel_crtc_state
> *crtc_state,
> > +					 const struct intel_cx0pll_state
> *pll_state,
> > +					 bool is_dp, int port_clock,
> >  					 bool lane_reversal)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder); @@
> > -2751,18 +2763,17 @@ static void intel_program_port_clock_ctl(struct
> > intel_encoder *encoder,
> >
> >  	val |= XELPDP_FORWARD_CLOCK_UNGATE;
> >
> > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) &&
> > -	    is_hdmi_frl(crtc_state->port_clock))
> > +	if (!is_dp && is_hdmi_frl(port_clock))
> >  		val |=
> XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_DIV18CLK);
> >  	else
> >  		val |=
> XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_MAXPCLK);
> >
> >  	/* TODO: HDMI FRL */
> >  	/* DP2.0 10G and 20G rates enable MPLLA*/
> > -	if (crtc_state->port_clock == 1000000 || crtc_state->port_clock ==
> 2000000)
> > -		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ?
> XELPDP_SSC_ENABLE_PLLA : 0;
> > +	if (port_clock == 1000000 || port_clock == 2000000)
> > +		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
> >  	else
> > -		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ?
> XELPDP_SSC_ENABLE_PLLB : 0;
> > +		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
> >
> >  	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >  		     XELPDP_LANE1_PHY_CLOCK_SELECT |
> XELPDP_FORWARD_CLOCK_UNGATE |
> > @@ -2992,8 +3003,9 @@ static u32 intel_cx0_get_pclk_pll_ack(u8 lane_mask)
> >  	return val;
> >  }
> >
> > -static void intel_cx0pll_enable(struct intel_encoder *encoder,
> > -				const struct intel_crtc_state *crtc_state)
> > +static void __intel_cx0pll_enable(struct intel_encoder *encoder,
> > +				  const struct intel_cx0pll_state *pll_state,
> > +				  bool is_dp, int port_clock, int lane_count)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> >  	enum phy phy = intel_encoder_to_phy(encoder); @@ -3007,7 +3019,7
> @@
> > static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  	 * 1. Program PORT_CLOCK_CTL REGISTER to configure
> >  	 * clock muxes, gating and SSC
> >  	 */
> > -	intel_program_port_clock_ctl(encoder, crtc_state, lane_reversal);
> > +	intel_program_port_clock_ctl(encoder, pll_state, is_dp, port_clock,
> > +lane_reversal);
> >
> >  	/* 2. Bring PHY out of reset. */
> >  	intel_cx0_phy_lane_reset(encoder, lane_reversal); @@ -3027,15
> > +3039,15 @@ static void intel_cx0pll_enable(struct intel_encoder
> > *encoder,
> >
> >  	/* 5. Program PHY internal PLL internal registers. */
> >  	if (intel_encoder_is_c10phy(encoder))
> > -		intel_c10_pll_program(display, crtc_state, encoder);
> > +		intel_c10_pll_program(display, encoder, &pll_state->c10);
> >  	else
> > -		intel_c20_pll_program(display, crtc_state, encoder);
> > +		intel_c20_pll_program(display, encoder, &pll_state->c20,
> > +port_clock, is_dp);
> >
> >  	/*
> >  	 * 6. Program the enabled and disabled owned PHY lane
> >  	 * transmitters over message bus
> >  	 */
> > -	intel_cx0_program_phy_lane(encoder, crtc_state->lane_count,
> lane_reversal);
> > +	intel_cx0_program_phy_lane(encoder, lane_count, lane_reversal);
> >
> >  	/*
> >  	 * 7. Follow the Display Voltage Frequency Switching - Sequence @@
> > -3046,8 +3058,7 @@ static void intel_cx0pll_enable(struct intel_encoder
> *encoder,
> >  	 * 8. Program DDI_CLK_VALFREQ to match intended DDI
> >  	 * clock frequency.
> >  	 */
> > -	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> > -		       crtc_state->port_clock);
> > +	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
> >
> >  	/*
> >  	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request @@ -3074,6
> > +3085,14 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  	intel_cx0_phy_transaction_end(encoder, wakeref);  }
> >
> > +static void intel_cx0pll_enable(struct intel_encoder *encoder,
> > +				const struct intel_crtc_state *crtc_state) {
> > +	__intel_cx0pll_enable(encoder, &crtc_state->dpll_hw_state.cx0pll,
> > +			      intel_crtc_has_dp_encoder(crtc_state),
> > +crtc_state->port_clock, crtc_state->lane_count);
> > +
> > +}
> > +
> >  int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)  {
> >  	struct intel_display *display = to_intel_display(encoder);
> 
> --
> Jani Nikula, Intel

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

[Kahola, Mika]

> -----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

Re: [PATCH 1/2] drm/i915/display: Drop crtc_state from C10/C20 pll programming

[Imre Deak]

On Wed, Jan 29, 2025 at 04:40:01PM +0200, Jani Nikula wrote:
> On Wed, 29 Jan 2025, Mika Kahola <mika.kahola@intel.com> wrote:
> > For PLL programming for C10 and C20 we don't need to
> > carry crtc_state but instead use only necessary parts
> > of the crtc_state i.e. pll_state.
> 
> This is not a good enough justification alone. Usually we pass
> crtc_state around because we're going to need more stuff from there
> anyway. In that case, someone's going to have to reverse this, or pass a
> bunch more parameters than just "is_dp".
> 
> I see that you're doing this because you actually need this in a context
> that doens't have crtc_state. Then *that* needs to be the rationale.
> 
> Even so, there's a good chance this will bite us later.

The problem with the alternative to create a temporary CRTC state and
pass that around is that this state would not be fully initialized. If
passing more parameters to the PLL functions (besides is_dp, port_clock,
lane_count) would be impractical due to the long parameter list, the
parameters could be passed instead via a cx0_pll_params structure.

> BR,
> Jani.
> 
> 
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 109 +++++++++++--------
> >  1 file changed, 64 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index 48b0b9755b2b..bffe3d4bbe8b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2021,13 +2021,12 @@ intel_c10pll_tables_get(struct intel_crtc_state *crtc_state,
> >  	return NULL;
> >  }
> >  
> > -static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
> > -				    struct intel_encoder *encoder)
> > +static void intel_cx0pll_update_ssc(struct intel_encoder *encoder,
> > +				    struct intel_cx0pll_state *pll_state, bool is_dp)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> > -	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
> >  
> > -	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > +	if (is_dp) {
> >  		if (intel_panel_use_ssc(display)) {
> >  			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  			pll_state->ssc_enabled =
> > @@ -2036,11 +2035,10 @@ static void intel_cx0pll_update_ssc(struct intel_crtc_state *crtc_state,
> >  	}
> >  }
> >  
> > -static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
> > -				    struct intel_encoder *encoder)
> > +static void intel_c10pll_update_pll(struct intel_encoder *encoder,
> > +				    struct intel_cx0pll_state *pll_state, bool is_dp)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> > -	struct intel_cx0pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll;
> >  	int i;
> >  
> >  	if (pll_state->ssc_enabled)
> > @@ -2051,32 +2049,49 @@ static void intel_c10pll_update_pll(struct intel_crtc_state *crtc_state,
> >  		pll_state->c10.pll[i] = 0;
> >  }
> >  
> > +static int intel_c10pll_calc_state_from_table(struct intel_encoder *encoder,
> > +					      const struct intel_c10pll_state * const *tables, int port_clock, bool is_dp,
> > +					      struct intel_cx0pll_state *pll_state)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; tables[i]; i++) {
> > +		if (port_clock == tables[i]->clock) {
> > +			pll_state->c10 = *tables[i];
> > +			intel_cx0pll_update_ssc(encoder, pll_state, is_dp);
> > +			intel_c10pll_update_pll(encoder, pll_state, is_dp);
> > +			pll_state->use_c10 = true;
> > +
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state,
> >  				   struct intel_encoder *encoder)
> >  {
> >  	const struct intel_c10pll_state * const *tables;
> > -	int i;
> > +	int val;
> >  
> >  	tables = intel_c10pll_tables_get(crtc_state, encoder);
> >  	if (!tables)
> >  		return -EINVAL;
> >  
> > -	for (i = 0; tables[i]; i++) {
> > -		if (crtc_state->port_clock == tables[i]->clock) {
> > -			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
> > -			intel_cx0pll_update_ssc(crtc_state, encoder);
> > -			intel_c10pll_update_pll(crtc_state, encoder);
> > -			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> > -
> > -			return 0;
> > -		}
> > -	}
> > +	val = intel_c10pll_calc_state_from_table(encoder, tables,
> > +						 crtc_state->port_clock, intel_crtc_has_dp_encoder(crtc_state),
> > +						 &crtc_state->dpll_hw_state.cx0pll);
> > +	if (val == 0)
> > +		return 0;
> >  
> >  	/* For HDMI PLLs try SNPS PHY algorithm, if there are no precomputed tables */
> >  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> >  		intel_snps_hdmi_pll_compute_c10pll(&crtc_state->dpll_hw_state.cx0pll.c10,
> >  						   crtc_state->port_clock);
> > -		intel_c10pll_update_pll(crtc_state, encoder);
> > +		intel_c10pll_update_pll(encoder,
> > +					&crtc_state->dpll_hw_state.cx0pll,
> > +					intel_crtc_has_dp_encoder(crtc_state));
> >  		crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> >  
> >  		return 0;
> > @@ -2112,10 +2127,9 @@ static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
> >  }
> >  
> >  static void intel_c10_pll_program(struct intel_display *display,
> > -				  const struct intel_crtc_state *crtc_state,
> > -				  struct intel_encoder *encoder)
> > +				  struct intel_encoder *encoder,
> > +				  const struct intel_c10pll_state *pll_state)
> >  {
> > -	const struct intel_c10pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c10;
> >  	int i;
> >  
> >  	intel_cx0_rmw(encoder, INTEL_CX0_BOTH_LANES, PHY_C10_VDR_CONTROL(1),
> > @@ -2334,7 +2348,9 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
> >  	for (i = 0; tables[i]; i++) {
> >  		if (crtc_state->port_clock == tables[i]->clock) {
> >  			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
> > -			intel_cx0pll_update_ssc(crtc_state, encoder);
> > +			intel_cx0pll_update_ssc(encoder,
> > +						&crtc_state->dpll_hw_state.cx0pll,
> > +						intel_crtc_has_dp_encoder(crtc_state));
> >  			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
> >  			return 0;
> >  		}
> > @@ -2600,19 +2616,14 @@ static int intel_get_c20_custom_width(u32 clock, bool dp)
> >  }
> >  
> >  static void intel_c20_pll_program(struct intel_display *display,
> > -				  const struct intel_crtc_state *crtc_state,
> > -				  struct intel_encoder *encoder)
> > +				  struct intel_encoder *encoder,
> > +				  const struct intel_c20pll_state *pll_state,
> > +				  int clock, bool dp)
> >  {
> > -	const struct intel_c20pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c20;
> > -	bool dp = false;
> >  	u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(encoder);
> > -	u32 clock = crtc_state->port_clock;
> >  	bool cntx;
> >  	int i;
> >  
> > -	if (intel_crtc_has_dp_encoder(crtc_state))
> > -		dp = true;
> > -
> >  	/* 1. Read current context selection */
> >  	cntx = intel_cx0_read(encoder, INTEL_CX0_LANE0, PHY_C20_VDR_CUSTOM_SERDES_RATE) & BIT(0);
> >  
> > @@ -2736,7 +2747,8 @@ static int intel_c10pll_calc_port_clock(struct intel_encoder *encoder,
> >  }
> >  
> >  static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
> > -					 const struct intel_crtc_state *crtc_state,
> > +					 const struct intel_cx0pll_state *pll_state,
> > +					 bool is_dp, int port_clock,
> >  					 bool lane_reversal)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> > @@ -2751,18 +2763,17 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
> >  
> >  	val |= XELPDP_FORWARD_CLOCK_UNGATE;
> >  
> > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) &&
> > -	    is_hdmi_frl(crtc_state->port_clock))
> > +	if (!is_dp && is_hdmi_frl(port_clock))
> >  		val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_DIV18CLK);
> >  	else
> >  		val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_MAXPCLK);
> >  
> >  	/* TODO: HDMI FRL */
> >  	/* DP2.0 10G and 20G rates enable MPLLA*/
> > -	if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == 2000000)
> > -		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
> > +	if (port_clock == 1000000 || port_clock == 2000000)
> > +		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0;
> >  	else
> > -		val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
> > +		val |= pll_state->ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
> >  
> >  	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> >  		     XELPDP_LANE1_PHY_CLOCK_SELECT | XELPDP_FORWARD_CLOCK_UNGATE |
> > @@ -2992,8 +3003,9 @@ static u32 intel_cx0_get_pclk_pll_ack(u8 lane_mask)
> >  	return val;
> >  }
> >  
> > -static void intel_cx0pll_enable(struct intel_encoder *encoder,
> > -				const struct intel_crtc_state *crtc_state)
> > +static void __intel_cx0pll_enable(struct intel_encoder *encoder,
> > +				  const struct intel_cx0pll_state *pll_state,
> > +				  bool is_dp, int port_clock, int lane_count)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> >  	enum phy phy = intel_encoder_to_phy(encoder);
> > @@ -3007,7 +3019,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  	 * 1. Program PORT_CLOCK_CTL REGISTER to configure
> >  	 * clock muxes, gating and SSC
> >  	 */
> > -	intel_program_port_clock_ctl(encoder, crtc_state, lane_reversal);
> > +	intel_program_port_clock_ctl(encoder, pll_state, is_dp, port_clock, lane_reversal);
> >  
> >  	/* 2. Bring PHY out of reset. */
> >  	intel_cx0_phy_lane_reset(encoder, lane_reversal);
> > @@ -3027,15 +3039,15 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  
> >  	/* 5. Program PHY internal PLL internal registers. */
> >  	if (intel_encoder_is_c10phy(encoder))
> > -		intel_c10_pll_program(display, crtc_state, encoder);
> > +		intel_c10_pll_program(display, encoder, &pll_state->c10);
> >  	else
> > -		intel_c20_pll_program(display, crtc_state, encoder);
> > +		intel_c20_pll_program(display, encoder, &pll_state->c20, port_clock, is_dp);
> >  
> >  	/*
> >  	 * 6. Program the enabled and disabled owned PHY lane
> >  	 * transmitters over message bus
> >  	 */
> > -	intel_cx0_program_phy_lane(encoder, crtc_state->lane_count, lane_reversal);
> > +	intel_cx0_program_phy_lane(encoder, lane_count, lane_reversal);
> >  
> >  	/*
> >  	 * 7. Follow the Display Voltage Frequency Switching - Sequence
> > @@ -3046,8 +3058,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  	 * 8. Program DDI_CLK_VALFREQ to match intended DDI
> >  	 * clock frequency.
> >  	 */
> > -	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> > -		       crtc_state->port_clock);
> > +	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
> >  
> >  	/*
> >  	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> > @@ -3074,6 +3085,14 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> >  	intel_cx0_phy_transaction_end(encoder, wakeref);
> >  }
> >  
> > +static void intel_cx0pll_enable(struct intel_encoder *encoder,
> > +				const struct intel_crtc_state *crtc_state)
> > +{
> > +	__intel_cx0pll_enable(encoder, &crtc_state->dpll_hw_state.cx0pll,
> > +			      intel_crtc_has_dp_encoder(crtc_state), crtc_state->port_clock, crtc_state->lane_count);
> > +
> > +}
> > +
> >  int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
> >  {
> >  	struct intel_display *display = to_intel_display(encoder);
> 
> -- 
> Jani Nikula, Intel