On Wed, 21 Oct 2015, Daniel Vetter <[email protected]> wrote: > On Wed, Sep 16, 2015 at 09:23:59AM +0200, Maarten Lankhorst wrote: >> When diagnosing a unrelated bug for someone on irc, it would seem the >> hardware can >> be brought up by the BIOS with the embedded displayport using the SPLL for >> spread spectrum. >> >> Right now this is not handled well in i915, and it calculates the crtc needs >> to >> be reprogrammed on the first modeset without SSC, but the SPLL itself was >> kept >> active. Fix this by exposing SPLL as a shared pll that will not be returned >> by intel_get_shared_dpll; you have to know it exists to use it. ;-) >> >> Cc: Emil Renner Berthing <[email protected]> >> Signed-off-by: Maarten Lankhorst <[email protected]> >> --- >> RFC because I haven't tested it with VGA, but it seems to work according to >> fix >> the problem mentioned above. >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 81adf89b92f1..cacdac67d9ba 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -345,6 +345,8 @@ enum intel_dpll_id { >> /* hsw/bdw */ >> DPLL_ID_WRPLL1 = 0, >> DPLL_ID_WRPLL2 = 1, >> + DPLL_ID_SPLL = 2, >> + >> /* skl */ >> DPLL_ID_SKL_DPLL1 = 0, >> DPLL_ID_SKL_DPLL2 = 1, >> diff --git a/drivers/gpu/drm/i915/intel_crt.c >> b/drivers/gpu/drm/i915/intel_crt.c >> index af5e43bef4a4..592d8fe9f991 100644 >> --- a/drivers/gpu/drm/i915/intel_crt.c >> +++ b/drivers/gpu/drm/i915/intel_crt.c >> @@ -138,18 +138,6 @@ static void hsw_crt_get_config(struct intel_encoder >> *encoder, >> pipe_config->base.adjusted_mode.flags |= intel_crt_get_flags(encoder); >> } >> >> -static void hsw_crt_pre_enable(struct intel_encoder *encoder) >> -{ >> - struct drm_device *dev = encoder->base.dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - >> - WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL already enabled\n"); >> - I915_WRITE(SPLL_CTL, >> - SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC); >> - POSTING_READ(SPLL_CTL); >> - udelay(20); >> -} >> - >> /* Note: The caller is required to filter out dpms modes not supported by >> the >> * platform. */ >> static void intel_crt_set_dpms(struct intel_encoder *encoder, int mode) >> @@ -216,19 +204,6 @@ static void pch_post_disable_crt(struct intel_encoder >> *encoder) >> intel_disable_crt(encoder); >> } >> >> -static void hsw_crt_post_disable(struct intel_encoder *encoder) >> -{ >> - struct drm_device *dev = encoder->base.dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - uint32_t val; >> - >> - DRM_DEBUG_KMS("Disabling SPLL\n"); >> - val = I915_READ(SPLL_CTL); >> - WARN_ON(!(val & SPLL_PLL_ENABLE)); >> - I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE); >> - POSTING_READ(SPLL_CTL); >> -} >> - >> static void intel_enable_crt(struct intel_encoder *encoder) >> { >> struct intel_crt *crt = intel_encoder_to_crt(encoder); >> @@ -280,6 +255,8 @@ static bool intel_crt_compute_config(struct >> intel_encoder *encoder, >> if (HAS_DDI(dev)) { >> pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; >> pipe_config->port_clock = 135000 * 2; >> + pipe_config->dpll_hw_state.wrpll = >> + SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; >> } >> >> return true; >> @@ -861,8 +838,6 @@ void intel_crt_init(struct drm_device *dev) >> if (HAS_DDI(dev)) { >> crt->base.get_config = hsw_crt_get_config; >> crt->base.get_hw_state = intel_ddi_get_hw_state; >> - crt->base.pre_enable = hsw_crt_pre_enable; >> - crt->base.post_disable = hsw_crt_post_disable; >> } else { >> crt->base.get_config = intel_crt_get_config; >> crt->base.get_hw_state = intel_crt_get_hw_state; >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 61575f67a626..dabd903147fa 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1269,6 +1269,18 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, >> } >> >> crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); >> + } else if (crtc_state->ddi_pll_sel == PORT_CLK_SEL_SPLL) { >> + struct drm_atomic_state *state = crtc_state->base.state; >> + struct intel_shared_dpll_config *spll = >> + >> &intel_atomic_get_shared_dpll_state(state)[DPLL_ID_SPLL]; >> + >> + if (spll->crtc_mask && >> + WARN_ON(spll->hw_state.wrpll != >> crtc_state->dpll_hw_state.wrpll)) >> + return false; >> + >> + crtc_state->shared_dpll = DPLL_ID_SPLL; >> + spll->hw_state.wrpll = crtc_state->dpll_hw_state.wrpll; >> + spll->crtc_mask |= 1 << intel_crtc->pipe; >> } >> >> return true; >> @@ -2414,19 +2426,31 @@ static void intel_disable_ddi(struct intel_encoder >> *intel_encoder) >> static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, >> struct intel_shared_dpll *pll) >> { >> - I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll); >> - POSTING_READ(WRPLL_CTL(pll->id)); >> + uint32_t reg; >> + >> + if (pll->id == DPLL_ID_SPLL) >> + reg = SPLL_CTL; >> + else >> + reg = WRPLL_CTL(pll->id); > > This is a bit ugly, and reusing hw_sate.wrpll is also a bit fragile due to > the overlaps. I think it'd be better to add hw_state.spll (including hw > state cross-checking) and specialising the enable/disable/get_hw_state > functions. > > With that this is Reviewed-by: Daniel Vetter <[email protected]>
Maarten, please update the patch according to Daniel's comments, as this fixes [1]. BR, Jani. [1] http://mid.gmane.org/[email protected] > >> + >> + I915_WRITE(reg, pll->config.hw_state.wrpll); >> + POSTING_READ(reg); >> udelay(20); >> } >> >> static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, >> struct intel_shared_dpll *pll) >> { >> - uint32_t val; >> + uint32_t reg, val; >> >> - val = I915_READ(WRPLL_CTL(pll->id)); >> - I915_WRITE(WRPLL_CTL(pll->id), val & ~WRPLL_PLL_ENABLE); >> - POSTING_READ(WRPLL_CTL(pll->id)); >> + if (pll->id == DPLL_ID_SPLL) >> + reg = SPLL_CTL; >> + else >> + reg = WRPLL_CTL(pll->id); >> + >> + val = I915_READ(reg); >> + I915_WRITE(reg, val & ~WRPLL_PLL_ENABLE); >> + POSTING_READ(reg); >> } >> >> static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, >> @@ -2438,23 +2462,28 @@ static bool hsw_ddi_pll_get_hw_state(struct >> drm_i915_private *dev_priv, >> if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) >> return false; >> >> - val = I915_READ(WRPLL_CTL(pll->id)); >> - hw_state->wrpll = val; >> + if (pll->id == DPLL_ID_SPLL) >> + val = I915_READ(SPLL_CTL); >> + else >> + val = I915_READ(WRPLL_CTL(pll->id)); >> >> + hw_state->wrpll = val; >> return val & WRPLL_PLL_ENABLE; >> } >> >> static const char * const hsw_ddi_pll_names[] = { >> "WRPLL 1", >> "WRPLL 2", >> + "SPLL" >> }; >> >> static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv) >> { >> int i; >> >> - dev_priv->num_shared_dpll = 2; >> + dev_priv->num_shared_dpll = 3; >> >> + /* SPLL is special, but needs to be initialized anyway.. */ >> for (i = 0; i < dev_priv->num_shared_dpll; i++) { >> dev_priv->shared_dplls[i].id = i; >> dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index ca9278be49f7..582360d2fa08 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4208,6 +4208,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct >> intel_crtc *crtc, >> struct intel_shared_dpll *pll; >> struct intel_shared_dpll_config *shared_dpll; >> enum intel_dpll_id i; >> + int max = dev_priv->num_shared_dpll; >> >> shared_dpll = >> intel_atomic_get_shared_dpll_state(crtc_state->base.state); >> >> @@ -4242,9 +4243,11 @@ struct intel_shared_dpll >> *intel_get_shared_dpll(struct intel_crtc *crtc, >> WARN_ON(shared_dpll[i].crtc_mask); >> >> goto found; >> - } >> + } else if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv)) >> + /* Do not consider SPLL */ >> + max = 2; >> >> - for (i = 0; i < dev_priv->num_shared_dpll; i++) { >> + for (i = 0; i < max; i++) { >> pll = &dev_priv->shared_dplls[i]; >> >> /* Only want to check enabled timings first */ >> @@ -9696,6 +9699,8 @@ static void haswell_get_ddi_pll(struct >> drm_i915_private *dev_priv, >> case PORT_CLK_SEL_WRPLL2: >> pipe_config->shared_dpll = DPLL_ID_WRPLL2; >> break; >> + case PORT_CLK_SEL_SPLL: >> + pipe_config->shared_dpll = DPLL_ID_SPLL; >> } >> } >> >> >> _______________________________________________ >> Intel-gfx mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
