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

[Paulo Zanoni]

2015-11-19 18:06 GMT-02:00 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> 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 :(

My first realization of this little problem was when I was
implementing runtime PM :)


>
> Really the immediate disable should save power. Where are these tons of
> vblank get/puts coming from actually?

I'll take a finer look tomorrow, but I assume it's probably some
application redrawing. I see it does calm down sometimes, but that's
not enough to get better PC7 residency.


> 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.

I tried replacing the register macros with the _FW version and that didn't help.


>
> There's also this note in Bspec (for HSW at least):

I think this not is present on most (all?) gens.


>  "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.

I'm aware of this, but I think the problem is that the resources
drained by the constant enable+disable+enable+disable outweigh the
resources saved by turning off vblanks. Not sure if there's an extra
reason why BSpec asks us to immediately disable vblanks though...

So, to summarize, the main (only?) reason is the BSpec comment?


> But maybe we'd actually have to frob IER too to avoid wasting
> power somehow?

With the interrupt masked on IMR, I don't think IER matters.

>
>> 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



-- 
Paulo Zanoni