On Thu, Dec 03, 2015 at 09:02:12AM +0100, Daniel Vetter wrote: > On Wed, Nov 25, 2015 at 04:35:31PM +0200, [email protected] wrote: > > From: Ville Syrjälä <[email protected]> > > > > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it > > forbidden to set it for LVDS/CRT as well. So let's move it out of the > > ironlake_compute_dpll() and just do it on demand in the pll enable hook. > > This allows the PLL to be shared in more cases when dealing with > > different output types. > > > > Note that we must now call the pll enable hook regarless of the current > > pll state, so that DPLL_SDVO_HIGH_SPEED gets updated appropriately. > > > > FIXME: maybe better to add a separate hook for the "pll already enabled" > > case? > > > > Signed-off-by: Ville Syrjälä <[email protected]> > > --- > > drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++-------- > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 26cafeea2845..ae58f1105458 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1908,6 +1908,8 @@ static void intel_enable_shared_dpll(struct > > intel_crtc *crtc) > > if (pll->active++) { > > WARN_ON(!pll->on); > > assert_shared_dpll_enabled(dev_priv, pll); > > + /* to update high speed IO clock state */ > > + pll->enable(dev_priv, pll); > > return; > > } > > WARN_ON(pll->on); > > @@ -8942,11 +8944,6 @@ static uint32_t ironlake_compute_dpll(struct > > intel_crtc *intel_crtc, > > dpll |= (crtc_state->pixel_multiplier - 1) > > << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT; > > > > - if (is_sdvo) > > - dpll |= DPLL_SDVO_HIGH_SPEED; > > - if (crtc_state->has_dp_encoder) > > - dpll |= DPLL_SDVO_HIGH_SPEED; > > - > > /* compute bitmask from p1 value */ > > dpll |= (1 << (crtc_state->dpll.p1 - 1)) << > > DPLL_FPA01_P1_POST_DIV_SHIFT; > > /* also FPA1 */ > > @@ -13626,7 +13623,7 @@ static bool ibx_pch_dpll_get_hw_state(struct > > drm_i915_private *dev_priv, > > return false; > > > > val = I915_READ(PCH_DPLL(pll->id)); > > - hw_state->dpll = val; > > + hw_state->dpll = val & ~DPLL_SDVO_HIGH_SPEED; > > hw_state->fp0 = I915_READ(PCH_FP0(pll->id)); > > hw_state->fp1 = I915_READ(PCH_FP1(pll->id)); > > > > @@ -13643,10 +13640,26 @@ static void ibx_pch_dpll_mode_set(struct > > drm_i915_private *dev_priv, > > static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv, > > struct intel_shared_dpll *pll) > > { > > + struct drm_device *dev = dev_priv->dev; > > + struct intel_crtc *crtc; > > + u32 dpll = pll->config.hw_state.dpll; > > + > > + /* Configure high speed IO clock as needed */ > > + for_each_intel_crtc(dev, crtc) { > > + if (intel_crtc_to_shared_dpll(crtc) == pll && > > + crtc->config->base.active && > > + (crtc->config->has_dp_encoder || > > + intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) || > > + intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))) { > > + dpll |= DPLL_SDVO_HIGH_SPEED; > > + break; > > + } > > + } > > This is a bit ugly. What about adding a needs_5x_clock to struct > intel_shared_dpll_config (i.e. outside of the hw pll state to avoid > unsharing and pipe checker warnings)? ilk_compute_dpll would set that, and > this function here obeys. > > Clearing would only happen when we completely disable a pll after the last > user is gone.
I don't really like potentially wasting power like that. I suppose it's a miniscule amount most likely, but still feels a bit wrong. > > I just find looping over anything in modeset code offensive ;-) > > Also please add your extended bspec research to the commit message. > -Daniel > > > + > > /* PCH refclock must be enabled first */ > > ibx_assert_pch_refclk_enabled(dev_priv); > > > > - I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll); > > + I915_WRITE(PCH_DPLL(pll->id), dpll); > > > > /* Wait for the clocks to stabilize. */ > > POSTING_READ(PCH_DPLL(pll->id)); > > @@ -13657,7 +13670,7 @@ static void ibx_pch_dpll_enable(struct > > drm_i915_private *dev_priv, > > * > > * So write it again. > > */ > > - I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll); > > + I915_WRITE(PCH_DPLL(pll->id), dpll); > > POSTING_READ(PCH_DPLL(pll->id)); > > udelay(200); > > } > > -- > > 2.4.10 > > > > _______________________________________________ > > Intel-gfx mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
