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

From: Imre Deak <imre.deak@intel.com>
To: "Kumar, Shobhit" <shobhit.kumar@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os
Date: Tue, 06 Oct 2015 13:41:49 +0300	[thread overview]
Message-ID: <1444128109.8306.10.camel@intel.com> (raw)
In-Reply-To: <56139AD9.4020205@linux.intel.com>

On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
> On 10/05/2015 09:05 PM, Imre Deak wrote:
> > On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
> >> Mostly reuse what is programmed by pre-os, but in case there is no
> >> pre-os initialization, init the cdclk with the default value.
> >>
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 2d3cc82..675c60d 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >>
> >>   		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);
> >> +
> >> +		skl_init_cdclk(dev_priv);
> >
> > How does this prevent changing the clock if BIOS did enable some output?
> > We shouldn't change the clock in that case.
> 
> In that case it will try to re-apply the same clock that BIOS enabled. 
> Not sure if this is allowed, but I checked the cdclock change sequence 
> and it is mostly followed in skl_init_cdclk.
> In my tests where BIOS does enable this, I faced no issues in
> initializing again in driver.

The first step in that sequence:
"Disable all display engine functions using the full mode set disable
sequence on all pipes, ports, and planes."

So the problem is not that the PLL itself may be enabled here (as BIOS
left it), but that some output is also enabled.

> I have noticed on some pre-os this value is programmed correctly except
> for the decimal part. That causes AUX transactions to fail on SKl. That
> is what triggered this patch actually. So other way is to completely
> validate the value in get_display_clock_speed instead of bit[28:26] and
> then if wrong then only do the cdclk init.

I think we'd need to detect at this point if outputs are enabled and
only attempt to work around the above BIOS problem if this is not the
case. Alternatively you could also disable the active outputs as a first
step.

> 
> Regards
> Shobhit
> 
> >
> >>   	} else if (IS_BROXTON(dev)) {
> >>   		broxton_init_cdclk(dev);
> >>   		broxton_ddi_phy_init(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