| Message ID | bd4ac9a69db1b2be4159b5ed97a16f2e845cd24d.1716906179.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: mem/fsb/rawclk freq cleanups | expand |
On Tue, May 28, 2024 at 05:24:51PM +0300, Jani Nikula wrote: > Clarify and unify the logging on not finding PNV CxSR latency config. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/i9xx_wm.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c > index 8657ec0abd2d..8b8a0f305c3a 100644 > --- a/drivers/gpu/drm/i915/display/i9xx_wm.c > +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c > @@ -75,7 +75,7 @@ static const struct cxsr_latency *pnv_get_cxsr_latency(struct drm_i915_private * > int i; > > if (i915->fsb_freq == 0 || i915->mem_freq == 0) > - return NULL; > + goto err; Is there even a need for this check? 0/0 will fail to match anything in the table and will just drop through to the debug message anyway, right? Matt > > for (i = 0; i < ARRAY_SIZE(cxsr_latency_table); i++) { > const struct cxsr_latency *latency = &cxsr_latency_table[i]; > @@ -88,7 +88,10 @@ static const struct cxsr_latency *pnv_get_cxsr_latency(struct drm_i915_private * > return latency; > } > > - drm_dbg_kms(&i915->drm, "Unknown FSB/MEM found, disable CxSR\n"); > +err: > + drm_dbg_kms(&i915->drm, > + "Could not find CxSR latency for DDR%s, FSB %u MHz, MEM %u MHz\n", > + i915->is_ddr3 ? "3" : "2", i915->fsb_freq, i915->mem_freq); > > return NULL; > } > @@ -637,8 +640,7 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) > > latency = pnv_get_cxsr_latency(dev_priv); > if (!latency) { > - drm_dbg_kms(&dev_priv->drm, > - "Unknown FSB/MEM found, disable CxSR\n"); > + drm_dbg_kms(&dev_priv->drm, "Unknown FSB/MEM, disabling CxSR\n"); > intel_set_memory_cxsr(dev_priv, false); > return; > } > @@ -4023,12 +4025,7 @@ void i9xx_wm_init(struct drm_i915_private *dev_priv) > dev_priv->display.funcs.wm = &g4x_wm_funcs; > } else if (IS_PINEVIEW(dev_priv)) { > if (!pnv_get_cxsr_latency(dev_priv)) { > - drm_info(&dev_priv->drm, > - "failed to find known CxSR latency " > - "(found ddr%s fsb freq %d, mem freq %d), " > - "disabling CxSR\n", > - (dev_priv->is_ddr3 == 1) ? "3" : "2", > - dev_priv->fsb_freq, dev_priv->mem_freq); > + drm_info(&dev_priv->drm, "Unknown FSB/MEM, disabling CxSR\n"); > /* Disable CxSR and never update its watermark again */ > intel_set_memory_cxsr(dev_priv, false); > dev_priv->display.funcs.wm = &nop_funcs; > -- > 2.39.2 >
On Wed, 29 May 2024, Matt Roper <matthew.d.roper@intel.com> wrote: > On Tue, May 28, 2024 at 05:24:51PM +0300, Jani Nikula wrote: >> Clarify and unify the logging on not finding PNV CxSR latency config. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/display/i9xx_wm.c | 17 +++++++---------- >> 1 file changed, 7 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c >> index 8657ec0abd2d..8b8a0f305c3a 100644 >> --- a/drivers/gpu/drm/i915/display/i9xx_wm.c >> +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c >> @@ -75,7 +75,7 @@ static const struct cxsr_latency *pnv_get_cxsr_latency(struct drm_i915_private * >> int i; >> >> if (i915->fsb_freq == 0 || i915->mem_freq == 0) >> - return NULL; >> + goto err; > > Is there even a need for this check? 0/0 will fail to match anything in > the table and will just drop through to the debug message anyway, right? True, could be dropped. I just thought it was more explicit this way, but maybe fewer lines is better. BR, Jani. > > > Matt > >> >> for (i = 0; i < ARRAY_SIZE(cxsr_latency_table); i++) { >> const struct cxsr_latency *latency = &cxsr_latency_table[i]; >> @@ -88,7 +88,10 @@ static const struct cxsr_latency *pnv_get_cxsr_latency(struct drm_i915_private * >> return latency; >> } >> >> - drm_dbg_kms(&i915->drm, "Unknown FSB/MEM found, disable CxSR\n"); >> +err: >> + drm_dbg_kms(&i915->drm, >> + "Could not find CxSR latency for DDR%s, FSB %u MHz, MEM %u MHz\n", >> + i915->is_ddr3 ? "3" : "2", i915->fsb_freq, i915->mem_freq); >> >> return NULL; >> } >> @@ -637,8 +640,7 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) >> >> latency = pnv_get_cxsr_latency(dev_priv); >> if (!latency) { >> - drm_dbg_kms(&dev_priv->drm, >> - "Unknown FSB/MEM found, disable CxSR\n"); >> + drm_dbg_kms(&dev_priv->drm, "Unknown FSB/MEM, disabling CxSR\n"); >> intel_set_memory_cxsr(dev_priv, false); >> return; >> } >> @@ -4023,12 +4025,7 @@ void i9xx_wm_init(struct drm_i915_private *dev_priv) >> dev_priv->display.funcs.wm = &g4x_wm_funcs; >> } else if (IS_PINEVIEW(dev_priv)) { >> if (!pnv_get_cxsr_latency(dev_priv)) { >> - drm_info(&dev_priv->drm, >> - "failed to find known CxSR latency " >> - "(found ddr%s fsb freq %d, mem freq %d), " >> - "disabling CxSR\n", >> - (dev_priv->is_ddr3 == 1) ? "3" : "2", >> - dev_priv->fsb_freq, dev_priv->mem_freq); >> + drm_info(&dev_priv->drm, "Unknown FSB/MEM, disabling CxSR\n"); >> /* Disable CxSR and never update its watermark again */ >> intel_set_memory_cxsr(dev_priv, false); >> dev_priv->display.funcs.wm = &nop_funcs; >> -- >> 2.39.2 >>
diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c index 8657ec0abd2d..8b8a0f305c3a 100644 --- a/drivers/gpu/drm/i915/display/i9xx_wm.c +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c @@ -75,7 +75,7 @@ static const struct cxsr_latency *pnv_get_cxsr_latency(struct drm_i915_private * int i; if (i915->fsb_freq == 0 || i915->mem_freq == 0) - return NULL; + goto err; for (i = 0; i < ARRAY_SIZE(cxsr_latency_table); i++) { const struct cxsr_latency *latency = &cxsr_latency_table[i]; @@ -88,7 +88,10 @@ static const struct cxsr_latency *pnv_get_cxsr_latency(struct drm_i915_private * return latency; } - drm_dbg_kms(&i915->drm, "Unknown FSB/MEM found, disable CxSR\n"); +err: + drm_dbg_kms(&i915->drm, + "Could not find CxSR latency for DDR%s, FSB %u MHz, MEM %u MHz\n", + i915->is_ddr3 ? "3" : "2", i915->fsb_freq, i915->mem_freq); return NULL; } @@ -637,8 +640,7 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) latency = pnv_get_cxsr_latency(dev_priv); if (!latency) { - drm_dbg_kms(&dev_priv->drm, - "Unknown FSB/MEM found, disable CxSR\n"); + drm_dbg_kms(&dev_priv->drm, "Unknown FSB/MEM, disabling CxSR\n"); intel_set_memory_cxsr(dev_priv, false); return; } @@ -4023,12 +4025,7 @@ void i9xx_wm_init(struct drm_i915_private *dev_priv) dev_priv->display.funcs.wm = &g4x_wm_funcs; } else if (IS_PINEVIEW(dev_priv)) { if (!pnv_get_cxsr_latency(dev_priv)) { - drm_info(&dev_priv->drm, - "failed to find known CxSR latency " - "(found ddr%s fsb freq %d, mem freq %d), " - "disabling CxSR\n", - (dev_priv->is_ddr3 == 1) ? "3" : "2", - dev_priv->fsb_freq, dev_priv->mem_freq); + drm_info(&dev_priv->drm, "Unknown FSB/MEM, disabling CxSR\n"); /* Disable CxSR and never update its watermark again */ intel_set_memory_cxsr(dev_priv, false); dev_priv->display.funcs.wm = &nop_funcs;
Clarify and unify the logging on not finding PNV CxSR latency config. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/i9xx_wm.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)