1095242 - Disable Vsync When Idle

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #552020 +++

From https://bugzilla.mozilla.org/show_bug.cgi?id=552020#c19, enable / disable hardware vsync when we have no vsync listeners. This is to prevent 60 idle thread wake ups which drain power.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Disable vsync after a preferenced number of vsyncs have passed

Disables vsync after we receive a configurable number of N vsync events. We create a counter that tracks how many vsync events occur without a composite. After N such vsyncs, we disable hardware vsync. The wrinkle is that touch starts have to enable vsync as we need to see if the next touch is a move to create a fling. The touch move with touch resampling enabled won't be dispatched until vsync events.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8526289 [details] [diff] [review]
WIP - Disable vsync after a preferenced number of vsyncs have passed

Review of attachment 8526289 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +254,5 @@
> +  MOZ_ASSERT(!CompositorParent::IsInCompositorThread());
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  MOZ_ASSERT(sVsyncDispatcher);
> +
> +  sVsyncDispatcher->EnableVsync();

What kind of latency would we expect to see with re-enabling?

@@ +297,5 @@
>    if (mNeedsComposite && mCompositorParent) {
>      mNeedsComposite = false;
>      mCompositorParent->CompositeCallback(aVsyncTimestamp);
> +    mUnusedVsyncCount = 0;
> +  } else if (mUnusedVsyncCount++ > gfxPrefs::DisableVsyncThreshold()) {

Is it worth having a special case for "never disable", maybe with a negative preference value?

::: gfx/thebes/gfxPrefs.h
@@ +218,5 @@
>  
>    // Use vsync events generated by hardware
>    DECL_GFX_PREF(Once, "gfx.vsync.hw-vsync.enabled",            HardwareVsyncEnabled, bool, false);
>    DECL_GFX_PREF(Once, "gfx.vsync.compositor",                  VsyncAlignedCompositor, bool, false);
> +  DECL_GFX_PREF(Once, "gfx.vsync.disable-threshold",           DisableVsyncThreshold, int32_t, 10);

There is not a lot of penalty in making this a "Live" preference, and may make testing easier?

Comment 3

[Mason Chang [Inactive] [:mchang]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Comment on attachment 8526289 [details] [diff] [review]
> WIP - Disable vsync after a preferenced number of vsyncs have passed
> 
> Review of attachment 8526289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +254,5 @@
> > +  MOZ_ASSERT(!CompositorParent::IsInCompositorThread());
> > +  MOZ_ASSERT(!NS_IsMainThread());
> > +  MOZ_ASSERT(sVsyncDispatcher);
> > +
> > +  sVsyncDispatcher->EnableVsync();
> 
> What kind of latency would we expect to see with re-enabling?

It depends on the platform. On b2g, we have to enable/disable on the main thread, so there could be some significant latency. On OS X, we can enable on the compositor thread, so there should be at most 1 frame of latency.

> 
> @@ +297,5 @@
> >    if (mNeedsComposite && mCompositorParent) {
> >      mNeedsComposite = false;
> >      mCompositorParent->CompositeCallback(aVsyncTimestamp);
> > +    mUnusedVsyncCount = 0;
> > +  } else if (mUnusedVsyncCount++ > gfxPrefs::DisableVsyncThreshold()) {
> 
> Is it worth having a special case for "never disable", maybe with a negative
> preference value?

Do we have a use case for never disable? 

> ::: gfx/thebes/gfxPrefs.h
> @@ +218,5 @@
> >  
> >    // Use vsync events generated by hardware
> >    DECL_GFX_PREF(Once, "gfx.vsync.hw-vsync.enabled",            HardwareVsyncEnabled, bool, false);
> >    DECL_GFX_PREF(Once, "gfx.vsync.compositor",                  VsyncAlignedCompositor, bool, false);
> > +  DECL_GFX_PREF(Once, "gfx.vsync.disable-threshold",           DisableVsyncThreshold, int32_t, 10);
> 
> There is not a lot of penalty in making this a "Live" preference, and may
> make testing easier?

At least on b2g, there isn't any benefit since we still have to edit prefs which restarts b2g. Might be useful on desktop.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mason Chang [:mchang] from comment #3)
> (In reply to Milan Sreckovic [:milan] from comment #2)
> > ...
> > 
> > Is it worth having a special case for "never disable", maybe with a negative
> > preference value?
> 
> Do we have a use case for never disable? 

Triaging, to rule it out as a cause of problems that show up.

> 
> > ::: gfx/thebes/gfxPrefs.h
> > @@ +218,5 @@
> > >  
> > >    // Use vsync events generated by hardware
> > >    DECL_GFX_PREF(Once, "gfx.vsync.hw-vsync.enabled",            HardwareVsyncEnabled, bool, false);
> > >    DECL_GFX_PREF(Once, "gfx.vsync.compositor",                  VsyncAlignedCompositor, bool, false);
> > > +  DECL_GFX_PREF(Once, "gfx.vsync.disable-threshold",           DisableVsyncThreshold, int32_t, 10);
> > 
> > There is not a lot of penalty in making this a "Live" preference, and may
> > make testing easier?
> 
> At least on b2g, there isn't any benefit since we still have to edit prefs
> which restarts b2g. Might be useful on desktop.

I can see the benefit.  We're picking a magic number, and it'll be easier to test for the right value if we can change it on the fly.  If the preference is live, we can add it to the developer prefs menu, which would let us change it without restarting b2g proc.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Disable Vsync When Idle

Disables vsync when the phone is idle. Which in our case when No Compositor or RefreshDriver VsyncDispatchers are listening to vsync. This version is based on just no compositor vsync dispatchers listening to vsync.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Disable Vsync When Idle

Rebased on master. Disables vsync when all CompositorVsyncDispatchers and RefreshTimerVsyncDispatchers are idle. Renables Vsync when we add a COmpositorVsyncDispatcher / RefreshTimer::StartTimer().

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1 - Display enables/disables vsync

This portion checks whether or not to enable/disable vsync if there are no CompositorVsyncDispatcher and if the RefreshDriverVsyncDispatcher reports that it has no need to listen to vsync.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Disable Vsync During Startup

When we create the GonkVsyncSource, automatically disable vsync during startup. Vsync will be enabled again once the Compositor is created.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Ensure Vsync Is Enabled

Check that when we enable vsync in HwcComposer, the device->eventControl actually succeeds.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Unsubscribe from Vsync Notifications in the CompositorVsyncDispatcher

When the CompositorVsyncObsever observes/unobserves vsync, we subscribe/unsubscribe from the display object.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Unsubscribe from Vsync Notifications in the RefreshTimerVsyncDispatcher

Like Part 4, when the RefreshTimerVsyncDispatcher changes status, we ask the display to check the vsync status. One major difference between the RefreshTimer and Compositor is that the Display asks the RefreshTimerVsyncDispatcher if it still needs vsync whereas the display can figure out that information on it's own with the CompositorDispatcher. This is because CompositorDispatchers actively subscribe to the display whereas the RefreshTimerVsyncDispatcher lives as long as the system process.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I'm not really a fan of this architecture, it seems very backwards to me to have to call UpdateStatus on the display. That should be encapsulated better. Even worse is that the UpdateStatus then turns around calls back into the refresh driver thingy with the NeedsVsync() call. Is there a reason you did it this way instead of just auto-disabling vsync when listeners get unregistered and re-registering when listeners get registered?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

s/re-registering/re-enabling vsync/

Comment 14

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> I'm not really a fan of this architecture, it seems very backwards to me to
> have to call UpdateStatus on the display. That should be encapsulated
> better. Even worse is that the UpdateStatus then turns around calls back
> into the refresh driver thingy with the NeedsVsync() call. Is there a reason
> you did it this way instead of just auto-disabling vsync when listeners get
> unregistered and re-registering when listeners get registered?

Yeah, I agree, I'm not a super fan of this either. With only the compositor, this implementation auto enables/disables vsync when the CompositorVsyncDispatcher registers/unregisters and doesn't update the display.

The problem occurs because of the refresh driver vsync dispatcher, which doesn't actually subscribe / unsubscribe from the Display. It lives the lifetime of the refresh timer vsync dispatcher. There is nothing to subscribe/unsubscribe from at least from the Display. Refresh timers subscribe/unsubscribe to the RefreshVsyncDispatcher, not the Display itself, so the Display can't self encapsulate auto enabling/disabling vsync with regards to the RefreshVsyncDispatcher. This architecture essentially calls the Display to notify it when vsync observers subscribe/unsubscribe from the RefreshVsyncDispatcher instead of directly subscribing/unsubscribing to the Display.

I also don't really like that UpdateStatus calls into the RefreshVsyncDispatcher. But this handles the case where no refresh drivers are listening to vsync and the last CompositorVsyncDispatcher stops listening to vsync. I could update the patch to have some state in the Display to check if the RefreshVsyncDispatcher needs to listen to vsync instead of querying the RefreshVsyncDispatcher. This would eliminate the need for the call back into the RefreshVsyncDispatcher.

Another option would be to have both the CompositorVsyncDispatcher and RefreshTimerVsyncDispatcher keep track of their own internal need for vsync and have the Display periodically query all subscribers if they need vsync, but I'm not a big fan of the polling option either :/.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #14)
> I could update the patch to have some state in the
> Display to check if the RefreshVsyncDispatcher needs to listen to vsync
> instead of querying the RefreshVsyncDispatcher. This would eliminate the
> need for the call back into the RefreshVsyncDispatcher.
> 

I would prefer this approach. What I'm envisioning is a counter-based code pattern. So inside the Display objects you increment a counter every time EnableVsync() is called. That counter is decremented in DisableVsync() and you only actually disable hardware vsync when the counter reaches zero. Adding a CompositorVsyncDispatcher to a display should call EnableVsync() and removing one would call DisableVsync(). The RefreshTimerVsyncDispatcher would also get a reference to the Display and would likewise call EnableVsync() and DisableVsync() on it as necessary.

The only thing I'm not sure about is threading - if this model doesn't work with the current threading model (where Enable/DisableVsync are always called on the main thread) then feel free to do something equivalent that's easier. Does that sound reasonable?

Comment 16

[Michael Wu [:mwu]]

Comment on attachment 8549301 [details] [diff] [review]
Part 3: Ensure Vsync Is Enabled

Review of attachment 8549301 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.cpp
@@ +194,5 @@
>      if (!device) {
>        return false;
>      }
>  
> +    return (device->eventControl(device, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, aEnable) == 0) && aEnable;

nit: ! instead of == 0. Also, parens not required.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1 - Ensure Vsync Is Enabled

Carrying r+ from comment 16, updated with feedback.

Comment 18

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2 - Disable Vsync during Startup

Disable vsync during startup since the Compositor is initialized later than the HwcComposer.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3 - Disable Vsync only if vsync is enabled in software vsync

Ensure that during the SoftwareVsync's destructor, we only disable Vsync if vsync is enabled. Some asserts will fail if DisableVsync is called when vsync is not enabled, which I would prefer to keep.

Comment 20

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4 - Unobserve vsync in the CompositorVsyncDispatcher

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> (In reply to Mason Chang [:mchang] from comment #14)
> > I could update the patch to have some state in the
> > Display to check if the RefreshVsyncDispatcher needs to listen to vsync
> > instead of querying the RefreshVsyncDispatcher. This would eliminate the
> > need for the call back into the RefreshVsyncDispatcher.
> > 
> 
> I would prefer this approach. What I'm envisioning is a counter-based code
> pattern. So inside the Display objects you increment a counter every time
> EnableVsync() is called. That counter is decremented in DisableVsync() and
> you only actually disable hardware vsync when the counter reaches zero.
> Adding a CompositorVsyncDispatcher to a display should call EnableVsync()
> and removing one would call DisableVsync(). 

Updated to ensure that the display doesn't call back into either vsync dispatcher to update vsync status. The CompositorVsyncDispatcher only observes/unobserves vsync as needed. The RefreshTimerVsyncDispatcher queries its own internal state to see if it needs vsync and notifies the Display. The display has its own internal state keeping track of whether or not the RefreshTimer needs vsync. The Display updates vsync status as compositors observe/unobserve or as the RefreshTimerVsyncDispatcher updates the display.

I'm not a fan of counter based approaches since it's difficult to ensure the counter is always accurate. If the counter goes negative in some weird state, it's hard to find those bugs. I'd rather try to directly query internal state than calculated state. This approach has the RefreshTimerVsyncDispatcher, CompositorVSyncDispatcher, and Display have their own internal state if they need vsync or not. The Display aggregates this to enable/disable vsync. If you really feel strongly about a counter based approach, I can implement that.

Comment 21

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5 - Unobserve vsync in the RefreshTimerVsyncDispatcher

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8550765 [details] [diff] [review]
Part 4 - Unobserve vsync in the CompositorVsyncDispatcher

Review of attachment 8550765 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is the wrong patch.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8550762 [details] [diff] [review]
Part 2 - Disable Vsync during Startup

Review of attachment 8550762 [details] [diff] [review]:
-----------------------------------------------------------------

Why are you doing this only for gonk? There is an implicit contract that determines if vsync will be enabled or not after calling CreateHardwareVsyncSource and all platforms should behave consistently. If you want it to be disabled on startup that's fine but do it in the osx and software VsyncSources also.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8550763 [details] [diff] [review]
Part 3 - Disable Vsync only if vsync is enabled in software vsync

Review of attachment 8550763 [details] [diff] [review]:
-----------------------------------------------------------------

Firstly this patch seems unrelated to this bug but maybe it's relation will be more clear with the correct part 4 patch. Secondly both the gonk and osx versions allow calling DisableVsync while vsync is already disabled and it's just a no-op; I would prefer you did the same thing here. You can just return early if mVsyncEnabled is already false, and move the assert for IsRunning to after the early return.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8550766 [details] [diff] [review]
Part 5 - Unobserve vsync in the RefreshTimerVsyncDispatcher

LGTM

>+void
>+RefreshTimerVsyncDispatcher::UpdateVsyncStatus()
>+{
>+  if (!NS_IsMainThread())
>+  {

nit: move open brace up

Comment 26

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4 - Unobserve vsync in the CompositorVsyncDispatcher

Woops, sorry about the wrong patch. Here's the right one.

Comment 27

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2 - Disable Vsync during Startup. v2

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Comment on attachment 8550762 [details] [diff] [review]
> Part 2 - Disable Vsync during Startup
> 
> Review of attachment 8550762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are you doing this only for gonk? There is an implicit contract that
> determines if vsync will be enabled or not after calling
> CreateHardwareVsyncSource and all platforms should behave consistently. If
> you want it to be disabled on startup that's fine but do it in the osx and
> software VsyncSources also.

Updated with review from comment 23. I originally did it only for gonk since that's where we will enable silk initially. I wanted to pin down b2g and make sure it worked well, then once we had the model working well, update the other platforms since other platforms are still off by default. This way, I wouldn't have to keep updating the other platforms with every bug until we knew what worked. This patch unifies the other platforms though.

Comment 28

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3 - Make Enable/Disable vsync noops if vsync is already enabled/disabled

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> Comment on attachment 8550763 [details] [diff] [review]
> Part 3 - Disable Vsync only if vsync is enabled in software vsync
> 
> Review of attachment 8550763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Firstly this patch seems unrelated to this bug but maybe it's relation will
> be more clear with the correct part 4 patch. Secondly both the gonk and osx
> versions allow calling DisableVsync while vsync is already disabled and it's
> just a no-op; I would prefer you did the same thing here. You can just
> return early if mVsyncEnabled is already false, and move the assert for
> IsRunning to after the early return.

Originally, it only affected SoftwareVsync because it was failing mochitests because assertions in DisableVsync would fail during gPlatform destruction. You're right though, enabling/disabling should just be a no-op. Actually this prevents a bug on OS X because calling EnableVsync() twice would actually create another thread! 

This patch makes enable/disable vsync no-ops across platforms if vsync is already enabled/disabled.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8550949 [details] [diff] [review]
Part 3 - Make Enable/Disable vsync noops if vsync is already enabled/disabled

Review of attachment 8550949 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/SoftwareVsyncSource.cpp
@@ +21,5 @@
>    mGlobalDisplay = nullptr;
>  }
>  
>  SoftwareDisplay::SoftwareDisplay()
> +  : mVsyncEnabled(false)

Technically this initialization should go into part 2.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8550922 [details] [diff] [review]
Part 4 - Unobserve vsync in the CompositorVsyncDispatcher

Review of attachment 8550922 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.cpp
@@ +89,5 @@
>    MOZ_ASSERT(aCompositorVsyncDispatcher);
> +  { // Scope lock
> +    MutexAutoLock lock(mDispatcherLock);
> +    if (mCompositorVsyncDispatchers.Contains(aCompositorVsyncDispatcher)) {
> +      mCompositorVsyncDispatchers.RemoveElement(aCompositorVsyncDispatcher);

Checking Contains here is kind of redundant but whatever.

@@ +118,5 @@
> +    MutexAutoLock lock(mDispatcherLock);
> +    enableVsync = !mCompositorVsyncDispatchers.IsEmpty() || mRefreshTimerNeedsVsync;
> +  }
> +
> +  if (enableVsync && !IsVsyncEnabled()) {

No need to check !IsVsyncEnabled() here since EnableVsync is a no-op if it's already enabled.

@@ +120,5 @@
> +  }
> +
> +  if (enableVsync && !IsVsyncEnabled()) {
> +    EnableVsync();
> +  } else if (!enableVsync && IsVsyncEnabled()) {

Ditto

Comment 31

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Unobseve Vsync in CompositorVsyncDispatcher

Carrying r+, updated with feedback from comment 30.

Comment 32

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Unobserve Vsync in the RefreshTimerVsyncDispatcher

Carrying r+. Updated with feedback from comment 25.

Comment 33

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6632cabb36
https://hg.mozilla.org/integration/mozilla-inbound/rev/1435616b69c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/25deed6f1d60
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbc31d1d04f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7baad81da308

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae00c9f7575

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f6632cabb36
https://hg.mozilla.org/mozilla-central/rev/1435616b69c1
https://hg.mozilla.org/mozilla-central/rev/25deed6f1d60
https://hg.mozilla.org/mozilla-central/rev/cfbc31d1d04f
https://hg.mozilla.org/mozilla-central/rev/7baad81da308

Comment 35

[Mason Chang [Inactive] [:mchang]]

Attachment: Disable vsync when idle, rebased for b2g37_2_2

Rollup patch rebased for b2g37_2_2, from 232321:53421e00294d.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Required for project silk, bug 987532
User impact if declined: When silk is enabled, we will never shut off hardware vsync, which will cause the hardware vsync thread to always execute every 16.6 ms. This will cause needless battery drain.
Testing completed: Testing on master, manual testing to ensure vsync is enabled/disabled including when turning power on/off. Should also be tested in bug 1118530.
Risk to taking this patch (and alternatives if risky): Medium - This will actively enable/disable vsync, which controls composites. If vsync does not correctly turn on when requested, the device may not respond to user input.
String or UUID changes made by this patch: None

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/71c240f166cf