| Message ID | 20231204115856.176240-3-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Convert link bitrate to clock | expand |
Hi Mika, > -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@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.Taylor@intel.com> > Signed-off-by: Mika Kahola <mika.kahola@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. --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
> -----Original Message----- > From: Sripada, Radhakrishna <radhakrishna.sripada@intel.com> > Sent: Tuesday, December 5, 2023 3:36 AM > To: Kahola, Mika <mika.kahola@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-bounces@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.Taylor@intel.com> > > Signed-off-by: Mika Kahola <mika.kahola@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? > > --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
Hi Mika, > -----Original Message----- > From: Kahola, Mika <mika.kahola@intel.com> > Sent: Tuesday, December 5, 2023 12:28 AM > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel- > gfx@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.sripada@intel.com> > > Sent: Tuesday, December 5, 2023 3:36 AM > > To: Kahola, Mika <mika.kahola@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-bounces@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.Taylor@intel.com> > > > Signed-off-by: Mika Kahola <mika.kahola@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? --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
> -----Original Message----- > From: Sripada, Radhakrishna <radhakrishna.sripada@intel.com> > Sent: Tuesday, December 5, 2023 8:09 PM > To: Kahola, Mika <mika.kahola@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.kahola@intel.com> > > Sent: Tuesday, December 5, 2023 12:28 AM > > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel- > > gfx@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.sripada@intel.com> > > > Sent: Tuesday, December 5, 2023 3:36 AM > > > To: Kahola, Mika <mika.kahola@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-bounces@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.Taylor@intel.com> > > > > Signed-off-by: Mika Kahola <mika.kahola@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
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); + 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); }