Re: [PATCH] drm/i915/skl: CDCLK change during modeset based on VCO in use.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 20, 2015 at 10:10:43AM -0800, Clint Taylor wrote:
> On 11/20/2015 05:55 AM, Ville Syrjälä wrote:
> > On Thu, Nov 19, 2015 at 09:20:16AM -0800, clinton.a.taylor@intel.com wrote:
> >> From: Clint Taylor <clinton.a.taylor@intel.com>
> >>
> >> Add SKL and KBL cdclk changes during modeset. Taking into account new
> >> linkrates available using 8640 VCO.
> >>
> >> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_display.c |   68 ++++++++++++++++++++++++++++++++++
> >>   1 file changed, 68 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 2d7ea95..bed03cb 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -9727,6 +9727,69 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> >>   	broadwell_set_cdclk(dev, req_cdclk);
> >>   }
> >>
> >> +static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >> +{
> >> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >> +	int max_pixclk = ilk_max_pixel_rate(state);
> >> +	int cdclk;
> >> +	uint32_t linkrate;
> >> +
> >> +	linkrate = (I915_READ(DPLL_CTRL1) &
> >> +		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
> >
> > I don't think we should read this from the hardware here. Instead
> > we should stash the proper vco somewhere under dev_priv. Well, we
> 
> I have no problem stuffing the proper VCO based on the link rate 
> determined in skl_edp_set_pll_config(). I think just storing a uint32 of 
> the required rate would be enough as we know 216000 and 432000 require 
> the 8640 VCO.
> 
> static const int skl_rates[] = { 162000, 216000, 270000,
> 				  324000, 432000, 540000 };
> 
> 
> > already have the boot_cdclk there, which is more or less just that.
> > What's really missing is code to fix up our initial boot_cdclk
> > choice if it turns out to be wrong. Where would it get fixed? I
> > assume we'd do that during/after eDP probing since then we should
> > know what link rate we want to use.
> 
> boot_cdclk is irrelevant as soon as boot has completed.

No. It's the thing we want to look at to see what we want the DPLL0
VCO to be. We should just rename it to something better, and probably
just store the VCO there instead of the cdclk.

> I agree we need 
> to fixup cdclk in cases when incorrect like at boot or during resume. 
> Both cases are outside the scope of this patch which is just to fix 
> cdclk during modeset when the boot_cdclk is < required CDCLK after an 
> HPD event.

I'm not thinking about fixing anything explicitly during resume/init.
We will do a modeset during resume, and the normal cdclk programming
will happen there. So there should be nothing more to do. And the
same thing will happen during boot in case the current cdclk is not
what we actually want.

So we just need to figure out is which VCO should we use. And for
that we need to know the eDP link frequency. Oh and we also need
to update max_cdclk to match in case it was originally determined
using the wrong VCO.

> 
> >
> >> +
> >> +	/*
> >> +	* FIXME should also account for plane ratio
> >> +	* once 64bpp pixel formats are supported.
> >> +	*/
> >> +
> >> +	if (linkrate == DPLL_CTRL1_LINK_RATE_2160 ||
> >> +	    linkrate == DPLL_CTRL1_LINK_RATE_1080) {
> >> +		/* vco 8640 */
> >> +		if (max_pixclk > 540000)
> >> +			cdclk = 617140;
> >> +		else if (max_pixclk > 432000)
> >> +			cdclk = 540000;
> >> +		else if (max_pixclk > 308570)
> >> +			cdclk = 432000;
> >> +		else
> >> +			cdclk = 308570;
> >> +	}
> >> +	else {
> >> +		/* VCO 8100 */
> >> +		if (max_pixclk > 540000)
> >> +			cdclk = 675000;
> >> +		else if (max_pixclk > 450000)
> >> +			cdclk = 540000;
> >> +		else if (max_pixclk > 337500)
> >> +			cdclk = 450000;
> >> +		else
> >> +			cdclk = 337500;
> >> +	}
> >> +
> >> +	/*
> >> +	 * FIXME move the cdclk caclulation to
> >> +	 * compute_config() so we can fail gracefully.
> >> +	 */
> >> +	if (cdclk > dev_priv->max_cdclk_freq) {
> >> +		DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> >> +			  cdclk, dev_priv->max_cdclk_freq);
> >> +		cdclk = dev_priv->max_cdclk_freq;
> >> +	}
> >> +
> >> +	to_intel_atomic_state(state)->cdclk = cdclk;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> >> +{
> >> +	struct drm_device *dev = old_state->dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> >> +
> >> +	skl_set_cdclk(dev_priv, req_cdclk);
> >> +}
> >> +
> >>   static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> >>   				      struct intel_crtc_state *crtc_state)
> >>   {
> >> @@ -14831,6 +14894,11 @@ static void intel_init_display(struct drm_device *dev)
> >>   			broxton_modeset_commit_cdclk;
> >>   		dev_priv->display.modeset_calc_cdclk =
> >>   			broxton_modeset_calc_cdclk;
> >> +	} else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
> >> +		dev_priv->display.modeset_commit_cdclk =
> >> +			skl_modeset_commit_cdclk;
> >> +		dev_priv->display.modeset_calc_cdclk =
> >> +			skl_modeset_calc_cdclk;
> >>   	}
> >>
> >>   	switch (INTEL_INFO(dev)->gen) {
> >> --
> >> 1.7.9.5
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx