[Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:
> 2014-05-26 11:26 GMT-03:00  <ville.syrjala at linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Now that the vblank races are plugged, we can opt out of using
> > the vblank disable timer and just let vblank interrupts get
> > disabled immediately when the last reference is dropped.
> >
> > Gen2 is the exception since it has no hardware frame counter.
> 
> Hi
> 
> Remember last week's FBC vblank optimization patch that had an
> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
> After I fixed the bug in the patch I realized that it was the
> unbalanced vblank_get() call that moved PC state residency up.
> 
> I did some experiments, and on my specific BDW machine, after running
> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
> residency goes from 60-70% to 85-90% when I revert this patch. I'm
> running just an idle Cinnamon with an open terminal.
> 
> So, since the commit message lacks more details, what are the
> downsides of reverting this patch? What are the advantages of opting
> out of the vblank timer? I see my desktop does tons and tons of vblank
> get/put calls per second, so the disable timer makes a lot of sense.

"Idle" desktop :(

Really the immediate disable should save power. Where are these tons of
vblank get/puts coming from actually? I would assume you'd get a handful
per frame at most, and that when you're actually doing something. On an
idle system I would expect nothing at all happens during most frames.

Not sure, but I guess it's possible the extra register accesses in the
get/puts actually cause the display to exit low power states all the time,
or something.

There's also this note in Bspec (for HSW at least):
 "Workaround : Do not enable and unmask this interrupt if the associated
  pipe is disabled.  Do not leave this interrupt enabled and unmasked
  after the associated pipe is disabled."
which we took to mean that having the interrupt masked but enabled is
fine. But maybe we'd actually have to frob IER too to avoid wasting
power somehow?

> I also wish there was some easy way to check how this patch (or its
> revert) affect a bunch of different workloads...
> 
> (Also CCing Chris for insightful comments on performance)

IIRC Chris had a patch to not disable the interrupt immediately when
the refcount drops to 0, but instead delay the disable until the next
interrupt actually happens. But I guess it didn't go in? Probably I
should have reviewed it but didn't. It sounds like a decent idea to
me in any case for the active use case.

> 
> Thanks,
> Paulo
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 28bae6e..4b2e7af 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
> >                 dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
> >         }
> >
> > +       /*
> > +        * Opt out of the vblank disable timer on everything except gen2.
> > +        * Gen2 doesn't have a hardware frame counter and so depends on
> > +        * vblank interrupts to produce sane vblank seuquence numbers.
> > +        */
> > +       if (!IS_GEN2(dev))
> > +               dev->vblank_disable_immediate = true;
> > +
> >         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >                 dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> >                 dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> > --
> > 1.8.5.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC