Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2176703002:
gpu: Disable multisample on more adreno renderers (Closed)
Descriptiongpu: Disable multisample on more adreno renderers
We know for sure the initial N release for N5X/N6P will have multisample
bugs. There is evidence as well that adreno 530 drivers in existing
devices like samsung s7 is affected as well. Expand the workaround to
cover these devices.
Planning on merging back to m52
BUG=612474
CQ_INCLUDE_TRYBOTS=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
Committed: https://crrev.com/1243666b4f5c7c48312233e1f6e6fda46191b184
Cr-Commit-Position: refs/heads/master@{#407311}
Patch Set 1 #
Total comments: 1
Patch Set 2 : new rules #
Total comments: 1
Patch Set 3 : disable extension #
Total comments: 2
Messages
Total messages: 49 (21 generated)
Description was changed from ========== gpu: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. BUG=612474 ========== to ========== gpu: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. BUG=612474 CQ_INCLUDE_TRYBOTS=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 ==========
boliu@chromium.org changed reviewers: + sievers@chromium.org
let's discuss on chat about this..
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== gpu: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. BUG=612474 CQ_INCLUDE_TRYBOTS=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: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. BUG=612474 CQ_INCLUDE_TRYBOTS=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 ==========
sievers@chromium.org changed reviewers: + bsalomon@chromium.org
Seems fine if it has stability problems. So is the question if it's worth to try to do this more fine-grained in any way? +bsalomon for comment regarding impact wrt canvas. I vaguely remember some developer complaints about lack of AA.
Description was changed from ========== gpu: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. BUG=612474 CQ_INCLUDE_TRYBOTS=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: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. Planning on merging back to m52 BUG=612474 CQ_INCLUDE_TRYBOTS=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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bsalomon@chromium.org changed reviewers: + senorblanco@chromium.org
Canvas doesn't use MSAA (except in the display list case). +senorblanco.
https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1135: // Mean to cover N only. NMR1 has this fixed. If the bug is only in N, could we leave MSAA enabled for L and M? There are a growing number of WebView apps that depend on decent GPU path rendering performance, e.g. mapping apps. (This was one of the motivating use cases for the MSAA tessellating path renderer in the first place.) Note also that you may need to duplicate the original blacklist entry for < 5.1, since that was a different bug.
On 2016/07/22 18:18:15, Stephen White wrote: > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > File gpu/config/gpu_driver_bug_list_json.cc (right): > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > gpu/config/gpu_driver_bug_list_json.cc:1135: // Mean to cover N only. NMR1 has > this fixed. > If the bug is only in N, could we leave MSAA enabled for L and M? There are a > growing number of WebView apps that depend on decent GPU path rendering > performance, e.g. mapping apps. (This was one of the motivating use cases for > the MSAA tessellating path renderer in the first place.) > > Note also that you may need to duplicate the original blacklist entry for < 5.1, > since that was a different bug. It's probably safe to assume the bug has always existed on adreno 530 devices since launch, and there are plenty of Android 6.0 devices with adreno 530. The original question was what's the product impact of disabling multisample?
On 2016/07/22 18:30:17, boliu wrote: > On 2016/07/22 18:18:15, Stephen White wrote: > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > gpu/config/gpu_driver_bug_list_json.cc:1135: // Mean to cover N only. NMR1 has > > this fixed. > > If the bug is only in N, could we leave MSAA enabled for L and M? There are a > > growing number of WebView apps that depend on decent GPU path rendering > > performance, e.g. mapping apps. (This was one of the motivating use cases for > > the MSAA tessellating path renderer in the first place.) > > > > Note also that you may need to duplicate the original blacklist entry for < > 5.1, > > since that was a different bug. > > It's probably safe to assume the bug has always existed on adreno 530 devices > since launch, and there are plenty of Android 6.0 devices with adreno 530. Could we create a new entry for 530 or 5XX, then? I've done a fair bit of testing on Adreno 420 and 430 (Nexus 6 and 6P) and haven't noticed any issues in L or M. > The original question was what's the product impact of disabling multisample? It will negatively affect the performance of GPU rasterization, and negatively affect the quality pages rendered in WebGL. For the former, it affects pages containing concave paths. Without MSAA, these drop to software rasterization. This is currently about 2% of pages in total, but will increase as a percentage as we roll out GPU rasterization more broadly on Android. For an example of the latter, see acko.net.
On 2016/07/22 18:59:33, Stephen White wrote: > On 2016/07/22 18:30:17, boliu wrote: > > On 2016/07/22 18:18:15, Stephen White wrote: > > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > > gpu/config/gpu_driver_bug_list_json.cc:1135: // Mean to cover N only. NMR1 > has > > > this fixed. > > > If the bug is only in N, could we leave MSAA enabled for L and M? There are > a > > > growing number of WebView apps that depend on decent GPU path rendering > > > performance, e.g. mapping apps. (This was one of the motivating use cases > for > > > the MSAA tessellating path renderer in the first place.) > > > > > > Note also that you may need to duplicate the original blacklist entry for < > > 5.1, > > > since that was a different bug. > > > > It's probably safe to assume the bug has always existed on adreno 530 devices > > since launch, and there are plenty of Android 6.0 devices with adreno 530. > > Could we create a new entry for 530 or 5XX, then? Sounds like two new entries, one for 5xx, and one for 4xx on 7.0 (becasue they broke it again). Unless there is a way to do "or" operation for version checks.. > I've done a fair bit of > testing on Adreno 420 and 430 (Nexus 6 and 6P) and haven't noticed any issues > in L or M. Honestly, qualcomm driver is so fragmented, I don't actually have high hopes that nexus device driver quality translates to the rest of the ecosystem. But at least they definitely confirmed they broke it in N for 6P. > > > The original question was what's the product impact of disabling multisample? > > It will negatively affect the performance of GPU rasterization, and negatively > affect the quality pages rendered in WebGL. For the former, it affects pages > containing concave paths. Without MSAA, these drop to software rasterization. > This is currently about 2% of pages in total, Is that 2% pages with gpu rasterization, or 2% with gpu rasterization + concave path? > but will increase as a percentage > as we roll out GPU rasterization more broadly on Android. For an example of the > latter, see http://acko.net.
On 2016/07/22 19:41:12, boliu wrote: > On 2016/07/22 18:59:33, Stephen White wrote: > > On 2016/07/22 18:30:17, boliu wrote: > > > On 2016/07/22 18:18:15, Stephen White wrote: > > > > > > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > > > gpu/config/gpu_driver_bug_list_json.cc:1135: // Mean to cover N only. NMR1 > > has > > > > this fixed. > > > > If the bug is only in N, could we leave MSAA enabled for L and M? There > are > > a > > > > growing number of WebView apps that depend on decent GPU path rendering > > > > performance, e.g. mapping apps. (This was one of the motivating use cases > > for > > > > the MSAA tessellating path renderer in the first place.) > > > > > > > > Note also that you may need to duplicate the original blacklist entry for > < > > > 5.1, > > > > since that was a different bug. > > > > > > It's probably safe to assume the bug has always existed on adreno 530 > devices > > > since launch, and there are plenty of Android 6.0 devices with adreno 530. > > > > Could we create a new entry for 530 or 5XX, then? > > Sounds like two new entries, one for 5xx, and one for 4xx on 7.0 (becasue they > broke it again). Unless there is a way to do "or" operation for version checks.. Also if we need to do complex changes like that, then I'm no longer comfortable merging back to m52, which no longer has any beta soak time. meh > > > I've done a fair bit of > > testing on Adreno 420 and 430 (Nexus 6 and 6P) and haven't noticed any issues > > in L or M. > > Honestly, qualcomm driver is so fragmented, I don't actually have high hopes > that nexus device driver quality translates to the rest of the ecosystem. > > But at least they confirmed they broke it in N for 5X/6P. > > > > > > The original question was what's the product impact of disabling > multisample? > > > > It will negatively affect the performance of GPU rasterization, and negatively > > > affect the quality pages rendered in WebGL. For the former, it affects pages > > containing concave paths. Without MSAA, these drop to software rasterization. > > This is currently about 2% of pages in total, > > Is that 2% pages with gpu rasterization, or 2% with gpu rasterization + concave > path? > > > but will increase as a percentage > > as we roll out GPU rasterization more broadly on Android. For an example of > the > > latter, see http://acko.net.
Ok... falling back to software raster is bad I guess, but I do want this for m52. So how about this, land PS1 as is, merge back to m53/52. Then write another CL that enables multisample for adreno 4xx on android 6, and merge that back to m53. (Or I can talk to TPM and only land PS1 on m52, which a bit less work) Thoughts?
On 2016/07/22 19:41:12, boliu wrote: > On 2016/07/22 18:59:33, Stephen White wrote: > > On 2016/07/22 18:30:17, boliu wrote: > > > On 2016/07/22 18:18:15, Stephen White wrote: > > > > > > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2176703002/diff/1/gpu/config/gpu_driver_bug_l... > > > > gpu/config/gpu_driver_bug_list_json.cc:1135: // Mean to cover N only. NMR1 > > has > > > > this fixed. > > > > If the bug is only in N, could we leave MSAA enabled for L and M? There > are > > a > > > > growing number of WebView apps that depend on decent GPU path rendering > > > > performance, e.g. mapping apps. (This was one of the motivating use cases > > for > > > > the MSAA tessellating path renderer in the first place.) > > > > > > > > Note also that you may need to duplicate the original blacklist entry for > < > > > 5.1, > > > > since that was a different bug. > > > > > > It's probably safe to assume the bug has always existed on adreno 530 > devices > > > since launch, and there are plenty of Android 6.0 devices with adreno 530. > > > > Could we create a new entry for 530 or 5XX, then? > > Sounds like two new entries, one for 5xx, and one for 4xx on 7.0 (becasue they > broke it again). Unless there is a way to do "or" operation for version checks.. > > > I've done a fair bit of > > testing on Adreno 420 and 430 (Nexus 6 and 6P) and haven't noticed any issues > > in L or M. > > Honestly, qualcomm driver is so fragmented, I don't actually have high hopes > that nexus device driver quality translates to the rest of the ecosystem. > > But at least they definitely confirmed they broke it in N for 6P. > > > > > > The original question was what's the product impact of disabling > multisample? > > > > It will negatively affect the performance of GPU rasterization, and negatively > > > affect the quality pages rendered in WebGL. For the former, it affects pages > > containing concave paths. Without MSAA, these drop to software rasterization. > > This is currently about 2% of pages in total, > > Is that 2% pages with gpu rasterization, or 2% with gpu rasterization + concave > path? 10% of pages with GPU raster. So 2% of all pages (.1 of pages with gpu raster * .2 gpu raster = .02). (Although those numbers are old, and may have changed with tweaks to the veto.) > > but will increase as a percentage > > as we roll out GPU rasterization more broadly on Android. For an example of > the > > latter, see http://acko.net.
On 2016/07/22 19:58:52, boliu wrote: > Ok... falling back to software raster is bad I guess, but I do want this for > m52. > > So how about this, land PS1 as is, merge back to m53/52. Then write another CL > that enables multisample for adreno 4xx on android 6, and merge that back to > m53. (Or I can talk to TPM and only land PS1 on m52, which a bit less work) > > Thoughts? What would be the risk if we only blacklist on 5xx, and leave 4xx alone for m52? The risk that N launches in the m52 stable period and 4xx is broken on N? (AFAIK it's fine on L & M).
On 2016/07/22 20:17:08, Stephen White wrote: > On 2016/07/22 19:58:52, boliu wrote: > > Ok... falling back to software raster is bad I guess, but I do want this for > > m52. > > > > So how about this, land PS1 as is, merge back to m53/52. Then write another CL > > that enables multisample for adreno 4xx on android 6, and merge that back to > > m53. (Or I can talk to TPM and only land PS1 on m52, which a bit less work) > > > > Thoughts? > > What would be the risk if we only blacklist on 5xx, and leave 4xx alone for > m52? The risk that N launches in the m52 stable period and 4xx is broken on N? > (AFAIK it's fine on L & M). Yep.
On 2016/07/22 20:23:49, boliu wrote: > On 2016/07/22 20:17:08, Stephen White wrote: > > On 2016/07/22 19:58:52, boliu wrote: > > > Ok... falling back to software raster is bad I guess, but I do want this for > > > m52. > > > > > > So how about this, land PS1 as is, merge back to m53/52. Then write another > CL > > > that enables multisample for adreno 4xx on android 6, and merge that back to > > > m53. (Or I can talk to TPM and only land PS1 on m52, which a bit less work) > > > > > > Thoughts? > > > > What would be the risk if we only blacklist on 5xx, and leave 4xx alone for > > m52? The risk that N launches in the m52 stable period and 4xx is broken on N? > > (AFAIK it's fine on L & M). > > Yep. And the downside to your plan is we'd have 4xx regress perf & quality on L & M in m52. *sigh*. OK, I guess I can live with that. Let's only merge this to 52 though, and merge the more refined blacklist to 53.
On 2016/07/22 20:34:38, Stephen White wrote: > On 2016/07/22 20:23:49, boliu wrote: > > On 2016/07/22 20:17:08, Stephen White wrote: > > > On 2016/07/22 19:58:52, boliu wrote: > > > > Ok... falling back to software raster is bad I guess, but I do want this > for > > > > m52. > > > > > > > > So how about this, land PS1 as is, merge back to m53/52. Then write > another > > CL > > > > that enables multisample for adreno 4xx on android 6, and merge that back > to > > > > m53. (Or I can talk to TPM and only land PS1 on m52, which a bit less > work) > > > > > > > > Thoughts? > > > > > > What would be the risk if we only blacklist on 5xx, and leave 4xx alone for > > > m52? The risk that N launches in the m52 stable period and 4xx is broken on > N? > > > (AFAIK it's fine on L & M). > > > > Yep. > > And the downside to your plan is we'd have 4xx regress perf & quality on L & M > in m52. *sigh*. OK, I guess I can live with that. Let's only merge this to 52 > though, and merge the more refined blacklist to 53. Good news. Talked to the TPM. TPM is willing to take two new rules for m52, as long as I get all the devices and test all the conditions. So two new rules it is then.
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
I haven't tested all the conditions yet, but new patch set up that does not disable on 4xx + android 6
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2176703002/diff/20001/gpu/config/gpu_driver_b... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2176703002/diff/20001/gpu/config/gpu_driver_b... gpu/config/gpu_driver_bug_list_json.cc:1892: "disable_multisampled_render_to_texture" since these are new rules, should we use disabled_extensions instead something like this? "disabled_extensions": [ "GL_EXT_multisampled_render_to_texture" ]
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
senorblanco@chromium.org changed reviewers: + zmo@chromium.org
Looks good (up to my knowledge of blacklist syntax). zmo@ and/or sievers@ should probably take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2176703002/diff/40001/gpu/config/gpu_driver_b... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2176703002/diff/40001/gpu/config/gpu_driver_b... gpu/config/gpu_driver_bug_list_json.cc:1903: "value": "7.0" BTW, does this mean 5xx is ok in 7.0? Or should this be <= 7.0.99?
https://codereview.chromium.org/2176703002/diff/40001/gpu/config/gpu_driver_b... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2176703002/diff/40001/gpu/config/gpu_driver_b... gpu/config/gpu_driver_bug_list_json.cc:1903: "value": "7.0" On 2016/07/22 21:39:04, Stephen White wrote: > BTW, does this mean 5xx is ok in 7.0? Yep > Or should this be <= 7.0.99?
On 2016/07/22 21:40:08, boliu wrote: > https://codereview.chromium.org/2176703002/diff/40001/gpu/config/gpu_driver_b... > File gpu/config/gpu_driver_bug_list_json.cc (right): > > https://codereview.chromium.org/2176703002/diff/40001/gpu/config/gpu_driver_b... > gpu/config/gpu_driver_bug_list_json.cc:1903: "value": "7.0" > On 2016/07/22 21:39:04, Stephen White wrote: > > BTW, does this mean 5xx is ok in 7.0? > > Yep > > > Or should this be <= 7.0.99? lgtm
Tested on: samsung s7, adreno 530, android 6.0, disabled nexus 5x, adreno 418, android 7.0, disabled nexus 6, adreno 420, android 6.0, enabled that covers all the cases I think?
lgtm
lgtm
The CQ bit was unchecked by boliu@chromium.org
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== gpu: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. Planning on merging back to m52 BUG=612474 CQ_INCLUDE_TRYBOTS=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: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. Planning on merging back to m52 BUG=612474 CQ_INCLUDE_TRYBOTS=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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== gpu: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. Planning on merging back to m52 BUG=612474 CQ_INCLUDE_TRYBOTS=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: Disable multisample on more adreno renderers We know for sure the initial N release for N5X/N6P will have multisample bugs. There is evidence as well that adreno 530 drivers in existing devices like samsung s7 is affected as well. Expand the workaround to cover these devices. Planning on merging back to m52 BUG=612474 CQ_INCLUDE_TRYBOTS=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 Committed: https://crrev.com/1243666b4f5c7c48312233e1f6e6fda46191b184 Cr-Commit-Position: refs/heads/master@{#407311} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1243666b4f5c7c48312233e1f6e6fda46191b184 Cr-Commit-Position: refs/heads/master@{#407311} |
