Disable rendering when suspending on chrome os

Disable rendering when suspending on chrome os

New chrome os devices are going to make more frequent use of the dark
resume state - a hybrid power state where several hardware devices are
left off to save power.  One of these is the display.  Unfortunately
this creates issues with chrome, which believes that the display is on
and attempts to render things to the screen.  These calls fail in the
kernel and trigger a logical memory leak in the error handling code in
the GPU process.

In order to make this interaction more robust, have chrome stop sending
rendering requests at suspend time and restart them at resume time.
There are a few things to watch out for here.  If the user has requested
that the screen be locked at suspend time then rendering cannot be
stopped until the screen locker is visible.  Otherwise the contents of
the user's screen will be visible at resume time, which is a security
issue.  Additionally, the suspend must be delayed until any pending
requests have been handled by the GPU process to prevent a race where
the system suspends before all requests have been processed and they end
up being processed when the system later enters dark resume, triggering
the memory leak mentioned in the previous paragraph.

BUG=chrome-os-partner:35699
Committed: https://crrev.com/40e4cd8a507016a99740011899ca4b9779cd9eef
Cr-Commit-Position: refs/heads/master@{#316946}

[Chirantan Ekbote, 2015-02-11 00:51:07]

chirantan@chromium.org changed reviewers:
+ derat@chromium.org, piman@chromium.org

[Chirantan Ekbote, 2015-02-11 00:51:07]

This is my first stab at implementing chrome-side blocking of rendering at
suspend time.  There is still an issue where the user pod on the lock screen
takes about a second to appear at resume time and I'm currently tracking down
why that is happening.  Please take a look to see if the general idea is okay.

[piman, 2015-02-11 02:48:22]

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
File ash/system/chromeos/power/power_event_observer.cc (right):

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer.cc:108:
compositor->ScheduleDraw();
I don't think the ScheduleDraw should be needed.

https://codereview.chromium.org/910393002/diff/20001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/910393002/diff/20001/ui/compositor/compositor...
ui/compositor/compositor.cc:231: void Compositor::DisableSwap() {
I'd prefer calling it FinishAllRendering(). It doesn't in itself disable swap.

[Daniel Erat, 2015-02-11 14:11:43]

lgtm with some comments for the suspend part of this

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
File ash/system/chromeos/power/power_event_observer.h (right):

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer.h:49: void
StopRenderingRequests();
nit: switch the order of these two methods to mirror the calling pattern

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
File ash/system/chromeos/power/power_event_observer_unittest.cc (right):

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer_unittest.cc:36: void
ExpectCompositorVisibility(bool expected) {
to get more-meaningful failure messages, how about changing this to
GetNumVisibleCompositors() and then comparing it against the total number of
compositors or 0 (depending on whether compositing should be active or frozen)
in the tests below?

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer_unittest.cc:37: for (const auto&
window : Shell::GetAllRootWindows()) {
nit: omit curly brackets

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer_unittest.cc:106: // canceled
before the lock screen is ready.
nit: s/canceled/canceled or resumed/

[Chirantan Ekbote, 2015-02-11 23:40:41]

chirantan@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Chirantan Ekbote, 2015-02-11 23:40:42]

+nkostylev

With this change, rendering is stopped when the lock screen is ready but that
does not mean that the user pod animation has finished.  As a result there is a
delay of about a second on the first resume before the user pod becomes visible.

Ideally I want to stop rendering after the user pod animation has completed. 
Nikita, any ideas on some place in the code that I could hook into for this?

[Chirantan Ekbote, 2015-02-12 03:27:23]

I might have answered my own question.  There is EVENT_LOCK_ANIMATION_FINISHED
in ash::LockStateObserver that looks like exactly what I need.

[Nikita (slow), 2015-02-12 11:23:20]

nkostylev@chromium.org changed reviewers:
+ dpolukhin@chromium.org

[Nikita (slow), 2015-02-12 11:28:06]

+Dmitry, Denis

On 2015/02/12 03:27:23, Chirantan Ekbote wrote:
> I might have answered my own question.  There is EVENT_LOCK_ANIMATION_FINISHED
> in ash::LockStateObserver that looks like exactly what I need.

Thanks for checking.

This event only covers ash part of the lock animation.
There's additional WebUI animation that is scheduled right after it.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

This animation is for WebUI "control bar" opacity over 2s.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

There're no event after this particular animation is complete but it can be
easily added.

[Nikita (slow), 2015-02-12 11:28:27]

nkostylev@chromium.org changed reviewers:
+ antrim@google.com

[Nikita (slow), 2015-02-12 11:28:38]

+Denis for realz

[Chirantan Ekbote, 2015-02-12 20:36:36]

On 2015/02/12 11:28:06, Nikita wrote:
> +Dmitry, Denis
> 
> On 2015/02/12 03:27:23, Chirantan Ekbote wrote:
> > I might have answered my own question.  There is
EVENT_LOCK_ANIMATION_FINISHED
> > in ash::LockStateObserver that looks like exactly what I need.
> 
> Thanks for checking.
> 
> This event only covers ash part of the lock animation.
> There's additional WebUI animation that is scheduled right after it.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> This animation is for WebUI "control bar" opacity over 2s.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> There're no event after this particular animation is complete but it can be
> easily added.

Hmm so I'm not sure what the right thing is to do here.  This change will affect
all chrome os devices with displays and not having the control bar visible on
the first resume looks a little weird.  On the other hand, delaying the suspend
for an extra 2 seconds just for the control bar animation also doesn't feel
right.  It would be nice to get input from a ui PM.  Who would be the right
person for this?

[Chirantan Ekbote, 2015-02-13 00:24:37]

On 2015/02/12 20:36:36, Chirantan Ekbote wrote:
> On 2015/02/12 11:28:06, Nikita wrote:
> > +Dmitry, Denis
> > 
> > On 2015/02/12 03:27:23, Chirantan Ekbote wrote:
> > > I might have answered my own question.  There is
> EVENT_LOCK_ANIMATION_FINISHED
> > > in ash::LockStateObserver that looks like exactly what I need.
> > 
> > Thanks for checking.
> > 
> > This event only covers ash part of the lock animation.
> > There's additional WebUI animation that is scheduled right after it.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > 
> > This animation is for WebUI "control bar" opacity over 2s.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > 
> > There're no event after this particular animation is complete but it can be
> > easily added.
> 
> Hmm so I'm not sure what the right thing is to do here.  This change will
affect
> all chrome os devices with displays and not having the control bar visible on
> the first resume looks a little weird.  On the other hand, delaying the
suspend
> for an extra 2 seconds just for the control bar animation also doesn't feel
> right.  It would be nice to get input from a ui PM.  Who would be the right
> person for this?

Actually would it be possible to use the fast animation
(login-header-bar-animate-fast) when the system is suspending.  If that isn't
too hard to do then I don't think it would be a problem to block the suspend
until the animation completes.

[Daniel Erat, 2015-02-13 02:44:12]

On 2015/02/13 00:24:37, Chirantan Ekbote wrote:
> On 2015/02/12 20:36:36, Chirantan Ekbote wrote:
> > On 2015/02/12 11:28:06, Nikita wrote:
> > > +Dmitry, Denis
> > > 
> > > On 2015/02/12 03:27:23, Chirantan Ekbote wrote:
> > > > I might have answered my own question.  There is
> > EVENT_LOCK_ANIMATION_FINISHED
> > > > in ash::LockStateObserver that looks like exactly what I need.
> > > 
> > > Thanks for checking.
> > > 
> > > This event only covers ash part of the lock animation.
> > > There's additional WebUI animation that is scheduled right after it.
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > > 
> > > This animation is for WebUI "control bar" opacity over 2s.
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > > 
> > > There're no event after this particular animation is complete but it can
be
> > > easily added.
> > 
> > Hmm so I'm not sure what the right thing is to do here.  This change will
> affect
> > all chrome os devices with displays and not having the control bar visible
on
> > the first resume looks a little weird.  On the other hand, delaying the
> suspend
> > for an extra 2 seconds just for the control bar animation also doesn't feel
> > right.  It would be nice to get input from a ui PM.  Who would be the right
> > person for this?
> 
> Actually would it be possible to use the fast animation
> (login-header-bar-animate-fast) when the system is suspending.  If that isn't
> too hard to do then I don't think it would be a problem to block the suspend
> until the animation completes.

can we just skip the animation altogether for non-user-initiated locks, or ones
where the display is off?

[Nikita (slow), 2015-02-13 12:13:54]

On 2015/02/13 02:44:12, Daniel Erat wrote:
> On 2015/02/13 00:24:37, Chirantan Ekbote wrote:
> > On 2015/02/12 20:36:36, Chirantan Ekbote wrote:
> > > On 2015/02/12 11:28:06, Nikita wrote:
> > > > +Dmitry, Denis
> > > > 
> > > > On 2015/02/12 03:27:23, Chirantan Ekbote wrote:
> > > > > I might have answered my own question.  There is
> > > EVENT_LOCK_ANIMATION_FINISHED
> > > > > in ash::LockStateObserver that looks like exactly what I need.
> > > > 
> > > > Thanks for checking.
> > > > 
> > > > This event only covers ash part of the lock animation.
> > > > There's additional WebUI animation that is scheduled right after it.
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > > > 
> > > > This animation is for WebUI "control bar" opacity over 2s.
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > > > 
> > > > There're no event after this particular animation is complete but it can
> be
> > > > easily added.
> > > 
> > > Hmm so I'm not sure what the right thing is to do here.  This change will
> > affect
> > > all chrome os devices with displays and not having the control bar visible
> on
> > > the first resume looks a little weird.  On the other hand, delaying the
> > suspend
> > > for an extra 2 seconds just for the control bar animation also doesn't
feel
> > > right.  It would be nice to get input from a ui PM.  Who would be the
right
> > > person for this?
> > 
> > Actually would it be possible to use the fast animation
> > (login-header-bar-animate-fast) when the system is suspending.  If that
isn't
> > too hard to do then I don't think it would be a problem to block the suspend
> > until the animation completes.
> 
> can we just skip the animation altogether for non-user-initiated locks, or
ones
> where the display is off?

We can try to do that or use fast animation.

[Chirantan Ekbote, 2015-02-18 02:35:19]

New patchsets have been uploaded after l-g-t-m from derat@chromium.org

[Chirantan Ekbote, 2015-02-18 02:47:14]

PTAL.  With this version, the suspend is delayed until the lock screen
animations have completed and the header-bar uses the fast animation (~200 ms)
rather than the slow one (~2 sec).  The only side effect of this is that just
locking the screen, for example via Ctrl-Shift-L, will still use the fast
animation for the header bar.  It doesn't look that bad to me though.

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
File ash/system/chromeos/power/power_event_observer.cc (right):

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer.cc:108:
compositor->ScheduleDraw();
On 2015/02/11 02:48:22, piman (Very slow to review) wrote:
> I don't think the ScheduleDraw should be needed.

Done.

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
File ash/system/chromeos/power/power_event_observer_unittest.cc (right):

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer_unittest.cc:36: void
ExpectCompositorVisibility(bool expected) {
On 2015/02/11 14:11:43, Daniel Erat wrote:
> to get more-meaningful failure messages, how about changing this to
> GetNumVisibleCompositors() and then comparing it against the total number of
> compositors or 0 (depending on whether compositing should be active or frozen)
> in the tests below?

Done.

https://codereview.chromium.org/910393002/diff/20001/ash/system/chromeos/powe...
ash/system/chromeos/power/power_event_observer_unittest.cc:106: // canceled
before the lock screen is ready.
On 2015/02/11 14:11:43, Daniel Erat wrote:
> nit: s/canceled/canceled or resumed/

Done.

https://codereview.chromium.org/910393002/diff/20001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/910393002/diff/20001/ui/compositor/compositor...
ui/compositor/compositor.cc:231: void Compositor::DisableSwap() {
On 2015/02/11 02:48:22, piman (Very slow to review) wrote:
> I'd prefer calling it FinishAllRendering(). It doesn't in itself disable swap.

Done.

[Daniel Erat, 2015-02-18 03:53:24]

lgtm

[Nikita (slow), 2015-02-18 13:21:38]

lgtm

[piman, 2015-02-18 16:26:42]

lgtm

https://codereview.chromium.org/910393002/diff/60001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/910393002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.h:207: // Get the visibility of the underlying
compositor.
nit: Gets

[Chirantan Ekbote, 2015-02-18 19:25:33]

New patchsets have been uploaded after l-g-t-m from
derat@chromium.org,nkostylev@chromium.org,piman@chromium.org

[Chirantan Ekbote, 2015-02-18 19:26:50]

Thanks for the fast reviews!

https://codereview.chromium.org/910393002/diff/60001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/910393002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.h:207: // Get the visibility of the underlying
compositor.
On 2015/02/18 16:26:42, piman (Very slow to review) wrote:
> nit: Gets

Done.

[Chirantan Ekbote, 2015-02-18 19:26:54]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2015-02-18 19:27:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393002/80001

[commit-bot: I haz the power, 2015-02-18 19:36:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-18 19:36:28]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

[Chirantan Ekbote, 2015-02-18 23:28:27]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:29:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393002/100001

[commit-bot: I haz the power, 2015-02-19 01:08:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-19 01:09:47]

Patchset 6 (id:??) landed as
https://crrev.com/40e4cd8a507016a99740011899ca4b9779cd9eef
Cr-Commit-Position: refs/heads/master@{#316946}