Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

Trunk
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(6 attachments, 13 obsolete attachments)

792 bytes, patch
mchang
: review+
Details | Diff | Splinter Review
2.62 KB, patch
kats
: review+
Details | Diff | Splinter Review
4.47 KB, patch
kats
: review+
Details | Diff | Splinter Review
8.21 KB, patch
mchang
: review+
Details | Diff | Splinter Review
3.21 KB, patch
mchang
: review+
Details | Diff | Splinter Review
17.04 KB, patch
Details | Diff | Splinter Review
+++ 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.
Depends on: 1102453
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 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?
(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.
(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.
Posted patch WIP - Disable Vsync When Idle (obsolete) — Splinter Review
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.
Attachment #8526289 - Attachment is obsolete: true
Posted patch WIP - Disable Vsync When Idle (obsolete) — Splinter Review
Rebased on master. Disables vsync when all CompositorVsyncDispatchers and RefreshTimerVsyncDispatchers are idle. Renables Vsync when we add a COmpositorVsyncDispatcher / RefreshTimer::StartTimer().
Attachment #8548604 - Attachment is obsolete: true
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.
Attachment #8549212 - Attachment is obsolete: true
Attachment #8549299 - Flags: review?(bugmail.mozilla)
When we create the GonkVsyncSource, automatically disable vsync during startup. Vsync will be enabled again once the Compositor is created.
Attachment #8549300 - Flags: review?(bugmail.mozilla)
Check that when we enable vsync in HwcComposer, the device->eventControl actually succeeds.
Attachment #8549301 - Flags: review?(mwu)
Attachment #8549299 - Flags: review?(bugmail.mozilla)
Attachment #8549300 - Flags: review?(bugmail.mozilla)
When the CompositorVsyncObsever observes/unobserves vsync, we subscribe/unsubscribe from the display object.
Attachment #8549306 - Flags: review?(bugmail.mozilla)
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.
Attachment #8549310 - Flags: review?(bugmail.mozilla)
Attachment #8549299 - Flags: review?(bugmail.mozilla)
Attachment #8549300 - Flags: review?(bugmail.mozilla)
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?
s/re-registering/re-enabling vsync/
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :/.
(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 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.
Attachment #8549301 - Flags: review?(mwu) → review+
Carrying r+ from comment 16, updated with feedback.
Attachment #8549299 - Attachment is obsolete: true
Attachment #8549300 - Attachment is obsolete: true
Attachment #8549301 - Attachment is obsolete: true
Attachment #8549306 - Attachment is obsolete: true
Attachment #8549310 - Attachment is obsolete: true
Attachment #8549299 - Flags: review?(bugmail.mozilla)
Attachment #8549300 - Flags: review?(bugmail.mozilla)
Attachment #8549306 - Flags: review?(bugmail.mozilla)
Attachment #8549310 - Flags: review?(bugmail.mozilla)
Attachment #8550760 - Flags: review+
Disable vsync during startup since the Compositor is initialized later than the HwcComposer.
Attachment #8550762 - Flags: review?(bugmail.mozilla)
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.
Attachment #8550763 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8550765 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #8550762 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #8550763 - Flags: review?(bugmail.mozilla) → review-
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
Attachment #8550766 - Flags: review?(bugmail.mozilla) → review+
Woops, sorry about the wrong patch. Here's the right one.
Attachment #8550765 - Attachment is obsolete: true
Attachment #8550922 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8550762 - Attachment is obsolete: true
Attachment #8550939 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8550763 - Attachment is obsolete: true
Attachment #8550949 - Flags: review?(bugmail.mozilla)
Attachment #8550939 - Flags: review?(bugmail.mozilla) → review+
Attachment #8550949 - Flags: review?(bugmail.mozilla) → review+
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 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
Attachment #8550922 - Flags: review?(bugmail.mozilla) → review+
Carrying r+, updated with feedback from comment 30.
Attachment #8550922 - Attachment is obsolete: true
Attachment #8551371 - Flags: review+
Carrying r+. Updated with feedback from comment 25.
Attachment #8550766 - Attachment is obsolete: true
Attachment #8551372 - Flags: review+
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
Attachment #8552578 - Flags: approval-mozilla-b2g37?
Attachment #8552578 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.