> -----Original Message-----
> From: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> Sent: Tuesday, December 5, 2023 8:09 PM
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/3] drm/i915/display: Convert link bitrate to corresponding PLL clock
>
> Hi Mika,
>
> > -----Original Message-----
> > From: Kahola, Mika <mika.kahola@xxxxxxxxx>
> > Sent: Tuesday, December 5, 2023 12:28 AM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>; intel-
> > gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: RE: [PATCH 2/3] drm/i915/display: Convert link
> > bitrate to corresponding PLL clock
> >
> > > -----Original Message-----
> > > From: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> > > Sent: Tuesday, December 5, 2023 3:36 AM
> > > To: Kahola, Mika <mika.kahola@xxxxxxxxx>;
> > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Subject: RE: [PATCH 2/3] drm/i915/display: Convert link
> > > bitrate to
> > corresponding PLL clock
> > >
> > > Hi Mika,
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On
> > > > Behalf Of Mika Kahola
> > > > Sent: Monday, December 4, 2023 3:59 AM
> > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Subject: [PATCH 2/3] drm/i915/display: Convert link
> > > > bitrate to corresponding PLL clock
> > > >
> > > > Compute clock during PLL readout. This prevents warn when only c20
> > > > phy is connected during modprobe. The
> > > > intel_c20pll_calc_port_clock() function returns link bitrate which
> > > > in DP2.0 and HDMI cases does not match with the clock rate. Hence,
> > > > conversion function is needed to convert link bitrate to corresponding PLL clock rate.
> > > >
> > > > while at it, update clock on C10 pll state as well.
> > > >
> > > > Signed-off-by: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
> > > > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 38
> > > > ++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > > > ++++++++++++++++++| 1 +
> > > > drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> > > > 3 files changed, 37 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > index 2e6412fc2258..02efe2786c6a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > > @@ -1871,6 +1871,7 @@ static int intel_c10pll_calc_state(struct
> > > > intel_crtc_state *crtc_state, }
> > > >
> > > > static void intel_c10pll_readout_hw_state(struct intel_encoder
> > > > *encoder,
> > > > + struct intel_crtc_state *crtc_state,
> > > > struct intel_c10pll_state *pll_state) {
> > > > struct drm_i915_private *i915 = to_i915(encoder->base.dev); @@
> > > > -1894,6 +1895,7 @@ static void
> > > > intel_c10pll_readout_hw_state(struct
> > > > intel_encoder *encoder,
> > > >
> > > > pll_state->cmn = intel_cx0_read(i915, encoder->port, lane,
> > > > PHY_C10_VDR_CMN(0));
> > > > pll_state->tx = intel_cx0_read(i915, encoder->port, lane,
> > > > PHY_C10_VDR_TX(0));
> > > > + pll_state->clock = crtc_state->port_clock;
> > > >
> > > > intel_cx0_phy_transaction_end(encoder, wakeref); } @@ -2445,12
> > > > +2447,33 @@ static void intel_program_port_clock_ctl(struct
> > > > intel_encoder *encoder,
> > > > XELPDP_SSC_ENABLE_PLLB, val); }
> > > >
> > > > +static int intel_link_bitrate_to_clock(struct intel_encoder *encoder,
> > > > + struct intel_crtc_state *crtc_state,
> > > > + int link_bit_rate)
> > > > +{
> > > > + const struct intel_c20pll_state * const *tables;
> > > > + int i;
> > > > +
> > > > + tables = intel_c20_pll_tables_get(crtc_state, encoder);
> > > This will produce incorrect result. intel_c20_pll_tables_get depends
> > > on
> > intel_crtc_has_{dp_encoder,hdmi..} which depends
> > > crtc_state->output_types to determine edp/dp/hdmi table which is not
> > initialized until later in mtl_ddi_init_config under
> > > intel_ddi_get_config -> intel_ddi_read_func_ctl
> > >
> > > We might be needing a separate sanitization of initial pll state to
> > > be done after
> > intel_ddi_get_config. Or a special case to handle
> > > initial modeset.
> > I actually noticed this while testing it that at first we don't even
> > get the tables and function returns with -EINVAL. Eventually we do get the correct table and clock.
> > Maybe it would be better to simply use the if-else ladder for those
> > cases that differs from link bitrate vs. clock?
> I am skeptical about making 2 different entries for the same data. Why don’t we make intel_c20_get_dp_rate work on link bit rate
> numbers which is what intel_c20pll_calc_port_clock returns and that is what is stored in crtc_state/pipe_config->port_clock and
> use this field to program instead of relying on pll_state->clock in step5 of pll programming sequence?
Probably, we need to clean this up properly. Let's discard this patch and work on link bit rate rather than clock. We probably end up cleaner solution.
-Mika-
>
> --Radhakrishna
> >
> >
> > >
> > > --Radhakrishna Sripada
> > > > + if (!tables)
> > > > + return -EINVAL;
> > > > +
> > > > + for (i = 0; tables[i]; i++) {
> > > > + if (link_bit_rate == tables[i]->link_bit_rate)
> > > > + return tables[i]->clock;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > static void intel_c20pll_readout_hw_state(struct intel_encoder
> > > > *encoder,
> > > > + struct intel_crtc_state *crtc_state,
> > > > struct intel_c20pll_state *pll_state) {
> > > > struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > > bool cntx;
> > > > intel_wakeref_t wakeref;
> > > > + int clock;
> > > > int i;
> > > >
> > > > wakeref = intel_cx0_phy_transaction_begin(encoder);
> > > > @@ -2500,6 +2523,13 @@ static void
> > > > intel_c20pll_readout_hw_state(struct
> > > > intel_encoder *encoder,
> > > > }
> > > > }
> > > >
> > > > + pll_state->link_bit_rate = intel_c20pll_calc_port_clock(encoder,
> > > > pll_state);
> > > > + clock = intel_link_bitrate_to_clock(encoder, crtc_state,
> > > > + pll_state->link_bit_rate);
> > > > +
> > > > + if (clock >= 0)
> > > > + pll_state->clock = clock;
> > > > +
> > > > intel_cx0_phy_transaction_end(encoder, wakeref); }
> > > >
> > > > @@ -3053,15 +3083,16 @@ static void
> > > > intel_c10pll_state_verify(const struct intel_crtc_state *state, }
> > > >
> > > > void intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
> > > > + struct intel_crtc_state *crtc_state,
> > > > struct intel_cx0pll_state *pll_state) {
> > > > struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > > enum phy phy = intel_port_to_phy(i915, encoder->port);
> > > >
> > > > if (intel_is_c10phy(i915, phy))
> > > > - intel_c10pll_readout_hw_state(encoder, &pll_state->c10);
> > > > + intel_c10pll_readout_hw_state(encoder, crtc_state, &pll_state-
> > > > >c10);
> > > > else
> > > > - intel_c20pll_readout_hw_state(encoder, &pll_state->c20);
> > > > + intel_c20pll_readout_hw_state(encoder, crtc_state, &pll_state-
> > > > >c20);
> > > > }
> > > >
> > > > int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
> > > > @@
> > > > -3145,7 +3176,8 @@ void intel_cx0pll_state_verify(struct
> > > > intel_atomic_state *state,
> > > > if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
> > > > return;
> > > >
> > > > - intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
> > > > + intel_cx0pll_readout_hw_state(encoder, (struct
> > > > intel_crtc_state*)new_crtc_state,
> > > > + &mpll_hw_state);
> > > >
> > > > if (intel_is_c10phy(i915, phy))
> > > > intel_c10pll_state_verify(new_crtc_state, crtc, encoder,
> > > > &mpll_hw_state.c10); diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > > > index c6682677253a..eac7354e9a4e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > > > @@ -32,6 +32,7 @@ intel_mtl_port_pll_type(struct intel_encoder
> > > > *encoder,
> > > >
> > > > int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state,
> > > > struct intel_encoder *encoder); void
> > > > intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
> > > > + struct intel_crtc_state *crtc_state,
> > > > struct intel_cx0pll_state *pll_state); int
> > > > intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
> > > > const struct intel_cx0pll_state *pll_state); diff -
> > -git
> > > > a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index 38f28c480b38..508d3363e89f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -3953,7 +3953,7 @@ static void mtl_ddi_get_config(struct
> > > > intel_encoder *encoder,
> > > > if (intel_tc_port_in_tbt_alt_mode(dig_port)) {
> > > > crtc_state->port_clock =
> > > > intel_mtl_tbt_calc_port_clock(encoder);
> > > > } else {
> > > > - intel_cx0pll_readout_hw_state(encoder, &crtc_state-
> > > > >cx0pll_state);
> > > > + intel_cx0pll_readout_hw_state(encoder, crtc_state, &crtc_state-
> > > > >cx0pll_state);
> > > > crtc_state->port_clock = intel_cx0pll_calc_port_clock(encoder,
> > > > &crtc_state->cx0pll_state);
> > > > }
> > > >
> > > > --
> > > > 2.34.1
