Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
Kudos to Ville for noticing the race conditions.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: stable@vger.kernel.org # v5.5+ Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++------- drivers/gpu/drm/i915/display/intel_display.c | 2 + .../gpu/drm/i915/display/intel_display_core.h | 4 + .../i915/display/intel_display_power_well.c | 6 +- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 58 +++++------- drivers/gpu/drm/i915/display/intel_tc.c | 94 +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_tc.h | 9 ++ drivers/gpu/drm/i915/i915_reg.h | 3 + 8 files changed, 167 insertions(+), 76 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 971356237eca3..8e2b338883858 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -1262,33 +1262,30 @@ static void tgl_dkl_phy_set_signal_levels(struct intel_encoder *encoder, for (ln = 0; ln < 2; ln++) { int level;
- intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, ln)); - - intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), 0); + intel_tc_dkl_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), ln, 0);
level = intel_ddi_level(encoder, crtc_state, 2*ln+0);
- intel_de_rmw(dev_priv, DKL_TX_DPCNTL0(tc_port), - DKL_TX_PRESHOOT_COEFF_MASK | - DKL_TX_DE_EMPAHSIS_COEFF_MASK | - DKL_TX_VSWING_CONTROL_MASK, - DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) | - DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) | - DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing)); + intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL0(tc_port), ln, + DKL_TX_PRESHOOT_COEFF_MASK | + DKL_TX_DE_EMPAHSIS_COEFF_MASK | + DKL_TX_VSWING_CONTROL_MASK, + DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) | + DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) | + DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing));
level = intel_ddi_level(encoder, crtc_state, 2*ln+1);
- intel_de_rmw(dev_priv, DKL_TX_DPCNTL1(tc_port), - DKL_TX_PRESHOOT_COEFF_MASK | - DKL_TX_DE_EMPAHSIS_COEFF_MASK | - DKL_TX_VSWING_CONTROL_MASK, - DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) | - DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) | - DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing)); + intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL1(tc_port), ln, + DKL_TX_PRESHOOT_COEFF_MASK | + DKL_TX_DE_EMPAHSIS_COEFF_MASK | + DKL_TX_VSWING_CONTROL_MASK, + DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) | + DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) | + DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing));
- intel_de_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port), - DKL_TX_DP20BITMODE, 0); + intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port), ln, + DKL_TX_DP20BITMODE, 0);
if (IS_ALDERLAKE_P(dev_priv)) { u32 val; @@ -1306,10 +1303,10 @@ static void tgl_dkl_phy_set_signal_levels(struct intel_encoder *encoder, val |= DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX2(0); }
- intel_de_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port), - DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX1_MASK | - DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX2_MASK, - val); + intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port), ln, + DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX1_MASK | + DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX2_MASK, + val); } } } @@ -2019,12 +2016,8 @@ icl_program_mg_dp_mode(struct intel_digital_port *dig_port, return;
if (DISPLAY_VER(dev_priv) >= 12) { - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x0)); - ln0 = intel_de_read(dev_priv, DKL_DP_MODE(tc_port)); - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x1)); - ln1 = intel_de_read(dev_priv, DKL_DP_MODE(tc_port)); + ln0 = intel_tc_dkl_read(dev_priv, DKL_DP_MODE(tc_port), 0); + ln1 = intel_tc_dkl_read(dev_priv, DKL_DP_MODE(tc_port), 1); } else { ln0 = intel_de_read(dev_priv, MG_DP_MODE(0, tc_port)); ln1 = intel_de_read(dev_priv, MG_DP_MODE(1, tc_port)); @@ -2085,12 +2078,8 @@ icl_program_mg_dp_mode(struct intel_digital_port *dig_port, }
if (DISPLAY_VER(dev_priv) >= 12) { - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x0)); - intel_de_write(dev_priv, DKL_DP_MODE(tc_port), ln0); - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x1)); - intel_de_write(dev_priv, DKL_DP_MODE(tc_port), ln1); + intel_tc_dkl_write(dev_priv, DKL_DP_MODE(tc_port), 0, ln0); + intel_tc_dkl_write(dev_priv, DKL_DP_MODE(tc_port), 1, ln1); } else { intel_de_write(dev_priv, MG_DP_MODE(0, tc_port), ln0); intel_de_write(dev_priv, MG_DP_MODE(1, tc_port), ln1); @@ -3094,10 +3083,8 @@ static void adlp_tbt_to_dp_alt_switch_wa(struct intel_encoder *encoder) enum tc_port tc_port = intel_port_to_tc(i915, encoder->port); int ln;
- for (ln = 0; ln < 2; ln++) { - intel_de_write(i915, HIP_INDEX_REG(tc_port), HIP_INDEX_VAL(tc_port, ln)); - intel_de_rmw(i915, DKL_PCS_DW5(tc_port), DKL_PCS_DW5_CORE_SOFTRESET, 0); - } + for (ln = 0; ln < 2; ln++) + intel_tc_dkl_rmw(i915, DKL_PCS_DW5(tc_port), ln, DKL_PCS_DW5_CORE_SOFTRESET, 0); }
static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp, diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 606f9140d024f..68ebf49e6fdbd 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -7907,6 +7907,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
intel_pps_unlock_regs_wa(dev_priv);
+ spin_lock_init(&dev_priv->display.tc.dkl_lock); + if (!HAS_DISPLAY(dev_priv)) return;
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h index 96cf994b0ad1f..6e8371e8ebdb6 100644 --- a/drivers/gpu/drm/i915/display/intel_display_core.h +++ b/drivers/gpu/drm/i915/display/intel_display_core.h @@ -394,6 +394,10 @@ struct intel_display { u32 block_time_us; } sagv;
+ struct { + spinlock_t dkl_lock; + } tc; + struct { /* ordered wq for modesets */ struct workqueue_struct *modeset; diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c index df7ee4969ef17..c37d2d5bbd983 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c @@ -529,11 +529,9 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, enum tc_port tc_port;
tc_port = TGL_AUX_PW_TO_TC_PORT(i915_power_well_instance(power_well)->hsw.idx); - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x2));
- if (intel_de_wait_for_set(dev_priv, DKL_CMN_UC_DW_27(tc_port), - DKL_CMN_UC_DW27_UC_HEALTH, 1)) + if (wait_for(intel_tc_dkl_read(dev_priv, DKL_CMN_UC_DW_27(tc_port), 2) & + DKL_CMN_UC_DW27_UC_HEALTH, 1)) drm_warn(&dev_priv->drm, "Timeout waiting TC uC health\n"); } diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index b63600d8ebeb0..28895e51c9c21 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -3486,15 +3486,12 @@ static bool dkl_pll_get_hw_state(struct drm_i915_private *dev_priv, * All registers read here have the same HIP_INDEX_REG even though * they are on different building blocks */ - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x2)); - - hw_state->mg_refclkin_ctl = intel_de_read(dev_priv, - DKL_REFCLKIN_CTL(tc_port)); + hw_state->mg_refclkin_ctl = intel_tc_dkl_read(dev_priv, + DKL_REFCLKIN_CTL(tc_port), 2); hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
hw_state->mg_clktop2_hsclkctl = - intel_de_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port)); + intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), 2); hw_state->mg_clktop2_hsclkctl &= MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK | MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK | @@ -3502,32 +3499,32 @@ static bool dkl_pll_get_hw_state(struct drm_i915_private *dev_priv, MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
hw_state->mg_clktop2_coreclkctl1 = - intel_de_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port)); + intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), 2); hw_state->mg_clktop2_coreclkctl1 &= MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
- hw_state->mg_pll_div0 = intel_de_read(dev_priv, DKL_PLL_DIV0(tc_port)); + hw_state->mg_pll_div0 = intel_tc_dkl_read(dev_priv, DKL_PLL_DIV0(tc_port), 2); val = DKL_PLL_DIV0_MASK; if (dev_priv->display.vbt.override_afc_startup) val |= DKL_PLL_DIV0_AFC_STARTUP_MASK; hw_state->mg_pll_div0 &= val;
- hw_state->mg_pll_div1 = intel_de_read(dev_priv, DKL_PLL_DIV1(tc_port)); + hw_state->mg_pll_div1 = intel_tc_dkl_read(dev_priv, DKL_PLL_DIV1(tc_port), 2); hw_state->mg_pll_div1 &= (DKL_PLL_DIV1_IREF_TRIM_MASK | DKL_PLL_DIV1_TDC_TARGET_CNT_MASK);
- hw_state->mg_pll_ssc = intel_de_read(dev_priv, DKL_PLL_SSC(tc_port)); + hw_state->mg_pll_ssc = intel_tc_dkl_read(dev_priv, DKL_PLL_SSC(tc_port), 2); hw_state->mg_pll_ssc &= (DKL_PLL_SSC_IREF_NDIV_RATIO_MASK | DKL_PLL_SSC_STEP_LEN_MASK | DKL_PLL_SSC_STEP_NUM_MASK | DKL_PLL_SSC_EN);
- hw_state->mg_pll_bias = intel_de_read(dev_priv, DKL_PLL_BIAS(tc_port)); + hw_state->mg_pll_bias = intel_tc_dkl_read(dev_priv, DKL_PLL_BIAS(tc_port), 2); hw_state->mg_pll_bias &= (DKL_PLL_BIAS_FRAC_EN_H | DKL_PLL_BIAS_FBDIV_FRAC_MASK);
hw_state->mg_pll_tdc_coldst_bias = - intel_de_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port)); + intel_tc_dkl_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2); hw_state->mg_pll_tdc_coldst_bias &= (DKL_PLL_TDC_SSC_STEP_SIZE_MASK | DKL_PLL_TDC_FEED_FWD_GAIN_MASK);
@@ -3715,61 +3712,58 @@ static void dkl_pll_write(struct drm_i915_private *dev_priv, * All registers programmed here have the same HIP_INDEX_REG even * though on different building block */ - intel_de_write(dev_priv, HIP_INDEX_REG(tc_port), - HIP_INDEX_VAL(tc_port, 0x2)); - /* All the registers are RMW */ - val = intel_de_read(dev_priv, DKL_REFCLKIN_CTL(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_REFCLKIN_CTL(tc_port), 2); val &= ~MG_REFCLKIN_CTL_OD_2_MUX_MASK; val |= hw_state->mg_refclkin_ctl; - intel_de_write(dev_priv, DKL_REFCLKIN_CTL(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_REFCLKIN_CTL(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), 2); val &= ~MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK; val |= hw_state->mg_clktop2_coreclkctl1; - intel_de_write(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), 2); val &= ~(MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK | MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK | MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK | MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK); val |= hw_state->mg_clktop2_hsclkctl; - intel_de_write(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), 2, val);
val = DKL_PLL_DIV0_MASK; if (dev_priv->display.vbt.override_afc_startup) val |= DKL_PLL_DIV0_AFC_STARTUP_MASK; - intel_de_rmw(dev_priv, DKL_PLL_DIV0(tc_port), val, - hw_state->mg_pll_div0); + intel_tc_dkl_rmw(dev_priv, DKL_PLL_DIV0(tc_port), 2, val, + hw_state->mg_pll_div0);
- val = intel_de_read(dev_priv, DKL_PLL_DIV1(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_PLL_DIV1(tc_port), 2); val &= ~(DKL_PLL_DIV1_IREF_TRIM_MASK | DKL_PLL_DIV1_TDC_TARGET_CNT_MASK); val |= hw_state->mg_pll_div1; - intel_de_write(dev_priv, DKL_PLL_DIV1(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_PLL_DIV1(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_PLL_SSC(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_PLL_SSC(tc_port), 2); val &= ~(DKL_PLL_SSC_IREF_NDIV_RATIO_MASK | DKL_PLL_SSC_STEP_LEN_MASK | DKL_PLL_SSC_STEP_NUM_MASK | DKL_PLL_SSC_EN); val |= hw_state->mg_pll_ssc; - intel_de_write(dev_priv, DKL_PLL_SSC(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_PLL_SSC(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_PLL_BIAS(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_PLL_BIAS(tc_port), 2); val &= ~(DKL_PLL_BIAS_FRAC_EN_H | DKL_PLL_BIAS_FBDIV_FRAC_MASK); val |= hw_state->mg_pll_bias; - intel_de_write(dev_priv, DKL_PLL_BIAS(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_PLL_BIAS(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port)); + val = intel_tc_dkl_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2); val &= ~(DKL_PLL_TDC_SSC_STEP_SIZE_MASK | DKL_PLL_TDC_FEED_FWD_GAIN_MASK); val |= hw_state->mg_pll_tdc_coldst_bias; - intel_de_write(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), val); + intel_tc_dkl_write(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2, val);
- intel_de_posting_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port)); + intel_tc_dkl_posting_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2); }
static void icl_pll_power_enable(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c index 8cecd41ed0033..8123699d3dbfb 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.c +++ b/drivers/gpu/drm/i915/display/intel_tc.c @@ -5,6 +5,7 @@
#include "i915_drv.h" #include "i915_reg.h" +#include "intel_de.h" #include "intel_display.h" #include "intel_display_power_map.h" #include "intel_display_types.h" @@ -894,6 +895,99 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port) intel_tc_port_flush_work(dig_port); }
+static void dkl_set_hip_idx(struct drm_i915_private *i915, i915_reg_t reg, int idx) +{ + enum tc_port tc_port = DKL_REG_TC_PORT(reg); + + drm_WARN_ON(&i915->drm, tc_port < TC_PORT_1 || tc_port >= I915_MAX_TC_PORTS); + + intel_de_write(i915, + HIP_INDEX_REG(tc_port), + HIP_INDEX_VAL(tc_port, idx)); +} + +/** + * intel_tc_dkl_read - read a Dekel PHY register + * @i915: i915 device instance + * @reg: Dekel PHY register + * @ln: lane instance of @reg + * + * Read the @reg Dekel PHY register. + * + * Returns the read value. + */ +u32 intel_tc_dkl_read(struct drm_i915_private *i915, i915_reg_t reg, int ln) +{ + u32 val; + + spin_lock(&i915->display.tc.dkl_lock); + + dkl_set_hip_idx(i915, reg, ln); + val = intel_de_read(i915, reg); + + spin_unlock(&i915->display.tc.dkl_lock); + + return val; +} + +/** + * intel_tc_dkl_write - write a Dekel PHY register + * @i915: i915 device instance + * @reg: Dekel PHY register + * @ln: lane instance of @reg + * @val: value to write + * + * Write @val to the @reg Dekel PHY register. + */ +void intel_tc_dkl_write(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 val) +{ + spin_lock(&i915->display.tc.dkl_lock); + + dkl_set_hip_idx(i915, reg, ln); + intel_de_write(i915, reg, val); + + spin_unlock(&i915->display.tc.dkl_lock); +} + +/** + * intel_tc_dkl_rmw - read-modify-write a Dekel PHY register + * @i915: i915 device instance + * @reg: Dekel PHY register + * @ln: lane instance of @reg + * @clear: mask to clear + * @set: mask to set + * + * Read the @reg Dekel PHY register, clearing then setting the @clear/@set bits in it, and writing + * this value back to the register if the value differs from the read one. + */ +void intel_tc_dkl_rmw(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 clear, u32 set) +{ + spin_lock(&i915->display.tc.dkl_lock); + + dkl_set_hip_idx(i915, reg, ln); + intel_de_rmw(i915, reg, clear, set); + + spin_unlock(&i915->display.tc.dkl_lock); +} + +/** + * intel_tc_dkl_posting_read - do a posting read from a Dekel PHY register + * @i915: i915 device instance + * @reg: Dekel PHY register + * @ln: lane instance of @reg + * + * Read the @reg Dekel PHY register without returning the read value. + */ +void intel_tc_dkl_posting_read(struct drm_i915_private *i915, i915_reg_t reg, int ln) +{ + spin_lock(&i915->display.tc.dkl_lock); + + dkl_set_hip_idx(i915, reg, ln); + intel_de_posting_read(i915, reg); + + spin_unlock(&i915->display.tc.dkl_lock); +} + static bool tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port) { diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h index d54082e2d5e8d..330ff0fb12f16 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.h +++ b/drivers/gpu/drm/i915/display/intel_tc.h @@ -9,6 +9,10 @@ #include <linux/mutex.h> #include <linux/types.h>
+#include "i915_reg_defs.h" + +struct drm_i915_private; + struct intel_digital_port; struct intel_encoder;
@@ -34,6 +38,11 @@ void intel_tc_port_get_link(struct intel_digital_port *dig_port, void intel_tc_port_put_link(struct intel_digital_port *dig_port); bool intel_tc_port_ref_held(struct intel_digital_port *dig_port);
+u32 intel_tc_dkl_read(struct drm_i915_private *i915, i915_reg_t reg, int ln); +void intel_tc_dkl_write(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 val); +void intel_tc_dkl_rmw(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 clear, u32 set); +void intel_tc_dkl_posting_read(struct drm_i915_private *i915, i915_reg_t reg, int ln); + void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy);
bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 99a8535193957..2b7c019725462 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7442,6 +7442,9 @@ enum skl_power_gate { #define _DKL_PHY5_BASE 0x16C000 #define _DKL_PHY6_BASE 0x16D000
+#define _DKL_BANK_SHIFT 12 +#define DKL_REG_TC_PORT(reg) (((reg).reg - _DKL_PHY1_BASE) >> _DKL_BANK_SHIFT) + /* DEKEL PHY MMIO Address = Phy base + (internal address & ~index_mask) */ #define _DKL_PCS_DW5 0x14 #define DKL_PCS_DW5(tc_port) _MMIO(_PORT(tc_port, _DKL_PHY1_BASE, \
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote:
Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that. Also requires separate bank selection for every write, nearly doubling the MMIO writes.
I think the choice of intel_tc.c is indicative of the problems in abstraction. That file has zero DKL PHY register access currently, but becomes the focal point of DKL PHY.
I'd aim at adding intel_dkl_phy.c which is the only place that includes intel_dkl_phy_regs.h and only place that directly uses the DKL PHY registers. And with that, maybe you don't need to add any DKL PHY specific register accessors.
BR, Jani.
Kudos to Ville for noticing the race conditions.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: stable@vger.kernel.org # v5.5+ Signed-off-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/i915/display/intel_ddi.c | 67 ++++++------- drivers/gpu/drm/i915/display/intel_display.c | 2 + .../gpu/drm/i915/display/intel_display_core.h | 4 + .../i915/display/intel_display_power_well.c | 6 +- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 58 +++++------- drivers/gpu/drm/i915/display/intel_tc.c | 94 +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_tc.h | 9 ++ drivers/gpu/drm/i915/i915_reg.h | 3 + 8 files changed, 167 insertions(+), 76 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 971356237eca3..8e2b338883858 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -1262,33 +1262,30 @@ static void tgl_dkl_phy_set_signal_levels(struct intel_encoder *encoder, for (ln = 0; ln < 2; ln++) { int level;
intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, ln));intel_de_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), 0);
intel_tc_dkl_write(dev_priv, DKL_TX_PMD_LANE_SUS(tc_port), ln, 0);level = intel_ddi_level(encoder, crtc_state, 2*ln+0);
intel_de_rmw(dev_priv, DKL_TX_DPCNTL0(tc_port),DKL_TX_PRESHOOT_COEFF_MASK |DKL_TX_DE_EMPAHSIS_COEFF_MASK |DKL_TX_VSWING_CONTROL_MASK,DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) |DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) |DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing));
intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL0(tc_port), ln,DKL_TX_PRESHOOT_COEFF_MASK |DKL_TX_DE_EMPAHSIS_COEFF_MASK |DKL_TX_VSWING_CONTROL_MASK,DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) |DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) |DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing));level = intel_ddi_level(encoder, crtc_state, 2*ln+1);
intel_de_rmw(dev_priv, DKL_TX_DPCNTL1(tc_port),DKL_TX_PRESHOOT_COEFF_MASK |DKL_TX_DE_EMPAHSIS_COEFF_MASK |DKL_TX_VSWING_CONTROL_MASK,DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) |DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) |DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing));
intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL1(tc_port), ln,DKL_TX_PRESHOOT_COEFF_MASK |DKL_TX_DE_EMPAHSIS_COEFF_MASK |DKL_TX_VSWING_CONTROL_MASK,DKL_TX_PRESHOOT_COEFF(trans->entries[level].dkl.preshoot) |DKL_TX_DE_EMPHASIS_COEFF(trans->entries[level].dkl.de_emphasis) |DKL_TX_VSWING_CONTROL(trans->entries[level].dkl.vswing));
intel_de_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port),DKL_TX_DP20BITMODE, 0);
intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port), ln,DKL_TX_DP20BITMODE, 0);if (IS_ALDERLAKE_P(dev_priv)) { u32 val; @@ -1306,10 +1303,10 @@ static void tgl_dkl_phy_set_signal_levels(struct intel_encoder *encoder, val |= DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX2(0); }
intel_de_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port),DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX1_MASK |DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX2_MASK,val);
intel_tc_dkl_rmw(dev_priv, DKL_TX_DPCNTL2(tc_port), ln,DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX1_MASK |DKL_TX_DPCNTL2_CFG_LOADGENSELECT_TX2_MASK,val);} @@ -2019,12 +2016,8 @@ icl_program_mg_dp_mode(struct intel_digital_port *dig_port, return; if (DISPLAY_VER(dev_priv) >= 12) {
intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, 0x0));ln0 = intel_de_read(dev_priv, DKL_DP_MODE(tc_port));intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, 0x1));ln1 = intel_de_read(dev_priv, DKL_DP_MODE(tc_port));
ln0 = intel_tc_dkl_read(dev_priv, DKL_DP_MODE(tc_port), 0);ln1 = intel_tc_dkl_read(dev_priv, DKL_DP_MODE(tc_port), 1);@@ -2085,12 +2078,8 @@ icl_program_mg_dp_mode(struct intel_digital_port *dig_port, } if (DISPLAY_VER(dev_priv) >= 12) {
intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, 0x0));intel_de_write(dev_priv, DKL_DP_MODE(tc_port), ln0);intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, 0x1));intel_de_write(dev_priv, DKL_DP_MODE(tc_port), ln1);
intel_tc_dkl_write(dev_priv, DKL_DP_MODE(tc_port), 0, ln0);intel_tc_dkl_write(dev_priv, DKL_DP_MODE(tc_port), 1, ln1);@@ -3094,10 +3083,8 @@ static void adlp_tbt_to_dp_alt_switch_wa(struct intel_encoder *encoder) enum tc_port tc_port = intel_port_to_tc(i915, encoder->port); int ln;
- for (ln = 0; ln < 2; ln++) {
intel_de_write(i915, HIP_INDEX_REG(tc_port), HIP_INDEX_VAL(tc_port, ln));intel_de_rmw(i915, DKL_PCS_DW5(tc_port), DKL_PCS_DW5_CORE_SOFTRESET, 0);- }
- for (ln = 0; ln < 2; ln++)
intel_tc_dkl_rmw(i915, DKL_PCS_DW5(tc_port), ln, DKL_PCS_DW5_CORE_SOFTRESET, 0);} static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp, diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 606f9140d024f..68ebf49e6fdbd 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -7907,6 +7907,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv) intel_pps_unlock_regs_wa(dev_priv);
- spin_lock_init(&dev_priv->display.tc.dkl_lock);
- if (!HAS_DISPLAY(dev_priv)) return;
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h index 96cf994b0ad1f..6e8371e8ebdb6 100644 --- a/drivers/gpu/drm/i915/display/intel_display_core.h +++ b/drivers/gpu/drm/i915/display/intel_display_core.h @@ -394,6 +394,10 @@ struct intel_display { u32 block_time_us; } sagv;
- struct {
spinlock_t dkl_lock;- } tc;
- struct { /* ordered wq for modesets */ struct workqueue_struct *modeset;
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c index df7ee4969ef17..c37d2d5bbd983 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c @@ -529,11 +529,9 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, enum tc_port tc_port; tc_port = TGL_AUX_PW_TO_TC_PORT(i915_power_well_instance(power_well)->hsw.idx);
intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, 0x2));
if (intel_de_wait_for_set(dev_priv, DKL_CMN_UC_DW_27(tc_port),DKL_CMN_UC_DW27_UC_HEALTH, 1))
if (wait_for(intel_tc_dkl_read(dev_priv, DKL_CMN_UC_DW_27(tc_port), 2) &DKL_CMN_UC_DW27_UC_HEALTH, 1)) drm_warn(&dev_priv->drm, "Timeout waiting TC uC health\n");diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index b63600d8ebeb0..28895e51c9c21 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -3486,15 +3486,12 @@ static bool dkl_pll_get_hw_state(struct drm_i915_private *dev_priv, * All registers read here have the same HIP_INDEX_REG even though * they are on different building blocks */
- intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),
HIP_INDEX_VAL(tc_port, 0x2));- hw_state->mg_refclkin_ctl = intel_de_read(dev_priv,
DKL_REFCLKIN_CTL(tc_port));
- hw_state->mg_refclkin_ctl = intel_tc_dkl_read(dev_priv,
DKL_REFCLKIN_CTL(tc_port), 2);hw_state->mg_clktop2_hsclkctl =
intel_de_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port));
intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), 2);@@ -3502,32 +3499,32 @@ static bool dkl_pll_get_hw_state(struct drm_i915_private *dev_priv, MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK; hw_state->mg_clktop2_coreclkctl1 =
intel_de_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port));
intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), 2);
- hw_state->mg_pll_div0 = intel_de_read(dev_priv, DKL_PLL_DIV0(tc_port));
- hw_state->mg_pll_div0 = intel_tc_dkl_read(dev_priv, DKL_PLL_DIV0(tc_port), 2); val = DKL_PLL_DIV0_MASK; if (dev_priv->display.vbt.override_afc_startup) val |= DKL_PLL_DIV0_AFC_STARTUP_MASK; hw_state->mg_pll_div0 &= val;
- hw_state->mg_pll_div1 = intel_de_read(dev_priv, DKL_PLL_DIV1(tc_port));
- hw_state->mg_pll_div1 = intel_tc_dkl_read(dev_priv, DKL_PLL_DIV1(tc_port), 2); hw_state->mg_pll_div1 &= (DKL_PLL_DIV1_IREF_TRIM_MASK | DKL_PLL_DIV1_TDC_TARGET_CNT_MASK);
- hw_state->mg_pll_ssc = intel_de_read(dev_priv, DKL_PLL_SSC(tc_port));
- hw_state->mg_pll_ssc = intel_tc_dkl_read(dev_priv, DKL_PLL_SSC(tc_port), 2); hw_state->mg_pll_ssc &= (DKL_PLL_SSC_IREF_NDIV_RATIO_MASK | DKL_PLL_SSC_STEP_LEN_MASK | DKL_PLL_SSC_STEP_NUM_MASK | DKL_PLL_SSC_EN);
- hw_state->mg_pll_bias = intel_de_read(dev_priv, DKL_PLL_BIAS(tc_port));
- hw_state->mg_pll_bias = intel_tc_dkl_read(dev_priv, DKL_PLL_BIAS(tc_port), 2); hw_state->mg_pll_bias &= (DKL_PLL_BIAS_FRAC_EN_H | DKL_PLL_BIAS_FBDIV_FRAC_MASK);
hw_state->mg_pll_tdc_coldst_bias =
intel_de_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port));
intel_tc_dkl_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2);@@ -3715,61 +3712,58 @@ static void dkl_pll_write(struct drm_i915_private *dev_priv, * All registers programmed here have the same HIP_INDEX_REG even * though on different building block */
- intel_de_write(dev_priv, HIP_INDEX_REG(tc_port),
HIP_INDEX_VAL(tc_port, 0x2));- /* All the registers are RMW */
- val = intel_de_read(dev_priv, DKL_REFCLKIN_CTL(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_REFCLKIN_CTL(tc_port), 2); val &= ~MG_REFCLKIN_CTL_OD_2_MUX_MASK; val |= hw_state->mg_refclkin_ctl;
- intel_de_write(dev_priv, DKL_REFCLKIN_CTL(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_REFCLKIN_CTL(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), 2); val &= ~MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK; val |= hw_state->mg_clktop2_coreclkctl1;
- intel_de_write(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_CLKTOP2_CORECLKCTL1(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), 2); val &= ~(MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK | MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK | MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK | MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK); val |= hw_state->mg_clktop2_hsclkctl;
- intel_de_write(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_CLKTOP2_HSCLKCTL(tc_port), 2, val);
val = DKL_PLL_DIV0_MASK; if (dev_priv->display.vbt.override_afc_startup) val |= DKL_PLL_DIV0_AFC_STARTUP_MASK;
- intel_de_rmw(dev_priv, DKL_PLL_DIV0(tc_port), val,
hw_state->mg_pll_div0);
- intel_tc_dkl_rmw(dev_priv, DKL_PLL_DIV0(tc_port), 2, val,
hw_state->mg_pll_div0);
- val = intel_de_read(dev_priv, DKL_PLL_DIV1(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_PLL_DIV1(tc_port), 2); val &= ~(DKL_PLL_DIV1_IREF_TRIM_MASK | DKL_PLL_DIV1_TDC_TARGET_CNT_MASK); val |= hw_state->mg_pll_div1;
- intel_de_write(dev_priv, DKL_PLL_DIV1(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_PLL_DIV1(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_PLL_SSC(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_PLL_SSC(tc_port), 2); val &= ~(DKL_PLL_SSC_IREF_NDIV_RATIO_MASK | DKL_PLL_SSC_STEP_LEN_MASK | DKL_PLL_SSC_STEP_NUM_MASK | DKL_PLL_SSC_EN); val |= hw_state->mg_pll_ssc;
- intel_de_write(dev_priv, DKL_PLL_SSC(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_PLL_SSC(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_PLL_BIAS(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_PLL_BIAS(tc_port), 2); val &= ~(DKL_PLL_BIAS_FRAC_EN_H | DKL_PLL_BIAS_FBDIV_FRAC_MASK); val |= hw_state->mg_pll_bias;
- intel_de_write(dev_priv, DKL_PLL_BIAS(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_PLL_BIAS(tc_port), 2, val);
- val = intel_de_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port));
- val = intel_tc_dkl_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2); val &= ~(DKL_PLL_TDC_SSC_STEP_SIZE_MASK | DKL_PLL_TDC_FEED_FWD_GAIN_MASK); val |= hw_state->mg_pll_tdc_coldst_bias;
- intel_de_write(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), val);
- intel_tc_dkl_write(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2, val);
- intel_de_posting_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port));
- intel_tc_dkl_posting_read(dev_priv, DKL_PLL_TDC_COLDST_BIAS(tc_port), 2);
} static void icl_pll_power_enable(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c index 8cecd41ed0033..8123699d3dbfb 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.c +++ b/drivers/gpu/drm/i915/display/intel_tc.c @@ -5,6 +5,7 @@ #include "i915_drv.h" #include "i915_reg.h" +#include "intel_de.h" #include "intel_display.h" #include "intel_display_power_map.h" #include "intel_display_types.h" @@ -894,6 +895,99 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port) intel_tc_port_flush_work(dig_port); } +static void dkl_set_hip_idx(struct drm_i915_private *i915, i915_reg_t reg, int idx) +{
- enum tc_port tc_port = DKL_REG_TC_PORT(reg);
- drm_WARN_ON(&i915->drm, tc_port < TC_PORT_1 || tc_port >= I915_MAX_TC_PORTS);
- intel_de_write(i915,
HIP_INDEX_REG(tc_port),HIP_INDEX_VAL(tc_port, idx));+}
+/**
- intel_tc_dkl_read - read a Dekel PHY register
- @i915: i915 device instance
- @reg: Dekel PHY register
- @ln: lane instance of @reg
- Read the @reg Dekel PHY register.
- Returns the read value.
- */
+u32 intel_tc_dkl_read(struct drm_i915_private *i915, i915_reg_t reg, int ln) +{
- u32 val;
- spin_lock(&i915->display.tc.dkl_lock);
- dkl_set_hip_idx(i915, reg, ln);
- val = intel_de_read(i915, reg);
- spin_unlock(&i915->display.tc.dkl_lock);
- return val;
+}
+/**
- intel_tc_dkl_write - write a Dekel PHY register
- @i915: i915 device instance
- @reg: Dekel PHY register
- @ln: lane instance of @reg
- @val: value to write
- Write @val to the @reg Dekel PHY register.
- */
+void intel_tc_dkl_write(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 val) +{
- spin_lock(&i915->display.tc.dkl_lock);
- dkl_set_hip_idx(i915, reg, ln);
- intel_de_write(i915, reg, val);
- spin_unlock(&i915->display.tc.dkl_lock);
+}
+/**
- intel_tc_dkl_rmw - read-modify-write a Dekel PHY register
- @i915: i915 device instance
- @reg: Dekel PHY register
- @ln: lane instance of @reg
- @clear: mask to clear
- @set: mask to set
- Read the @reg Dekel PHY register, clearing then setting the @clear/@set bits in it, and writing
- this value back to the register if the value differs from the read one.
- */
+void intel_tc_dkl_rmw(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 clear, u32 set) +{
- spin_lock(&i915->display.tc.dkl_lock);
- dkl_set_hip_idx(i915, reg, ln);
- intel_de_rmw(i915, reg, clear, set);
- spin_unlock(&i915->display.tc.dkl_lock);
+}
+/**
- intel_tc_dkl_posting_read - do a posting read from a Dekel PHY register
- @i915: i915 device instance
- @reg: Dekel PHY register
- @ln: lane instance of @reg
- Read the @reg Dekel PHY register without returning the read value.
- */
+void intel_tc_dkl_posting_read(struct drm_i915_private *i915, i915_reg_t reg, int ln) +{
- spin_lock(&i915->display.tc.dkl_lock);
- dkl_set_hip_idx(i915, reg, ln);
- intel_de_posting_read(i915, reg);
- spin_unlock(&i915->display.tc.dkl_lock);
+}
static bool tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port) { diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h index d54082e2d5e8d..330ff0fb12f16 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.h +++ b/drivers/gpu/drm/i915/display/intel_tc.h @@ -9,6 +9,10 @@ #include <linux/mutex.h> #include <linux/types.h> +#include "i915_reg_defs.h"
+struct drm_i915_private;
struct intel_digital_port; struct intel_encoder; @@ -34,6 +38,11 @@ void intel_tc_port_get_link(struct intel_digital_port *dig_port, void intel_tc_port_put_link(struct intel_digital_port *dig_port); bool intel_tc_port_ref_held(struct intel_digital_port *dig_port); +u32 intel_tc_dkl_read(struct drm_i915_private *i915, i915_reg_t reg, int ln); +void intel_tc_dkl_write(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 val); +void intel_tc_dkl_rmw(struct drm_i915_private *i915, i915_reg_t reg, int ln, u32 clear, u32 set); +void intel_tc_dkl_posting_read(struct drm_i915_private *i915, i915_reg_t reg, int ln);
void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 99a8535193957..2b7c019725462 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7442,6 +7442,9 @@ enum skl_power_gate { #define _DKL_PHY5_BASE 0x16C000 #define _DKL_PHY6_BASE 0x16D000 +#define _DKL_BANK_SHIFT 12 +#define DKL_REG_TC_PORT(reg) (((reg).reg - _DKL_PHY1_BASE) >> _DKL_BANK_SHIFT)
/* DEKEL PHY MMIO Address = Phy base + (internal address & ~index_mask) */ #define _DKL_PCS_DW5 0x14 #define DKL_PCS_DW5(tc_port) _MMIO(_PORT(tc_port, _DKL_PHY1_BASE, \
On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote:
Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that.
It's no worse than uncore.lock. No one cares about that in these codepaths either.
Also requires separate bank selection for every write, nearly doubling the MMIO writes.
Drop in the ocean. This is all slow modeset stuff anyway.
IMO separate reg accessors is the correct way to handle indexed registers unless you have some very specific performance concerns to deal with.
I think the choice of intel_tc.c is indicative of the problems in abstraction. That file has zero DKL PHY register access currently, but becomes the focal point of DKL PHY.
I'd aim at adding intel_dkl_phy.c which is the only place that includes intel_dkl_phy_regs.h and only place that directly uses the DKL PHY registers. And with that, maybe you don't need to add any DKL PHY specific register accessors.
Ripping out the PHY specific junk from eg. intel_ddi.c I think would be a good goal. But that should also be done for the mg phy.
On Wed, Oct 19, 2022 at 12:19:49PM +0300, Ville Syrjälä wrote:
On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote:
Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that.
It's no worse than uncore.lock. No one cares about that in these codepaths either.
Also requires separate bank selection for every write, nearly doubling the MMIO writes.
Drop in the ocean. This is all slow modeset stuff anyway.
IMO separate reg accessors is the correct way to handle indexed registers unless you have some very specific performance concerns to deal with.
Now, whether those accessors need to be visible everywere is another matter. It should certainly be possible to suck all dkl phy stuff into one file and keep the accessors static. But currently eveything is grouped by function (PLLs in one file, vswing stuff in another, etc.). We'd have to flip that around so that all the sub functions of of each IP block is in the same file. Is that a better apporach? Not sure.
On Wed, 19 Oct 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Oct 19, 2022 at 12:19:49PM +0300, Ville Syrjälä wrote:
On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote:
Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that.
It's no worse than uncore.lock. No one cares about that in these codepaths either.
Also requires separate bank selection for every write, nearly doubling the MMIO writes.
Drop in the ocean. This is all slow modeset stuff anyway.
IMO separate reg accessors is the correct way to handle indexed registers unless you have some very specific performance concerns to deal with.
Fair enough.
Now, whether those accessors need to be visible everywere is another matter. It should certainly be possible to suck all dkl phy stuff into one file and keep the accessors static. But currently eveything is grouped by function (PLLs in one file, vswing stuff in another, etc.). We'd have to flip that around so that all the sub functions of of each IP block is in the same file. Is that a better apporach? Not sure.
I'm often interested in the precedent a change makes. What's the direction we want to go to?
So here, we're saying the DKL PHY registers are special, and need to be accessed via dedicated register accessors. To enforce this, we create strong typing for DKL PHY registers. We go out of our way to make it safe to access DKL PHY registers anywhere in the driver.
Do we want to add more and more register types in the future? And duplicate the accessors for each? Oops, looks like we're already on our way [1].
My argument is that maybe access to such a hardware block should instead be limited to a module (.c file) that abstracts the details. Abstract hardware blocks and function, not registers. How many files need big changes to add a new PHY?
BR, Jani.
[1] https://lore.kernel.org/r/20221014230239.1023689-13-matthew.d.roper@intel.co...
On Wed, Oct 19, 2022 at 03:30:58PM +0300, Jani Nikula wrote:
On Wed, 19 Oct 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Oct 19, 2022 at 12:19:49PM +0300, Ville Syrjälä wrote:
On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote:
Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that.
It's no worse than uncore.lock. No one cares about that in these codepaths either.
Also requires separate bank selection for every write, nearly doubling the MMIO writes.
Drop in the ocean. This is all slow modeset stuff anyway.
IMO separate reg accessors is the correct way to handle indexed registers unless you have some very specific performance concerns to deal with.
Fair enough.
Now, whether those accessors need to be visible everywere is another matter. It should certainly be possible to suck all dkl phy stuff into one file and keep the accessors static. But currently eveything is grouped by function (PLLs in one file, vswing stuff in another, etc.). We'd have to flip that around so that all the sub functions of of each IP block is in the same file. Is that a better apporach? Not sure.
I'm often interested in the precedent a change makes. What's the direction we want to go to?
So here, we're saying the DKL PHY registers are special, and need to be accessed via dedicated register accessors. To enforce this, we create strong typing for DKL PHY registers. We go out of our way to make it safe to access DKL PHY registers anywhere in the driver.
Do we want to add more and more register types in the future? And duplicate the accessors for each? Oops, looks like we're already on our way [1].
Making the DKL PHY accesses type safe was just a bonus, the main reason for adding the dkl_phy_reg struct (in a later refactoring patch) is that defining those registers only makes sense to me specifying all the attributes (both MMIO and the bank index) of the register at one place. That's instead of the current way of having to pass these separately to each accessor functions. For instance to be able to call
read(DKL_PLL_DIV0(tc_port))
instead of having to remember the index of each (non lane-instanced) register and call
read(DKL_PLL_DIV0(tc_port), 2)
It also makes more sense to me that the register itself is parametric if that's needed (lane-instanced registers), for instance
read(DKL_TX_DPCNTL0(tc_port, 0))
instead of this being a separate parameter of each accessor function:
read(DKL_TX_DPCNTL0(tc_port), 0)
My argument is that maybe access to such a hardware block should instead be limited to a module (.c file) that abstracts the details. Abstract hardware blocks and function, not registers. How many files need big changes to add a new PHY?
I think the accessors could be added to a new intel_tc_phy.c file instead? (That would still allow further refactoring of both the MG and DKL functionality as a follow-up to this change for -stable.)
BR, Jani.
[1] https://lore.kernel.org/r/20221014230239.1023689-13-matthew.d.roper@intel.co...
-- Jani Nikula, Intel Open Source Graphics Center
On Wed, 19 Oct 2022, Imre Deak imre.deak@intel.com wrote:
On Wed, Oct 19, 2022 at 03:30:58PM +0300, Jani Nikula wrote:
On Wed, 19 Oct 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Oct 19, 2022 at 12:19:49PM +0300, Ville Syrjälä wrote:
On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote:
Accessing the TypeC DKL PHY registers during modeset-commit, -verification, DP link-retraining and AUX power well toggling is racy due to these code paths being concurrent and the PHY register bank selection register (HIP_INDEX_REG) being shared between PHY instances (aka TC ports) and the bank selection being not atomic wrt. the actual PHY register access.
Add the required locking around each PHY register bank selection-> register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that.
It's no worse than uncore.lock. No one cares about that in these codepaths either.
Also requires separate bank selection for every write, nearly doubling the MMIO writes.
Drop in the ocean. This is all slow modeset stuff anyway.
IMO separate reg accessors is the correct way to handle indexed registers unless you have some very specific performance concerns to deal with.
Fair enough.
Now, whether those accessors need to be visible everywere is another matter. It should certainly be possible to suck all dkl phy stuff into one file and keep the accessors static. But currently eveything is grouped by function (PLLs in one file, vswing stuff in another, etc.). We'd have to flip that around so that all the sub functions of of each IP block is in the same file. Is that a better apporach? Not sure.
I'm often interested in the precedent a change makes. What's the direction we want to go to?
So here, we're saying the DKL PHY registers are special, and need to be accessed via dedicated register accessors. To enforce this, we create strong typing for DKL PHY registers. We go out of our way to make it safe to access DKL PHY registers anywhere in the driver.
Do we want to add more and more register types in the future? And duplicate the accessors for each? Oops, looks like we're already on our way [1].
Making the DKL PHY accesses type safe was just a bonus, the main reason for adding the dkl_phy_reg struct (in a later refactoring patch) is that defining those registers only makes sense to me specifying all the attributes (both MMIO and the bank index) of the register at one place. That's instead of the current way of having to pass these separately to each accessor functions. For instance to be able to call
read(DKL_PLL_DIV0(tc_port))
instead of having to remember the index of each (non lane-instanced) register and call
read(DKL_PLL_DIV0(tc_port), 2)
It also makes more sense to me that the register itself is parametric if that's needed (lane-instanced registers), for instance
read(DKL_TX_DPCNTL0(tc_port, 0))
instead of this being a separate parameter of each accessor function:
read(DKL_TX_DPCNTL0(tc_port), 0)
This is actually a very good point.
An alternative to this that I've been pondering separately, before these patches, is expanding i915_reg_t to encode things like "display register", "mcr register", etc.
So you'd still have only one i915_reg_t type, and only one set of accessors, but they could be smarter behind the scenes. But I don't like teaching intel uncore about stuff like dkl either. And the main point would be avoiding all the duplication that C type safety requires.
But it's a moot point anyway because we also need something to backport to stable, and I acknowledge your approach makes a lot of sense for that too.
My argument is that maybe access to such a hardware block should instead be limited to a module (.c file) that abstracts the details. Abstract hardware blocks and function, not registers. How many files need big changes to add a new PHY?
I think the accessors could be added to a new intel_tc_phy.c file instead? (That would still allow further refactoring of both the MG and DKL functionality as a follow-up to this change for -stable.)
So, why intel_tc_anything? Why not just intel_dkl_phy.[ch], intel_dkl_phy_regs.h? Even if initially limited to the register accessors, you could easily move things like tgl_dkl_phy_set_signal_levels() there, just like intel_snps_phy_set_signal_levels() is in intel_snps_phy.c. And you could have intel_mg_phy.c for MG stuff.
I guess intel_tc_phy_regs.h would mostly be split to intel_dkl_phy_regs.h and intel_mg_phy_regs.h.
BR, Jani.
BR, Jani.
[1] https://lore.kernel.org/r/20221014230239.1023689-13-matthew.d.roper@intel.co...
-- Jani Nikula, Intel Open Source Graphics Center
On Wed, Oct 19, 2022 at 05:57:30PM +0300, Jani Nikula wrote:
On Wed, 19 Oct 2022, Imre Deak imre.deak@intel.com wrote:
On Wed, Oct 19, 2022 at 03:30:58PM +0300, Jani Nikula wrote:
On Wed, 19 Oct 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Oct 19, 2022 at 12:19:49PM +0300, Ville Syrjälä wrote:
On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
On Tue, 18 Oct 2022, Imre Deak imre.deak@intel.com wrote: > Accessing the TypeC DKL PHY registers during modeset-commit, > -verification, DP link-retraining and AUX power well toggling is racy > due to these code paths being concurrent and the PHY register bank > selection register (HIP_INDEX_REG) being shared between PHY instances > (aka TC ports) and the bank selection being not atomic wrt. the actual > PHY register access. > > Add the required locking around each PHY register bank selection-> > register access sequence.
I honestly think the abstraction here is at a too low level.
Too many places are doing DKL PHY register access to begin with. IMO the solution should be to abstract DKL PHY better, not to provide low level DKL PHY register accessors.
Indeed, this level of granularity leads to a lot of unnecessary lock/unlock that could have a longer span otherwise, and the interface does not lend itself for that.
It's no worse than uncore.lock. No one cares about that in these codepaths either.
Also requires separate bank selection for every write, nearly doubling the MMIO writes.
Drop in the ocean. This is all slow modeset stuff anyway.
IMO separate reg accessors is the correct way to handle indexed registers unless you have some very specific performance concerns to deal with.
Fair enough.
Now, whether those accessors need to be visible everywere is another matter. It should certainly be possible to suck all dkl phy stuff into one file and keep the accessors static. But currently eveything is grouped by function (PLLs in one file, vswing stuff in another, etc.). We'd have to flip that around so that all the sub functions of of each IP block is in the same file. Is that a better apporach? Not sure.
I'm often interested in the precedent a change makes. What's the direction we want to go to?
So here, we're saying the DKL PHY registers are special, and need to be accessed via dedicated register accessors. To enforce this, we create strong typing for DKL PHY registers. We go out of our way to make it safe to access DKL PHY registers anywhere in the driver.
Do we want to add more and more register types in the future? And duplicate the accessors for each? Oops, looks like we're already on our way [1].
Making the DKL PHY accesses type safe was just a bonus, the main reason for adding the dkl_phy_reg struct (in a later refactoring patch) is that defining those registers only makes sense to me specifying all the attributes (both MMIO and the bank index) of the register at one place. That's instead of the current way of having to pass these separately to each accessor functions. For instance to be able to call
read(DKL_PLL_DIV0(tc_port))
instead of having to remember the index of each (non lane-instanced) register and call
read(DKL_PLL_DIV0(tc_port), 2)
It also makes more sense to me that the register itself is parametric if that's needed (lane-instanced registers), for instance
read(DKL_TX_DPCNTL0(tc_port, 0))
instead of this being a separate parameter of each accessor function:
read(DKL_TX_DPCNTL0(tc_port), 0)
This is actually a very good point.
An alternative to this that I've been pondering separately, before these patches, is expanding i915_reg_t to encode things like "display register", "mcr register", etc.
So you'd still have only one i915_reg_t type, and only one set of accessors, but they could be smarter behind the scenes. But I don't like teaching intel uncore about stuff like dkl either. And the main point would be avoiding all the duplication that C type safety requires.
But it's a moot point anyway because we also need something to backport to stable, and I acknowledge your approach makes a lot of sense for that too.
My argument is that maybe access to such a hardware block should instead be limited to a module (.c file) that abstracts the details. Abstract hardware blocks and function, not registers. How many files need big changes to add a new PHY?
I think the accessors could be added to a new intel_tc_phy.c file instead? (That would still allow further refactoring of both the MG and DKL functionality as a follow-up to this change for -stable.)
So, why intel_tc_anything? Why not just intel_dkl_phy.[ch], intel_dkl_phy_regs.h? Even if initially limited to the register accessors, you could easily move things like tgl_dkl_phy_set_signal_levels() there, just like intel_snps_phy_set_signal_levels() is in intel_snps_phy.c. And you could have intel_mg_phy.c for MG stuff.
MG and DKL share some register flags and programming sequences (calculating PLL params, setting pin assignments for instance), hence thought to have one file for both PHY implementations. But there are also differences and sharing code would also be possible between the MG and DKL files, so ok if no objections I can add the accessors to intel_dkl_phy.[ch].
I guess intel_tc_phy_regs.h would mostly be split to intel_dkl_phy_regs.h and intel_mg_phy_regs.h.
Ok, so I suppose this means renaming the current intel_tc_phy_regs.h to intel_mg_phy_regs.h, move the DKL regs to intel_dkl_phy_regs.h and copy/rename the shared MG_* flags in the former to DKL_* in the latter.
--Imre
BR, Jani.
BR, Jani.
[1] https://lore.kernel.org/r/20221014230239.1023689-13-matthew.d.roper@intel.co...
-- Jani Nikula, Intel Open Source Graphics Center
-- Jani Nikula, Intel Open Source Graphics Center
linux-stable-mirror@lists.linaro.org
-
Imre Deak -
Jani Nikula -
Ville Syrjälä