| Message ID | 1445001533-28070-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/16/2015 06:48 PM, Shobhit Kumar wrote: > Especially in cases where pre-os does not enable display, cdclk might > not be in sane state. During sanitization initialize cdclk with maximum > value till we get dynamic cdclk support. > > 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 > > v3: Move the validation logic in a separate sanitize function (Ville) Ville, does this come even close to what you had in mind ? > > 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 | 12 ++++++++---- > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index b25e99a..86d43e6 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2949,10 +2949,14 @@ 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); > + if (skl_sanitize_cdclk(dev_priv)) > + DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); > + else { > + 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 5f37f84..98333d3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) > DRM_ERROR("DBuf power enable timeout\n"); > } > > +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) > +{ > + uint32_t lcpll1 = I915_READ(LCPLL1_CTL); > + uint32_t cdctl = I915_READ(CDCLK_CTL); > + int freq = dev_priv->skl_boot_cdclk; > + > + /* Is PLL enabled and locked ? */ > + if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) > + goto sanitize; > + > + /* DPLL okay; verify the cdclock > + * > + * 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))) > + /* All well; nothing to sanitize */ > + return false; > +sanitize: > + /* > + * As of now initialize with max cdclk till > + * we get dynamic cdclk support > + * */ > + dev_priv->skl_boot_cdclk = 675000; > + skl_init_cdclk(dev_priv); > + > + /* we did have to sanitize */ > + return true; > +} > + > /* Adjust CDclk dividers to allow high res or save power if possible */ > static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) > { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0598932..ec10e6a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev); > void bxt_enable_dc9(struct drm_i915_private *dev_priv); > void bxt_disable_dc9(struct drm_i915_private *dev_priv); > void skl_init_cdclk(struct drm_i915_private *dev_priv); > +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); > void skl_uninit_cdclk(struct drm_i915_private *dev_priv); > void intel_dp_get_m_n(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config); >
On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote: > Especially in cases where pre-os does not enable display, cdclk might > not be in sane state. During sanitization initialize cdclk with maximum > value till we get dynamic cdclk support. > > 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 > > v3: Move the validation logic in a separate sanitize function (Ville) > > 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 | 12 ++++++++---- > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index b25e99a..86d43e6 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2949,10 +2949,14 @@ 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); > + if (skl_sanitize_cdclk(dev_priv)) > + DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); > + else { > + if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) > + DRM_ERROR("LCPLL1 is disabled\n"); Since skl_sanitize_cdclk() will enable the PLL this shouldn't happen anymore, right? > + 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 5f37f84..98333d3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) > DRM_ERROR("DBuf power enable timeout\n"); > } > > +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) > +{ > + uint32_t lcpll1 = I915_READ(LCPLL1_CTL); > + uint32_t cdctl = I915_READ(CDCLK_CTL); > + int freq = dev_priv->skl_boot_cdclk; > + > + /* Is PLL enabled and locked ? */ > + if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) > + goto sanitize; > + > + /* DPLL okay; verify the cdclock > + * > + * 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))) > + /* All well; nothing to sanitize */ > + return false; > +sanitize: > + /* > + * As of now initialize with max cdclk till > + * we get dynamic cdclk support > + * */ > + dev_priv->skl_boot_cdclk = 675000; Should be '= dev_priv->max_cdclk' I think we end up doing the intel_update_cdclk() before this gets called, so max_cdclk should already contain something sensible. The whole init sequence is a bit messy currently, but I think we can put off cleaning it up after the dc6 stuff gets sorted. > + skl_init_cdclk(dev_priv); > + > + /* we did have to sanitize */ > + return true; > +} > + > /* Adjust CDclk dividers to allow high res or save power if possible */ > static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) > { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0598932..ec10e6a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev); > void bxt_enable_dc9(struct drm_i915_private *dev_priv); > void bxt_disable_dc9(struct drm_i915_private *dev_priv); > void skl_init_cdclk(struct drm_i915_private *dev_priv); > +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); > void skl_uninit_cdclk(struct drm_i915_private *dev_priv); > void intel_dp_get_m_n(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config); > -- > 2.4.3
On 10/19/2015 07:18 PM, Ville Syrjälä wrote: > On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote: >> Especially in cases where pre-os does not enable display, cdclk might >> not be in sane state. During sanitization initialize cdclk with maximum >> value till we get dynamic cdclk support. >> >> 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 >> >> v3: Move the validation logic in a separate sanitize function (Ville) >> >> 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 | 12 ++++++++---- >> drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> 3 files changed, 40 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index b25e99a..86d43e6 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2949,10 +2949,14 @@ 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); >> + if (skl_sanitize_cdclk(dev_priv)) >> + DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); >> + else { >> + if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) >> + DRM_ERROR("LCPLL1 is disabled\n"); > > Since skl_sanitize_cdclk() will enable the PLL this shouldn't > happen anymore, right? > Yeah right. Will remove. >> + 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 5f37f84..98333d3 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) >> DRM_ERROR("DBuf power enable timeout\n"); >> } >> >> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) >> +{ >> + uint32_t lcpll1 = I915_READ(LCPLL1_CTL); >> + uint32_t cdctl = I915_READ(CDCLK_CTL); >> + int freq = dev_priv->skl_boot_cdclk; >> + >> + /* Is PLL enabled and locked ? */ >> + if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) >> + goto sanitize; >> + >> + /* DPLL okay; verify the cdclock >> + * >> + * 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))) >> + /* All well; nothing to sanitize */ >> + return false; >> +sanitize: >> + /* >> + * As of now initialize with max cdclk till >> + * we get dynamic cdclk support >> + * */ >> + dev_priv->skl_boot_cdclk = 675000; > > Should be '= dev_priv->max_cdclk' > > I think we end up doing the intel_update_cdclk() before this gets > called, so max_cdclk should already contain something sensible. The > whole init sequence is a bit messy currently, but I think we can put > off cleaning it up after the dc6 stuff gets sorted. > Actually at the end of skl_init_cdclock, intel_update_cdclk will be called and initialize the max_cdclk correctly. In case there was no sanitization needed, the BIOS programmed cdclk is stored in skl_boot_cdclk and that is used inside the skl_init_cdclk. Same is invoked resume time, so we need to update this variable or else change the logic. >> + skl_init_cdclk(dev_priv); >> + >> + /* we did have to sanitize */ >> + return true; >> +} >> + >> /* Adjust CDclk dividers to allow high res or save power if possible */ >> static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) >> { >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 0598932..ec10e6a 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev); >> void bxt_enable_dc9(struct drm_i915_private *dev_priv); >> void bxt_disable_dc9(struct drm_i915_private *dev_priv); >> void skl_init_cdclk(struct drm_i915_private *dev_priv); >> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); >> void skl_uninit_cdclk(struct drm_i915_private *dev_priv); >> void intel_dp_get_m_n(struct intel_crtc *crtc, >> struct intel_crtc_state *pipe_config); >> -- >> 2.4.3 >
On Tue, Oct 20, 2015 at 03:27:24PM +0530, Kumar, Shobhit wrote: > On 10/19/2015 07:18 PM, Ville Syrjälä wrote: > > On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote: > >> Especially in cases where pre-os does not enable display, cdclk might > >> not be in sane state. During sanitization initialize cdclk with maximum > >> value till we get dynamic cdclk support. > >> > >> 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 > >> > >> v3: Move the validation logic in a separate sanitize function (Ville) > >> > >> 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 | 12 ++++++++---- > >> drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> 3 files changed, 40 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index b25e99a..86d43e6 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -2949,10 +2949,14 @@ 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); > >> + if (skl_sanitize_cdclk(dev_priv)) > >> + DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); > >> + else { > >> + if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) > >> + DRM_ERROR("LCPLL1 is disabled\n"); > > > > Since skl_sanitize_cdclk() will enable the PLL this shouldn't > > happen anymore, right? > > > > Yeah right. Will remove. > > >> + 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 5f37f84..98333d3 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) > >> DRM_ERROR("DBuf power enable timeout\n"); > >> } > >> > >> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) > >> +{ > >> + uint32_t lcpll1 = I915_READ(LCPLL1_CTL); > >> + uint32_t cdctl = I915_READ(CDCLK_CTL); > >> + int freq = dev_priv->skl_boot_cdclk; > >> + > >> + /* Is PLL enabled and locked ? */ > >> + if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) > >> + goto sanitize; > >> + > >> + /* DPLL okay; verify the cdclock > >> + * > >> + * 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))) > >> + /* All well; nothing to sanitize */ > >> + return false; > >> +sanitize: > >> + /* > >> + * As of now initialize with max cdclk till > >> + * we get dynamic cdclk support > >> + * */ > >> + dev_priv->skl_boot_cdclk = 675000; > > > > Should be '= dev_priv->max_cdclk' > > > > I think we end up doing the intel_update_cdclk() before this gets > > called, so max_cdclk should already contain something sensible. The > > whole init sequence is a bit messy currently, but I think we can put > > off cleaning it up after the dc6 stuff gets sorted. > > > > Actually at the end of skl_init_cdclock, intel_update_cdclk will be > called and initialize the max_cdclk correctly. It gets called already earlier from intel_modeset_init(). > In case there was no > sanitization needed, the BIOS programmed cdclk is stored in > skl_boot_cdclk and that is used inside the skl_init_cdclk. Same is > invoked resume time, so we need to update this variable or else change > the logic. > > >> + skl_init_cdclk(dev_priv); > >> + > >> + /* we did have to sanitize */ > >> + return true; > >> +} > >> + > >> /* Adjust CDclk dividers to allow high res or save power if possible */ > >> static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) > >> { > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 0598932..ec10e6a 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev); > >> void bxt_enable_dc9(struct drm_i915_private *dev_priv); > >> void bxt_disable_dc9(struct drm_i915_private *dev_priv); > >> void skl_init_cdclk(struct drm_i915_private *dev_priv); > >> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); > >> void skl_uninit_cdclk(struct drm_i915_private *dev_priv); > >> void intel_dp_get_m_n(struct intel_crtc *crtc, > >> struct intel_crtc_state *pipe_config); > >> -- > >> 2.4.3 > >
On 10/20/2015 04:49 PM, Ville Syrjälä wrote: > On Tue, Oct 20, 2015 at 03:27:24PM +0530, Kumar, Shobhit wrote: >> On 10/19/2015 07:18 PM, Ville Syrjälä wrote: >>> On Fri, Oct 16, 2015 at 06:48:53PM +0530, Shobhit Kumar wrote: >>>> Especially in cases where pre-os does not enable display, cdclk might >>>> not be in sane state. During sanitization initialize cdclk with maximum >>>> value till we get dynamic cdclk support. >>>> >>>> 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 >>>> >>>> v3: Move the validation logic in a separate sanitize function (Ville) >>>> >>>> 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 | 12 ++++++++---- >>>> drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 1 + >>>> 3 files changed, 40 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>> index b25e99a..86d43e6 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -2949,10 +2949,14 @@ 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); >>>> + if (skl_sanitize_cdclk(dev_priv)) >>>> + DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); >>>> + else { >>>> + if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) >>>> + DRM_ERROR("LCPLL1 is disabled\n"); >>> >>> Since skl_sanitize_cdclk() will enable the PLL this shouldn't >>> happen anymore, right? >>> >> >> Yeah right. Will remove. >> >>>> + 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 5f37f84..98333d3 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) >>>> DRM_ERROR("DBuf power enable timeout\n"); >>>> } >>>> >>>> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) >>>> +{ >>>> + uint32_t lcpll1 = I915_READ(LCPLL1_CTL); >>>> + uint32_t cdctl = I915_READ(CDCLK_CTL); >>>> + int freq = dev_priv->skl_boot_cdclk; >>>> + >>>> + /* Is PLL enabled and locked ? */ >>>> + if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) >>>> + goto sanitize; >>>> + >>>> + /* DPLL okay; verify the cdclock >>>> + * >>>> + * 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))) >>>> + /* All well; nothing to sanitize */ >>>> + return false; >>>> +sanitize: >>>> + /* >>>> + * As of now initialize with max cdclk till >>>> + * we get dynamic cdclk support >>>> + * */ >>>> + dev_priv->skl_boot_cdclk = 675000; >>> >>> Should be '= dev_priv->max_cdclk' >>> >>> I think we end up doing the intel_update_cdclk() before this gets >>> called, so max_cdclk should already contain something sensible. The >>> whole init sequence is a bit messy currently, but I think we can put >>> off cleaning it up after the dc6 stuff gets sorted. >>> >> >> Actually at the end of skl_init_cdclock, intel_update_cdclk will be >> called and initialize the max_cdclk correctly. > > It gets called already earlier from intel_modeset_init(). Yeah, I misunderstood your comment earlier. > >> In case there was no >> sanitization needed, the BIOS programmed cdclk is stored in >> skl_boot_cdclk and that is used inside the skl_init_cdclk. Same is >> invoked resume time, so we need to update this variable or else change >> the logic. >> >>>> + skl_init_cdclk(dev_priv); >>>> + >>>> + /* we did have to sanitize */ >>>> + return true; >>>> +} >>>> + >>>> /* Adjust CDclk dividers to allow high res or save power if possible */ >>>> static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) >>>> { >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>> index 0598932..ec10e6a 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev); >>>> void bxt_enable_dc9(struct drm_i915_private *dev_priv); >>>> void bxt_disable_dc9(struct drm_i915_private *dev_priv); >>>> void skl_init_cdclk(struct drm_i915_private *dev_priv); >>>> +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); >>>> void skl_uninit_cdclk(struct drm_i915_private *dev_priv); >>>> void intel_dp_get_m_n(struct intel_crtc *crtc, >>>> struct intel_crtc_state *pipe_config); >>>> -- >>>> 2.4.3 >>> >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index b25e99a..86d43e6 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2949,10 +2949,14 @@ 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); + if (skl_sanitize_cdclk(dev_priv)) + DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); + else { + 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 5f37f84..98333d3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5784,6 +5784,37 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) DRM_ERROR("DBuf power enable timeout\n"); } +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) +{ + uint32_t lcpll1 = I915_READ(LCPLL1_CTL); + uint32_t cdctl = I915_READ(CDCLK_CTL); + int freq = dev_priv->skl_boot_cdclk; + + /* Is PLL enabled and locked ? */ + if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) + goto sanitize; + + /* DPLL okay; verify the cdclock + * + * 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))) + /* All well; nothing to sanitize */ + return false; +sanitize: + /* + * As of now initialize with max cdclk till + * we get dynamic cdclk support + * */ + dev_priv->skl_boot_cdclk = 675000; + skl_init_cdclk(dev_priv); + + /* we did have to sanitize */ + return true; +} + /* Adjust CDclk dividers to allow high res or save power if possible */ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932..ec10e6a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1152,6 +1152,7 @@ void broxton_ddi_phy_uninit(struct drm_device *dev); void bxt_enable_dc9(struct drm_i915_private *dev_priv); void bxt_disable_dc9(struct drm_i915_private *dev_priv); void skl_init_cdclk(struct drm_i915_private *dev_priv); +int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); void skl_uninit_cdclk(struct drm_i915_private *dev_priv); void intel_dp_get_m_n(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config);
Especially in cases where pre-os does not enable display, cdclk might not be in sane state. During sanitization initialize cdclk with maximum value till we get dynamic cdclk support. 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 v3: Move the validation logic in a separate sanitize function (Ville) 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 | 12 ++++++++---- drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 40 insertions(+), 4 deletions(-)