On Wed, 01 Oct 2025, Mika Kahola <[email protected]> wrote:
> From: Imre Deak <[email protected]>
>
> The Cx0 PLL enable programming requires the enabled lane count. The PLL
> manager framework doesn't pass the CRTC state to the PLL's enable hook,
> so prepare here for the conversion to use the PLL manager, by tracking
> the enabled lane count in the PLL state as well. This has the advantage,
> that the enabled lane count can be verified against the PHY/PLL's
> enabled TX lanes.
>
> This also allows dropping the lane count param from the
> __intel_cx0pll_enable() function, since it can retrieve this now from
> the PLL state.
>
> Signed-off-by: Imre Deak <[email protected]>
> Signed-off-by: Mika Kahola <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 55 ++++++++++++++++---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 1 +
> 2 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 2aba1ebae428..d69ff9115659 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -12,6 +12,7 @@
> #include "intel_alpm.h"
> #include "intel_cx0_phy.h"
> #include "intel_cx0_phy_regs.h"
> +#include "intel_display_regs.h"
> #include "intel_ddi.h"
> #include "intel_ddi_buf_trans.h"
> #include "intel_de.h"
> @@ -2083,7 +2084,7 @@ static void intel_c10pll_update_pll(struct
> intel_encoder *encoder,
> */
> static int intel_c10pll_calc_state_from_table(struct intel_encoder *encoder,
> const struct intel_c10pll_state *
> const *tables,
> - bool is_dp, int port_clock,
> + bool is_dp, int port_clock, int
> lane_count,
> struct intel_cx0pll_state
> *pll_state)
> {
> int i;
> @@ -2093,7 +2094,9 @@ static int intel_c10pll_calc_state_from_table(struct
> intel_encoder *encoder,
> pll_state->c10 = *tables[i];
> intel_cx0pll_update_ssc(encoder, pll_state, is_dp);
> intel_c10pll_update_pll(encoder, pll_state);
> +
> pll_state->use_c10 = true;
> + pll_state->lane_count = lane_count;
>
> return 0;
> }
> @@ -2114,7 +2117,7 @@ static int intel_c10pll_calc_state(struct
> intel_crtc_state *crtc_state,
>
> err = intel_c10pll_calc_state_from_table(encoder, tables,
>
> intel_crtc_has_dp_encoder(crtc_state),
> - crtc_state->port_clock,
> + crtc_state->port_clock,
> crtc_state->lane_count,
>
> &crtc_state->dpll_hw_state.cx0pll);
>
> if (err == 0 || !intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> @@ -2126,6 +2129,7 @@ static int intel_c10pll_calc_state(struct
> intel_crtc_state *crtc_state,
> intel_c10pll_update_pll(encoder,
> &crtc_state->dpll_hw_state.cx0pll);
> crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> + crtc_state->dpll_hw_state.cx0pll.lane_count = crtc_state->lane_count;
>
> return 0;
> }
> @@ -2157,6 +2161,37 @@ static int intel_c10pll_calc_port_clock(struct
> intel_encoder *encoder,
> return tmpclk;
> }
>
> +static int readout_enabled_lane_count(struct intel_encoder *encoder)
> +{
> + struct intel_display *display = to_intel_display(encoder);
> + u8 enabled_tx_lane_count = 0;
> + int max_tx_lane_count;
> + int tx_lane;
> +
> + /*
> + * TODO: also check inactive TX lanes in all PHY lanes owned by the
> + * display. For now checking only those PHY lane(s) which are owned
> + * based on the active TX lane count (i.e.
> + * 1,2 active TX lanes -> PHY lane#0
> + * 3,4 active TX lanes -> PHY lane#0 and PHY lane#1).
> + */
> + max_tx_lane_count = DDI_PORT_WIDTH_GET(intel_de_read(display,
> DDI_BUF_CTL(encoder->port)));
Hmm, feels slightly wrong to be touching DDI_BUF_CTL() here. I think
it's already being used in too many places. But I'm not sure what the
clean alternative should be either.
> + if (!drm_WARN_ON(display->drm, max_tx_lane_count == 0))
> + max_tx_lane_count = roundup_pow_of_two(max_tx_lane_count);
So I don't want to see WARN and the happy day scenario mixed like this.
This is fine, and common:
if (WARN_ON(something that should not happen))
handle_the_error_case();
But not:
if (!WARN_ON(something_that_should_not_happen))
handle_the_normal_case();
Looks like this could be just:
if (drm_WARN_ON(display->drm, max_tx_lane_count == 0))
return 0;
max_tx_lane_count = roundup_pow_of_two(max_tx_lane_count);
And it reads like it should.
> +
> + for (tx_lane = 0; tx_lane < max_tx_lane_count; tx_lane++) {
> + u8 phy_lane_mask = tx_lane < 2 ? INTEL_CX0_LANE0 :
> INTEL_CX0_LANE1;
> + int tx = tx_lane % 2 + 1;
> + u8 val;
> +
> + val = intel_cx0_read(encoder, phy_lane_mask,
> PHY_CX0_TX_CONTROL(tx, 2));
> + if (!(val & CONTROL2_DISABLE_SINGLE_TX))
> + enabled_tx_lane_count++;
> + }
> +
> + return enabled_tx_lane_count;
> +}
> +
> static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
> struct intel_cx0pll_state
> *cx0pll_state)
> {
> @@ -2175,6 +2210,8 @@ static void intel_c10pll_readout_hw_state(struct
> intel_encoder *encoder,
> */
> intel_c10_msgbus_access_begin(encoder, lane);
>
> + cx0pll_state->lane_count = readout_enabled_lane_count(encoder);
> +
> for (i = 0; i < ARRAY_SIZE(pll_state->pll); i++)
> pll_state->pll[i] = intel_cx0_read(encoder, lane,
> PHY_C10_VDR_PLL(i));
>
> @@ -2581,6 +2618,7 @@ static int intel_c20pll_calc_state(struct
> intel_crtc_state *crtc_state,
> int err = -ENOENT;
>
> crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
> + crtc_state->dpll_hw_state.cx0pll.lane_count = crtc_state->lane_count;
>
> /* try computed C20 HDMI tables before using consolidated tables */
> if (!is_dp)
> @@ -2670,6 +2708,8 @@ static void intel_c20pll_readout_hw_state(struct
> intel_encoder *encoder,
>
> wakeref = intel_cx0_phy_transaction_begin(encoder);
>
> + cx0pll_state->lane_count = readout_enabled_lane_count(encoder);
> +
> /* 1. Read VDR params and current context selection */
> intel_c20_readout_vdr_params(encoder, &pll_state->vdr, &cntx);
>
> @@ -3107,7 +3147,7 @@ static u32 intel_cx0_get_pclk_pll_ack(u8 lane_mask)
>
> 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)
> + bool is_dp, int port_clock)
> {
> struct intel_display *display = to_intel_display(encoder);
> enum phy phy = intel_encoder_to_phy(encoder);
> @@ -3149,7 +3189,7 @@ static void __intel_cx0pll_enable(struct intel_encoder
> *encoder,
> * 6. Program the enabled and disabled owned PHY lane
> * transmitters over message bus
> */
> - intel_cx0_program_phy_lane(encoder, lane_count, lane_reversal);
> + intel_cx0_program_phy_lane(encoder, pll_state->lane_count,
> lane_reversal);
>
> /*
> * 7. Follow the Display Voltage Frequency Switching - Sequence
> @@ -3192,7 +3232,7 @@ static void intel_cx0pll_enable(struct intel_encoder
> *encoder,
> {
> __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);
> + crtc_state->port_clock);
> }
>
> int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
> @@ -3723,6 +3763,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display
> *display)
> for_each_intel_encoder(display->drm, encoder) {
> struct intel_cx0pll_state pll_state = {};
> int port_clock = 162000;
> + int lane_count = 4;
>
> if (!intel_encoder_is_dig_port(encoder))
> continue;
> @@ -3735,7 +3776,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display
> *display)
>
> if (intel_c10pll_calc_state_from_table(encoder,
> mtl_c10_edp_tables,
> - true, port_clock,
> + true, port_clock,
> lane_count,
> &pll_state) < 0) {
> drm_WARN_ON(display->drm,
> "Unable to calc C10 state from the
> tables\n");
> @@ -3746,7 +3787,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display
> *display)
> "[ENCODER:%d:%s] Applying power saving workaround
> on disabled PLL\n",
> encoder->base.base.id, encoder->base.name);
>
> - __intel_cx0pll_enable(encoder, &pll_state, true, port_clock, 4);
> + __intel_cx0pll_enable(encoder, &pll_state, true, port_clock);
> intel_cx0pll_disable(encoder);
> }
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index 43c7200050e9..839b1a98534f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -267,6 +267,7 @@ struct intel_cx0pll_state {
> struct intel_c10pll_state c10;
> struct intel_c20pll_state c20;
> };
> + int lane_count;
> bool ssc_enabled;
> bool use_c10;
> bool tbt_mode;
--
Jani Nikula, Intel
