On Wed, 01 Oct 2025, Mika Kahola <[email protected]> wrote:
> From: Imre Deak <[email protected]>
>
> The Cx0 PLL enable programming needs to know if the PLL is in DP or HDMI
> mode. 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
> for Cx0 PHY PLLs by tracking the DP/HDMI mode in the PLL state.
>
> This change has the advantage, that the VDR HW/SW state can be verified
> now.
>
> A follow up change will convert the PLL enable function to retrieve the
> DP/HDMI mode parameter from the PLL state.
>
> This also allows dropping the is_dp and port clock params from the
> intel_c20_pll_program() function, since it can retrieve these now from
> the PLL state.
>
> Signed-off-by: Imre Deak <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 115 +++++++++++++-----
> drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 5 +
> 2 files changed, 89 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index b2fc93954933..3f426f5b4929 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2362,6 +2362,76 @@ intel_c20_pll_tables_get(const struct intel_crtc_state
> *crtc_state,
> return NULL;
> }
>
> +static u8 intel_c20_get_dp_rate(u32 clock);
> +static u8 intel_c20_get_hdmi_rate(u32 clock);
> +static bool is_hdmi_frl(u32 clock);
> +static int intel_get_c20_custom_width(u32 clock, bool dp);
> +
> +static void intel_c20_calc_vdr_params(struct intel_c20pll_vdr_state *vdr,
> bool is_dp,
> + int port_clock)
> +{
> + vdr->custom_width = intel_get_c20_custom_width(port_clock, is_dp);
> +
> + vdr->serdes_rate = 0;
> + vdr->hdmi_rate = 0;
> +
> + if (is_dp) {
> + vdr->serdes_rate = PHY_C20_IS_DP |
> +
> PHY_C20_DP_RATE(intel_c20_get_dp_rate(port_clock));
> + } else {
> + if (is_hdmi_frl(port_clock))
> + vdr->serdes_rate = PHY_C20_IS_HDMI_FRL;
> +
> + vdr->hdmi_rate = intel_c20_get_hdmi_rate(port_clock);
> + }
> +}
> +
> +#define PHY_C20_SERDES_RATE_MASK (PHY_C20_IS_DP | PHY_C20_DP_RATE_MASK |
> PHY_C20_IS_HDMI_FRL)
> +
> +static void intel_c20_readout_vdr_params(struct intel_encoder *encoder,
> + struct intel_c20pll_vdr_state *vdr,
> bool *cntx)
> +{
> + u8 serdes;
> +
> + serdes = intel_cx0_read(encoder, INTEL_CX0_LANE0,
> PHY_C20_VDR_CUSTOM_SERDES_RATE);
> + *cntx = serdes & PHY_C20_CONTEXT_TOGGLE;
> +
> + vdr->custom_width = intel_cx0_read(encoder, INTEL_CX0_LANE0,
> PHY_C20_VDR_CUSTOM_WIDTH) &
> + PHY_C20_CUSTOM_WIDTH_MASK;
> +
> + vdr->serdes_rate = serdes & PHY_C20_SERDES_RATE_MASK;
I'm not a huge fan of storing ŕegister contents directly in state. This
sounds like three separate fields, is_dp, rate, and is_hdmi_frl, or
similar.
> + if (!(vdr->serdes_rate & PHY_C20_IS_DP))
> + vdr->hdmi_rate = intel_cx0_read(encoder, INTEL_CX0_LANE0,
> PHY_C20_VDR_HDMI_RATE) &
> + PHY_C20_HDMI_RATE_MASK;
> + else
> + vdr->hdmi_rate = 0;
> +}
> +
> +static void intel_c20_program_vdr_params(struct intel_encoder *encoder,
> + const struct intel_c20pll_vdr_state
> *vdr,
> + u8 owned_lane_mask)
> +{
> + struct intel_display *display = to_intel_display(encoder);
> +
> + drm_WARN_ON(display->drm, vdr->custom_width &
> ~PHY_C20_CUSTOM_WIDTH_MASK);
> + intel_cx0_rmw(encoder, owned_lane_mask, PHY_C20_VDR_CUSTOM_WIDTH,
> + PHY_C20_CUSTOM_WIDTH_MASK, vdr->custom_width,
> + MB_WRITE_COMMITTED);
> +
> + drm_WARN_ON(display->drm, vdr->serdes_rate & ~PHY_C20_SERDES_RATE_MASK);
> + intel_cx0_rmw(encoder, owned_lane_mask, PHY_C20_VDR_CUSTOM_SERDES_RATE,
> + PHY_C20_SERDES_RATE_MASK, vdr->serdes_rate,
> + MB_WRITE_COMMITTED);
> +
> + if (vdr->serdes_rate & PHY_C20_IS_DP)
Case in point. Please let's not mix software state and hardware register
definitions.
> + return;
> +
> + drm_WARN_ON(display->drm, vdr->hdmi_rate & ~PHY_C20_HDMI_RATE_MASK);
> + intel_cx0_rmw(encoder, INTEL_CX0_BOTH_LANES, PHY_C20_VDR_HDMI_RATE,
> + PHY_C20_HDMI_RATE_MASK, vdr->hdmi_rate,
> + MB_WRITE_COMMITTED);
> +}
> +
> static const struct intel_c20pll_state *
> intel_c20_pll_find_table(const struct intel_crtc_state *crtc_state,
> struct intel_encoder *encoder)
> @@ -2400,19 +2470,26 @@ static int intel_c20pll_calc_state_from_table(struct
> intel_crtc_state *crtc_stat
> static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
> struct intel_encoder *encoder)
> {
> + bool is_dp = intel_crtc_has_dp_encoder(crtc_state);
> int err = -ENOENT;
>
> crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
>
> /* try computed C20 HDMI tables before using consolidated tables */
> - if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> + if (!is_dp)
> /* TODO: Update SSC state for HDMI as well */
> err = intel_c20_compute_hdmi_tmds_pll(crtc_state);
>
> if (err)
> err = intel_c20pll_calc_state_from_table(crtc_state, encoder);
>
> - return err;
> + if (err)
> + return err;
> +
> + intel_c20_calc_vdr_params(&crtc_state->dpll_hw_state.cx0pll.c20.vdr,
> + is_dp, crtc_state->port_clock);
> +
> + return 0;
> }
>
> int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state,
> @@ -2486,8 +2563,8 @@ static void intel_c20pll_readout_hw_state(struct
> intel_encoder *encoder,
>
> wakeref = intel_cx0_phy_transaction_begin(encoder);
>
> - /* 1. Read current context selection */
> - cntx = intel_cx0_read(encoder, INTEL_CX0_LANE0,
> PHY_C20_VDR_CUSTOM_SERDES_RATE) & PHY_C20_CONTEXT_TOGGLE;
> + /* 1. Read VDR params and current context selection */
> + intel_c20_readout_vdr_params(encoder, &pll_state->vdr, &cntx);
>
> /* Read Tx configuration */
> for (i = 0; i < ARRAY_SIZE(pll_state->tx); i++) {
> @@ -2676,11 +2753,9 @@ static int intel_get_c20_custom_width(u32 clock, bool
> dp)
>
> static void intel_c20_pll_program(struct intel_display *display,
> struct intel_encoder *encoder,
> - const struct intel_c20pll_state *pll_state,
> - bool is_dp, int port_clock)
> + const struct intel_c20pll_state *pll_state)
> {
> u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(encoder);
> - u8 serdes;
> bool cntx;
> int i;
>
> @@ -2750,30 +2825,8 @@ static void intel_c20_pll_program(struct intel_display
> *display,
> }
>
> /* 4. Program custom width to match the link protocol */
> - intel_cx0_rmw(encoder, owned_lane_mask, PHY_C20_VDR_CUSTOM_WIDTH,
> - PHY_C20_CUSTOM_WIDTH_MASK,
> -
> PHY_C20_CUSTOM_WIDTH(intel_get_c20_custom_width(port_clock, is_dp)),
> - MB_WRITE_COMMITTED);
> -
> /* 5. For DP or 6. For HDMI */
> - serdes = 0;
> -
> - if (is_dp)
> - serdes = PHY_C20_IS_DP |
> - PHY_C20_DP_RATE(intel_c20_get_dp_rate(port_clock));
> - else if (is_hdmi_frl(port_clock))
> - serdes = PHY_C20_IS_HDMI_FRL;
> -
> - intel_cx0_rmw(encoder, owned_lane_mask, PHY_C20_VDR_CUSTOM_SERDES_RATE,
> - PHY_C20_IS_DP | PHY_C20_DP_RATE_MASK |
> PHY_C20_IS_HDMI_FRL,
> - serdes,
> - MB_WRITE_COMMITTED);
> -
> - if (!is_dp)
> - intel_cx0_rmw(encoder, INTEL_CX0_BOTH_LANES,
> PHY_C20_VDR_HDMI_RATE,
> - PHY_C20_HDMI_RATE_MASK,
> - intel_c20_get_hdmi_rate(port_clock),
> - MB_WRITE_COMMITTED);
> + intel_c20_program_vdr_params(encoder, &pll_state->vdr, owned_lane_mask);
>
> /*
> * 7. Write Vendor specific registers to toggle context setting to load
> @@ -3098,7 +3151,7 @@ static void __intel_cx0pll_enable(struct intel_encoder
> *encoder,
> if (intel_encoder_is_c10phy(encoder))
> intel_c10_pll_program(display, encoder, &pll_state->c10);
> else
> - intel_c20_pll_program(display, encoder, &pll_state->c20, is_dp,
> port_clock);
> + intel_c20_pll_program(display, encoder, &pll_state->c20);
>
> /*
> * 6. Program the enabled and disabled owned PHY lane
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index f131bdd1c975..43c7200050e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -255,6 +255,11 @@ struct intel_c20pll_state {
> u16 mplla[10];
> u16 mpllb[11];
> };
> + struct intel_c20pll_vdr_state {
> + u8 custom_width;
> + u8 serdes_rate;
> + u8 hdmi_rate;
> + } vdr;
> };
>
> struct intel_cx0pll_state {
--
Jani Nikula, Intel
