| Message ID | 1444058564-13663-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > } else if (IS_BROXTON(dev)) { > broxton_init_cdclk(dev); > broxton_ddi_phy_init(dev);
On Mon, 05 Oct 2015, Imre Deak <imre.deak@intel.com> 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. Random comment from the back, what about intentional cdclk change on module load? We're supposed to have that for bdw [1] but last I checked it failed... [2]. BR, Jani. [1] http://mid.gmane.org/1433335514-4156-8-git-send-email-mika.kahola@intel.com [2] http://mid.gmane.org/87bngf4xk3.fsf@intel.com > >> } else if (IS_BROXTON(dev)) { >> broxton_init_cdclk(dev); >> broxton_ddi_phy_init(dev); > >
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. 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. 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 >
On 10/06/2015 12:05 PM, Jani Nikula wrote: > On Mon, 05 Oct 2015, Imre Deak <imre.deak@intel.com> 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. > > Random comment from the back, what about intentional cdclk change on > module load? We're supposed to have that for bdw [1] but last I checked > it failed... [2]. Yes. We will require cdclk change run time as well say if we hot plug a DP 4k@60. So this capability is something that might really be needed. Regards Shobhit > > BR, > Jani. > > [1] http://mid.gmane.org/1433335514-4156-8-git-send-email-mika.kahola@intel.com > [2] http://mid.gmane.org/87bngf4xk3.fsf@intel.com > > >> >>> } else if (IS_BROXTON(dev)) { >>> broxton_init_cdclk(dev); >>> broxton_ddi_phy_init(dev); >> >> >
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 > >
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. Regards Shobhit > >> >> 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 >>> > >
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. -Daniel
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. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 :( -Daniel
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. Regards Shobhit > -Daniel >
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. -Daniel
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. I would just keep it somewhere in init/resume path rather than polluting .get_display_clock_speed with it. We should be calling intel_update_cdclk() in those paths somewhere, so doing the fixup just before that should be sufficient. > 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. Can we figure out the eDP link rate requirements ahead of time from VBT? That should dictate what the VCO frequency has to be. If we get it wrong, then we'd have to change the VCO again after the eDP probe has told us what we actually need. Such a second fixup could actually be left up to the normal modeset calc_cdclk stuff, which shouldn't really need any special handling for it apart from actually updating the DPLL0 settings if needed, in addition to the normal cdclk divider stuff.
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.
On 10/06/2015 06:55 PM, Ville Syrjälä 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. > > I would just keep it somewhere in init/resume path rather than polluting > .get_display_clock_speed with it. We should be calling > intel_update_cdclk() in those paths somewhere, so doing the fixup just > before that should be sufficient. I think, get_display_clock_speed should be okay where it returns valid cdclk if programmed correctly else returns -EINVAL. In case of boot, invalid means pre-OS did not try to enable display and based on the error, during ddi_pll_init, we can safely call skl_init_cdclk. > >> 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. > > Can we figure out the eDP link rate requirements ahead of time from > VBT? That should dictate what the VCO frequency has to be. If we > get it wrong, then we'd have to change the VCO again after the eDP > probe has told us what we actually need. Such a second fixup could > actually be left up to the normal modeset calc_cdclk stuff, which > shouldn't really need any special handling for it apart from actually > updating the DPLL0 settings if needed, in addition to the normal cdclk > divider stuff. >
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); } else if (IS_BROXTON(dev)) { broxton_init_cdclk(dev); broxton_ddi_phy_init(dev);
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(-)