| Message ID | 1444278535-16483-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote: > Reuse what is programmed by pre-os, but in case there is no pre-os > initialization, init the cdclk with the max value by default untill > dynamic cdclk support comes. > > 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 > > 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 | 18 ++++++++++++----- > drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++--------- > 2 files changed, 42 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 2d3cc82..3ec5618 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev) > int cdclk_freq; > > 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); > + > + /* Invalid CDCLK from BIOS ? */ > + if (cdclk_freq < 0) { > + /* program to maximum cdclk till we have dynamic cdclk support */ > + dev_priv->skl_boot_cdclk = 675000; > + skl_init_cdclk(dev_priv); This would still try to reprogram CDCLK if BIOS enabled an output with an incorrect CDCLK decimal part. Isn't this the exact scenario you're seeing? As said before reprogramming CDCLK in that case would require disabling the outputs first. We would also need to call skl_init_cdclk if the BIOS hasn't enabled the PLL for some reason. But I guess that could be a separate change. > + } else { > + 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); > + } > } 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 bbeb6d3..f734410 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) > uint32_t lcpll1 = I915_READ(LCPLL1_CTL); > uint32_t cdctl = I915_READ(CDCLK_CTL); > uint32_t linkrate; > + int freq; > > if (!(lcpll1 & LCPLL_PLL_ENABLE)) > return 24000; /* 24MHz is the cd freq with NSSC ref */ > > - if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) > - return 540000; > + if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) { > + freq = 540000; > + goto verify; > + } > > linkrate = (I915_READ(DPLL_CTRL1) & > DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1; > @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) > /* vco 8640 */ > switch (cdctl & CDCLK_FREQ_SEL_MASK) { > case CDCLK_FREQ_450_432: > - return 432000; > + freq = 432000; > + break; > case CDCLK_FREQ_337_308: > - return 308570; > + freq = 308570; > + break; > case CDCLK_FREQ_675_617: > - return 617140; > + freq = 617140; > + break; > default: > WARN(1, "Unknown cd freq selection\n"); > + return -EINVAL; > } > } else { > /* vco 8100 */ > switch (cdctl & CDCLK_FREQ_SEL_MASK) { > case CDCLK_FREQ_450_432: > - return 450000; > + freq = 450000; > + break; > case CDCLK_FREQ_337_308: > - return 337500; > + freq = 337500; > + break; > case CDCLK_FREQ_675_617: > - return 675000; > + freq = 675000; > + break; > default: > WARN(1, "Unknown cd freq selection\n"); > + return -EINVAL; > } > } > > - /* error case, do as if DPLL0 isn't enabled */ > - return 24000; > +verify: > + /* > + * 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))) > + return freq; > + else > + return -EINVAL; > } > > static int broxton_get_display_clock_speed(struct drm_device *dev)
On 10/08/2015 04:59 PM, Imre Deak wrote: > On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote: >> Reuse what is programmed by pre-os, but in case there is no pre-os >> initialization, init the cdclk with the max value by default untill >> dynamic cdclk support comes. >> >> 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 >> >> 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 | 18 ++++++++++++----- >> drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++--------- >> 2 files changed, 42 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 2d3cc82..3ec5618 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev) >> int cdclk_freq; >> >> 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); >> + >> + /* Invalid CDCLK from BIOS ? */ >> + if (cdclk_freq < 0) { >> + /* program to maximum cdclk till we have dynamic cdclk support */ >> + dev_priv->skl_boot_cdclk = 675000; >> + skl_init_cdclk(dev_priv); > > This would still try to reprogram CDCLK if BIOS enabled an output with > an incorrect CDCLK decimal part. Isn't this the exact scenario you're > seeing? As said before reprogramming CDCLK in that case would require > disabling the outputs first. I went with the hypothesis that if VBIOS/GOP was loaded it would have to correct the cdclock, with wrong decimal value display cannot be enabled. For example AUX will fail on SKL. So for correct display output enabled cdclock has to be correctly programmed. If it is wrong display was not enabled at all. The scenario which I am seeing is VBIOS/GOP is not loaded at all, and the pre-os is not leaving cdclock in correct state. > > We would also need to call skl_init_cdclk if the BIOS hasn't enabled the > PLL for some reason. But I guess that could be a separate change. > >> + } else { >> + 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); >> + } >> } 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 bbeb6d3..f734410 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) >> uint32_t lcpll1 = I915_READ(LCPLL1_CTL); >> uint32_t cdctl = I915_READ(CDCLK_CTL); >> uint32_t linkrate; >> + int freq; >> >> if (!(lcpll1 & LCPLL_PLL_ENABLE)) >> return 24000; /* 24MHz is the cd freq with NSSC ref */ >> >> - if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) >> - return 540000; >> + if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) { >> + freq = 540000; >> + goto verify; >> + } >> >> linkrate = (I915_READ(DPLL_CTRL1) & >> DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1; >> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) >> /* vco 8640 */ >> switch (cdctl & CDCLK_FREQ_SEL_MASK) { >> case CDCLK_FREQ_450_432: >> - return 432000; >> + freq = 432000; >> + break; >> case CDCLK_FREQ_337_308: >> - return 308570; >> + freq = 308570; >> + break; >> case CDCLK_FREQ_675_617: >> - return 617140; >> + freq = 617140; >> + break; >> default: >> WARN(1, "Unknown cd freq selection\n"); >> + return -EINVAL; >> } >> } else { >> /* vco 8100 */ >> switch (cdctl & CDCLK_FREQ_SEL_MASK) { >> case CDCLK_FREQ_450_432: >> - return 450000; >> + freq = 450000; >> + break; >> case CDCLK_FREQ_337_308: >> - return 337500; >> + freq = 337500; >> + break; >> case CDCLK_FREQ_675_617: >> - return 675000; >> + freq = 675000; >> + break; >> default: >> WARN(1, "Unknown cd freq selection\n"); >> + return -EINVAL; >> } >> } >> >> - /* error case, do as if DPLL0 isn't enabled */ >> - return 24000; >> +verify: >> + /* >> + * 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))) >> + return freq; >> + else >> + return -EINVAL; >> } >> >> static int broxton_get_display_clock_speed(struct drm_device *dev) > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote: > On 10/08/2015 04:59 PM, Imre Deak wrote: > > On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote: > >> Reuse what is programmed by pre-os, but in case there is no pre-os > >> initialization, init the cdclk with the max value by default untill > >> dynamic cdclk support comes. > >> > >> 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 > >> > >> 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 | 18 ++++++++++++----- > >> drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++--------- > >> 2 files changed, 42 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index 2d3cc82..3ec5618 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev) > >> int cdclk_freq; > >> > >> 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); > >> + > >> + /* Invalid CDCLK from BIOS ? */ > >> + if (cdclk_freq < 0) { > >> + /* program to maximum cdclk till we have dynamic cdclk support */ > >> + dev_priv->skl_boot_cdclk = 675000; > >> + skl_init_cdclk(dev_priv); > > > > This would still try to reprogram CDCLK if BIOS enabled an output with > > an incorrect CDCLK decimal part. Isn't this the exact scenario you're > > seeing? As said before reprogramming CDCLK in that case would require > > disabling the outputs first. > > I went with the hypothesis that if VBIOS/GOP was loaded it would have to > correct the cdclock, with wrong decimal value display cannot be enabled. > For example AUX will fail on SKL. So for correct display output enabled > cdclock has to be correctly programmed. If it is wrong display was not > enabled at all. > > The scenario which I am seeing is VBIOS/GOP is not loaded at all, and > the pre-os is not leaving cdclock in correct state. But it still enabled DPLL0? Why does it do that if it doesn't set up cdclk? > > > > > We would also need to call skl_init_cdclk if the BIOS hasn't enabled the > > PLL for some reason. But I guess that could be a separate change. > > > >> + } else { > >> + 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); > >> + } > >> } 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 bbeb6d3..f734410 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) > >> uint32_t lcpll1 = I915_READ(LCPLL1_CTL); > >> uint32_t cdctl = I915_READ(CDCLK_CTL); > >> uint32_t linkrate; > >> + int freq; > >> > >> if (!(lcpll1 & LCPLL_PLL_ENABLE)) > >> return 24000; /* 24MHz is the cd freq with NSSC ref */ > >> > >> - if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) > >> - return 540000; > >> + if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) { > >> + freq = 540000; > >> + goto verify; > >> + } > >> > >> linkrate = (I915_READ(DPLL_CTRL1) & > >> DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1; > >> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) > >> /* vco 8640 */ > >> switch (cdctl & CDCLK_FREQ_SEL_MASK) { > >> case CDCLK_FREQ_450_432: > >> - return 432000; > >> + freq = 432000; > >> + break; > >> case CDCLK_FREQ_337_308: > >> - return 308570; > >> + freq = 308570; > >> + break; > >> case CDCLK_FREQ_675_617: > >> - return 617140; > >> + freq = 617140; > >> + break; > >> default: > >> WARN(1, "Unknown cd freq selection\n"); > >> + return -EINVAL; > >> } > >> } else { > >> /* vco 8100 */ > >> switch (cdctl & CDCLK_FREQ_SEL_MASK) { > >> case CDCLK_FREQ_450_432: > >> - return 450000; > >> + freq = 450000; > >> + break; > >> case CDCLK_FREQ_337_308: > >> - return 337500; > >> + freq = 337500; > >> + break; > >> case CDCLK_FREQ_675_617: > >> - return 675000; > >> + freq = 675000; > >> + break; > >> default: > >> WARN(1, "Unknown cd freq selection\n"); > >> + return -EINVAL; > >> } > >> } > >> > >> - /* error case, do as if DPLL0 isn't enabled */ > >> - return 24000; > >> +verify: > >> + /* > >> + * 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))) > >> + return freq; > >> + else > >> + return -EINVAL; > >> } > >> > >> static int broxton_get_display_clock_speed(struct drm_device *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
On 10/08/2015 05:54 PM, Ville Syrjälä wrote: > On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote: >> On 10/08/2015 04:59 PM, Imre Deak wrote: >>> On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote: >>>> Reuse what is programmed by pre-os, but in case there is no pre-os >>>> initialization, init the cdclk with the max value by default untill >>>> dynamic cdclk support comes. >>>> >>>> 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 >>>> >>>> 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 | 18 ++++++++++++----- >>>> drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++--------- >>>> 2 files changed, 42 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>> index 2d3cc82..3ec5618 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev) >>>> int cdclk_freq; >>>> >>>> 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); >>>> + >>>> + /* Invalid CDCLK from BIOS ? */ >>>> + if (cdclk_freq < 0) { >>>> + /* program to maximum cdclk till we have dynamic cdclk support */ >>>> + dev_priv->skl_boot_cdclk = 675000; >>>> + skl_init_cdclk(dev_priv); >>> >>> This would still try to reprogram CDCLK if BIOS enabled an output with >>> an incorrect CDCLK decimal part. Isn't this the exact scenario you're >>> seeing? As said before reprogramming CDCLK in that case would require >>> disabling the outputs first. >> >> I went with the hypothesis that if VBIOS/GOP was loaded it would have to >> correct the cdclock, with wrong decimal value display cannot be enabled. >> For example AUX will fail on SKL. So for correct display output enabled >> cdclock has to be correctly programmed. If it is wrong display was not >> enabled at all. >> >> The scenario which I am seeing is VBIOS/GOP is not loaded at all, and >> the pre-os is not leaving cdclock in correct state. > > But it still enabled DPLL0? Why does it do that if it doesn't set up > cdclk? So I had discussion with some folks in BIOS team and came to know that on some boards even if higher cdclocks are supported, we might want to limit maximum cdclock. This is done by BIOS programming the cdclock to allowed maximum and VBIOS/GOP uses that as the max value rather than the real possible max. So the BIOS is supposed to program cdclock correctly and hence DPLL0 as well. My only assumption is in this case it has not correctly programmed due to some bug. I know if I work with them and ensure that BIOS programs correctly, the problem will be solved, but do you think it is worth while to remove the tight dependency altogether ? > >> >>> >>> We would also need to call skl_init_cdclk if the BIOS hasn't enabled the >>> PLL for some reason. But I guess that could be a separate change. >>> >>>> + } else { >>>> + 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); >>>> + } >>>> } 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 bbeb6d3..f734410 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) >>>> uint32_t lcpll1 = I915_READ(LCPLL1_CTL); >>>> uint32_t cdctl = I915_READ(CDCLK_CTL); >>>> uint32_t linkrate; >>>> + int freq; >>>> >>>> if (!(lcpll1 & LCPLL_PLL_ENABLE)) >>>> return 24000; /* 24MHz is the cd freq with NSSC ref */ >>>> >>>> - if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) >>>> - return 540000; >>>> + if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) { >>>> + freq = 540000; >>>> + goto verify; >>>> + } >>>> >>>> linkrate = (I915_READ(DPLL_CTRL1) & >>>> DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1; >>>> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) >>>> /* vco 8640 */ >>>> switch (cdctl & CDCLK_FREQ_SEL_MASK) { >>>> case CDCLK_FREQ_450_432: >>>> - return 432000; >>>> + freq = 432000; >>>> + break; >>>> case CDCLK_FREQ_337_308: >>>> - return 308570; >>>> + freq = 308570; >>>> + break; >>>> case CDCLK_FREQ_675_617: >>>> - return 617140; >>>> + freq = 617140; >>>> + break; >>>> default: >>>> WARN(1, "Unknown cd freq selection\n"); >>>> + return -EINVAL; >>>> } >>>> } else { >>>> /* vco 8100 */ >>>> switch (cdctl & CDCLK_FREQ_SEL_MASK) { >>>> case CDCLK_FREQ_450_432: >>>> - return 450000; >>>> + freq = 450000; >>>> + break; >>>> case CDCLK_FREQ_337_308: >>>> - return 337500; >>>> + freq = 337500; >>>> + break; >>>> case CDCLK_FREQ_675_617: >>>> - return 675000; >>>> + freq = 675000; >>>> + break; >>>> default: >>>> WARN(1, "Unknown cd freq selection\n"); >>>> + return -EINVAL; >>>> } >>>> } >>>> >>>> - /* error case, do as if DPLL0 isn't enabled */ >>>> - return 24000; >>>> +verify: >>>> + /* >>>> + * 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))) >>>> + return freq; >>>> + else >>>> + return -EINVAL; >>>> } >>>> >>>> static int broxton_get_display_clock_speed(struct drm_device *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 >
On 10/08/2015 05:54 PM, Ville Syrjälä wrote: > On Thu, Oct 08, 2015 at 05:43:41PM +0530, Kumar, Shobhit wrote: >> On 10/08/2015 04:59 PM, Imre Deak wrote: >>> On to, 2015-10-08 at 09:58 +0530, Shobhit Kumar wrote: >>>> Reuse what is programmed by pre-os, but in case there is no pre-os >>>> initialization, init the cdclk with the max value by default untill >>>> dynamic cdclk support comes. >>>> >>>> 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 >>>> >>>> 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 | 18 ++++++++++++----- >>>> drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++--------- >>>> 2 files changed, 42 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>> index 2d3cc82..3ec5618 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev) >>>> int cdclk_freq; >>>> >>>> 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); >>>> + >>>> + /* Invalid CDCLK from BIOS ? */ >>>> + if (cdclk_freq < 0) { >>>> + /* program to maximum cdclk till we have dynamic cdclk support */ >>>> + dev_priv->skl_boot_cdclk = 675000; >>>> + skl_init_cdclk(dev_priv); >>> >>> This would still try to reprogram CDCLK if BIOS enabled an output with >>> an incorrect CDCLK decimal part. Isn't this the exact scenario you're >>> seeing? As said before reprogramming CDCLK in that case would require >>> disabling the outputs first. >> >> I went with the hypothesis that if VBIOS/GOP was loaded it would have to >> correct the cdclock, with wrong decimal value display cannot be enabled. >> For example AUX will fail on SKL. So for correct display output enabled >> cdclock has to be correctly programmed. If it is wrong display was not >> enabled at all. >> >> The scenario which I am seeing is VBIOS/GOP is not loaded at all, and >> the pre-os is not leaving cdclock in correct state. > > But it still enabled DPLL0? Why does it do that if it doesn't set up > cdclk? > Okay digging more into FSP code, found a bug where even when display is not initialized in pre-os, still there was an initialization path for enabling display audio for HDMI which was enabling DPLL0. That also should have been not invoked when display itself is not being loaded. Not sure what going on in there !! Will work with FSP team and get this rectified. After that sanitize patch(following up this mail) can check the clock related programming and sanitize if needed as discussed with you Ville on IRC. Regards Shobhit >> >>> >>> We would also need to call skl_init_cdclk if the BIOS hasn't enabled the >>> PLL for some reason. But I guess that could be a separate change. >>> >>>> + } else { >>>> + 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); >>>> + } >>>> } 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 bbeb6d3..f734410 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) >>>> uint32_t lcpll1 = I915_READ(LCPLL1_CTL); >>>> uint32_t cdctl = I915_READ(CDCLK_CTL); >>>> uint32_t linkrate; >>>> + int freq; >>>> >>>> if (!(lcpll1 & LCPLL_PLL_ENABLE)) >>>> return 24000; /* 24MHz is the cd freq with NSSC ref */ >>>> >>>> - if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) >>>> - return 540000; >>>> + if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) { >>>> + freq = 540000; >>>> + goto verify; >>>> + } >>>> >>>> linkrate = (I915_READ(DPLL_CTRL1) & >>>> DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1; >>>> @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) >>>> /* vco 8640 */ >>>> switch (cdctl & CDCLK_FREQ_SEL_MASK) { >>>> case CDCLK_FREQ_450_432: >>>> - return 432000; >>>> + freq = 432000; >>>> + break; >>>> case CDCLK_FREQ_337_308: >>>> - return 308570; >>>> + freq = 308570; >>>> + break; >>>> case CDCLK_FREQ_675_617: >>>> - return 617140; >>>> + freq = 617140; >>>> + break; >>>> default: >>>> WARN(1, "Unknown cd freq selection\n"); >>>> + return -EINVAL; >>>> } >>>> } else { >>>> /* vco 8100 */ >>>> switch (cdctl & CDCLK_FREQ_SEL_MASK) { >>>> case CDCLK_FREQ_450_432: >>>> - return 450000; >>>> + freq = 450000; >>>> + break; >>>> case CDCLK_FREQ_337_308: >>>> - return 337500; >>>> + freq = 337500; >>>> + break; >>>> case CDCLK_FREQ_675_617: >>>> - return 675000; >>>> + freq = 675000; >>>> + break; >>>> default: >>>> WARN(1, "Unknown cd freq selection\n"); >>>> + return -EINVAL; >>>> } >>>> } >>>> >>>> - /* error case, do as if DPLL0 isn't enabled */ >>>> - return 24000; >>>> +verify: >>>> + /* >>>> + * 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))) >>>> + return freq; >>>> + else >>>> + return -EINVAL; >>>> } >>>> >>>> static int broxton_get_display_clock_speed(struct drm_device *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 >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 2d3cc82..3ec5618 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2946,11 +2946,19 @@ void intel_ddi_pll_init(struct drm_device *dev) int cdclk_freq; 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); + + /* Invalid CDCLK from BIOS ? */ + if (cdclk_freq < 0) { + /* program to maximum cdclk till we have dynamic cdclk support */ + dev_priv->skl_boot_cdclk = 675000; + skl_init_cdclk(dev_priv); + } else { + 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); + } } 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 bbeb6d3..f734410 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6634,12 +6634,15 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) uint32_t lcpll1 = I915_READ(LCPLL1_CTL); uint32_t cdctl = I915_READ(CDCLK_CTL); uint32_t linkrate; + int freq; if (!(lcpll1 & LCPLL_PLL_ENABLE)) return 24000; /* 24MHz is the cd freq with NSSC ref */ - if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) - return 540000; + if ((cdctl & CDCLK_FREQ_SEL_MASK) == CDCLK_FREQ_540) { + freq = 540000; + goto verify; + } linkrate = (I915_READ(DPLL_CTRL1) & DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0)) >> 1; @@ -6649,30 +6652,46 @@ static int skylake_get_display_clock_speed(struct drm_device *dev) /* vco 8640 */ switch (cdctl & CDCLK_FREQ_SEL_MASK) { case CDCLK_FREQ_450_432: - return 432000; + freq = 432000; + break; case CDCLK_FREQ_337_308: - return 308570; + freq = 308570; + break; case CDCLK_FREQ_675_617: - return 617140; + freq = 617140; + break; default: WARN(1, "Unknown cd freq selection\n"); + return -EINVAL; } } else { /* vco 8100 */ switch (cdctl & CDCLK_FREQ_SEL_MASK) { case CDCLK_FREQ_450_432: - return 450000; + freq = 450000; + break; case CDCLK_FREQ_337_308: - return 337500; + freq = 337500; + break; case CDCLK_FREQ_675_617: - return 675000; + freq = 675000; + break; default: WARN(1, "Unknown cd freq selection\n"); + return -EINVAL; } } - /* error case, do as if DPLL0 isn't enabled */ - return 24000; +verify: + /* + * 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))) + return freq; + else + return -EINVAL; } static int broxton_get_display_clock_speed(struct drm_device *dev)
Reuse what is programmed by pre-os, but in case there is no pre-os initialization, init the cdclk with the max value by default untill dynamic cdclk support comes. 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 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 | 18 ++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 15 deletions(-)