| Message ID | 1447681332-6318-1-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 16 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> 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. > > Changes since v1: > - Create a separate dpll_hw_state.spll for spll, and use > separate pll functions for spll. > > Tested-by: Emil Renner Berthing <kernel@esmil.dk> #v1 > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > Emil, can you retest? Gabriel, you too please! BR, Jani. > > drivers/gpu/drm/i915/i915_drv.h | 3 ++ > drivers/gpu/drm/i915/intel_crt.c | 31 ++------------- > drivers/gpu/drm/i915/intel_ddi.c | 75 +++++++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_display.c | 15 ++++++-- > 4 files changed, 83 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 85c84c624ed1..b6b62387cbe1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -351,6 +351,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, > @@ -367,6 +369,7 @@ struct intel_dpll_hw_state { > > /* hsw, bdw */ > uint32_t wrpll; > + uint32_t spll; > > /* skl */ > /* > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index b84aaa0bb48a..6a2c76e367a5 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,10 @@ 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 = 0; > + pipe_config->dpll_hw_state.spll = > + SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; > } > > return true; > @@ -860,8 +839,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 abb4a265a6df..90fad7ad12ac 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1286,6 +1286,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.spll != crtc_state->dpll_hw_state.spll)) > + return false; > + > + crtc_state->shared_dpll = DPLL_ID_SPLL; > + spll->hw_state.spll = crtc_state->dpll_hw_state.spll; > + spll->crtc_mask |= 1 << intel_crtc->pipe; > } > > return true; > @@ -2446,7 +2458,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > } > } > > -static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, > +static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll) > { > I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll); > @@ -2454,9 +2466,17 @@ static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, > udelay(20); > } > > -static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, > +static void hsw_ddi_spll_enable(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll) > { > + I915_WRITE(SPLL_CTL, pll->config.hw_state.spll); > + POSTING_READ(SPLL_CTL); > + udelay(20); > +} > + > +static void hsw_ddi_wrpll_disable(struct drm_i915_private *dev_priv, > + struct intel_shared_dpll *pll) > +{ > uint32_t val; > > val = I915_READ(WRPLL_CTL(pll->id)); > @@ -2464,9 +2484,19 @@ static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, > POSTING_READ(WRPLL_CTL(pll->id)); > } > > -static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > - struct intel_shared_dpll *pll, > - struct intel_dpll_hw_state *hw_state) > +static void hsw_ddi_spll_disable(struct drm_i915_private *dev_priv, > + struct intel_shared_dpll *pll) > +{ > + uint32_t val; > + > + val = I915_READ(SPLL_CTL); > + I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE); > + POSTING_READ(SPLL_CTL); > +} > + > +static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv, > + struct intel_shared_dpll *pll, > + struct intel_dpll_hw_state *hw_state) > { > uint32_t val; > > @@ -2479,25 +2509,50 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > return val & WRPLL_PLL_ENABLE; > } > > +static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, > + struct intel_shared_dpll *pll, > + struct intel_dpll_hw_state *hw_state) > +{ > + uint32_t val; > + > + if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) > + return false; > + > + val = I915_READ(SPLL_CTL); > + hw_state->spll = val; > + > + return val & SPLL_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; > > - for (i = 0; i < dev_priv->num_shared_dpll; i++) { > + for (i = 0; i < 2; i++) { > dev_priv->shared_dplls[i].id = i; > dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; > - dev_priv->shared_dplls[i].disable = hsw_ddi_pll_disable; > - dev_priv->shared_dplls[i].enable = hsw_ddi_pll_enable; > + dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable; > + dev_priv->shared_dplls[i].enable = hsw_ddi_wrpll_enable; > dev_priv->shared_dplls[i].get_hw_state = > - hsw_ddi_pll_get_hw_state; > + hsw_ddi_wrpll_get_hw_state; > } > + > + /* SPLL is special, but needs to be initialized anyway.. */ > + dev_priv->shared_dplls[i].id = i; > + dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; > + dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable; > + dev_priv->shared_dplls[i].enable = hsw_ddi_spll_enable; > + dev_priv->shared_dplls[i].get_hw_state = hsw_ddi_spll_get_hw_state; > + > } > > static const char * const skl_ddi_pll_names[] = { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3ca38fc493de..b383cbe3964c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4210,6 +4210,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); > > @@ -4244,9 +4245,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 */ > @@ -9767,6 +9770,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; > } > } > > @@ -12070,9 +12075,10 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, > pipe_config->dpll_hw_state.cfgcr1, > pipe_config->dpll_hw_state.cfgcr2); > } else if (HAS_DDI(dev)) { > - DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x\n", > + DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x spll: 0x%x\n", > pipe_config->ddi_pll_sel, > - pipe_config->dpll_hw_state.wrpll); > + pipe_config->dpll_hw_state.wrpll, > + pipe_config->dpll_hw_state.spll); > } else { > DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, " > "fp0: 0x%x, fp1: 0x%x\n", > @@ -12607,6 +12613,7 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_X(dpll_hw_state.fp0); > PIPE_CONF_CHECK_X(dpll_hw_state.fp1); > PIPE_CONF_CHECK_X(dpll_hw_state.wrpll); > + PIPE_CONF_CHECK_X(dpll_hw_state.spll); > PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1); > PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1); > PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);
On 16.11.2015 17:53, Jani Nikula wrote: > On Mon, 16 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> 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. >> >> Changes since v1: >> - Create a separate dpll_hw_state.spll for spll, and use >> separate pll functions for spll. >> >> Tested-by: Emil Renner Berthing <kernel@esmil.dk> #v1 >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> Emil, can you retest? > > Gabriel, you too please! I retested it and it's ok. Regards, Gabriel. > > BR, > Jani. > >> >> drivers/gpu/drm/i915/i915_drv.h | 3 ++ >> drivers/gpu/drm/i915/intel_crt.c | 31 ++------------- >> drivers/gpu/drm/i915/intel_ddi.c | 75 +++++++++++++++++++++++++++++++----- >> drivers/gpu/drm/i915/intel_display.c | 15 ++++++-- >> 4 files changed, 83 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 85c84c624ed1..b6b62387cbe1 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -351,6 +351,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, >> @@ -367,6 +369,7 @@ struct intel_dpll_hw_state { >> >> /* hsw, bdw */ >> uint32_t wrpll; >> + uint32_t spll; >> >> /* skl */ >> /* >> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c >> index b84aaa0bb48a..6a2c76e367a5 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,10 @@ 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 = 0; >> + pipe_config->dpll_hw_state.spll = >> + SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; >> } >> >> return true; >> @@ -860,8 +839,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 abb4a265a6df..90fad7ad12ac 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1286,6 +1286,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.spll != crtc_state->dpll_hw_state.spll)) >> + return false; >> + >> + crtc_state->shared_dpll = DPLL_ID_SPLL; >> + spll->hw_state.spll = crtc_state->dpll_hw_state.spll; >> + spll->crtc_mask |= 1 << intel_crtc->pipe; >> } >> >> return true; >> @@ -2446,7 +2458,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) >> } >> } >> >> -static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, >> +static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv, >> struct intel_shared_dpll *pll) >> { >> I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll); >> @@ -2454,9 +2466,17 @@ static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, >> udelay(20); >> } >> >> -static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, >> +static void hsw_ddi_spll_enable(struct drm_i915_private *dev_priv, >> struct intel_shared_dpll *pll) >> { >> + I915_WRITE(SPLL_CTL, pll->config.hw_state.spll); >> + POSTING_READ(SPLL_CTL); >> + udelay(20); >> +} >> + >> +static void hsw_ddi_wrpll_disable(struct drm_i915_private *dev_priv, >> + struct intel_shared_dpll *pll) >> +{ >> uint32_t val; >> >> val = I915_READ(WRPLL_CTL(pll->id)); >> @@ -2464,9 +2484,19 @@ static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, >> POSTING_READ(WRPLL_CTL(pll->id)); >> } >> >> -static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, >> - struct intel_shared_dpll *pll, >> - struct intel_dpll_hw_state *hw_state) >> +static void hsw_ddi_spll_disable(struct drm_i915_private *dev_priv, >> + struct intel_shared_dpll *pll) >> +{ >> + uint32_t val; >> + >> + val = I915_READ(SPLL_CTL); >> + I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE); >> + POSTING_READ(SPLL_CTL); >> +} >> + >> +static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv, >> + struct intel_shared_dpll *pll, >> + struct intel_dpll_hw_state *hw_state) >> { >> uint32_t val; >> >> @@ -2479,25 +2509,50 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, >> return val & WRPLL_PLL_ENABLE; >> } >> >> +static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, >> + struct intel_shared_dpll *pll, >> + struct intel_dpll_hw_state *hw_state) >> +{ >> + uint32_t val; >> + >> + if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) >> + return false; >> + >> + val = I915_READ(SPLL_CTL); >> + hw_state->spll = val; >> + >> + return val & SPLL_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; >> >> - for (i = 0; i < dev_priv->num_shared_dpll; i++) { >> + for (i = 0; i < 2; i++) { >> dev_priv->shared_dplls[i].id = i; >> dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; >> - dev_priv->shared_dplls[i].disable = hsw_ddi_pll_disable; >> - dev_priv->shared_dplls[i].enable = hsw_ddi_pll_enable; >> + dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable; >> + dev_priv->shared_dplls[i].enable = hsw_ddi_wrpll_enable; >> dev_priv->shared_dplls[i].get_hw_state = >> - hsw_ddi_pll_get_hw_state; >> + hsw_ddi_wrpll_get_hw_state; >> } >> + >> + /* SPLL is special, but needs to be initialized anyway.. */ >> + dev_priv->shared_dplls[i].id = i; >> + dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; >> + dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable; >> + dev_priv->shared_dplls[i].enable = hsw_ddi_spll_enable; >> + dev_priv->shared_dplls[i].get_hw_state = hsw_ddi_spll_get_hw_state; >> + >> } >> >> static const char * const skl_ddi_pll_names[] = { >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 3ca38fc493de..b383cbe3964c 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4210,6 +4210,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); >> >> @@ -4244,9 +4245,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 */ >> @@ -9767,6 +9770,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; >> } >> } >> >> @@ -12070,9 +12075,10 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, >> pipe_config->dpll_hw_state.cfgcr1, >> pipe_config->dpll_hw_state.cfgcr2); >> } else if (HAS_DDI(dev)) { >> - DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x\n", >> + DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x spll: 0x%x\n", >> pipe_config->ddi_pll_sel, >> - pipe_config->dpll_hw_state.wrpll); >> + pipe_config->dpll_hw_state.wrpll, >> + pipe_config->dpll_hw_state.spll); >> } else { >> DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, " >> "fp0: 0x%x, fp1: 0x%x\n", >> @@ -12607,6 +12613,7 @@ intel_pipe_config_compare(struct drm_device *dev, >> PIPE_CONF_CHECK_X(dpll_hw_state.fp0); >> PIPE_CONF_CHECK_X(dpll_hw_state.fp1); >> PIPE_CONF_CHECK_X(dpll_hw_state.wrpll); >> + PIPE_CONF_CHECK_X(dpll_hw_state.spll); >> PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1); >> PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1); >> PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2); >
On 18 November 2015 at 13:31, Gabriel Feceoru <gabriel.feceoru@intel.com> wrote: > > > On 16.11.2015 17:53, Jani Nikula wrote: >> >> On Mon, 16 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> 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. >>> >>> Changes since v1: >>> - Create a separate dpll_hw_state.spll for spll, and use >>> separate pll functions for spll. >>> >>> Tested-by: Emil Renner Berthing <kernel@esmil.dk> #v1 >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> --- >>> Emil, can you retest? >> >> >> Gabriel, you too please! > > > I retested it and it's ok. It works fine for me too. I haven't run the tests yet, but I've run the patch applied to v4.3 the last couple of days. /Emil
On Wed, 18 Nov 2015, Emil Renner Berthing <kernel@esmil.dk> wrote: > On 18 November 2015 at 13:31, Gabriel Feceoru <gabriel.feceoru@intel.com> wrote: >> >> >> On 16.11.2015 17:53, Jani Nikula wrote: >>> >>> On Mon, 16 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> 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. >>>> >>>> Changes since v1: >>>> - Create a separate dpll_hw_state.spll for spll, and use >>>> separate pll functions for spll. >>>> >>>> Tested-by: Emil Renner Berthing <kernel@esmil.dk> #v1 >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> Emil, can you retest? >>> >>> >>> Gabriel, you too please! >> >> >> I retested it and it's ok. > > It works fine for me too. I haven't run the tests yet, but I've run > the patch applied to v4.3 the last couple of days. Pushed to drm-intel-fixes, thanks for the patch, review and testing. BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 85c84c624ed1..b6b62387cbe1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -351,6 +351,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, @@ -367,6 +369,7 @@ struct intel_dpll_hw_state { /* hsw, bdw */ uint32_t wrpll; + uint32_t spll; /* skl */ /* diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index b84aaa0bb48a..6a2c76e367a5 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,10 @@ 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 = 0; + pipe_config->dpll_hw_state.spll = + SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; } return true; @@ -860,8 +839,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 abb4a265a6df..90fad7ad12ac 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1286,6 +1286,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.spll != crtc_state->dpll_hw_state.spll)) + return false; + + crtc_state->shared_dpll = DPLL_ID_SPLL; + spll->hw_state.spll = crtc_state->dpll_hw_state.spll; + spll->crtc_mask |= 1 << intel_crtc->pipe; } return true; @@ -2446,7 +2458,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) } } -static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, +static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv, struct intel_shared_dpll *pll) { I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll); @@ -2454,9 +2466,17 @@ static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv, udelay(20); } -static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, +static void hsw_ddi_spll_enable(struct drm_i915_private *dev_priv, struct intel_shared_dpll *pll) { + I915_WRITE(SPLL_CTL, pll->config.hw_state.spll); + POSTING_READ(SPLL_CTL); + udelay(20); +} + +static void hsw_ddi_wrpll_disable(struct drm_i915_private *dev_priv, + struct intel_shared_dpll *pll) +{ uint32_t val; val = I915_READ(WRPLL_CTL(pll->id)); @@ -2464,9 +2484,19 @@ static void hsw_ddi_pll_disable(struct drm_i915_private *dev_priv, POSTING_READ(WRPLL_CTL(pll->id)); } -static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, - struct intel_shared_dpll *pll, - struct intel_dpll_hw_state *hw_state) +static void hsw_ddi_spll_disable(struct drm_i915_private *dev_priv, + struct intel_shared_dpll *pll) +{ + uint32_t val; + + val = I915_READ(SPLL_CTL); + I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE); + POSTING_READ(SPLL_CTL); +} + +static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv, + struct intel_shared_dpll *pll, + struct intel_dpll_hw_state *hw_state) { uint32_t val; @@ -2479,25 +2509,50 @@ static bool hsw_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, return val & WRPLL_PLL_ENABLE; } +static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, + struct intel_shared_dpll *pll, + struct intel_dpll_hw_state *hw_state) +{ + uint32_t val; + + if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) + return false; + + val = I915_READ(SPLL_CTL); + hw_state->spll = val; + + return val & SPLL_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; - for (i = 0; i < dev_priv->num_shared_dpll; i++) { + for (i = 0; i < 2; i++) { dev_priv->shared_dplls[i].id = i; dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; - dev_priv->shared_dplls[i].disable = hsw_ddi_pll_disable; - dev_priv->shared_dplls[i].enable = hsw_ddi_pll_enable; + dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable; + dev_priv->shared_dplls[i].enable = hsw_ddi_wrpll_enable; dev_priv->shared_dplls[i].get_hw_state = - hsw_ddi_pll_get_hw_state; + hsw_ddi_wrpll_get_hw_state; } + + /* SPLL is special, but needs to be initialized anyway.. */ + dev_priv->shared_dplls[i].id = i; + dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; + dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable; + dev_priv->shared_dplls[i].enable = hsw_ddi_spll_enable; + dev_priv->shared_dplls[i].get_hw_state = hsw_ddi_spll_get_hw_state; + } static const char * const skl_ddi_pll_names[] = { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3ca38fc493de..b383cbe3964c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4210,6 +4210,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); @@ -4244,9 +4245,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 */ @@ -9767,6 +9770,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; } } @@ -12070,9 +12075,10 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, pipe_config->dpll_hw_state.cfgcr1, pipe_config->dpll_hw_state.cfgcr2); } else if (HAS_DDI(dev)) { - DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x\n", + DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: wrpll: 0x%x spll: 0x%x\n", pipe_config->ddi_pll_sel, - pipe_config->dpll_hw_state.wrpll); + pipe_config->dpll_hw_state.wrpll, + pipe_config->dpll_hw_state.spll); } else { DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, " "fp0: 0x%x, fp1: 0x%x\n", @@ -12607,6 +12613,7 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_X(dpll_hw_state.fp0); PIPE_CONF_CHECK_X(dpll_hw_state.fp1); PIPE_CONF_CHECK_X(dpll_hw_state.wrpll); + PIPE_CONF_CHECK_X(dpll_hw_state.spll); PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1); PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1); PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);