Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1114503003:
Disable egl_khr_wait_sync on PowerVR Rogue G6 (Closed)
|
Created:
5 years, 7 months ago by Tobias Sargeant Modified:
5 years, 7 months ago CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable egl_khr_wait_sync on PowerVR Rogue G6
BUG=481955
Committed: https://crrev.com/a8834e5bce16bf6eefd01bc0eb07038aef01b79a
Cr-Commit-Position: refs/heads/master@{#327353}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add OS end range #Messages
Total messages: 19 (6 generated)
tobiasjs@chromium.org changed reviewers: + zmo@chromium.org
This is a P0 bug, and the fix needs to be merged back to M42 urgently. Thanks.
The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1114503003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
tobiasjs@chromium.org changed reviewers: + boliu@chromium.org
boliu@chromium.org changed reviewers: + sievers@chromium.org
Manually tested that this works already?
On 2015/04/28 16:11:03, boliu wrote: > Manually tested that this works already? Yes. Correctly disables egl_khr_wait_sync on Nexus Player.
https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1240: "op": ">=", Do we need this? At the same time, can we deprecate this for whatever future version? https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1246: "driver_version": { Uh, I'd double-check the logic and add a test, since the driver version parsing when added was specific to the Qualcomm-format only.
https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1240: "op": ">=", On 2015/04/28 17:20:39, sievers wrote: > Do we need this? > At the same time, can we deprecate this for whatever future version? Let's put in <= 5.1.99 or something like that? https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1246: "driver_version": { On 2015/04/28 17:20:39, sievers wrote: > Uh, I'd double-check the logic and add a test, since the driver version parsing > when added was specific to the Qualcomm-format only. Apparently it works. The whole GL_VERSION is something like: "OpenGL ES 3.1 build 1.4@3234138"
https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1240: "op": ">=", On 2015/04/28 17:24:02, boliu wrote: > On 2015/04/28 17:20:39, sievers wrote: > > Do we need this? > > At the same time, can we deprecate this for whatever future version? > > Let's put in <= 5.1.99 or something like that? Done. https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1246: "driver_version": { On 2015/04/28 17:24:02, boliu wrote: > On 2015/04/28 17:20:39, sievers wrote: > > Uh, I'd double-check the logic and add a test, since the driver version > parsing > > when added was specific to the Qualcomm-format only. > > Apparently it works. The whole GL_VERSION is something like: > > "OpenGL ES 3.1 build 1.4@3234138" Could we please address this in a follow-up CL, so that we can get to cherry-picking immediately? Manual testing shows that the test passes on Nexus Player. chrome://gpu shows that the driver version is correctly parsed as 1.4.
On 2015/04/28 17:24:02, boliu wrote: > https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... > File gpu/config/gpu_driver_bug_list_json.cc (right): > > https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... > gpu/config/gpu_driver_bug_list_json.cc:1240: "op": ">=", > On 2015/04/28 17:20:39, sievers wrote: > > Do we need this? > > At the same time, can we deprecate this for whatever future version? > > Let's put in <= 5.1.99 or something like that? > Yea whatever works. > https://codereview.chromium.org/1114503003/diff/1/gpu/config/gpu_driver_bug_l... > gpu/config/gpu_driver_bug_list_json.cc:1246: "driver_version": { > On 2015/04/28 17:20:39, sievers wrote: > > Uh, I'd double-check the logic and add a test, since the driver version > parsing > > when added was specific to the Qualcomm-format only. > > Apparently it works. The whole GL_VERSION is something like: > > "OpenGL ES 3.1 build 1.4@3234138" Yes but only by accident, see the comment in gpu_info_collector_android.cc: // Extract driver version from the second number in a string like: // "OpenGL ES 2.0 V@6.0 AU@ (CL@2946718)" Somebody could easily go and break your use case based on the comment. We can add some very simple unit tests to make sure it handles all the vendor specific formats we are interested in parsing in gpu_info_collector_unittest.cc. But we can do that as a follow-up.
lgtm
The CQ bit was checked by tobiasjs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1114503003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a8834e5bce16bf6eefd01bc0eb07038aef01b79a Cr-Commit-Position: refs/heads/master@{#327353} |
