On Fri, Sep 19, 2014 at 12:44:08PM -0700, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
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@xxxxxxxxx> wrote:
>
> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
