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

[Clint Taylor <clinton.a.taylor@intel.com>]

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. 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.

>
>> +
>> +	/*
>> +	* 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
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx