| Message ID | 1411153495-2658-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> But I think it would be good now to change fbc_status interface on debugfs to show the current bit state as well. On Fri, Sep 19, 2014 at 12:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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 <rodrigo.vivi@intel.com> > 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 <rodrigo.vivi@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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 > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Fri, Sep 19, 2014 at 12:44:08PM -0700, Rodrigo Vivi wrote: > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Both patches merged to dinq. > But I think it would be good now to change fbc_status interface on debugfs > to show the current bit state as well. Seconded. Also some locking for fbc would be awesome ;-) Cheers, Daniel > > > On Fri, Sep 19, 2014 at 12:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > 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 <rodrigo.vivi@intel.com> > > 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 <rodrigo.vivi@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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 > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 */