Reland of: [Ozone-GBM] Handle GPU crashes

Reland of: [Ozone-GBM] Handle GPU crashes

Original CL: https://codereview.chromium.org/522463005

TBR=kenrb@chromium.org
BUG=none
NOTRY=true

Committed: https://crrev.com/01410fcf142cfd1fe038a4d089f7fc4e3cb3b312
Cr-Commit-Position: refs/heads/master@{#293956}

[dnicoara, 2014-09-08 21:24:44]

dnicoara@chromium.org changed reviewers:
+ alexst@chromium.org, kenrb@chromium.org, spang@chromium.org

[dnicoara, 2014-09-08 21:24:44]

alexst@, spang@ for ui/ozone
kenrb@ for IPC (no changes from previous CL version)

Patch 1 is the previous version and patch 2 is the updated.

Issue was related to --ash-host-window-bounds which caused window creation
before display configuration had a chance to happen. Since all the controllers
were disabled the window didn't get matched with a controller.

[spang, 2014-09-08 22:49:17]

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
File ui/ozone/platform/dri/chromeos/display_message_handler.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
ui/ozone/platform/dri/chromeos/display_message_handler.cc:64: void
DisplayMessageHandler::OnRefreshNativeDisplays(
There's now a big overlap between RefreshNativeDisplays and Configure/Disable.
In fact it looks like Configure/Disable are now redundant, and should just be
removed.

You'd have just one combined operation: send me the desired configuration, and
then return the actual configuration once complete.

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
sender_(sender), has_connection_(false) {
Having every window track the connection state seems excessive. Can't you just
add GpuPlatformSupportHostGbm::IsAlive()?

Better yet, why sweat sending message while it's dead at all? You have a
queue...

[dnicoara, 2014-09-09 14:29:29]

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
File ui/ozone/platform/dri/chromeos/display_message_handler.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
ui/ozone/platform/dri/chromeos/display_message_handler.cc:64: void
DisplayMessageHandler::OnRefreshNativeDisplays(
On 2014/09/08 22:49:17, spang wrote:
> There's now a big overlap between RefreshNativeDisplays and Configure/Disable.
> In fact it looks like Configure/Disable are now redundant, and should just be
> removed.
> 
> You'd have just one combined operation: send me the desired configuration, and
> then return the actual configuration once complete.

Not quite. RefreshNativeDisplays is passed a stale configuration (and possibly
none) and is used as a trigger to refresh the configuration. Whereas
Configure/Disable are called right after probing for configuration changes.

This would be the call sequence:

1) Browser: Display hotplug event -> send RefreshNativeDisplays IPC
2) GPU: OnRefreshNativeDisplays -> query driver for configuration changes
3) GPU: Send updated list of displays -> send UpdateNativeDisplays(...)
4) Browser: OnUpdateNativeDisplays -> Configure/Disable displays

In case of GPU crashes only the browser process knows the configuration, so it
needs to update the GPU side accordingly. For simplicity this is done in
OnRefreshNativeDisplays.

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
sender_(sender), has_connection_(false) {
On 2014/09/08 22:49:17, spang wrote:
> Having every window track the connection state seems excessive. Can't you just
> add GpuPlatformSupportHostGbm::IsAlive()?
> 

Done.

> Better yet, why sweat sending message while it's dead at all? You have a
> queue...
> 

I find the current form to be straight forward since creation of the channel
triggers sending the CreateWindowDelegate message.

[spang, 2014-09-09 14:44:25]

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
File ui/ozone/platform/dri/chromeos/display_message_handler.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
ui/ozone/platform/dri/chromeos/display_message_handler.cc:64: void
DisplayMessageHandler::OnRefreshNativeDisplays(
On 2014/09/09 14:29:28, dnicoara wrote:
> On 2014/09/08 22:49:17, spang wrote:
> > There's now a big overlap between RefreshNativeDisplays and
Configure/Disable.
> > In fact it looks like Configure/Disable are now redundant, and should just
be
> > removed.
> > 
> > You'd have just one combined operation: send me the desired configuration,
and
> > then return the actual configuration once complete.
> 
> Not quite. RefreshNativeDisplays is passed a stale configuration (and possibly
> none) and is used as a trigger to refresh the configuration. Whereas
> Configure/Disable are called right after probing for configuration changes.
> 
> This would be the call sequence:
> 
> 1) Browser: Display hotplug event -> send RefreshNativeDisplays IPC
> 2) GPU: OnRefreshNativeDisplays -> query driver for configuration changes
> 3) GPU: Send updated list of displays -> send UpdateNativeDisplays(...)
> 4) Browser: OnUpdateNativeDisplays -> Configure/Disable displays
> 
> In case of GPU crashes only the browser process knows the configuration, so it
> needs to update the GPU side accordingly. For simplicity this is done in
> OnRefreshNativeDisplays.

What do you mean by stale configuration? Does it actually matter?

GPU crash means you need a way to sync the whole state from browser to GPU. So
you've bolted that onto refresh. This seems fine, there's no reason not to do
them both in one go.

However if you have a way to sync the whole state, then you don't also need a
way to update parts of it. The duplication here is unwarranted.

Just have one path, that synchronizes everything. Browser sending an incomplete
configuration can be handled easily.

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
sender_(sender), has_connection_(false) {
On 2014/09/09 14:29:28, dnicoara wrote:
> On 2014/09/08 22:49:17, spang wrote:
> > Having every window track the connection state seems excessive. Can't you
just
> > add GpuPlatformSupportHostGbm::IsAlive()?
> > 
> 
> Done.
> 
> > Better yet, why sweat sending message while it's dead at all? You have a
> > queue...
> > 
> 
> I find the current form to be straight forward since creation of the channel
> triggers sending the CreateWindowDelegate message.

So.. do you still need the queue at all?

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/sc...
File ui/ozone/platform/dri/screen_manager.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/sc...
ui/ozone/platform/dri/screen_manager.cc:78: it = --controllers_.end();
This is subtle and isn't guaranteed to work.

it = controllers_.end() - 1;

or

it = controllers_.end();
--it;

[dnicoara, 2014-09-09 15:10:01]

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
File ui/ozone/platform/dri/chromeos/display_message_handler.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
ui/ozone/platform/dri/chromeos/display_message_handler.cc:64: void
DisplayMessageHandler::OnRefreshNativeDisplays(
On 2014/09/09 14:44:25, spang wrote:
> On 2014/09/09 14:29:28, dnicoara wrote:
> > On 2014/09/08 22:49:17, spang wrote:
> > > There's now a big overlap between RefreshNativeDisplays and
> Configure/Disable.
> > > In fact it looks like Configure/Disable are now redundant, and should just
> be
> > > removed.
> > > 
> > > You'd have just one combined operation: send me the desired configuration,
> and
> > > then return the actual configuration once complete.
> > 
> > Not quite. RefreshNativeDisplays is passed a stale configuration (and
possibly
> > none) and is used as a trigger to refresh the configuration. Whereas
> > Configure/Disable are called right after probing for configuration changes.
> > 
> > This would be the call sequence:
> > 
> > 1) Browser: Display hotplug event -> send RefreshNativeDisplays IPC
> > 2) GPU: OnRefreshNativeDisplays -> query driver for configuration changes
> > 3) GPU: Send updated list of displays -> send UpdateNativeDisplays(...)
> > 4) Browser: OnUpdateNativeDisplays -> Configure/Disable displays
> > 
> > In case of GPU crashes only the browser process knows the configuration, so
it
> > needs to update the GPU side accordingly. For simplicity this is done in
> > OnRefreshNativeDisplays.
> 
> What do you mean by stale configuration? Does it actually matter?

The configuration could be nothing (since CrOS just started up) or the last
polled configuration (which could have happened hours ago).

> 
> GPU crash means you need a way to sync the whole state from browser to GPU. So
> you've bolted that onto refresh. This seems fine, there's no reason not to do
> them both in one go.
> 
> However if you have a way to sync the whole state, then you don't also need a
> way to update parts of it. The duplication here is unwarranted.
> 
> Just have one path, that synchronizes everything. Browser sending an
incomplete
> configuration can be handled easily.

OnRefreshNativeDisplays is called in the following 2 cases:
1) On hotplug event,
2) On GPU restart.

For (1) the configuration known to the browser process is stale, so the browser
process needs to get the new one before it can make a decision.

For (2) the configuration is likely the same and we only want to sync the state
across processes.

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
sender_(sender), has_connection_(false) {
On 2014/09/09 14:44:25, spang wrote:
> On 2014/09/09 14:29:28, dnicoara wrote:
> > On 2014/09/08 22:49:17, spang wrote:
> > > Having every window track the connection state seems excessive. Can't you
> just
> > > add GpuPlatformSupportHostGbm::IsAlive()?
> > > 
> > 
> > Done.
> > 
> > > Better yet, why sweat sending message while it's dead at all? You have a
> > > queue...
> > > 
> > 
> > I find the current form to be straight forward since creation of the channel
> > triggers sending the CreateWindowDelegate message.
> 
> So.. do you still need the queue at all?

Unfortunately yes, for cursors.

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/sc...
File ui/ozone/platform/dri/screen_manager.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/sc...
ui/ozone/platform/dri/screen_manager.cc:78: it = --controllers_.end();
On 2014/09/09 14:44:25, spang wrote:
> This is subtle and isn't guaranteed to work.
> 
> it = controllers_.end() - 1;
> 
> or
> 
> it = controllers_.end();
> --it;

Done.

[spang, 2014-09-09 15:18:54]

On 2014/09/09 15:10:01, dnicoara wrote:
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
> File ui/ozone/platform/dri/chromeos/display_message_handler.cc (right):
> 
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/ch...
> ui/ozone/platform/dri/chromeos/display_message_handler.cc:64: void
> DisplayMessageHandler::OnRefreshNativeDisplays(
> On 2014/09/09 14:44:25, spang wrote:
> > On 2014/09/09 14:29:28, dnicoara wrote:
> > > On 2014/09/08 22:49:17, spang wrote:
> > > > There's now a big overlap between RefreshNativeDisplays and
> > Configure/Disable.
> > > > In fact it looks like Configure/Disable are now redundant, and should
just
> > be
> > > > removed.
> > > > 
> > > > You'd have just one combined operation: send me the desired
configuration,
> > and
> > > > then return the actual configuration once complete.
> > > 
> > > Not quite. RefreshNativeDisplays is passed a stale configuration (and
> possibly
> > > none) and is used as a trigger to refresh the configuration. Whereas
> > > Configure/Disable are called right after probing for configuration
changes.
> > > 
> > > This would be the call sequence:
> > > 
> > > 1) Browser: Display hotplug event -> send RefreshNativeDisplays IPC
> > > 2) GPU: OnRefreshNativeDisplays -> query driver for configuration changes
> > > 3) GPU: Send updated list of displays -> send UpdateNativeDisplays(...)
> > > 4) Browser: OnUpdateNativeDisplays -> Configure/Disable displays
> > > 
> > > In case of GPU crashes only the browser process knows the configuration,
so
> it
> > > needs to update the GPU side accordingly. For simplicity this is done in
> > > OnRefreshNativeDisplays.
> > 
> > What do you mean by stale configuration? Does it actually matter?
> 
> The configuration could be nothing (since CrOS just started up) or the last
> polled configuration (which could have happened hours ago).
> 
> > 
> > GPU crash means you need a way to sync the whole state from browser to GPU.
So
> > you've bolted that onto refresh. This seems fine, there's no reason not to
do
> > them both in one go.
> > 
> > However if you have a way to sync the whole state, then you don't also need
a
> > way to update parts of it. The duplication here is unwarranted.
> > 
> > Just have one path, that synchronizes everything. Browser sending an
> incomplete
> > configuration can be handled easily.
> 
> OnRefreshNativeDisplays is called in the following 2 cases:
> 1) On hotplug event,
> 2) On GPU restart.
> 
> For (1) the configuration known to the browser process is stale, so the
browser
> process needs to get the new one before it can make a decision.
> 
> For (2) the configuration is likely the same and we only want to sync the
state
> across processes.

I still think (1) and (2) can be treated the same, with a suitable
implementation.

"Here's what I think the current state should be, and by the way send me the
updated state"

> 
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
> File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):
> 
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
> sender_(sender), has_connection_(false) {
> On 2014/09/09 14:44:25, spang wrote:
> > On 2014/09/09 14:29:28, dnicoara wrote:
> > > On 2014/09/08 22:49:17, spang wrote:
> > > > Having every window track the connection state seems excessive. Can't
you
> > just
> > > > add GpuPlatformSupportHostGbm::IsAlive()?
> > > > 
> > > 
> > > Done.
> > > 
> > > > Better yet, why sweat sending message while it's dead at all? You have a
> > > > queue...
> > > > 
> > > 
> > > I find the current form to be straight forward since creation of the
channel
> > > triggers sending the CreateWindowDelegate message.
> > 
> > So.. do you still need the queue at all?
> 
> Unfortunately yes, for cursors.
> 
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/sc...
> File ui/ozone/platform/dri/screen_manager.cc (right):
> 
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/sc...
> ui/ozone/platform/dri/screen_manager.cc:78: it = --controllers_.end();
> On 2014/09/09 14:44:25, spang wrote:
> > This is subtle and isn't guaranteed to work.
> > 
> > it = controllers_.end() - 1;
> > 
> > or
> > 
> > it = controllers_.end();
> > --it;
> 
> Done.

[spang, 2014-09-09 15:21:06]

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):

https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
sender_(sender), has_connection_(false) {
On 2014/09/09 15:10:00, dnicoara wrote:
> On 2014/09/09 14:44:25, spang wrote:
> > On 2014/09/09 14:29:28, dnicoara wrote:
> > > On 2014/09/08 22:49:17, spang wrote:
> > > > Having every window track the connection state seems excessive. Can't
you
> > just
> > > > add GpuPlatformSupportHostGbm::IsAlive()?
> > > > 
> > > 
> > > Done.
> > > 
> > > > Better yet, why sweat sending message while it's dead at all? You have a
> > > > queue...
> > > > 
> > > 
> > > I find the current form to be straight forward since creation of the
channel
> > > triggers sending the CreateWindowDelegate message.
> > 
> > So.. do you still need the queue at all?
> 
> Unfortunately yes, for cursors.

which reminds me.. we need to set the cursor after a crash, right?

queueing only worked before we handled crashes. It's no longer enough.

[spang, 2014-09-09 15:50:17]

On 2014/09/09 15:21:06, spang wrote:
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
> File ui/ozone/platform/dri/dri_window_delegate_proxy.cc (right):
> 
>
https://codereview.chromium.org/554083002/diff/20001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_window_delegate_proxy.cc:15: : widget_(widget),
> sender_(sender), has_connection_(false) {
> On 2014/09/09 15:10:00, dnicoara wrote:
> > On 2014/09/09 14:44:25, spang wrote:
> > > On 2014/09/09 14:29:28, dnicoara wrote:
> > > > On 2014/09/08 22:49:17, spang wrote:
> > > > > Having every window track the connection state seems excessive. Can't
> you
> > > just
> > > > > add GpuPlatformSupportHostGbm::IsAlive()?
> > > > > 
> > > > 
> > > > Done.
> > > > 
> > > > > Better yet, why sweat sending message while it's dead at all? You have
a
> > > > > queue...
> > > > > 
> > > > 
> > > > I find the current form to be straight forward since creation of the
> channel
> > > > triggers sending the CreateWindowDelegate message.
> > > 
> > > So.. do you still need the queue at all?
> > 
> > Unfortunately yes, for cursors.
> 
> which reminds me.. we need to set the cursor after a crash, right?
> 
> queueing only worked before we handled crashes. It's no longer enough.

lgtm 

ok let's fix the cursor in a followup. I'll post a patch that shows how I wash
hoping to dedupe the IPCs.

[dnicoara, 2014-09-09 16:54:53]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-09-09 16:57:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/554083002/60001

[commit-bot: I haz the power, 2014-09-09 16:59:26]

Committed patchset #4 (id:60001) as 8b2544126f803ae60a8485f0392bf057a2d89294

[commit-bot: I haz the power, 2014-09-10 03:53:56]

Patchset 4 (id:??) landed as
https://crrev.com/01410fcf142cfd1fe038a4d089f7fc4e3cb3b312
Cr-Commit-Position: refs/heads/master@{#293956}

[kenrb, 2014-09-10 15:00:42]

ipc lgtm again