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

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

On Tue, Oct 06, 2015 at 03:04:28PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 06:13:52PM +0530, Kumar, Shobhit wrote:
> > On 10/06/2015 05:49 PM, Daniel Vetter wrote:
> > >On Tue, Oct 06, 2015 at 02:41:44PM +0300, Ville Syrjälä wrote:
> > >>On Tue, Oct 06, 2015 at 01:19:52PM +0200, Daniel Vetter wrote:
> > >>>On Tue, Oct 06, 2015 at 04:33:43PM +0530, Kumar, Shobhit wrote:
> > >>>>On 10/06/2015 04:11 PM, Imre Deak wrote:
> > >>>>>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."
> > >>>>
> > >>>>Oh, yeah, I again made mistake of assuming that display is not enabled in
> > >>>>the first place. You are right, though it works if I change the clock again.
> > >>>>
> > >>>>>
> > >>>>>So the problem is not that the PLL itself may be enabled here (as BIOS
> > >>>>>left it), but that some output is also enabled.
> > >>>>
> > >>>>Yes.
> > >>>>
> > >>>>>
> > >>>>>>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.
> > >>>>
> > >>>>Ok, let me detect if any output is enabled by BIOS and accordingly
> > >>>>initialize cdclk.
> > >>>
> > >>>These kind of fixiups should be done after the hw state readout. We
> > >>>already have sanitize_crtc/pll/encoder functions, probably best if we add
> > >>>a sanitize_cdclk or similar for this at the very end of the hw state
> > >>>sanitize sequence.
> > >>
> > >>Can't be done if we already need a somewhat sane cdclk for the
> > >>eDP AUX probing and whatnot.
> > >>
> > >>For actually enabling the cdclk for pushing pixels, we wouldn't need
> > >>to do anything except actually plug ia a calc_cdclk for SKL. No idea
> > >>why we're not doing that currently. Some extra care may be needed
> > >>due to the eDP DPLL0 usag IIRC.
> > >
> > >Hm right, cdlck is in the top-level power domain. Added fun is that with
> > >dmc the firmware is supposed to handle it. Messy :(
> > 
> > Yes, exactly. How about just adding verify_cdclk and calling in
> > get_display_clock_speed to check if cdclk is programmed correctly along with
> > related DPLL0 VCO settings for now. If all looks good, then skip else
> > initialize. Now in that case if we have to initialize where do we get the
> > cdclock to initialize with at this point ? Any default in VBT ? Or go with
> > minimum by default and it can be bumped up later if needed.
> 
> Just initialize to the slowest possible value, once we have dynamic cdclk
> switching for skl. But that seems to be stuck behind resolving that big
> confusion around dmc and the sequence for runtime pm. Without dynamic
> cdclk we have to pick the maximum.

I suppose we could simply disable DC5/6 around the cdclk update if
needed. But I'm not sure we'd need to do that since any register access
should anyway bring it out of DC5/6, and when it goes back into DC5/6
it'll save the current values and restore them again on the next wakeup.

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