| Message ID | 1389186748-1954-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 08, 2014 at 11:12:28AM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > LPT does have PCH refclk, but it's different form the IBX/CPT/PPT one > and doesn't use the same structs. It is wrong to have a message saying > that "LPT does not has PCH refclk" (sic). While at it, signal that we > only want this function on IBX/CPT/PPT by renaming it and adding a > WARN. > > On HSW we also print "0 shared PLLs initialized", but we *do* have > shared PLLs on HSW (LCPLL, WRPLL, SPLL) and we *do* initialize them. > We just don't use "struct intel_shared_dpll". So remove the debug > message. > > In the future we may want to rename all that "intel shared pll" code > to "ibx shared pll", but I'll leave this to another patch. I remember Daniel saying that his plan was to make the DDI clocks use the shared PLL struct. I guess that'd take care of the debug message correctness. So I defer this one to higher authorities, you have my r-b tag as the patch makes sense, not sure if Daniel will take it though :) > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 034952c..df3cf69 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1211,15 +1211,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, > } > } > > -static void assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) > +static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) > { > u32 val; > bool enabled; > > - if (HAS_PCH_LPT(dev_priv->dev)) { > - DRM_DEBUG_DRIVER("LPT does not has PCH refclk, skipping check\n"); > - return; > - } > + WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev))); > > val = I915_READ(PCH_DREF_CONTROL); > enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK | > @@ -10067,7 +10064,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll) > { > /* PCH refclock must be enabled first */ > - assert_pch_refclk_enabled(dev_priv); > + ibx_assert_pch_refclk_enabled(dev_priv); > > I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll); > > @@ -10135,8 +10132,6 @@ static void intel_shared_dpll_init(struct drm_device *dev) > dev_priv->num_shared_dpll = 0; > > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > - DRM_DEBUG_KMS("%i shared PLLs initialized\n", > - dev_priv->num_shared_dpll); > } > > static void intel_crtc_init(struct drm_device *dev, int pipe) > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2014/1/8 Damien Lespiau <damien.lespiau@intel.com>: > On Wed, Jan 08, 2014 at 11:12:28AM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> LPT does have PCH refclk, but it's different form the IBX/CPT/PPT one >> and doesn't use the same structs. It is wrong to have a message saying >> that "LPT does not has PCH refclk" (sic). While at it, signal that we >> only want this function on IBX/CPT/PPT by renaming it and adding a >> WARN. >> >> On HSW we also print "0 shared PLLs initialized", but we *do* have >> shared PLLs on HSW (LCPLL, WRPLL, SPLL) and we *do* initialize them. >> We just don't use "struct intel_shared_dpll". So remove the debug >> message. >> >> In the future we may want to rename all that "intel shared pll" code >> to "ibx shared pll", but I'll leave this to another patch. > > I remember Daniel saying that his plan was to make the DDI clocks use > the shared PLL struct. I guess that'd take care of the debug message > correctness. Yes, but there's currently no plan actually do that conversion, so we'll stay with wrong messages for an undefined amount of time. The first message I removed is not even ever print, and the second message always prints "2" for IBX/CPT/PPT and 0 for LPT+: you can easily check that by looking at the code, so the messages are not even useful. Thanks for the reviews! Paulo > > So I defer this one to higher authorities, you have my r-b tag as the > patch makes sense, not sure if Daniel will take it though :) > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 11 +++-------- >> 1 file changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 034952c..df3cf69 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -1211,15 +1211,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, >> } >> } >> >> -static void assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) >> +static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) >> { >> u32 val; >> bool enabled; >> >> - if (HAS_PCH_LPT(dev_priv->dev)) { >> - DRM_DEBUG_DRIVER("LPT does not has PCH refclk, skipping check\n"); >> - return; >> - } >> + WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev))); >> >> val = I915_READ(PCH_DREF_CONTROL); >> enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK | >> @@ -10067,7 +10064,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv, >> struct intel_shared_dpll *pll) >> { >> /* PCH refclock must be enabled first */ >> - assert_pch_refclk_enabled(dev_priv); >> + ibx_assert_pch_refclk_enabled(dev_priv); >> >> I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll); >> >> @@ -10135,8 +10132,6 @@ static void intel_shared_dpll_init(struct drm_device *dev) >> dev_priv->num_shared_dpll = 0; >> >> BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); >> - DRM_DEBUG_KMS("%i shared PLLs initialized\n", >> - dev_priv->num_shared_dpll); >> } >> >> static void intel_crtc_init(struct drm_device *dev, int pipe) >> -- >> 1.8.4.2 >> >> _______________________________________________ >> 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_display.c b/drivers/gpu/drm/i915/intel_display.c index 034952c..df3cf69 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1211,15 +1211,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, } } -static void assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) +static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) { u32 val; bool enabled; - if (HAS_PCH_LPT(dev_priv->dev)) { - DRM_DEBUG_DRIVER("LPT does not has PCH refclk, skipping check\n"); - return; - } + WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev))); val = I915_READ(PCH_DREF_CONTROL); enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK | @@ -10067,7 +10064,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv, struct intel_shared_dpll *pll) { /* PCH refclock must be enabled first */ - assert_pch_refclk_enabled(dev_priv); + ibx_assert_pch_refclk_enabled(dev_priv); I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll); @@ -10135,8 +10132,6 @@ static void intel_shared_dpll_init(struct drm_device *dev) dev_priv->num_shared_dpll = 0; BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); - DRM_DEBUG_KMS("%i shared PLLs initialized\n", - dev_priv->num_shared_dpll); } static void intel_crtc_init(struct drm_device *dev, int pipe)