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.d...@intel.com>
> ---
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
