<div dir="ltr"><div>Reviewed-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>></div><div> </div>But I think it would be good now to change fbc_status interface on debugfs to show the current bit state as well.<div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Sep 19, 2014 at 12:04 PM, Paulo Zanoni <<a href="mailto:przanoni@gmail.com" target="_blank">przanoni@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>> Currently, calling intel_fbc_enabled() will trigger a register read. And we call it a lot of times, even when FBC is disabled, so saving a few cycles would be a good thing. Another reason for this patch is because we currently call intel_fbc_enabled() while the HW is runtime suspended, so the read makes no sense and triggers a WARN. This happens even if FBC is disabled by default. Of course one could argue that we just shouldn't be calling intel_fbc_enabled() while the driver is runtime suspended, and I agree that's a good argument, but I still think that the reason explained in the first paragraph already justifies the patch. This problem can easily be reproduced with many subtests of igt/pm_rpm, and it is a regression introduced by: commit c5ad011d7d256ecbe173324029e992817194d2b0 Author: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>> Date: Mon Aug 4 03:51:38 2014 -0700 drm/i915: FBC flush nuke for BDW Testcase: igt/pm_rpm/cursor (and others) Cc: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>> Signed-off-by: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++++++++++----------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5fce16c..999bd57 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -662,6 +662,10 @@ struct i915_fbc { bool false_color; + /* Tracks whether the HW is actually enabled, not whether the feature is + * possible. */ + bool enabled; + struct intel_fbc_work { struct delayed_work work; struct drm_crtc *crtc; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2ca9fdb..6b41620 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -71,6 +71,8 @@ static void i8xx_disable_fbc(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; u32 fbc_ctl; + dev_priv->fbc.enabled = false; + /* Disable compression */ fbc_ctl = I915_READ(FBC_CONTROL); if ((fbc_ctl & FBC_CTL_EN) == 0) @@ -99,6 +101,8 @@ static void i8xx_enable_fbc(struct drm_crtc *crtc) int i; u32 fbc_ctl; + dev_priv->fbc.enabled = true; + cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE; if (fb->pitches[0] < cfb_pitch) cfb_pitch = fb->pitches[0]; @@ -153,6 +157,8 @@ static void g4x_enable_fbc(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); u32 dpfc_ctl; + dev_priv->fbc.enabled = true; + dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane) | DPFC_SR_EN; if (drm_format_plane_cpp(fb->pixel_format, 0) == 2) dpfc_ctl |= DPFC_CTL_LIMIT_2X; @@ -173,6 +179,8 @@ static void g4x_disable_fbc(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; u32 dpfc_ctl; + dev_priv->fbc.enabled = false; + /* Disable compression */ dpfc_ctl = I915_READ(DPFC_CONTROL); if (dpfc_ctl & DPFC_CTL_EN) { @@ -224,6 +232,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); u32 dpfc_ctl; + dev_priv->fbc.enabled = true; + dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane); if (drm_format_plane_cpp(fb->pixel_format, 0) == 2) dev_priv->fbc.threshold++; @@ -264,6 +274,8 @@ static void ironlake_disable_fbc(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; u32 dpfc_ctl; + dev_priv->fbc.enabled = false; + /* Disable compression */ dpfc_ctl = I915_READ(ILK_DPFC_CONTROL); if (dpfc_ctl & DPFC_CTL_EN) { @@ -290,6 +302,8 @@ static void gen7_enable_fbc(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); u32 dpfc_ctl; + dev_priv->fbc.enabled = true; + dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane); if (drm_format_plane_cpp(fb->pixel_format, 0) == 2) dev_priv->fbc.threshold++; @@ -339,16 +353,7 @@ bool intel_fbc_enabled(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - /* If it wasn't never enabled by kernel parameter or platform default - * we can avoid reading registers so many times in vain - */ - if (!i915.enable_fbc) - return false; - - if (!dev_priv->display.fbc_enabled) - return false; - - return dev_priv->display.fbc_enabled(dev); + return dev_priv->fbc.enabled; } void gen8_fbc_sw_flush(struct drm_device *dev, u32 value) @@ -7360,8 +7365,10 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv) static void intel_init_fbc(struct drm_i915_private *dev_priv) { - if (!HAS_FBC(dev_priv)) + if (!HAS_FBC(dev_priv)) { + dev_priv->fbc.enabled = false; return; + } if (INTEL_INFO(dev_priv)->gen >= 7) { dev_priv->display.fbc_enabled = ironlake_fbc_enabled; @@ -7383,6 +7390,8 @@ static void intel_init_fbc(struct drm_i915_private *dev_priv) /* This value was pulled out of someone's hat */ I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT); } + + dev_priv->fbc.enabled = dev_priv->display.fbc_enabled(dev_priv->dev); } /* Set up chip specific power management-related functions */ -- 2.1.0 _______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div> <div> </div>-- <div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div> </div>