gpu: Empty swaps for surfaceless output surfaces.

gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2829543003
Cr-Commit-Position: refs/heads/master@{#466245}
Committed: https://chromium.googlesource.com/chromium/src/+/bba53b15c45ce0fa66d3343befae28aa695311c1

[reveman, 2017-04-19 11:46:05]

Description was changed from

==========
gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
==========

to

==========
gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[reveman, 2017-04-19 11:46:05]

Description was changed from

==========
gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[reveman, 2017-04-19 11:47:41]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[reveman, 2017-04-19 11:47:49]

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

[commit-bot: I haz the power, 2017-04-19 11:48:17]

Dry run: 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-04-19 11:51:37]

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

[commit-bot: I haz the power, 2017-04-19 11:51:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[reveman, 2017-04-19 15:45:06]

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

[commit-bot: I haz the power, 2017-04-19 15:45:25]

Dry run: 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-04-19 16:17:12]

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

[commit-bot: I haz the power, 2017-04-19 16:17:13]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[reveman, 2017-04-19 16:25:29]

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

[commit-bot: I haz the power, 2017-04-19 16:25:52]

Dry run: 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-04-19 17:04:16]

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

[commit-bot: I haz the power, 2017-04-19 17:04:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-04-19 17:25:14]

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

[commit-bot: I haz the power, 2017-04-19 17:25:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniele Castagna, 2017-04-19 19:10:36]

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
cc/output/direct_renderer.cc:354: if (!skip_drawing_root_render_pass)
In the comment you removed it mentioned that calling
BindFramebufferToOutputSurface would cause allocation of the backbuffer (and
that was desired). Is that not the case anymore?

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
cc/output/direct_renderer.h:190: bool use_partial_swap_ = false;
This names are a little bit confusing, or at least, it seems like a look at the
code is needed to clear out what they do.

Can we rename them?

Maybe explicitly mentioning if post_sub_buffers is used or not.

[commit-bot: I haz the power, 2017-04-19 19:15:35]

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

[commit-bot: I haz the power, 2017-04-19 19:15:36]

Dry run: This issue passed the CQ dry run.

[reveman, 2017-04-19 23:59:02]

reveman@chromium.org changed reviewers:
+ jbauman@chromium.org

[reveman, 2017-04-19 23:59:03]

+jbauman, please take a look.

FYI, we'll need to merge this back to M58 as non-fullscreen HW overlays in M58
will otherwise cause a significant performance regression on Chromebook Plus.

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
cc/output/direct_renderer.cc:354: if (!skip_drawing_root_render_pass)
On 2017/04/19 at 19:10:36, Daniele Castagna wrote:
> In the comment you removed it mentioned that calling
BindFramebufferToOutputSurface would cause allocation of the backbuffer (and
that was desired). Is that not the case anymore?

Yes, that was kind of a hack. Before this we still had to produce a new buffer
for empty swap to work but now that we can handle the empty swap properly that's
not the case. I think the new behavior is how you'd expect it to work and
doesn't need a comment. If we're not drawing anything then there's no longer a
need to bind the output surface to the fbo target.

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
cc/output/direct_renderer.h:190: bool use_partial_swap_ = false;
On 2017/04/19 at 19:10:36, Daniele Castagna wrote:
> This names are a little bit confusing, or at least, it seems like a look at
the code is needed to clear out what they do.
> 
> Can we rename them?
> 
> Maybe explicitly mentioning if post_sub_buffers is used or not.

Sure. Either use_post_sub_buffer or use_sub_buffer_rect instead of
use_partial_swap might make this easier to read. I'll do that if other reviewers
agree.

[jbauman, 2017-04-20 00:23:47]

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/capabilities.h (right):

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/capabilities.h:176: bool
disable_non_empty_post_sub_buffers = false;
Wouldn't an empty PostSubBuffers with a surfaceless compositor be essentially
equal to CommitOverlayPlanes? Maybe we should just be using that instead.

[reveman, 2017-04-20 18:21:35]

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/capabilities.h (right):

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/capabilities.h:176: bool
disable_non_empty_post_sub_buffers = false;
On 2017/04/20 at 00:23:47, jbauman wrote:
> Wouldn't an empty PostSubBuffers with a surfaceless compositor be essentially
equal to CommitOverlayPlanes? Maybe we should just be using that instead.

Almost. We still need to commit the primary plane and not just the overlays for
now. That can change in the future as it's not a requirement at the lowest
level, I think. I can look at that as a follow up but as we need this to be
merged into M58 I'd like to first land this SwapBuffers based approach.

[jbauman, 2017-04-20 21:30:07]

