[Intel-gfx] [v2] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

[Kumar, Shobhit]

On 10/08/2015 04:59 PM, Imre Deak wrote:
> On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote:
>> Reuse what is programmed by pre-os, but in case there is no pre-os
>> initialization, init the cdclk with the max value by default untill
>> dynamic cdclk support comes.
>>
>> v2: Check if BIOS programmed correctly rather than always calling init
>>      - Do validation of programmed cdctl and what it is expected
>>      - Only do slk_init_cdclk if validation failed else reuse BIOS
>>        programmed value
>>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++-----
>>   drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
>>   2 files changed, 42 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 2d3cc82..3ec5618 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>   		int cdclk_freq;
>>
>>   		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>> -		dev_priv->skl_boot_cdclk = cdclk_freq;
>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> -			DRM_ERROR("LCPLL1 is disabled\n");
>> -		else
>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +
>> +		/* Invalid CDCLK from BIOS ? */
>> +		if (cdclk_freq < 0) {
>> +			/* program to maximum cdclk till we have dynamic cdclk support */
>> +			dev_priv->skl_boot_cdclk = 675000;
>> +			skl_init_cdclk(dev_priv);
>
> This would still try to reprogram CDCLK if BIOS enabled an output with
> an incorrect CDCLK decimal part. Isn't this the exact scenario you're
> seeing? As said before reprogramming CDCLK in that case would require
> disabling the outputs first.

I went with the hypothesis that if VBIOS/GOP was loaded it would have to 
correct the cdclock, with wrong decimal value display cannot be enabled. 
For example AUX will fail on SKL. So for correct display output enabled 
cdclock has to be correctly programmed. If it is wrong display was not 
enabled at all.

The scenario which I am seeing is VBIOS/GOP is not loaded at all, and 
the pre-os is not leaving cdclock in correct state.

>
> We would also need to call skl_init_cdclk if the BIOS hasn't enabled the
> PLL for some reason. But I guess that could be a separate change.
>
>> +		} else {
>> +			dev_priv->skl_boot_cdclk = cdclk_freq;
>> +			if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> +				DRM_ERROR("LCPLL1 is disabled\n");
>> +			else
>> +				intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +		}
>>   	} else if (IS_BROXTON(dev)) {
>>   		broxton_init_cdclk(dev);
>>   		broxton_ddi_phy_init(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index bbeb6d3..f734410 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>   	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>>   	uint32_t cdctl = I915_READ(CDCLK_CTL);
>>   	uint32_t linkrate;
>> +	int freq;
>>
>>   	if (!(lcpll1 & LCPLL_PLL_ENABLE))
>>   		return 24000; /* 24MHz is the cd freq with NSSC ref */
>>
>> -	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540)
>> -		return 540000;
>> +	if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) {
>> +		freq = 540000;
>> +		goto verify;
>> +	}
>>
>>   	linkrate = (I915_READ(DPLL_CTRL1) &
>>   		    DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1;
>> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev)
>>   		/* vco 8640 */
>>   		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>   		case CDCLK_FREQ_450_432:
>> -			return 432000;
>> +			freq = 432000;
>> +			break;
>>   		case CDCLK_FREQ_337_308:
>> -			return 308570;
>> +			freq = 308570;
>> +			break;
>>   		case CDCLK_FREQ_675_617:
>> -			return 617140;
>> +			freq = 617140;
>> +			break;
>>   		default:
>>   			WARN(1, "Unknown cd freq selection\n");
>> +			return -EINVAL;
>>   		}
>>   	} else {
>>   		/* vco 8100 */
>>   		switch (cdctl & CDCLK_FREQ_SEL_MASK) {
>>   		case CDCLK_FREQ_450_432:
>> -			return 450000;
>> +			freq = 450000;
>> +			break;
>>   		case CDCLK_FREQ_337_308:
>> -			return 337500;
>> +			freq = 337500;
>> +			break;
>>   		case CDCLK_FREQ_675_617:
>> -			return 675000;
>> +			freq = 675000;
>> +			break;
>>   		default:
>>   			WARN(1, "Unknown cd freq selection\n");
>> +			return -EINVAL;
>>   		}
>>   	}
>>
>> -	/* error case, do as if DPLL0 isn't enabled */
>> -	return 24000;
>> +verify:
>> +	/*
>> +	 * Noticed in some instances that the freq selection is correct but
>> +	 * decimal part is programmed wrong from BIOS where pre-os does not
>> +	 * enable display. Verify the same as well.
>> +	 */
>> +	if (cdctl == ((cdctl & CDCLK_FREQ_SEL_MASK) | skl_cdclk_decimal(freq)))
>> +		return freq;
>> +	else
>> +		return -EINVAL;
>>   }
>>
>>   static int broxton_get_display_clock_speed(struct drm_device *dev)
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>