[Intel-gfx] [PATCH 07/14] drm/i915: add PLL sharing support to handle 3 pipes

[Jesse Barnes]

On Thu, 20 Oct 2011 23:18:06 -0700
Keith Packard <keithp at keithp.com> wrote:

> On Wed, 19 Oct 2011 08:12:08 -0700, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> 
> >  	/* PCH eDP needs FDI, but CPU eDP does not */
> > -	if (!has_edp_encoder || intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
> > +	if (!intel_crtc->no_pll &&
> > +	    (!has_edp_encoder ||
> > +	     intel_encoder_is_pch_edp(&has_edp_encoder->base))) {
> >  		I915_WRITE(PCH_FP0(pipe), fp);
> >  		I915_WRITE(PCH_DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> >  
> >  		POSTING_READ(PCH_DPLL(pipe));
> >  		udelay(150);
> > +	} else {
> > +		if (dpll == (I915_READ(PCH_DPLL(0)) & 0x7fffffff) &&
> > +		    fp == I915_READ(PCH_FP0(0))) {
> > +			intel_crtc->use_pll_a = true;
> > +			DRM_DEBUG_KMS("using pipe a dpll\n");
> > +		} else if (dpll == (I915_READ(PCH_DPLL(1)) & 0x7fffffff) &&
> > +			   fp == I915_READ(PCH_FP0(1))) {
> > +			intel_crtc->use_pll_a = false;
> > +			DRM_DEBUG_KMS("using pipe b dpll\n");
> > +		} else {
> > +			DRM_DEBUG_KMS("no matching PLL configuration for pipe 2\n");
> > +			return -EINVAL;
> > +		}
> 
> This hunk breaks eDP nicely -- you end up in the 'else' clause still
> looking for a DPLL to use.
> 
> Here's my suggested replacement:
> 
>  	/* PCH eDP needs FDI, but CPU eDP does not */
> -	if (!has_edp_encoder || intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
> -		I915_WRITE(PCH_FP0(pipe), fp);
> -		I915_WRITE(PCH_DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> -
> -		POSTING_READ(PCH_DPLL(pipe));
> -		udelay(150);
> +	if (!intel_crtc->no_pll) {
> +		if (!has_edp_encoder ||
> +		    intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
> +			I915_WRITE(PCH_FP0(pipe), fp);
> +			I915_WRITE(PCH_DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> +
> +			POSTING_READ(PCH_DPLL(pipe));
> +			udelay(150);
> +		}
> +	} else {
> +		if (dpll == (I915_READ(PCH_DPLL(0)) & 0x7fffffff) &&
> +		    fp == I915_READ(PCH_FP0(0))) {
> +			intel_crtc->use_pll_a = true;
> +			DRM_DEBUG_KMS("using pipe a dpll\n");
> +		} else if (dpll == (I915_READ(PCH_DPLL(1)) & 0x7fffffff) &&
> +			   fp == I915_READ(PCH_FP0(1))) {
> +			intel_crtc->use_pll_a = false;
> +			DRM_DEBUG_KMS("using pipe b dpll\n");
> +		} else {
> +			DRM_DEBUG_KMS("no matching PLL configuration for pipe 2\n");
> +			return -EINVAL;
> +		}
>  	}
> 

Yeah works much better with this applied.  Makes me want to change the
PLL sharing code a bit though; this should be factored out into a
separate function and I should probably just add a tiny PLL allocator
for the pipes.  That would make some of the code a little cleaner I
think, though add a bit more configuration complexity...

Tested-by: Jesse Barnes <jbarnes at virtuousgeek.org>
Acked-by: Jesse Barnes <jbarnes at virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center