On Tue, Feb 07, 2023 at 01:52:20PM -0300, Gustavo Sousa wrote: > I feel like some parts of this patch actually belong to previous patches as > fixups instead, namely the implementation and usage of > intel_cx0_phy_transaction_{begin,end}() and other fixes unrelated to vswing > programming. > > Also, please see some comments inline. > > On Thu, Jan 05, 2023 at 02:54:31PM +0200, Mika Kahola wrote: > > From: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx> > > > > C10 phys uses direct mapping internally for voltage and pre-emphasis levels. > > Program the levels directly to the fields in the VDR Registers. > > > > Bspec: 65449 > > > > v2: From table "C10: Tx EQ settings for DP 1.4x" it shows level 1 > > and preemphasis 1 instead of two times of level 1 preemphasis 0. > > Fix this in the driver code as well. > > > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Cc: Uma Shankar <uma.shankar@xxxxxxxxx> > > Signed-off-by: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx> > > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx> > > Link: https://patchwork.freedesktop.org/patch/msgid/20221014124740.774835-7-mika.kahola@xxxxxxxxx > > --- > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 131 ++++++++++++++++-- > > drivers/gpu/drm/i915/display/intel_cx0_phy.h | 2 + > > .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 6 + > > drivers/gpu/drm/i915/display/intel_ddi.c | 4 +- > > .../drm/i915/display/intel_ddi_buf_trans.c | 36 ++++- > > .../drm/i915/display/intel_ddi_buf_trans.h | 6 + > > .../i915/display/intel_display_power_map.c | 1 + > > 7 files changed, 175 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > index 746c905538a7..3d86b0f1c36d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > @@ -6,10 +6,14 @@ > > #include "i915_reg.h" > > #include "intel_cx0_phy.h" > > #include "intel_cx0_phy_regs.h" > > +#include "intel_ddi.h" > > +#include "intel_ddi_buf_trans.h" > > #include "intel_de.h" > > #include "intel_display_types.h" > > #include "intel_dp.h" > > #include "intel_panel.h" > > +#include "intel_psr.h" > > +#include "intel_uncore.h" > > > > bool intel_is_c10phy(struct drm_i915_private *dev_priv, enum phy phy) > > { > > @@ -19,6 +23,15 @@ bool intel_is_c10phy(struct drm_i915_private *dev_priv, enum phy phy) > > return false; > > } > > > > +static void > > +assert_dc_off(struct drm_i915_private *i915) > > +{ > > + bool enabled; > > + > > + enabled = intel_display_power_is_enabled(i915, POWER_DOMAIN_DC_OFF); > > + drm_WARN_ON(&i915->drm, !enabled); > > +} > > + > > static void intel_cx0_bus_reset(struct drm_i915_private *i915, enum port port, int lane) > > { > > enum phy phy = intel_port_to_phy(i915, port); > > @@ -111,6 +124,8 @@ static u8 intel_cx0_read(struct drm_i915_private *i915, enum port port, > > int i, status = 0; > > u32 val; > > > > + assert_dc_off(i915); > > + > > for (i = 0; i < 3; i++) { > > status = __intel_cx0_read(i915, port, lane, addr, &val); > > > > @@ -193,6 +208,8 @@ static void __intel_cx0_write(struct drm_i915_private *i915, enum port port, > > enum phy phy = intel_port_to_phy(i915, port); > > int i, status; > > > > + assert_dc_off(i915); > > + > > for (i = 0; i < 3; i++) { > > status = __intel_cx0_write_once(i915, port, lane, addr, data, committed); > > > > @@ -240,6 +257,80 @@ static void intel_cx0_rmw(struct drm_i915_private *i915, enum port port, > > } > > } > > > > +/* > > + * Prepare HW for CX0 phy transactions. > > + * > > + * It is required that PSR and DC5/6 are disabled before any CX0 message > > + * bus transaction is executed. > > + */ > > +static intel_wakeref_t intel_cx0_phy_transaction_begin(struct intel_encoder *encoder) > > +{ > > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + intel_psr_pause(intel_dp); > > Shouldn't we check if this encoder is really for DP before calling > intel_psr_{pause,resume}()? > > > + return intel_display_power_get(i915, POWER_DOMAIN_DC_OFF); > > +} > > + > > +static void intel_cx0_phy_transaction_end(struct intel_encoder *encoder, intel_wakeref_t wakeref) > > +{ > > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + intel_psr_resume(intel_dp); > > + intel_display_power_put(i915, POWER_DOMAIN_DC_OFF, wakeref); > > +} > > + > > +void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + bool lane_reversal = dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL; > > + u8 master_lane = lane_reversal ? INTEL_CX0_LANE1 : > > + INTEL_CX0_LANE0; > > + u8 follower_lane = lane_reversal ? INTEL_CX0_LANE0 : > > + INTEL_CX0_LANE1; > > + const struct intel_ddi_buf_trans *trans; > > + intel_wakeref_t wakeref; > > + int n_entries, ln; > > + > > + wakeref = intel_cx0_phy_transaction_begin(encoder); > > + > > + trans = encoder->get_buf_trans(encoder, crtc_state, &n_entries); > > + if (drm_WARN_ON_ONCE(&i915->drm, !trans)) > > + return; > > + > > + intel_cx0_rmw(i915, encoder->port, INTEL_CX0_BOTH_LANES, PHY_C10_VDR_CONTROL(1), > > + 0, C10_VDR_CTRL_MSGBUS_ACCESS, MB_WRITE_COMMITTED); > > The register PHY_C10_VDR_CONTROL(1) is only defined for the C10 phy and this > step is only documented for C10. Maybe we should do this conditionally on the > phy being the C10. > > This would also apply for other places doing the same step (in this or other > patches). > > > + > > + for (ln = 0; ln < 4; ln++) { > > + int level = intel_ddi_level(encoder, crtc_state, ln); > > + int lane, tx; > > + > > + lane = ln / 2 + 1; > > + tx = ln % 2 + 1; > > + > > + intel_cx0_rmw(i915, encoder->port, lane, PHY_CX0_TX_CONTROL(tx, 2), > > + C10_PHY_VSWING_PREEMPH_MASK, > > + C10_PHY_VSWING_PREEMPH(trans->entries[level].direct.preemph), > > + MB_WRITE_COMMITTED); > > + intel_cx0_rmw(i915, encoder->port, lane, PHY_CX0_TX_CONTROL(tx, 8), > > + C10_PHY_VSWING_LEVEL_MASK, > > + C10_PHY_VSWING_LEVEL(trans->entries[level].direct.level), > > + MB_WRITE_COMMITTED); > > + } > > Cc: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx> Forgot to add Clint in the loop :-) > > Clint has done the proper implementation for this (via override method) in our > internal client tree, copying him here. > > > + > > + intel_cx0_write(i915, encoder->port, follower_lane, PHY_C10_VDR_CONTROL(1), > > + C10_VDR_CTRL_MSGBUS_ACCESS | C10_VDR_CTRL_UPDATE_CFG, > > + MB_WRITE_COMMITTED); > > + intel_cx0_write(i915, encoder->port, master_lane, PHY_C10_VDR_CONTROL(1), > > + C10_VDR_CTRL_MASTER_LANE | C10_VDR_CTRL_MSGBUS_ACCESS | > > + C10_VDR_CTRL_UPDATE_CFG, MB_WRITE_COMMITTED); > > + > > + intel_cx0_phy_transaction_end(encoder, wakeref); > > +} > > + > > /* > > * Basic DP link rates with 38.4 MHz reference clock. > > * Note: The tables below are with SSC. In non-ssc > > @@ -1092,9 +1183,12 @@ void intel_c10mpllb_readout_hw_state(struct intel_encoder *encoder, > > u8 lane = lane_reversal ? INTEL_CX0_LANE1 : > > INTEL_CX0_LANE0; > > enum phy phy = intel_port_to_phy(i915, encoder->port); > > + intel_wakeref_t wakeref; > > int i; > > u8 cmn, tx0; > > > > + wakeref = intel_cx0_phy_transaction_begin(encoder); > > + > > /* > > * According to C10 VDR Register programming Sequence we need > > * to do this to read PHY internal registers from MsgBus. > > @@ -1113,6 +1207,8 @@ void intel_c10mpllb_readout_hw_state(struct intel_encoder *encoder, > > cmn != C10_CMN0_HDMI_VAL)) > > drm_dbg_kms(&i915->drm, "Unexpected tx: %x or cmn: %x for phy: %c.\n", > > tx0, cmn, phy_name(phy)); > > + > > + intel_cx0_phy_transaction_end(encoder, wakeref); > > } > > > > static void intel_c10_pll_program(struct drm_i915_private *i915, > > @@ -1250,8 +1346,11 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder, > > > > if (intel_crtc_has_dp_encoder(crtc_state)) { > > intel_dp = enc_to_intel_dp(encoder); > > - ssc_enabled = intel_dp->dpcd[DP_MAX_DOWNSPREAD] & > > - DP_MAX_DOWNSPREAD_0_5; > > + ssc_enabled = (intel_dp->dpcd[DP_MAX_DOWNSPREAD] & > > + DP_MAX_DOWNSPREAD_0_5); > > + > > + if (intel_dp_is_edp(intel_dp) && !intel_panel_use_ssc(i915)) > > + ssc_enabled = false; > > The "if" statement below makes this one above unnecessary: ssc_enabled will be > false if !intel_panel_use_ssc(i915) regardless of intel_dp_is_edp(intel_dp). > > > > > if (!intel_panel_use_ssc(i915)) > > ssc_enabled = false; > > @@ -1259,11 +1358,11 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder, > > /* TODO: DP2.0 10G and 20G rates enable MPLLA*/ > > val |= ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0; > > } > > + > > intel_de_rmw(i915, XELPDP_PORT_CLOCK_CTL(encoder->port), > > - XELPDP_LANE1_PHY_CLOCK_SELECT | > > - XELPDP_FORWARD_CLOCK_UNGATE | > > + XELPDP_LANE1_PHY_CLOCK_SELECT | XELPDP_FORWARD_CLOCK_UNGATE | > > XELPDP_DDI_CLOCK_SELECT_MASK | > > - XELPDP_SSC_ENABLE_PLLB, val); > > + XELPDP_SSC_ENABLE_PLLA | XELPDP_SSC_ENABLE_PLLB, val); > > } > > > > static u32 intel_cx0_get_powerdown_update(u8 lane) > > @@ -1401,9 +1500,12 @@ static void intel_c10_program_phy_lane(struct drm_i915_private *i915, > > { > > u8 l0t1, l0t2, l1t1, l1t2; > > > > - intel_cx0_rmw(i915, port, INTEL_CX0_BOTH_LANES, PHY_C10_VDR_CONTROL(1), > > - C10_VDR_CTRL_MSGBUS_ACCESS, C10_VDR_CTRL_MSGBUS_ACCESS, > > - MB_WRITE_COMMITTED); > > + intel_cx0_rmw(i915, port, INTEL_CX0_LANE1, PHY_C10_VDR_CONTROL(1), > > + C10_VDR_CTRL_MSGBUS_ACCESS | C10_VDR_CTRL_UPDATE_CFG, > > + C10_VDR_CTRL_MSGBUS_ACCESS, MB_WRITE_COMMITTED); > > + intel_cx0_rmw(i915, port, INTEL_CX0_LANE0, PHY_C10_VDR_CONTROL(1), > > + C10_VDR_CTRL_MSGBUS_ACCESS | C10_VDR_CTRL_UPDATE_CFG, > > + C10_VDR_CTRL_MASTER_LANE | C10_VDR_CTRL_MSGBUS_ACCESS, MB_WRITE_COMMITTED); > > > > l0t1 = intel_cx0_read(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(1, 2)); > > l0t2 = intel_cx0_read(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(2, 2)); > > @@ -1559,9 +1661,14 @@ void intel_cx0pll_enable(struct intel_encoder *encoder, > > { > > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > enum phy phy = intel_port_to_phy(i915, encoder->port); > > + intel_wakeref_t wakeref; > > + > > + wakeref = intel_cx0_phy_transaction_begin(encoder); > > > > drm_WARN_ON(&i915->drm, !intel_is_c10phy(i915, phy)); > > intel_c10pll_enable(encoder, crtc_state); > > + > > + intel_cx0_phy_transaction_end(encoder, wakeref); > > } > > > > static void intel_c10pll_disable(struct intel_encoder *encoder) > > @@ -1606,7 +1713,8 @@ static void intel_c10pll_disable(struct intel_encoder *encoder) > > > > /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */ > > intel_de_rmw(i915, XELPDP_PORT_CLOCK_CTL(encoder->port), > > - XELPDP_DDI_CLOCK_SELECT_MASK | > > + XELPDP_DDI_CLOCK_SELECT_MASK, 0); > > + intel_de_rmw(i915, XELPDP_PORT_CLOCK_CTL(encoder->port), > > XELPDP_FORWARD_CLOCK_UNGATE, 0); > > This could rather be a fixup on the patch that originally introduced step (7). > > > } > > > > @@ -1614,9 +1722,14 @@ void intel_cx0pll_disable(struct intel_encoder *encoder) > > { > > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > enum phy phy = intel_port_to_phy(i915, encoder->port); > > + intel_wakeref_t wakeref; > > + > > + wakeref = intel_cx0_phy_transaction_begin(encoder); > > > > drm_WARN_ON(&i915->drm, !intel_is_c10phy(i915, phy)); > > intel_c10pll_disable(encoder); > > + > > + intel_cx0_phy_transaction_end(encoder, wakeref); > > } > > > > void intel_c10mpllb_state_verify(struct intel_atomic_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 f8023f240727..952c7deeffaa 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > > @@ -40,5 +40,7 @@ int intel_c10mpllb_calc_port_clock(struct intel_encoder *encoder, > > void intel_c10mpllb_state_verify(struct intel_atomic_state *state, > > struct intel_crtc_state *new_crtc_state); > > int intel_c10_phy_check_hdmi_link_rate(int clock); > > +void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state); > > > > #endif /* __INTEL_CX0_PHY_H__ */ > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > > index f6002b70ddbe..fd48c75ee532 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > > @@ -169,4 +169,10 @@ > > #define PHY_CX0_TX_CONTROL(tx, control) (0x400 + ((tx) - 1) * 0x200 + (control)) > > #define CONTROL2_DISABLE_SINGLE_TX REG_BIT(6) > > > > +/* C10 Phy VSWING Masks */ > > +#define C10_PHY_VSWING_LEVEL_MASK REG_GENMASK8(2, 0) > > +#define C10_PHY_VSWING_LEVEL(val) REG_FIELD_PREP8(C10_PHY_VSWING_LEVEL_MASK, val) > > +#define C10_PHY_VSWING_PREEMPH_MASK REG_GENMASK8(1, 0) > > +#define C10_PHY_VSWING_PREEMPH(val) REG_FIELD_PREP8(C10_PHY_VSWING_PREEMPH_MASK, val) > > + > > #endif /* __INTEL_CX0_PHY_REGS_H__ */ > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 099776b62052..e796e5cf10f0 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -4472,7 +4472,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > > encoder->get_config = hsw_ddi_get_config; > > } > > > > - if (IS_DG2(dev_priv)) { > > + if (DISPLAY_VER(dev_priv) >= 14) { > > + encoder->set_signal_levels = intel_cx0_phy_set_signal_levels; > > + } else if (IS_DG2(dev_priv)) { > > encoder->set_signal_levels = intel_snps_phy_set_signal_levels; > > } else if (DISPLAY_VER(dev_priv) >= 12) { > > if (intel_phy_is_combo(dev_priv, phy)) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c > > index 006a2e979000..ca2acaa73a64 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c > > @@ -1035,6 +1035,30 @@ static const struct intel_ddi_buf_trans dg2_snps_trans_uhbr = { > > .num_entries = ARRAY_SIZE(_dg2_snps_trans_uhbr), > > }; > > > > +/* > > + * Some platforms don't need a mapping table and only expect us to > > + * to program the vswing + preemphasis levels directly since the > > + * hardware will do its own mapping to tuning values internally. > > + */ > > +static const union intel_ddi_buf_trans_entry direct_map_trans[] = { > > + { .direct = { .level = 0, .preemph = 0 } }, > > + { .direct = { .level = 0, .preemph = 1 } }, > > + { .direct = { .level = 0, .preemph = 2 } }, > > + { .direct = { .level = 0, .preemph = 3 } }, > > + { .direct = { .level = 1, .preemph = 0 } }, > > + { .direct = { .level = 1, .preemph = 1 } }, > > + { .direct = { .level = 1, .preemph = 2 } }, > > + { .direct = { .level = 2, .preemph = 0 } }, > > + { .direct = { .level = 2, .preemph = 1 } }, > > + { .direct = { .level = 3, .preemph = 0 } }, > > +}; > > + > > +static const struct intel_ddi_buf_trans mtl_cx0c10_trans = { > > + .entries = direct_map_trans, > > + .num_entries = ARRAY_SIZE(direct_map_trans), > > + .hdmi_default_entry = ARRAY_SIZE(direct_map_trans) - 1, > > +}; > > + > > bool is_hobl_buf_trans(const struct intel_ddi_buf_trans *table) > > { > > return table == &tgl_combo_phy_trans_edp_hbr2_hobl; > > @@ -1606,12 +1630,22 @@ dg2_get_snps_buf_trans(struct intel_encoder *encoder, > > return intel_get_buf_trans(&dg2_snps_trans, n_entries); > > } > > > > +static const struct intel_ddi_buf_trans * > > +mtl_get_cx0_buf_trans(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + int *n_entries) > > +{ > > + return intel_get_buf_trans(&mtl_cx0c10_trans, n_entries); > > +} > > + > > void intel_ddi_buf_trans_init(struct intel_encoder *encoder) > > { > > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > enum phy phy = intel_port_to_phy(i915, encoder->port); > > > > - if (IS_DG2(i915)) { > > + if (DISPLAY_VER(i915) >= 14) { > > + encoder->get_buf_trans = mtl_get_cx0_buf_trans; > > + } else if (IS_DG2(i915)) { > > encoder->get_buf_trans = dg2_get_snps_buf_trans; > > } else if (IS_ALDERLAKE_P(i915)) { > > if (intel_phy_is_combo(i915, phy)) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.h b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.h > > index 2133984a572b..e4a857b9829d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.h > > +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.h > > @@ -51,6 +51,11 @@ struct dg2_snps_phy_buf_trans { > > u8 post_cursor; > > }; > > > > +struct direct_phy_buf_trans { > > + u8 level; > > + u8 preemph; > > +}; > > + > > union intel_ddi_buf_trans_entry { > > struct hsw_ddi_buf_trans hsw; > > struct bxt_ddi_buf_trans bxt; > > @@ -58,6 +63,7 @@ union intel_ddi_buf_trans_entry { > > struct icl_mg_phy_ddi_buf_trans mg; > > struct tgl_dkl_phy_ddi_buf_trans dkl; > > struct dg2_snps_phy_buf_trans snps; > > + struct direct_phy_buf_trans direct; > > }; > > > > struct intel_ddi_buf_trans { > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c > > index f5d66ca85b19..f49da1c4eb22 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c > > @@ -1426,6 +1426,7 @@ I915_DECL_PW_DOMAINS(xelpdp_pwdoms_dc_off, > > XELPDP_PW_2_POWER_DOMAINS, > > POWER_DOMAIN_AUDIO_MMIO, > > POWER_DOMAIN_MODESET, > > + POWER_DOMAIN_DC_OFF, > > POWER_DOMAIN_AUX_A, > > POWER_DOMAIN_AUX_B, > > POWER_DOMAIN_DC_OFF, > > -- > > 2.34.1 > >