[PATCH 1/2] drm/i915/display: Drop crtc_state from C10/C20 pll programming
Kahola, Mika
mika.kahola at intel.com
Thu Jan 30 09:26:29 UTC 2025
> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, 29 January 2025 16.40
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org
> Cc: Deak, Imre <imre.deak at intel.com>; Kahola, Mika <mika.kahola at 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 at 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 at 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
More information about the Intel-xe
mailing list