On Tue, 2016-06-07 at 21:24 +0300, Imre Deak wrote: > So far we configured a static lane latency optimization during driver > loading/resuming. The specification changed at one point and now this > configuration depends on the lane count, so move the configuration > to modeset time accordingly. > > It's not clear when this lane configuration takes effect. The > specification only requires that the programming is done before enabling > the port. On CHV OTOH the lanes start to power up already right after > enabling the PLL. To be safe preserve the current order and set things > up already before enabling the PLL. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95476 > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_ddi.c | 131 +++++++++++++++++++++++----------- > - > drivers/gpu/drm/i915/intel_display.c | 5 ++ > drivers/gpu/drm/i915/intel_drv.h | 6 ++ > 3 files changed, 99 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index dee6dd0..f46117a 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1789,8 +1789,7 @@ static void broxton_phy_wait_grc_done(struct > drm_i915_private *dev_priv, > > void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy) > { > - enum port port; > - u32 ports, val; > + u32 val; > > if (bxt_ddi_phy_is_enabled(dev_priv, phy)) { > /* Still read out the GRC value for state verification */ > @@ -1825,28 +1824,6 @@ void bxt_ddi_phy_init(struct drm_i915_private > *dev_priv, enum dpio_phy phy) > DRM_ERROR("timeout during PHY%d power on\n", phy); > } > > - if (phy == DPIO_PHY0) > - ports = BIT(PORT_B) | BIT(PORT_C); > - else > - ports = BIT(PORT_A); > - > - for_each_port_masked(port, ports) { > - int lane; > - > - for (lane = 0; lane < 4; lane++) { > - val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane)); > - /* > - * Note that on CHV this flag is called UPAR, but has > - * the same function. > - */ > - val &= ~LATENCY_OPTIM; > - if (lane != 1) > - val |= LATENCY_OPTIM; > - > - I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val); > - } > - } > - > /* Program PLL Rcomp code offset */ > val = I915_READ(BXT_PORT_CL1CM_DW9(phy)); > val &= ~IREF0RC_OFFSET_MASK; > @@ -1956,8 +1933,6 @@ __phy_reg_verify_state(struct drm_i915_private > *dev_priv, enum dpio_phy phy, > bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv, > enum dpio_phy phy) > { > - enum port port; > - u32 ports; > uint32_t mask; > bool ok; > > @@ -1970,21 +1945,6 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private > *dev_priv, > > ok = true; > > - if (phy == DPIO_PHY0) > - ports = BIT(PORT_B) | BIT(PORT_C); > - else > - ports = BIT(PORT_A); > - > - for_each_port_masked(port, ports) { > - int lane; > - > - for (lane = 0; lane < 4; lane++) > - ok &= _CHK(BXT_PORT_TX_DW14_LN(port, lane), > - LATENCY_OPTIM, > - lane != 1 ? LATENCY_OPTIM : 0, > - "BXT_PORT_TX_DW14_LN(%d, %d)", port, > lane); > - } > - > /* PLL Rcomp code offset */ > ok &= _CHK(BXT_PORT_CL1CM_DW9(phy), > IREF0RC_OFFSET_MASK, 0xe4 << IREF0RC_OFFSET_SHIFT, > @@ -2028,6 +1988,75 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private > *dev_priv, > #undef _CHK > } > > +static uint8_t > +bxt_ddi_phy_calc_lane_lat_optim_mask(struct intel_encoder *encoder, > + struct intel_crtc_state *pipe_config) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + int optim_lane_count; > + > + switch (pipe_config->lane_count) { > + case 1: > + optim_lane_count = 0; > + break; > + case 2: > + optim_lane_count = 3; > + break; > + case 4: > + optim_lane_count = 4; > + break; > + default: > + MISSING_CASE(intel_crtc->config->lane_count); Since this is called from compute config, pipe_config and intel_crtc->config will be different. But anyway, I think this belongs in the dpll code. See below. > + > + return 0; > + } > + > + return (BIT(optim_lane_count) - 1) & ~BIT(1); Wouldn't it be easier to just return the right mask from the switch? There's already a case for each possible lane count. > +} > + > +static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder) > +{ > + struct intel_digital_port *dport = enc_to_dig_port(&encoder->base); > + struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev); > + enum port port = dport->port; > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + int lane; > + > + for (lane = 0; lane < 4; lane++) { > + u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane)); > + > + /* > + * Note that on CHV this flag is called UPAR, but has > + * the same function. > + */ > + val &= ~LATENCY_OPTIM; > + if (intel_crtc->config->lane_lat_optim_mask & BIT(lane)) > + val |= LATENCY_OPTIM; > + > + I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val); > + } > +} > + > +static uint8_t > +bxt_ddi_phy_get_lane_lat_optim_mask(struct intel_encoder *encoder) > +{ > + struct intel_digital_port *dport = enc_to_dig_port(&encoder->base); > + struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev); > + enum port port = dport->port; > + int lane; > + uint8_t mask; > + > + mask = 0; > + for (lane = 0; lane < 4; lane++) { > + u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane)); > + > + if (val & LATENCY_OPTIM) > + mask |= BIT(lane); > + } > + > + return mask; > +} > + > void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > @@ -2199,13 +2228,19 @@ void intel_ddi_get_config(struct intel_encoder > *encoder, > } > > intel_ddi_clock_get(encoder, pipe_config); > + > + if (IS_BROXTON(dev_priv)) > + pipe_config->lane_lat_optim_mask = > + bxt_ddi_phy_get_lane_lat_optim_mask(encoder); > } > > static bool intel_ddi_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config) > { > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > int type = encoder->type; > int port = intel_ddi_get_encoder_port(encoder); > + int ret; > > WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown > output!\n"); > > @@ -2213,9 +2248,17 @@ static bool intel_ddi_compute_config(struct > intel_encoder *encoder, > pipe_config->cpu_transcoder = TRANSCODER_EDP; > > if (type == INTEL_OUTPUT_HDMI) > - return intel_hdmi_compute_config(encoder, pipe_config); > + ret = intel_hdmi_compute_config(encoder, pipe_config); > else > - return intel_dp_compute_config(encoder, pipe_config); > + ret = intel_dp_compute_config(encoder, pipe_config); > + > + if (IS_BROXTON(dev_priv) && ret) > + pipe_config->lane_lat_optim_mask = > + bxt_ddi_phy_calc_lane_lat_optim_mask(encoder, > + pipe_config); > + > + return ret; > + > } > > static const struct drm_encoder_funcs intel_ddi_funcs = { > @@ -2314,6 +2357,8 @@ void intel_ddi_init(struct drm_device *dev, enum port > port) > > intel_encoder->compute_config = intel_ddi_compute_config; > intel_encoder->enable = intel_enable_ddi; > + if (IS_BROXTON(dev_priv)) > + intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable; > intel_encoder->pre_enable = intel_ddi_pre_enable; > intel_encoder->disable = intel_disable_ddi; > intel_encoder->post_disable = intel_ddi_post_disable; > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 60cba19..4bc7c48 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4841,6 +4841,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) > intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A, > false); > > + for_each_encoder_on_crtc(dev, crtc, encoder) > + if (encoder->pre_pll_enable) > + encoder->pre_pll_enable(encoder); > + > if (intel_crtc->config->shared_dpll) > intel_enable_shared_dpll(intel_crtc); Effectively this writes BXT_PORT_TX_DW14_LN before bxt_ddi_pll_enable() is called. If that would be the first register write in that function, it would avoid the extra detour. So we can calculate the optimization in bxt_get_dpll(), store it in struct intel_dpll_hw_state and use the pll state checking to verify the right values were programmed. That way there's no need to add a value specific to the bxt phy to struct intel_crtc_state or implement the pre_pll_enable hook. Ander > > @@ -12816,6 +12820,7 @@ intel_pipe_config_compare(struct drm_device *dev, > > PIPE_CONF_CHECK_I(has_dp_encoder); > PIPE_CONF_CHECK_I(lane_count); > + PIPE_CONF_CHECK_X(lane_lat_optim_mask); > > if (INTEL_INFO(dev)->gen < 8) { > PIPE_CONF_CHECK_M_N(dp_m_n); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 17445d7..ca98416 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -579,6 +579,12 @@ struct intel_crtc_state { > > uint8_t lane_count; > > + /* > + * Used by platforms having DP/HDMI PHY with programmable lane > + * latency optimization. > + */ > + uint8_t lane_lat_optim_mask; > + > /* Panel fitter controls for gen2-gen4 + VLV */ > struct { > u32 control; _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
