On Thu, 2016-03-03 at 14:33 +0100, Maarten Lankhorst wrote:
> Op 29-02-16 om 10:08 schreef Ander Conselvan de Oliveira:
> > Include DPLL0 in the managed dplls for SKL/KBL. While it has to be kept
> > enabled because of it driving CDCLK, it is better to special case that
> > inside the DPLL code than in the higher level.
> >
> > v2: Use INTEL_DPLL_ALWAYS_ON flag. (Ander)
> >
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@xxxxxxxxx>
> >
> > ---
> > I wasn't able to test this patch. Would really appreciate a tested by
> > from someone.
> Unfortunately no skl with eDP here either. :(
> > drivers/gpu/drm/i915/intel_ddi.c | 21 ------
> > drivers/gpu/drm/i915/intel_dp.c | 52 +-------------
> > drivers/gpu/drm/i915/intel_dpll_mgr.c | 131 +++++++++++++++++++++++++++----
> > ---
> > drivers/gpu/drm/i915/intel_dpll_mgr.h | 7 +-
> > 4 files changed, 109 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 3cb9f36..8ef4d2e 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1008,9 +1008,6 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
> > {
> > struct intel_shared_dpll *pll;
> >
> > - if (intel_encoder->type == INTEL_OUTPUT_EDP)
> > - return true;
> > -
> > pll = intel_get_shared_dpll(intel_crtc, crtc_state, intel_encoder);
> > if (pll == NULL) {
> > DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> > @@ -1570,24 +1567,6 @@ void intel_ddi_clk_select(struct intel_encoder
> > *encoder,
> > uint32_t dpll = pipe_config->ddi_pll_sel;
> > uint32_t val;
> >
> > - /*
> > - * DPLL0 is used for eDP and is the only "private" DPLL (as
> > - * opposed to shared) on SKL
> > - */
> > - if (encoder->type == INTEL_OUTPUT_EDP) {
> > - WARN_ON(dpll != SKL_DPLL0);
> > -
> > - val = I915_READ(DPLL_CTRL1);
> > -
> > - val &= ~(DPLL_CTRL1_HDMI_MODE(dpll) |
> > - DPLL_CTRL1_SSC(dpll) |
> > - DPLL_CTRL1_LINK_RATE_MASK(dpll));
> > - val |= pipe_config->dpll_hw_state.ctrl1 << (dpll *
> > 6);
> > -
> > - I915_WRITE(DPLL_CTRL1, val);
> > - POSTING_READ(DPLL_CTRL1);
> > - }
> > -
> > /* DDI -> PLL mapping */
> > val = I915_READ(DPLL_CTRL2);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 5be6892..9fef877 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1229,52 +1229,6 @@ intel_dp_connector_unregister(struct intel_connector
> > *intel_connector)
> > intel_connector_unregister(intel_connector);
> > }
> >
> > -static void
> > -skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
> > -{
> > - u32 ctrl1;
> > -
> > - memset(&pipe_config->dpll_hw_state, 0,
> > - sizeof(pipe_config->dpll_hw_state));
> > -
> > - pipe_config->ddi_pll_sel = SKL_DPLL0;
> > - pipe_config->dpll_hw_state.cfgcr1 = 0;
> > - pipe_config->dpll_hw_state.cfgcr2 = 0;
> > -
> > - ctrl1 = DPLL_CTRL1_OVERRIDE(SKL_DPLL0);
> > - switch (pipe_config->port_clock / 2) {
> > - case 81000:
> > - ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810,
> > - SKL_DPLL0);
> > - break;
> > - case 135000:
> > - ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1350,
> > - SKL_DPLL0);
> > - break;
> > - case 270000:
> > - ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2700,
> > - SKL_DPLL0);
> > - break;
> > - case 162000:
> > - ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620,
> > - SKL_DPLL0);
> > - break;
> > - /* TBD: For DP link rates 2.16 GHz and 4.32 GHz, VCO is 8640 which
> > - results in CDCLK change. Need to handle the change of CDCLK by
> > - disabling pipes and re-enabling them */
> > - case 108000:
> > - ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
> > - SKL_DPLL0);
> > - break;
> > - case 216000:
> > - ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160,
> > - SKL_DPLL0);
> > - break;
> > -
> > - }
> > - pipe_config->dpll_hw_state.ctrl1 = ctrl1;
> > -}
> > -
> > static int
> > intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
> > {
> > @@ -1632,11 +1586,7 @@ found:
> > &pipe_config->dp_m2_n2);
> > }
> >
> > - if ((IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) && is_edp(intel_dp))
> > - skl_edp_set_pll_config(pipe_config);
> > - else if (IS_BROXTON(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
> > - /* handled in ddi */;
> > - else
> > + if (!HAS_DDI(dev))
> > intel_dp_set_clock(encoder, pipe_config);
> >
> > return true;
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index d371680..6ec7836 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -785,7 +785,12 @@ struct skl_dpll_regs {
> > };
> >
> > /* this array is indexed by the *shared* pll id */
> > -static const struct skl_dpll_regs skl_dpll_regs[3] = {
> > +static const struct skl_dpll_regs skl_dpll_regs[4] = {
> > + {
> > + /* DPLL 0 */
> > + .ctl = LCPLL1_CTL,
> > + /* DPLL 0 doesn't support HDMI mode */
> > + },
> > {
> > /* DPLL 1 */
> > .ctl = LCPLL2_CTL,
> > @@ -806,24 +811,29 @@ static const struct skl_dpll_regs skl_dpll_regs[3] = {
> > },
> > };
> >
> > -static void skl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > - struct intel_shared_dpll *pll)
> > +static void skl_ddi_pll_write_ctrl1(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll)
> > {
> > uint32_t val;
> > - unsigned int dpll;
> > - const struct skl_dpll_regs *regs = skl_dpll_regs;
> > -
> > - /* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1
> > */
> > - dpll = pll->id + 1;
> >
> > val = I915_READ(DPLL_CTRL1);
> >
> > - val &= ~(DPLL_CTRL1_HDMI_MODE(dpll) | DPLL_CTRL1_SSC(dpll) |
> > - DPLL_CTRL1_LINK_RATE_MASK(dpll));
> > - val |= pll->config.hw_state.ctrl1 << (dpll * 6);
> > + val &= ~(DPLL_CTRL1_HDMI_MODE(pll->id) | DPLL_CTRL1_SSC(pll->id) |
> > + DPLL_CTRL1_LINK_RATE_MASK(pll->id));
> > + val |= pll->config.hw_state.ctrl1 << (pll->id * 6);
> >
> > I915_WRITE(DPLL_CTRL1, val);
> > POSTING_READ(DPLL_CTRL1);
> > +}
> > +
> > +static void skl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll)
> > +{
> > + const struct skl_dpll_regs *regs = skl_dpll_regs;
> > +
> > + WARN_ON(pll->id == DPLL_ID_SKL_DPLL0);
> > +
> > + skl_ddi_pll_write_ctrl1(dev_priv, pll);
> >
> > I915_WRITE(regs[pll->id].cfgcr1, pll->config.hw_state.cfgcr1);
> > I915_WRITE(regs[pll->id].cfgcr2, pll->config.hw_state.cfgcr2);
> > @@ -834,8 +844,15 @@ static void skl_ddi_pll_enable(struct drm_i915_private
> > *dev_priv,
> > I915_WRITE(regs[pll->id].ctl,
> > I915_READ(regs[pll->id].ctl) | LCPLL_PLL_ENABLE);
> >
> > - if (wait_for(I915_READ(DPLL_STATUS) & DPLL_LOCK(dpll), 5))
> > - DRM_ERROR("DPLL %d not locked\n", dpll);
> > + if (wait_for(I915_READ(DPLL_STATUS) & DPLL_LOCK(pll->id), 5))
> > + DRM_ERROR("DPLL %d not locked\n", pll->id);
> > +}
> > +
> > +static void skl_ddi_dpll0_enable(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll)
> > +{
> > + WARN_ON(pll->id != DPLL_ID_SKL_DPLL0);
> > + skl_ddi_pll_write_ctrl1(dev_priv, pll);
> > }
> >
> > static void skl_ddi_pll_disable(struct drm_i915_private *dev_priv,
> > @@ -849,32 +866,35 @@ static void skl_ddi_pll_disable(struct
> > drm_i915_private *dev_priv,
> > POSTING_READ(regs[pll->id].ctl);
> > }
> >
> > +static void skl_ddi_dpll0_disable(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll)
> > +{
> > +}
> > +
> > static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> > struct intel_shared_dpll *pll,
> > struct intel_dpll_hw_state *hw_state)
> > {
> > uint32_t val;
> > - unsigned int dpll;
> > const struct skl_dpll_regs *regs = skl_dpll_regs;
> > bool ret;
> >
> > + WARN_ON(pll->id == DPLL_ID_SKL_DPLL0);
> > +
> > if (!intel_display_power_get_if_enabled(dev_priv,
> > POWER_DOMAIN_PLLS))
> > return false;
> >
> > ret = false;
> >
> > - /* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1
> > */
> > - dpll = pll->id + 1;
> > -
> > val = I915_READ(regs[pll->id].ctl);
> > if (!(val & LCPLL_PLL_ENABLE))
> > goto out;
> >
> > val = I915_READ(DPLL_CTRL1);
> > - hw_state->ctrl1 = (val >> (dpll * 6)) & 0x3f;
> > + hw_state->ctrl1 = (val >> (pll->id * 6)) & 0x3f;
> >
> > /* avoid reading back stale values if HDMI mode is not enabled */
> > - if (val & DPLL_CTRL1_HDMI_MODE(dpll)) {
> > + if (val & DPLL_CTRL1_HDMI_MODE(pll->id)) {
> > hw_state->cfgcr1 = I915_READ(regs[pll->id].cfgcr1);
> > hw_state->cfgcr2 = I915_READ(regs[pll->id].cfgcr2);
> > }
> > @@ -886,6 +906,37 @@ out:
> > return ret;
> > }
> >
> > +static bool skl_ddi_dpll0_get_hw_state(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll,
> > + struct intel_dpll_hw_state
> > *hw_state)
> > +{
> > + uint32_t val;
> > + const struct skl_dpll_regs *regs = skl_dpll_regs;
> > + bool ret;
> > +
> > + WARN_ON(pll->id != DPLL_ID_SKL_DPLL0);
> I'm all for paranoia, but how can this ever happen?
>
> Seems pll0 has its own function pointers, so you'd have to do something like
> pll1->funcs->get_hw_state(pll0);
>
> I don't think this will ever happen, so all the paranoia from these functions
> that are called from the function pointer can be removed.
I went a bit overboard. Will respin.
> > + if (!intel_display_power_get_if_enabled(dev_priv,
> > POWER_DOMAIN_PLLS))
> > + return false;
> > +
> > + ret = false;
> > +
> > + /* DPLL0 is always enabled since it drives CDCLK */
> > + val = I915_READ(regs[pll->id].ctl);
> > + if (WARN_ON(!(val & LCPLL_PLL_ENABLE)))
> > + goto out;
> > +
> > + val = I915_READ(DPLL_CTRL1);
> > + hw_state->ctrl1 = (val >> (pll->id * 6)) & 0x3f;
> > +
> > + ret = true;
> > +
> > +out:
> > + intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > +
> > + return ret;
> > +}
> > +
> > struct skl_wrpll_context {
> > uint64_t min_deviation; /* current minimal deviation
> > */
> > uint64_t central_freq; /* chosen central freq */
> > @@ -1165,7 +1216,8 @@ skl_get_dpll(struct intel_crtc *crtc, struct
> > intel_crtc_state *crtc_state,
> > DPLL_CFGCR2_PDIV(wrpll_params.pdiv) |
> > wrpll_params.central_freq;
> > } else if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> > - encoder->type == INTEL_OUTPUT_DP_MST) {
> > + encoder->type == INTEL_OUTPUT_DP_MST ||
> > + encoder->type == INTEL_OUTPUT_EDP) {
> > switch (crtc_state->port_clock / 2) {
> > case 81000:
> > ctrl1 |=
> > DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810, 0);
> > @@ -1176,6 +1228,19 @@ skl_get_dpll(struct intel_crtc *crtc, struct
> > intel_crtc_state *crtc_state,
> > case 270000:
> > ctrl1 |=
> > DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2700, 0);
> > break;
> > + /* eDP 1.4 rates */
> > + case 162000:
> > + ctrl1 |=
> > DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620, 0);
> > + break;
> > + /* TBD: For DP link rates 2.16 GHz and 4.32 GHz, VCO is
> > 8640 which
> > + results in CDCLK change. Need to handle the change of CDCLK
> > by
> > + disabling pipes and re-enabling them */
> Sounds like we really need to start supporting cdclk changes on SKL, if it's
> based on this pll?
> modeset_calc_cdclk and modeset_commit_cdclk.
Clint Taylor has been working on it:
https://lists.freedesktop.org/archives/intel-gfx/2016-February/087796.html
Ander
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
