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@xxxxxxxxx> > >>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx> > >>>>--- > >>>> 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx