Re: [PATCH 3/4] drm/i915: WaFbcDisableDpfcrClockGating only with fbc

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Oct 25, 2013 at 03:14:50PM -0200, Paulo Zanoni wrote:
> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > We were turning this on for ILK regardless of whether or not we use FBC.
> > We can save the slightest amount of power if we don't disable it when
> > not using FBC.
> 
> Finally someone did what I requested months ago:
> http://lists.freedesktop.org/archives/intel-gfx/2013-June/028906.html
> :)
> 
> 
> >
> > The workaround should be bit 8 for ILK. Notice it is 1 bit difference
> > from SNB. This is actually DPFCR unit as we've defined it.
> 
> Ok, so we have bits 8 and 9. Judging by the register names, I would
> say bit 9 is WaFbcDisableDpfcClockGating (without 'r') and bit 8 is
> the ironlake-only WaFbcDisableDpfcrClockGating. Is that right?

Actually from what I remember in the database (not looking currently),
the workaround names are the same, but maybe my eyes missed the 'R'. I
differentiated it with the 'R' so that anyone who was reviewing it
didn't go insane.

> 
> 
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 686699c..bbcf100 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -238,6 +238,11 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> >                            SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> >                 I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> >                 sandybridge_blit_fbc_update(dev);
> > +       } else {
> > +               /* WaFbcDisableDpfcClockGating:ilk */
> 
> If you agree with me on the question above, then the WA name here is
> missing an 'r' char.

I guess I have to go back to the database now to check. I don't have the
bookmark anywhere on my machines here.

> 
> 
> > +               I915_WRITE(ILK_DSPCLK_GATE_D,
> > +                          I915_READ(ILK_DSPCLK_GATE_D) |
> > +                          ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
> >         }
> >
> >         DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> > @@ -254,6 +259,12 @@ static void ironlake_disable_fbc(struct drm_device *dev)
> >                 dpfc_ctl &= ~DPFC_CTL_EN;
> >                 I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
> >
> > +               if (IS_GEN5(dev))
> > +                       /* WaFbcDisableDpfcClockGating:ilk */
> 
> Same here.
> 
> 
> > +                       I915_WRITE(ILK_DSPCLK_GATE_D,
> > +                                  I915_READ(ILK_DSPCLK_GATE_D) &
> > +                                  ~ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
> > +
> >                 DRM_DEBUG_KMS("disabled FBC\n");
> >         }
> >  }
> > @@ -4932,9 +4943,9 @@ static void ironlake_init_clock_gating(struct drm_device *dev)
> >
> >         /*
> >          * Required for FBC
> > -        * WaFbcDisableDpfcClockGating:ilk
> > +        * WaFbcDisableDpfcClockGating:snb
> 
> The ":snb" part is certainly wrong since this function doesn't run on SNB.
> 
> The actual code (excluding comments) looks correct.
> 

Oops, thanks. The diff should just remove the line completely.

> 
> >          */
> > -       dspclk_gate |= ILK_DPFCRUNIT_CLOCK_GATE_DISABLE |
> > +       dspclk_gate |=
> >                    ILK_DPFCUNIT_CLOCK_GATE_DISABLE |
> >                    ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
> >
> > --
> > 1.8.4.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ben Widawsky, Intel Open Source Technology Center