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

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

On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote:
> 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@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@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.

But it still enabled DPLL0? Why does it do that if it doesn't set up
cdclk?

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

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