> -----Original Message----- > From: Kandpal, Suraj <[email protected]> > Sent: Wednesday, 12 November 2025 6.29 > To: Kahola, Mika <[email protected]>; [email protected]; > [email protected] > Cc: Kahola, Mika <[email protected]>; Deak, Imre <[email protected]> > Subject: RE: [CI 17/32] drm/i915/display: Update C10/C20 state calculation > > > Subject: [CI 17/32] drm/i915/display: Update C10/C20 state calculation > > > > For the dpll framework, the state must be computed into a port PLL > > state, which is separate from the dpll_hw_state in crtc_state. > > You have state the problem here but failed to mention what the commit does. > Also port PLL state? Ok, I will try to reword the commit message to better reflect what the patch does.
> Also two different changes are happening here struct crtc_state argument > becomes const struct crtc_state and a addition of a > new param dpll_hw_state maybe these need to be two separate patches. Switching to const struct crtc_state seemed to me a small change that could embedded into this patch. I will try to reword commit message include reasoning for this change. > > > > > Signed-off-by: Imre Deak <[email protected]> > > Signed-off-by: Mika Kahola <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 68 > > ++++++++++---------- drivers/gpu/drm/i915/display/intel_cx0_phy.h | 5 +- > > drivers/gpu/drm/i915/display/intel_dpll.c | 2 +- > > 3 files changed, 40 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > index 5332f33800e7..f5e6634a6389 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > @@ -2034,7 +2034,7 @@ static const struct intel_c20pll_state * const > > mtl_c20_hdmi_tables[] = { }; > > > > static const struct intel_c10pll_state * const * > > -intel_c10pll_tables_get(struct intel_crtc_state *crtc_state, > > +intel_c10pll_tables_get(const struct intel_crtc_state *crtc_state, > > struct intel_encoder *encoder) > > { > > if (intel_crtc_has_dp_encoder(crtc_state)) { @@ -2138,8 +2138,9 @@ > > static int intel_c10pll_calc_state_from_table(struct intel_encoder *encoder, > > return -EINVAL; > > } > > > > -static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static int intel_c10pll_calc_state(const struct intel_crtc_state > > *crtc_state, > > + struct intel_encoder *encoder, > > + struct intel_dpll_hw_state *hw_state) > > { > > struct intel_display *display = to_intel_display(encoder); > > bool is_dp = intel_crtc_has_dp_encoder(crtc_state); > > @@ -2152,21 +2153,20 @@ static int intel_c10pll_calc_state(struct > > intel_crtc_state *crtc_state, > > > > err = intel_c10pll_calc_state_from_table(encoder, tables, is_dp, > > crtc_state->port_clock, > > crtc_state->lane_count, > > - &crtc_state- > > >dpll_hw_state.cx0pll); > > + &hw_state->cx0pll); > > > > if (err == 0 || !intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) > > return err; > > > > /* For HDMI PLLs try SNPS PHY algorithm, if there are no precomputed > > tables */ > > - intel_snps_hdmi_pll_compute_c10pll(&crtc_state- > > >dpll_hw_state.cx0pll.c10, > > + intel_snps_hdmi_pll_compute_c10pll(&hw_state->cx0pll.c10, > > crtc_state->port_clock); > > - 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; > > + intel_c10pll_update_pll(encoder, &hw_state->cx0pll); > > > > - drm_WARN_ON(display->drm, > > - is_dp != c10pll_state_is_dp(&crtc_state- > > >dpll_hw_state.cx0pll.c10)); > > + hw_state->cx0pll.use_c10 = true; > > + hw_state->cx0pll.lane_count = crtc_state->lane_count; > > + > > + drm_WARN_ON(display->drm, is_dp != > > +c10pll_state_is_dp(&hw_state->cx0pll.c10)); > > > > return 0; > > } > > @@ -2355,7 +2355,7 @@ static bool is_arrowlake_s_by_host_bridge(void) > > return pdev && > > IS_ARROWLAKE_S_BY_HOST_BRIDGE_ID(host_bridge_pci_dev_id); > > } > > > > -static u16 intel_c20_hdmi_tmds_tx_cgf_1(struct intel_crtc_state > > *crtc_state) > > +static u16 intel_c20_hdmi_tmds_tx_cgf_1(const struct intel_crtc_state > > +*crtc_state) > > { > > struct intel_display *display = to_intel_display(crtc_state); > > u16 tx_misc; > > @@ -2379,9 +2379,9 @@ static u16 intel_c20_hdmi_tmds_tx_cgf_1(struct > > intel_crtc_state *crtc_state) > > C20_PHY_TX_DCC_BYPASS | > > C20_PHY_TX_TERM_CTL(tx_term_ctrl)); > > } > > > > -static int intel_c20_compute_hdmi_tmds_pll(struct intel_crtc_state > > *crtc_state) > > +static int intel_c20_compute_hdmi_tmds_pll(const struct > > +intel_crtc_state > > *crtc_state, > > + struct intel_c20pll_state *pll_state) > > { > > - struct intel_c20pll_state *pll_state = &crtc_state- > > >dpll_hw_state.cx0pll.c20; > > u64 datarate; > > u64 mpll_tx_clk_div; > > u64 vco_freq_shift; > > @@ -2648,8 +2648,9 @@ intel_c20_pll_find_table(const struct > > intel_crtc_state *crtc_state, > > return NULL; > > } > > > > -static int intel_c20pll_calc_state_from_table(struct intel_crtc_state > > *crtc_state, > > - struct intel_encoder *encoder) > > +static int intel_c20pll_calc_state_from_table(const struct > > +intel_crtc_state > > *crtc_state, > > + struct intel_encoder *encoder, > > + struct intel_cx0pll_state > > *pll_state) > > { > > const struct intel_c20pll_state *table; > > > > @@ -2657,52 +2658,53 @@ static int > > intel_c20pll_calc_state_from_table(struct > > intel_crtc_state *crtc_stat > > if (!table) > > return -EINVAL; > > > > - crtc_state->dpll_hw_state.cx0pll.c20 = *table; > > + pll_state->c20 = *table; > > > > - intel_cx0pll_update_ssc(encoder, &crtc_state->dpll_hw_state.cx0pll, > > - intel_crtc_has_dp_encoder(crtc_state)); > > + intel_cx0pll_update_ssc(encoder, pll_state, > > +intel_crtc_has_dp_encoder(crtc_state)); > > > > return 0; > > } > > > > -static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +static int intel_c20pll_calc_state(const struct intel_crtc_state > > *crtc_state, > > + struct intel_encoder *encoder, > > + struct intel_dpll_hw_state *hw_state) > > { > > struct intel_display *display = to_intel_display(encoder); > > bool is_dp = intel_crtc_has_dp_encoder(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; > > + hw_state->cx0pll.use_c10 = false; > > + hw_state->cx0pll.lane_count = crtc_state->lane_count; > > > > /* try computed C20 HDMI tables before using consolidated tables */ > > if (!is_dp) > > /* TODO: Update SSC state for HDMI as well */ > > - err = intel_c20_compute_hdmi_tmds_pll(crtc_state); > > + err = intel_c20_compute_hdmi_tmds_pll(crtc_state, > > +&hw_state->cx0pll.c20); > > > > if (err) > > - err = intel_c20pll_calc_state_from_table(crtc_state, encoder); > > + err = intel_c20pll_calc_state_from_table(crtc_state, encoder, > > + &hw_state->cx0pll); > > > > if (err) > > return err; > > > > - intel_c20_calc_vdr_params(&crtc_state->dpll_hw_state.cx0pll.c20.vdr, > > + intel_c20_calc_vdr_params(&hw_state->cx0pll.c20.vdr, > > is_dp, crtc_state->port_clock); > > > > - drm_WARN_ON(display->drm, > > - is_dp != c20pll_state_is_dp(&crtc_state- > > >dpll_hw_state.cx0pll.c20)); > > + drm_WARN_ON(display->drm, is_dp != > > +c20pll_state_is_dp(&hw_state->cx0pll.c20)); > > > > return 0; > > } > > > > -int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state, > > - struct intel_encoder *encoder) > > +int intel_cx0pll_calc_state(const struct intel_crtc_state *crtc_state, > > + struct intel_encoder *encoder, > > + struct intel_dpll_hw_state *hw_state) > > { > > - memset(&crtc_state->dpll_hw_state, 0, sizeof(crtc_state- > > >dpll_hw_state)); > > + memset(hw_state, 0, sizeof(*hw_state)); > > > > if (intel_encoder_is_c10phy(encoder)) > > - return intel_c10pll_calc_state(crtc_state, encoder); > > - return intel_c20pll_calc_state(crtc_state, encoder); > > + return intel_c10pll_calc_state(crtc_state, encoder, hw_state); > > + return intel_c20pll_calc_state(crtc_state, encoder, hw_state); > > } > > > > static bool intel_c20phy_use_mpllb(const struct intel_c20pll_state > > *state) diff -- git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > index 2b934b96af81..7b88c3fe9de1 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > @@ -16,6 +16,7 @@ struct intel_crtc; > > struct intel_crtc_state; > > struct intel_cx0pll_state; > > struct intel_display; > > +struct intel_dpll_hw_state; > > struct intel_encoder; > > struct intel_hdmi; > > > > @@ -27,7 +28,9 @@ enum icl_port_dpll_id > > intel_mtl_port_pll_type(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state); > > > > -int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state, > > struct intel_encoder *encoder); > > +int intel_cx0pll_calc_state(const struct intel_crtc_state *crtc_state, > > + struct intel_encoder *encoder, > > + struct intel_dpll_hw_state *hw_state); > > void intel_cx0pll_readout_hw_state(struct intel_encoder *encoder, > > struct intel_cx0pll_state *pll_state); int > > intel_cx0pll_calc_port_clock(struct intel_encoder *encoder, diff --git > > a/drivers/gpu/drm/i915/display/intel_dpll.c > > b/drivers/gpu/drm/i915/display/intel_dpll.c > > index f969c5399a51..7a48d6f0db10 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll.c > > +++ b/drivers/gpu/drm/i915/display/intel_dpll.c > > @@ -1220,7 +1220,7 @@ static int mtl_crtc_compute_clock(struct > > intel_atomic_state *state, > > intel_get_crtc_new_encoder(state, crtc_state); > > int ret; > > > > - ret = intel_cx0pll_calc_state(crtc_state, encoder); > > + ret = intel_cx0pll_calc_state(crtc_state, encoder, > > +&crtc_state->dpll_hw_state); > > So you are adding a new param which can be derived from the param you are > still passing to the function ? At this stage yes as the intel_cx0pll_calc_state() changed. This is, however, an intermediate step as we end up removing the whole mtl_crtc_compute_clock() function. > > Regards, > Suraj Kandpal > > > if (ret) > > return ret; > > > > -- > > 2.34.1
