New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 705290 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Enable partial swaps on Mali

Project Member Reported by reve...@chromium.org, Mar 26 2017

Issue description

Partial swaps where disabled on Mali because of  issue 457511 . This is a serious performance problem on newer device with high DPI screens and limits the benefit of HW overlays.
 
Cc: piman@chromium.org
https://codereview.chromium.org/2776923003 is an experimental patch that implements partial swap by expanding the damage rect instead of relying on a copy. This seems to be working correctly and improves performance and solves our HW overlay issues on Kevin.
Cc: zelidrag@chromium.org puneetster@chromium.org
Fixing this will likely provide a major performance and power usage improvement on Mali devices. Enough that we might want to consider merging it back to M58.
Cc: hoegsberg@chromium.org
Partial updates are a big improvement over always doing full screen updates but the benefit on Kevin doesn't seems as significant as on Intel devices. hoegsberg@, is it possible that AFBC is making this less efficient? Is it worth testing if this becomes more efficient without AFBC turned on?
What makes you think partial update would be more efficient without AFBC?
I don't think there is any practical scenario where using AFBC would be slower/less power efficient than without it.

I'm also confused about  crbug.com/457511 . Isn't Kristian's drawing client doing something similar: drawing to an FB with GL while it's being used for scanout?
Why don't we notice the same problem there?
> What makes you think partial update would be more efficient without AFBC?
> I don't think there is any practical scenario where using AFBC would be ?slower/less power efficient than without it.

No particular reason. I'm not perfectly familiar with how AFBC is implemented so asking to make sure it's not a problem. 

> I'm also confused about  crbug.com/457511 . Isn't Kristian's drawing client doing something similar: drawing to an FB with GL while it's being used for scanout?
> Why don't we notice the same problem there?

I haven't tested the client that uses GL for drawing on the client side but that might run into the same issue. If not, then maybe exporting the buffer using dmabuf and then re-importing it somehow avoids it.

What I know is that forcing the old partial update mechanism on Kevin caused Chrome to quickly freeze with a black screen, which seems consistent with  issue 457511 . We might want to fix this on the driver side for low-latency rendering but I'm still wondering if we shouldn't use the simpler logic for partial updates implemented by the CL above and avoid doing blits from the previous buffer to implement partial updates on Cros. Either approach is fine with me as long as we enable partial swaps on Mali devices asap.
I noticed that we're not setting the scissor rect when doing partial updates. Maybe that's something we should do to help tiled GPUs optimize partial updates?
That's something we were discussing with Kristian when he observed 8ms spent in GL finish to draw a few lines.

The tile renderer might have a high fixed cost just to prepare tiles, even if there is not much to draw.

If you search for scissor here: https://static.docs.arm.com/100019/0100/arm_mali_application_developer_best_practices_developer_guide_100019_0100_00_en2.pdf it seems like they suggest to always use the scissor box if clearing or drawing only to a specific area.
So, that's something we should definitely try.

Kristian, did you try to set it in your client? Did you notice any difference?
Created  crbug.com/705556  for the scissor rect.

Comment 9 by piman@chromium.org, Mar 27 2017

Cc: danakj@chromium.org
https://codereview.chromium.org/2776923003 is ready for review. Initially I'm more interested in feedback on the approach in general rather then details of the code but all feedback is appreciated.
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bba53b15c45ce0fa66d3343befae28aa695311c1

commit bba53b15c45ce0fa66d3343befae28aa695311c1
Author: reveman <reveman@chromium.org>
Date: Fri Apr 21 03:20:18 2017

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}

[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/cc/output/direct_renderer.cc
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/cc/output/overlay_unittest.cc
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/components/display_compositor/buffer_queue.cc
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/components/display_compositor/buffer_queue.h
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/gpu/command_buffer/common/capabilities.h
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/gpu/config/gpu_driver_bug_list.json
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/gpu/config/gpu_driver_bug_workaround_type.h
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/gpu/ipc/common/gpu_command_buffer_traits_multi.h
[modify] https://crrev.com/bba53b15c45ce0fa66d3343befae28aa695311c1/services/ui/surfaces/display_output_surface_ozone.cc

Labels: Merge-Request-58
We need this in M58 to not have a major performance regression when using non fullscreen HW overlays.
Project Member

Comment 13 by sheriffbot@chromium.org, Apr 21 2017

Labels: -Merge-Request-58 Merge-Review-58 Hotlist-Merge-Review
This bug requires manual review: We are only 3 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f26315f43d9d4214df8af3deb263a02fc47db5d

commit 5f26315f43d9d4214df8af3deb263a02fc47db5d
Author: dcastagna <dcastagna@chromium.org>
Date: Thu Apr 27 17:50:12 2017

cc: Don't subtract transparent overlay from damage.

After processing overlay candidates we used to remove the overlay rects
from the damage rect.
The assumption was that all the overlays would be opaque.

Since we started allowing alpha blended overaly, we should avoid
subtracting from the damage when blending might be allowed.
Otherwise any element animating below a transparent overlay might
not be updated.

We haven't noticed any problem so far since partial update was disabled
on Mali. crrev.com/2829543003 enables empty swap, surfacing the problem
when the damage region is a subset of a transparent overlay.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2842383003
Cr-Commit-Position: refs/heads/master@{#467727}

[modify] https://crrev.com/5f26315f43d9d4214df8af3deb263a02fc47db5d/cc/output/overlay_candidate.cc
[modify] https://crrev.com/5f26315f43d9d4214df8af3deb263a02fc47db5d/cc/output/overlay_candidate.h
[modify] https://crrev.com/5f26315f43d9d4214df8af3deb263a02fc47db5d/cc/output/overlay_processor.cc
[modify] https://crrev.com/5f26315f43d9d4214df8af3deb263a02fc47db5d/cc/output/overlay_unittest.cc

Labels: Merge-Request-59
Cc: bhthompson@chromium.org
https://chromium.googlesource.com/chromium/src.git/+/bba53b15c45ce0fa66d3343befae28aa695311c1 needs to be merged to M58 and M59 to prevent a major performance regression for Android apps

https://chromium.googlesource.com/chromium/src.git/+/5f26315f43d9d4214df8af3deb263a02fc47db5d needs to be merged to M59 when the above change is merged to prevent drawing artifacts for the laser pointer


Labels: -Hotlist-Merge-Review -Merge-Review-58 Merge-Approved-58
Labels: Merge-Approved-59
Project Member

Comment 19 by bugdroid1@chromium.org, Apr 27 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db31db28a02894025e1939362d75aff4665d0b8a

commit db31db28a02894025e1939362d75aff4665d0b8a
Author: David Reveman <reveman@chromium.org>
Date: Thu Apr 27 21:55:14 2017

cc: Don't subtract transparent overlay from damage.

After processing overlay candidates we used to remove the overlay rects
from the damage rect.
The assumption was that all the overlays would be opaque.

Since we started allowing alpha blended overaly, we should avoid
subtracting from the damage when blending might be allowed.
Otherwise any element animating below a transparent overlay might
not be updated.

We haven't noticed any problem so far since partial update was disabled
on Mali. crrev.com/2829543003 enables empty swap, surfacing the problem
when the damage region is a subset of a transparent overlay.

BUG=705290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2842383003
Cr-Commit-Position: refs/heads/master@{#467727}
(cherry picked from commit 5f26315f43d9d4214df8af3deb263a02fc47db5d)

R=dcastagna@chromium.org

Review-Url: https://codereview.chromium.org/2845193003 .
Cr-Commit-Position: refs/branch-heads/3071@{#273}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/db31db28a02894025e1939362d75aff4665d0b8a/cc/output/overlay_candidate.cc
[modify] https://crrev.com/db31db28a02894025e1939362d75aff4665d0b8a/cc/output/overlay_candidate.h
[modify] https://crrev.com/db31db28a02894025e1939362d75aff4665d0b8a/cc/output/overlay_processor.cc
[modify] https://crrev.com/db31db28a02894025e1939362d75aff4665d0b8a/cc/output/overlay_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Apr 28 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a21d8cc39dda13e06f1e138be19ec2e35df5defc

commit a21d8cc39dda13e06f1e138be19ec2e35df5defc
Author: Aaron Gable <agable@chromium.org>
Date: Fri Apr 28 19:52:35 2017

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}
(cherry picked from commit bba53b15c45ce0fa66d3343befae28aa695311c1)

TBR=reveman@chromium.org

Change-Id: I244c9bfd25749cc116dc1a57b2776760ea4d1345
Reviewed-on: https://chromium-review.googlesource.com/489806
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/branch-heads/3029@{#782}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/cc/output/direct_renderer.cc
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/cc/output/overlay_unittest.cc
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/components/display_compositor/buffer_queue.cc
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/components/display_compositor/buffer_queue.h
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/gpu/command_buffer/common/capabilities.h
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/gpu/config/gpu_driver_bug_list_json.cc
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/gpu/config/gpu_driver_bug_workaround_type.h
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/gpu/ipc/common/gpu_command_buffer_traits_multi.h
[modify] https://crrev.com/a21d8cc39dda13e06f1e138be19ec2e35df5defc/services/ui/surfaces/display_output_surface_ozone.cc

Project Member

Comment 21 by sheriffbot@chromium.org, Apr 29 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 22 by sheriffbot@chromium.org, May 3 2017

Cc: bhthompson@google.com gkihumba@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 23 by sheriffbot@chromium.org, May 8 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 24 by bugdroid1@chromium.org, May 26 2017

Labels: merge-merged-wk51_2014
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/vendor/mali-ddk/+/b9ce0340947f8336486af10c4479e628903b8aba

commit b9ce0340947f8336486af10c4479e628903b8aba
Author: Haixia Shi <hshi@chromium.org>
Date: Fri May 26 21:35:33 2017

Project Member

Comment 25 by bugdroid1@chromium.org, May 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c

commit d2d8cbbe67801309f69be3c5c941efccb6fcbf6c
Author: reveman <reveman@chromium.org>
Date: Sat May 27 21:23:26 2017

gpu: Use ANDROID_native_fence_sync instead of ARM_implicit_external_sync.

This allows partial swaps to be enabled on Mali devices.

BUG=705290
TBR=dcheng@chromium.org
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/2864483007
Cr-Commit-Position: refs/heads/master@{#475250}

[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/cc/output/direct_renderer.cc
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/components/exo/wayland/clients/client_base.cc
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/gpu/command_buffer/common/capabilities.h
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/gpu/config/gpu_driver_bug_list.json
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/gpu/config/gpu_driver_bug_workaround_type.h
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/gpu/ipc/common/gpu_command_buffer_traits_multi.h
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/ui/ozone/platform/drm/gpu/gbm_surfaceless.cc
[modify] https://crrev.com/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c/ui/ozone/platform/drm/gpu/gbm_surfaceless.h

Project Member

Comment 26 by bugdroid1@chromium.org, May 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9

commit 0b74898cc7b8d9fdfe31d7ee00a206380b498ec9
Author: acourbot <acourbot@chromium.org>
Date: Tue May 30 07:51:47 2017

Revert of gpu: Use ANDROID_native_fence_sync instead of ARM_implicit_external_sync. (patchset #5 id:80001 of https://codereview.chromium.org/2864483007/ )

Reason for revert:
Breaks UI on peach_pit

http://crbug.com/727462

Original issue's description:
> gpu: Use ANDROID_native_fence_sync instead of ARM_implicit_external_sync.
>
> This allows partial swaps to be enabled on Mali devices.
>
> BUG=705290
> TBR=dcheng@chromium.org
> 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/2864483007
> Cr-Commit-Position: refs/heads/master@{#475250}
> Committed: https://chromium.googlesource.com/chromium/src/+/d2d8cbbe67801309f69be3c5c941efccb6fcbf6c

TBR=dcastagna@chromium.org,dnicoara@chromium.org,piman@chromium.org,dcheng@chromium.org,reveman@chromium.org,hajimehoshi@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=705290

Review-Url: https://codereview.chromium.org/2908313002
Cr-Commit-Position: refs/heads/master@{#475455}

[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/cc/output/direct_renderer.cc
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/components/exo/wayland/clients/client_base.cc
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/gpu/command_buffer/common/capabilities.h
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/gpu/config/gpu_driver_bug_list.json
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/gpu/config/gpu_driver_bug_workaround_type.h
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/gpu/ipc/common/gpu_command_buffer_traits_multi.h
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/ui/ozone/platform/drm/gpu/gbm_surfaceless.cc
[modify] https://crrev.com/0b74898cc7b8d9fdfe31d7ee00a206380b498ec9/ui/ozone/platform/drm/gpu/gbm_surfaceless.h

Project Member

Comment 27 by sheriffbot@chromium.org, Jul 11 2017

Labels: -Merge-Approved-59
This issue hasn't been updated in the last 6 weeks, so removing its merge approval label. Please re-request a merge if needed.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 28 by bugdroid1@chromium.org, Jul 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8aec1567a9b44547797db47c5f6a3f392f6fa3d9

commit 8aec1567a9b44547797db47c5f6a3f392f6fa3d9
Author: David Reveman <reveman@chromium.org>
Date: Wed Jul 12 09:40:19 2017

cc: Fix overlay damage tracking.

Make sure overlay damage is updated correctly for fullscreen
and translucent overlays.

BUG=705290
TEST=cc_unittests --gtest_filter=*Overlay*

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I83c80ddb47495e185fc247c93e71aeb230504cb4
Reviewed-on: https://chromium-review.googlesource.com/558508
Commit-Queue: David Reveman <reveman@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485905}
[modify] https://crrev.com/8aec1567a9b44547797db47c5f6a3f392f6fa3d9/cc/output/overlay_processor.cc
[modify] https://crrev.com/8aec1567a9b44547797db47c5f6a3f392f6fa3d9/cc/output/overlay_unittest.cc

Please confirm, when would the CPFE image be available for Soraka, with the below fixes on the browser.

https://chromium-review.googlesource.com/c/567809/
https://chromium-review.googlesource.com/c/558508/
Triage nag: This Chrome OS bug has an owner but no component. Please add a component so that this can be tracked by the relevant team.
<UI triage> Bug owners, please add the appropriate component to your bug. Thanks!

Comment 32 by afakhry@chromium.org, Jan 18 (5 days ago)

Components: Internals>Compositing OS>Kernel>Display

Sign in to add a comment