This generally seems good, then.

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
cc/output/direct_renderer.cc:104: allow_partial_swap_ = false;
If you just set use_partial_swap_=false here, then allow_empty_swap_ will be
true and you shouldn't need the allow_partial_swap_ variable at all.

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3816: !surface_->IsOffscreen())
{
We need a different workaround for this. Otherwise we'll enable empty partial
swap on mesa and virtualbox, where it's probably buggy.

[reveman, 2017-04-20 22:11:40]

PTAL

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
cc/output/direct_renderer.cc:104: allow_partial_swap_ = false;
On 2017/04/20 at 21:30:06, jbauman wrote:
> If you just set use_partial_swap_=false here, then allow_empty_swap_ will be
true and you shouldn't need the allow_partial_swap_ variable at all.

Good idea. Done.

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3816: !surface_->IsOffscreen())
{
On 2017/04/20 at 21:30:06, jbauman wrote:
> We need a different workaround for this. Otherwise we'll enable empty partial
swap on mesa and virtualbox, where it's probably buggy.

Unlikely that we do empty swaps without overlays but still makes sense to have a
separate workaround to be safe. Done.

[jbauman, 2017-04-20 22:19:43]

lgtm

On 2017/04/20 22:11:40, reveman wrote:
> PTAL
> 
>
https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
> File cc/output/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/2829543003/diff/60001/cc/output/direct_render...
> cc/output/direct_renderer.cc:104: allow_partial_swap_ = false;
> On 2017/04/20 at 21:30:06, jbauman wrote:
> > If you just set use_partial_swap_=false here, then allow_empty_swap_ will be
> true and you shouldn't need the allow_partial_swap_ variable at all.
> 
> Good idea. Done.
> 
>
https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2829543003/diff/60001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:3816:
!surface_->IsOffscreen())
> {
> On 2017/04/20 at 21:30:06, jbauman wrote:
> > We need a different workaround for this. Otherwise we'll enable empty
partial
> swap on mesa and virtualbox, where it's probably buggy.
> 
> Unlikely that we do empty swaps without overlays but still makes sense to have
a
> separate workaround to be safe. Done.

They're pretty rare, but I think they can happen if there's a CopyOutputRequest
to be
executed.

[reveman, 2017-04-20 22:51:23]

reveman@chromium.org changed reviewers:
+ fsamuel@chromium.org, tsepez@chromium.org

[reveman, 2017-04-20 22:51:24]

+tsepez for gpu/ipc/common/gpu_command_buffer_traits_multi.h
+fsamuel for services/ui/surfaces/display_output_surface_ozone.cc

[reveman, 2017-04-20 22:51:30]

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

[commit-bot: I haz the power, 2017-04-20 22:52:00]

Dry run: 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-04-20 22:54:13]

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

[commit-bot: I haz the power, 2017-04-20 22:54:13]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[Tom Sepez, 2017-04-20 22:55:42]

lgtm

[Fady Samuel, 2017-04-20 23:04:46]

lgtm

[reveman, 2017-04-20 23:09:10]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2017-04-20 23:09:27]

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-04-20 23:11:36]

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

[commit-bot: I haz the power, 2017-04-20 23:11:37]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[reveman, 2017-04-21 02:25:10]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2017-04-21 02:25:29]

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-04-21 03:20:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492741510768820,
"parent_rev": "831c5ca6639c0b28b1134ffac2cc8b315dae17de", "commit_rev":
"bba53b15c45ce0fa66d3343befae28aa695311c1"}

[commit-bot: I haz the power, 2017-04-21 03:21:10]

Description was changed from

==========
gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Empty swaps for surfaceless output surfaces.

This improves hardware overlay support by avoiding updates to the
primary plane when only overlays are damaged. The previous buffer
is now reused for the primary plane when damage rect is empty.

An important side-effect of this is that we can always support empty
swaps without the need for partial updates.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2829543003
Cr-Commit-Position: refs/heads/master@{#466245}
Committed:
https://chromium.googlesource.com/chromium/src/+/bba53b15c45ce0fa66d3343befae...
==========

[commit-bot: I haz the power, 2017-04-21 03:21:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bba53b15c45ce0fa66d3343befae...

[Xiaosong, 2017-07-11 08:58:33]

On 2017/04/21 03:21:11, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/bba53b15c45ce0fa66d3343befae...

Hi,

I am trying to verify this "Empty Swap" feature on ChromeOS with a static web
page with video playback inside. 
From /sys/kernel/debug/drm/0/i915_display_info I can see that the video is
promoted to display via Overlay and the static background in the web page is
displayed via primary plane.
Per my understanding, in this case the damage_rect should be empty and then it
can avoid swapping buffer on the primary plane.
But the fact is that the damage_rect is always non-empty so that it can't run
into avoid swapping buffer. I remember that in previous chromium it can go to
empty-swap case with the same web page.
I am wondering if this "Empty Swap" is still valid in the latest chromium, if
so, is there any change needed to verify it?
Thanks!

[reveman, 2017-07-11 09:12:03]

On 2017/07/11 at 08:58:33, xiaosong.wei wrote:
> On 2017/04/21 03:21:11, commit-bot: I haz the power wrote:
> > Committed patchset #5 (id:80001) as
> >
https://chromium.googlesource.com/chromium/src/+/bba53b15c45ce0fa66d3343befae...
> 
> Hi,
> 
> I am trying to verify this "Empty Swap" feature on ChromeOS with a static web
page with video playback inside. 
> From /sys/kernel/debug/drm/0/i915_display_info I can see that the video is
promoted to display via Overlay and the static background in the web page is
displayed via primary plane.
> Per my understanding, in this case the damage_rect should be empty and then it
can avoid swapping buffer on the primary plane.
> But the fact is that the damage_rect is always non-empty so that it can't run
into avoid swapping buffer. I remember that in previous chromium it can go to
empty-swap case with the same web page.
> I am wondering if this "Empty Swap" is still valid in the latest chromium, if
so, is there any change needed to verify it?
> Thanks!

Intel uses partial swap so this is not needed in that case. Still, no
compositing should take place unless output surface buffer needs to be updated.