> -----Original Message----- > From: Sripada, Radhakrishna <radhakrishna.srip...@intel.com> > Sent: Tuesday, December 5, 2023 8:09 PM > To: Kahola, Mika <mika.kah...@intel.com>; intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH 2/3] drm/i915/display: Convert link bitrate > to corresponding PLL clock > > Hi Mika, > > > -----Original Message----- > > From: Kahola, Mika <mika.kah...@intel.com> > > Sent: Tuesday, December 5, 2023 12:28 AM > > To: Sripada, Radhakrishna <radhakrishna.srip...@intel.com>; intel- > > g...@lists.freedesktop.org > > Subject: RE: [Intel-gfx] [PATCH 2/3] drm/i915/display: Convert link > > bitrate to corresponding PLL clock > > > > > -----Original Message----- > > > From: Sripada, Radhakrishna <radhakrishna.srip...@intel.com> > > > Sent: Tuesday, December 5, 2023 3:36 AM > > > To: Kahola, Mika <mika.kah...@intel.com>; > > > intel-gfx@lists.freedesktop.org > > > Subject: RE: [Intel-gfx] [PATCH 2/3] drm/i915/display: Convert link > > > bitrate to > > corresponding PLL clock > > > > > > Hi Mika, > > > > > > > -----Original Message----- > > > > From: Intel-gfx <intel-gfx-boun...@lists.freedesktop.org> On > > > > Behalf Of Mika Kahola > > > > Sent: Monday, December 4, 2023 3:59 AM > > > > To: intel-gfx@lists.freedesktop.org > > > > Subject: [Intel-gfx] [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.tay...@intel.com> > > > > Signed-off-by: Mika Kahola <mika.kah...@intel.com> > > > > --- > > > > 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