[Intel-gfx] [PATCH 2/2] drm/i915: Allow PCH DPLL sharing regardles of DPLL_SDVO_HIGH_SPEED

[Ville Syrjälä]

On Thu, Nov 26, 2015 at 01:42:10PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 26, 2015 at 09:41:36AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 25, 2015 at 04:35:31PM +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > 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.
> > 
> > Nowhere is it forbidden feels a bit weak ... what's the benefits? Do we
> > really care?
> 
> We get to share the PLL eg. between HDMI and VGA. We already do that
> anyway if we clone HDMI+VGA. Othwerwise we always run out of PLLs when
> trying to do three pipes with something like VGA+HDMI+DP.

I read through the spec a bit more, and while they don't say things
entirely clearly, I think this bit just enables the x5 clock output
from the DPLL into the port. CRT doesn't need to deal with individual
bits, and so doesn't need this clock. And maybe gating it off at the
DPLL saves a bit of power.

Based on that I'm thinking that in theory we should be able to shared
the DPLL even between CRT and LVDS since the LVDS x7 clock is simply not
relevant for CRT. However the spec clearly groups the DAC with
SDVO/HDMI/DP when talking about the DPLL. Not entirely sure why that is.
If I had a machine with LVDS+CRT I would definitely test out this theory.

Anyways, I still think we want this patch.

> 
> > -Daniel
> > 
> > > 
> > > 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ä <ville.syrjala at linux.intel.com>
> > > ---
> > >  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;
> > > +		}
> > > +	}
> > > +
> > >  	/* 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
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC