Re: [PATCH v2] drm/i915: Make i9xx_crtc_clock_get() work for PCH DPLLs

From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915: Make i9xx_crtc_clock_get() work for PCH DPLLs
Date: Fri, 13 Sep 2013 16:47:53 +0300	[thread overview]
Message-ID: <8738p8n88m.fsf@intel.com> (raw)
In-Reply-To: <20130913130632.GT20128@intel.com>

On Fri, 13 Sep 2013, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 13, 2013 at 04:04:03PM +0300, Jani Nikula wrote:
>> On Mon, 09 Sep 2013, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Add the 120MHz refernce clock case for PCH DPLLs.
>> >
>> > Also determine the reference clock frequency more accurately by
>> > checking for the PLLB_REF_INPUT_SPREADSPECTRUMIN refclk input
>> > mode. The gen2 code already checked it, but it stil assumed a
>> > fixed 66MHz refclk. Instead we need to consult the VBT for the
>> > real value.
>> >
>> > v2: Fix refclk for SSC panel case
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++++++-----------
>> >  1 file changed, 21 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 754de85..4f07292 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -7324,6 +7324,22 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>> >  	mutex_unlock(&crtc->mutex);
>> >  }
>> >  
>> > +static int i9xx_pll_refclk(struct drm_device *dev,
>> > +			   const struct intel_crtc_config *pipe_config)
>> > +{
>> > +	struct drm_i915_private *dev_priv = dev->dev_private;
>> > +	u32 dpll = pipe_config->dpll_hw_state.dpll;
>> > +
>> > +	if ((dpll & PLL_REF_INPUT_MASK) == PLLB_REF_INPUT_SPREADSPECTRUMIN)
>> 
>> This seems wrong for at least gen3 and vlv. And it's a bit scary to go
>> change gen2 but oh well...
>
> Why? i8xx_update_pll(), i9xx_update_pll() and ironlake_compute_dpll()
> all have the same logic for setting that bit.

My Super Reliable(tm) gen3 spec says reserved for that value
there. *shrug*.

> For VLV I agree. But the clock readout there is totally busted anyway,
> so I don't care at this point.

Maybe Daniel can copy-paste something along those lines in the commit
message. Or not. *shrug. :)

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
>> 
>> Jani.
>> 
>> > +		return dev_priv->vbt.lvds_ssc_freq * 1000;
>> > +	else if (HAS_PCH_SPLIT(dev))
>> > +		return 120000;
>> > +	else if (!IS_GEN2(dev))
>> > +		return 96000;
>> > +	else
>> > +		return 48000;
>> > +}
>> > +
>> >  /* Returns the clock of the currently programmed mode of the given pipe. */
>> >  static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>> >  				struct intel_crtc_config *pipe_config)
>> > @@ -7334,6 +7350,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>> >  	u32 dpll = pipe_config->dpll_hw_state.dpll;
>> >  	u32 fp;
>> >  	intel_clock_t clock;
>> > +	int refclk = i9xx_pll_refclk(dev, pipe_config);
>> >  
>> >  	if ((dpll & DISPLAY_RATE_SELECT_FPA1) == 0)
>> >  		fp = pipe_config->dpll_hw_state.fp0;
>> > @@ -7373,9 +7390,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>> >  		}
>> >  
>> >  		if (IS_PINEVIEW(dev))
>> > -			pineview_clock(96000, &clock);
>> > +			pineview_clock(refclk, &clock);
>> >  		else
>> > -			i9xx_clock(96000, &clock);
>> > +			i9xx_clock(refclk, &clock);
>> >  	} else {
>> >  		bool is_lvds = (pipe == 1) && (I915_READ(LVDS) & LVDS_PORT_EN);
>> >  
>> > @@ -7383,13 +7400,6 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>> >  			clock.p1 = ffs((dpll & DPLL_FPA01_P1_POST_DIV_MASK_I830_LVDS) >>
>> >  				       DPLL_FPA01_P1_POST_DIV_SHIFT);
>> >  			clock.p2 = 14;
>> > -
>> > -			if ((dpll & PLL_REF_INPUT_MASK) ==
>> > -			    PLLB_REF_INPUT_SPREADSPECTRUMIN) {
>> > -				/* XXX: might not be 66MHz */
>> > -				i9xx_clock(66000, &clock);
>> > -			} else
>> > -				i9xx_clock(48000, &clock);
>> >  		} else {
>> >  			if (dpll & PLL_P1_DIVIDE_BY_TWO)
>> >  				clock.p1 = 2;
>> > @@ -7401,9 +7411,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>> >  				clock.p2 = 4;
>> >  			else
>> >  				clock.p2 = 2;
>> > -
>> > -			i9xx_clock(48000, &clock);
>> >  		}
>> > +
>> > +		i9xx_clock(refclk, &clock);
>> >  	}
>> >  
>> >  	pipe_config->adjusted_mode.clock = clock.dot;
>> > -- 
>> > 1.8.1.5
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx