ozone: fix broken overlay promotion bug at start-up time.

ozone: fix broken overlay promotion bug at start-up time.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed in GPU process. In the case, it reports
any planes are not overlayable. However, the client keeps this information as cache and the
overlay never happens.

To fix it, when the browser updates display configuration, it resets the cache.

BUG=683347
TEST=Run chrome on samus

Review-Url: https://codereview.chromium.org/2920613002
Cr-Commit-Position: refs/heads/master@{#476426}
Committed: https://chromium.googlesource.com/chromium/src/+/2278b11f49a4b57a7b58ab0b00b35a38f3161504

[dshwang, 2017-05-31 21:09:42]

dongseong.hwang@intel.com changed reviewers:
+ dnicoara@chromium.org

[dshwang, 2017-05-31 21:09:43]

dnicoara@, could you review?

On 2017/05/29 15:17:57, dnicoara wrote:
> Any idea how we're getting in that case? I'd expect it to never happen since
the
> cache is reset right after the window location is updated. Since the GPU
> executes all queries sequentially, the window should be matched with a display
> by the time an overlay query comes in. The only case this isn't true would be
if
> the window doesn't have any display (headless), but in that case it shouldn't
> matter.

I extract this code from https://codereview.chromium.org/2896173002/

Here's reproducable step. when I run chrome on chromeos device by terminal, it
happens
./chrome --ui-prioritize-in-gpu-process --use-gl=egl
--user-data-dir=/home/chronos/ --bwsi --login-user='$guest' --login-profile=user
--incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
http://browsertests.herokuapp.com/media/fullscreen.html

It's because renderer creation happens eariler than display settup for some
reasons. In addition, renderer doesn't know window location is updated in gpu
process. So it just reuses its own cache value.

[dnicoara, 2017-05-31 21:19:51]

On 2017/05/31 21:09:43, dshwang wrote:
> dnicoara@, could you review?
> 
> On 2017/05/29 15:17:57, dnicoara wrote:
> > Any idea how we're getting in that case? I'd expect it to never happen since
> the
> > cache is reset right after the window location is updated. Since the GPU
> > executes all queries sequentially, the window should be matched with a
display
> > by the time an overlay query comes in. The only case this isn't true would
be
> if
> > the window doesn't have any display (headless), but in that case it
shouldn't
> > matter.
> 
> I extract this code from https://codereview.chromium.org/2896173002/
> 
> Here's reproducable step. when I run chrome on chromeos device by terminal, it
> happens
> ./chrome --ui-prioritize-in-gpu-process --use-gl=egl
> --user-data-dir=/home/chronos/ --bwsi --login-user='$guest'
--login-profile=user
> --incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
> http://browsertests.herokuapp.com/media/fullscreen.html
> 
> It's because renderer creation happens eariler than display settup for some
> reasons. In addition, renderer doesn't know window location is updated in gpu
> process. So it just reuses its own cache value.

Ah, this is a renderer <-> GPU interaction. I was thinking it was a browser <->
GPU issue. I'll take a look.

[dshwang, 2017-05-31 21:37:19]

On 2017/05/31 21:19:51, dnicoara wrote:
> On 2017/05/31 21:09:43, dshwang wrote:
> > dnicoara@, could you review?
> > 
> > On 2017/05/29 15:17:57, dnicoara wrote:
> > > Any idea how we're getting in that case? I'd expect it to never happen
since
> > the
> > > cache is reset right after the window location is updated. Since the GPU
> > > executes all queries sequentially, the window should be matched with a
> display
> > > by the time an overlay query comes in. The only case this isn't true would
> be
> > if
> > > the window doesn't have any display (headless), but in that case it
> shouldn't
> > > matter.
> > 
> > I extract this code from https://codereview.chromium.org/2896173002/
> > 
> > Here's reproducable step. when I run chrome on chromeos device by terminal,
it
> > happens
> > ./chrome --ui-prioritize-in-gpu-process --use-gl=egl
> > --user-data-dir=/home/chronos/ --bwsi --login-user='$guest'
> --login-profile=user
> > --incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
> > http://browsertests.herokuapp.com/media/fullscreen.html
> > 
> > It's because renderer creation happens eariler than display settup for some
> > reasons. In addition, renderer doesn't know window location is updated in
gpu
> > process. So it just reuses its own cache value.
> 
> Ah, this is a renderer <-> GPU interaction. I was thinking it was a browser
<->
> GPU issue. I'll take a look.

Ah, it's browser <-> GPU interaction.

I should say "It's because renderer creation happens eariler than display settup
for some
reasons. In addition, BROWSER doesn't know window location is updated in gpu
process.
So it just reuses its own cache value."

[dnicoara, 2017-05-31 21:39:12]

On 2017/05/31 21:19:51, dnicoara wrote:
> On 2017/05/31 21:09:43, dshwang wrote:
> > dnicoara@, could you review?
> > 
> > On 2017/05/29 15:17:57, dnicoara wrote:
> > > Any idea how we're getting in that case? I'd expect it to never happen
since
> > the
> > > cache is reset right after the window location is updated. Since the GPU
> > > executes all queries sequentially, the window should be matched with a
> display
> > > by the time an overlay query comes in. The only case this isn't true would
> be
> > if
> > > the window doesn't have any display (headless), but in that case it
> shouldn't
> > > matter.
> > 
> > I extract this code from https://codereview.chromium.org/2896173002/
> > 
> > Here's reproducable step. when I run chrome on chromeos device by terminal,
it
> > happens
> > ./chrome --ui-prioritize-in-gpu-process --use-gl=egl
> > --user-data-dir=/home/chronos/ --bwsi --login-user='$guest'
> --login-profile=user
> > --incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
> > http://browsertests.herokuapp.com/media/fullscreen.html
> > 
> > It's because renderer creation happens eariler than display settup for some
> > reasons. In addition, renderer doesn't know window location is updated in
gpu
> > process. So it just reuses its own cache value.
> 
> Ah, this is a renderer <-> GPU interaction. I was thinking it was a browser
<->
> GPU issue. I'll take a look.

On a related note, this could still be an issue after windows move between
displays since the cache is never reset. There's currently not way to reset the
cache in the renderer, right?

[dnicoara, 2017-05-31 21:48:01]

On 2017/05/31 21:37:19, dshwang wrote:
> On 2017/05/31 21:19:51, dnicoara wrote:
> > On 2017/05/31 21:09:43, dshwang wrote:
> > > dnicoara@, could you review?
> > > 
> > > On 2017/05/29 15:17:57, dnicoara wrote:
> > > > Any idea how we're getting in that case? I'd expect it to never happen
> since
> > > the
> > > > cache is reset right after the window location is updated. Since the GPU
> > > > executes all queries sequentially, the window should be matched with a
> > display
> > > > by the time an overlay query comes in. The only case this isn't true
would
> > be
> > > if
> > > > the window doesn't have any display (headless), but in that case it
> > shouldn't
> > > > matter.
> > > 
> > > I extract this code from https://codereview.chromium.org/2896173002/
> > > 
> > > Here's reproducable step. when I run chrome on chromeos device by
terminal,
> it
> > > happens
> > > ./chrome --ui-prioritize-in-gpu-process --use-gl=egl
> > > --user-data-dir=/home/chronos/ --bwsi --login-user='$guest'
> > --login-profile=user
> > > --incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
> > > http://browsertests.herokuapp.com/media/fullscreen.html
> > > 
> > > It's because renderer creation happens eariler than display settup for
some
> > > reasons. In addition, renderer doesn't know window location is updated in
> gpu
> > > process. So it just reuses its own cache value.
> > 
> > Ah, this is a renderer <-> GPU interaction. I was thinking it was a browser
> <->
> > GPU issue. I'll take a look.
> 
> Ah, it's browser <-> GPU interaction.
> 
> I should say "It's because renderer creation happens eariler than display
settup
> for some
> reasons. In addition, BROWSER doesn't know window location is updated in gpu
> process.
> So it just reuses its own cache value."

The browser drives updates to window positioning and display configuration.
Based on that I would think that it should always be the case that the browser
knows and resets the cache when an update (display or window) happens.

[inactive_dshwang_plz_cc_intel, 2017-05-31 23:46:55]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 23:47:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2017-05-31 23:49:49]

Description was changed from

==========
ozone: fix broken overlay promotion bug at start-up time.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed. In the case, it reports any planes are
not overlayable. However, the client keeps this information as cache and the
overlay never happens.

In the case, this CL makes the validator report Invalid mark and the client
will request the validation again.

BUG=683347
TEST=Run chrome on samus, ozone_unittests
==========

to

==========
ozone: fix broken overlay promotion bug at start-up time.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed in GPU process. In the case, it reports
any planes are not overlayable. However, the client keeps this information as
cache and the
overlay never happens.

To fix it, when the browser updates display configuration, it resets the cache.

BUG=683347
TEST=Run chrome on samus
==========

[dshwang, 2017-05-31 23:50:11]

On 2017/05/31 21:48:01, dnicoara wrote:
> On 2017/05/31 21:37:19, dshwang wrote:
> > On 2017/05/31 21:19:51, dnicoara wrote:
> > > On 2017/05/31 21:09:43, dshwang wrote:
> > > > dnicoara@, could you review?
> > > > 
> > > > On 2017/05/29 15:17:57, dnicoara wrote:
> > > > > Any idea how we're getting in that case? I'd expect it to never happen
> > since
> > > > the
> > > > > cache is reset right after the window location is updated. Since the
GPU
> > > > > executes all queries sequentially, the window should be matched with a
> > > display
> > > > > by the time an overlay query comes in. The only case this isn't true
> would
> > > be
> > > > if
> > > > > the window doesn't have any display (headless), but in that case it
> > > shouldn't
> > > > > matter.
> > > > 
> > > > I extract this code from https://codereview.chromium.org/2896173002/
> > > > 
> > > > Here's reproducable step. when I run chrome on chromeos device by
> terminal,
> > it
> > > > happens
> > > > ./chrome --ui-prioritize-in-gpu-process --use-gl=egl
> > > > --user-data-dir=/home/chronos/ --bwsi --login-user='$guest'
> > > --login-profile=user
> > > > --incognito --use-cras --enable-hardware-overlays --enable-drm-atomic
> > > > http://browsertests.herokuapp.com/media/fullscreen.html
> > > > 
> > > > It's because renderer creation happens eariler than display settup for
> some
> > > > reasons. In addition, renderer doesn't know window location is updated
in
> > gpu
> > > > process. So it just reuses its own cache value.
> > > 
> > > Ah, this is a renderer <-> GPU interaction. I was thinking it was a
browser
> > <->
> > > GPU issue. I'll take a look.
> > 
> > Ah, it's browser <-> GPU interaction.
> > 
> > I should say "It's because renderer creation happens eariler than display
> settup
> > for some
> > reasons. In addition, BROWSER doesn't know window location is updated in gpu
> > process.
> > So it just reuses its own cache value."
> 
> The browser drives updates to window positioning and display configuration.
> Based on that I would think that it should always be the case that the browser
> knows and resets the cache when an update (display or window) happens.

Correct! I update this CL. it's now one-liner

[commit-bot: I haz the power, 2017-06-01 00:16:31]

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

[commit-bot: I haz the power, 2017-06-01 00:16:32]

Dry run: This issue passed the CQ dry run.

[dnicoara, 2017-06-01 14:55:48]

Thank you for the cleanup!

Could you move the call site to DrmDisplayHostManager::GpuConfiguredDisplay()?
DrmGpuPlatformSupportHost is just an IPC wrapper class, so I'd like to keep
useful logic outside of it. Also, that call happens after the display is
configured, so we'd avoid any potential issues with pending requests sneaking in
between the cache reset and the actual display configuration change. And the
last benefit is that it covers display disable events as well.

[inactive_dshwang_plz_cc_intel, 2017-06-01 19:19:28]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 19:19:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2017-06-01 19:19:57]

On 2017/06/01 14:55:48, dnicoara wrote:
> Thank you for the cleanup!
> 
> Could you move the call site to DrmDisplayHostManager::GpuConfiguredDisplay()?
> DrmGpuPlatformSupportHost is just an IPC wrapper class, so I'd like to keep
> useful logic outside of it. Also, that call happens after the display is
> configured, so we'd avoid any potential issues with pending requests sneaking
in
> between the cache reset and the actual display configuration change. And the
> last benefit is that it covers display disable events as well.

Thank you for give the right direction. Done. could you review again?

[dnicoara, 2017-06-01 19:27:41]

Thank you!

lgtm

[commit-bot: I haz the power, 2017-06-01 19:48:41]

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

[commit-bot: I haz the power, 2017-06-01 19:48:42]

Dry run: This issue passed the CQ dry run.

[dshwang, 2017-06-01 20:45:30]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2017-06-01 20:46:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 20:50:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496349930884420,
"parent_rev": "9d068dc8a60aea79f24f02f6a659972936387087", "commit_rev":
"2278b11f49a4b57a7b58ab0b00b35a38f3161504"}

[commit-bot: I haz the power, 2017-06-01 20:50:40]

Description was changed from

==========
ozone: fix broken overlay promotion bug at start-up time.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed in GPU process. In the case, it reports
any planes are not overlayable. However, the client keeps this information as
cache and the
overlay never happens.

To fix it, when the browser updates display configuration, it resets the cache.

BUG=683347
TEST=Run chrome on samus
==========

to

==========
ozone: fix broken overlay promotion bug at start-up time.

DrmOverlayValidator::TestPageFlip() can be called before
HardwareDisplayController is installed in GPU process. In the case, it reports
any planes are not overlayable. However, the client keeps this information as
cache and the
overlay never happens.

To fix it, when the browser updates display configuration, it resets the cache.

BUG=683347
TEST=Run chrome on samus

Review-Url: https://codereview.chromium.org/2920613002
Cr-Commit-Position: refs/heads/master@{#476426}
Committed:
https://chromium.googlesource.com/chromium/src/+/2278b11f49a4b57a7b58ab0b00b3...
==========

[commit-bot: I haz the power, 2017-06-01 20:50:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2278b11f49a4b57a7b58ab0b00b3...