Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781993002:
Get the proper GPU Info in GpuProcessHost (Closed)
DescriptionGet the proper GPU Info in GpuProcessHost
As a follow up to https://codereview.chromium.org/2770933008/,
a few changes were made:
- When setting Swiftshader in the GpuDataManagerImplPrivate,
gpu_info_ and gpu_feature_info_ are cleared of previously
recorded info regarding the GPU to reflect SwiftShader
currently being in use
- Made sure GpuProcessHost uses the proper GPUInfo, meaning
when SwiftShader is in use, it should get its info from
GpuDataManagerImpl instead of setting new GPUInfo into
GpuDataManagerImpl. The functions calls would return right
away anyway and GpuDataManagerImpl and GpuProcessHost would
then be out of sync.
BUG=630728
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
Review-Url: https://codereview.chromium.org/2781993002
Cr-Commit-Position: refs/heads/master@{#461322}
Committed: https://chromium.googlesource.com/chromium/src/+/4b4a2c65e6ef9e8609a6a7b3bcd32c0438b5655a
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added tests #
Total comments: 2
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by sugoi@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...
sugoi@chromium.org changed reviewers: + jbauman@chromium.org, kbr@chromium.org, zmo@chromium.org
This is a little cleanup which may fix issues where the GpuProcessHost would have had the wrong GPUInfo, when Swiftshader is enabled.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:1287: status = gpu::kGpuFeatureStatusBlacklisted; Is this status update correct? Isn't WebGL still supported in this configuration? Can we unify this code with the code which ultimately makes the blacklisting decisions? Could you please add a test to e.g. gpu_process_integration_test.py which forces the browser to go down this path and verifies that the reported GPU info is correct? Ideally the test would fail before this CL, and pass with it, so we know it acts as a regression test. Tell me if you have any trouble constructing it. Of course, you can skip it on non-Windows platforms for now.
https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:1287: status = gpu::kGpuFeatureStatusBlacklisted; On 2017/03/29 17:29:22, Ken Russell wrote: > Is this status update correct? Isn't WebGL still supported in this > configuration? Yes, but the feature is now called "GPU_FEATURE_TYPE_ACCELERATED_WEBGL", so it's only on when the GPU is present. WebGL support is tested through GpuDataManagerImplPrivate::IsWebGLEnabled(), which checks for SwiftShader. > Can we unify this code with the code which ultimately makes the blacklisting > decisions? I don't fully understand why this class has blacklisted_features_ when there's also gpu_feature_info_.status_values. > Could you please add a test to e.g. gpu_process_integration_test.py which forces > the browser to go down this path and verifies that the reported GPU info is > correct? Ideally the test would fail before this CL, and pass with it, so we > know it acts as a regression test. Tell me if you have any trouble constructing > it. Of course, you can skip it on non-Windows platforms for now. Will do.
https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:1287: status = gpu::kGpuFeatureStatusBlacklisted; On 2017/03/29 17:39:47, sugoi1 wrote: > On 2017/03/29 17:29:22, Ken Russell wrote: > > Is this status update correct? Isn't WebGL still supported in this > > configuration? > > Yes, but the feature is now called "GPU_FEATURE_TYPE_ACCELERATED_WEBGL", so it's > only on when the GPU is present. WebGL support is tested through > GpuDataManagerImplPrivate::IsWebGLEnabled(), which checks for SwiftShader. In theory the webgl status should be hardware_accelerated/software_rendered/disabled, so you can verify it's the right status. > > > Can we unify this code with the code which ultimately makes the blacklisting > > decisions? > > I don't fully understand why this class has blacklisted_features_ when there's > also gpu_feature_info_.status_values. gpu_feature_info_ is future directly, but currently it's only used for rasterization, and webgl isn't part of that. > > > Could you please add a test to e.g. gpu_process_integration_test.py which > forces > > the browser to go down this path and verifies that the reported GPU info is > > correct? Ideally the test would fail before this CL, and pass with it, so we > > know it acts as a regression test. Tell me if you have any trouble > constructing > > it. Of course, you can skip it on non-Windows platforms for now. > > Will do.
Description was changed from ========== Get the proper GPU Info in GpuProcessHost As a follow up to https://codereview.chromium.org/2770933008/, a few changes were made: - When setting Swiftshader in the GpuDataManagerImplPrivate, gpu_info_ and gpu_feature_info_ are cleared of previously recorded info regarding the GPU to reflect SwiftShader currently being in use - Made sure GpuProcessHost uses the proper GPUInfo, meaning when SwiftShader is in use, it should get its info from GpuDataManagerImpl instead of setting new GPUInfo into GpuDataManagerImpl. The functions calls would return right away anyway and GpuDataManagerImpl and GpuProcessHost would then be out of sync. BUG=630728 ========== to ========== Get the proper GPU Info in GpuProcessHost As a follow up to https://codereview.chromium.org/2770933008/, a few changes were made: - When setting Swiftshader in the GpuDataManagerImplPrivate, gpu_info_ and gpu_feature_info_ are cleared of previously recorded info regarding the GPU to reflect SwiftShader currently being in use - Made sure GpuProcessHost uses the proper GPUInfo, meaning when SwiftShader is in use, it should get its info from GpuDataManagerImpl instead of setting new GPUInfo into GpuDataManagerImpl. The functions calls would return right away anyway and GpuDataManagerImpl and GpuProcessHost would then be out of sync. BUG=630728 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 ==========
The CQ bit was checked by sugoi@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...
On 2017/03/29 17:48:48, Zhenyao Mo wrote: > https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > https://codereview.chromium.org/2781993002/diff/1/content/browser/gpu/gpu_dat... > content/browser/gpu/gpu_data_manager_impl_private.cc:1287: status = > gpu::kGpuFeatureStatusBlacklisted; > On 2017/03/29 17:39:47, sugoi1 wrote: > > On 2017/03/29 17:29:22, Ken Russell wrote: > > > Is this status update correct? Isn't WebGL still supported in this > > > configuration? > > > > Yes, but the feature is now called "GPU_FEATURE_TYPE_ACCELERATED_WEBGL", so > it's > > only on when the GPU is present. WebGL support is tested through > > GpuDataManagerImplPrivate::IsWebGLEnabled(), which checks for SwiftShader. > > In theory the webgl status should be > hardware_accelerated/software_rendered/disabled, so you can verify it's the > right status. Right now, this can be tested using a combination of these function calls: GpuDataManagerImplPrivate::IsWebGLEnabled() GpuDataManagerImplPrivate::IsFeatureBlacklisted(GPU_FEATURE_TYPE_ACCELERATED_WEBGL) I don't know if we want these combined in a single function call with a 3-state output. > > > Can we unify this code with the code which ultimately makes the blacklisting > > > decisions? > > > > I don't fully understand why this class has blacklisted_features_ when there's > > also gpu_feature_info_.status_values. > > gpu_feature_info_ is future directly, but currently it's only used for > rasterization, and webgl isn't part of that. Ok. We may want to refactor this code at some point so that there's less confusion between IsFeatureBlacklisted and IsFeatureEnabled, but that's beyond the scope of this small cl. > > > Could you please add a test to e.g. gpu_process_integration_test.py which > > forces > > > the browser to go down this path and verifies that the reported GPU info is > > > correct? Ideally the test would fail before this CL, and pass with it, so we > > > know it acts as a regression test. Tell me if you have any trouble > > constructing > > > it. Of course, you can skip it on non-Windows platforms for now. > > > > Will do. Done.
LGTM https://codereview.chromium.org/2781993002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/gpu_process_integration_test.py (right): https://codereview.chromium.org/2781993002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/gpu_process_integration_test.py:567: self.fail("Wrong device ID. Expected 0, got " + hex(device.device_id)) Great. Did these new parts fail without your patch? Hoping the answer is yes, so they act as a regression test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2781993002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/gpu_process_integration_test.py (right): https://codereview.chromium.org/2781993002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/gpu_process_integration_test.py:567: self.fail("Wrong device ID. Expected 0, got " + hex(device.device_id)) On 2017/03/31 22:47:11, Ken Russell wrote: > Great. Did these new parts fail without your patch? Hoping the answer is yes, so > they act as a regression test. Yes, locally, vendor_id and device_id tests failed without the patch. I also checked that software_rendering failed if I inverted its state when setting use_swiftshader_.
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491048543578650,
"parent_rev": "c45b1c4ef18fa397013d1109dd9abe1f01543b3c", "commit_rev":
"4b4a2c65e6ef9e8609a6a7b3bcd32c0438b5655a"}
Message was sent while issue was closed.
Description was changed from ========== Get the proper GPU Info in GpuProcessHost As a follow up to https://codereview.chromium.org/2770933008/, a few changes were made: - When setting Swiftshader in the GpuDataManagerImplPrivate, gpu_info_ and gpu_feature_info_ are cleared of previously recorded info regarding the GPU to reflect SwiftShader currently being in use - Made sure GpuProcessHost uses the proper GPUInfo, meaning when SwiftShader is in use, it should get its info from GpuDataManagerImpl instead of setting new GPUInfo into GpuDataManagerImpl. The functions calls would return right away anyway and GpuDataManagerImpl and GpuProcessHost would then be out of sync. BUG=630728 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 ========== Get the proper GPU Info in GpuProcessHost As a follow up to https://codereview.chromium.org/2770933008/, a few changes were made: - When setting Swiftshader in the GpuDataManagerImplPrivate, gpu_info_ and gpu_feature_info_ are cleared of previously recorded info regarding the GPU to reflect SwiftShader currently being in use - Made sure GpuProcessHost uses the proper GPUInfo, meaning when SwiftShader is in use, it should get its info from GpuDataManagerImpl instead of setting new GPUInfo into GpuDataManagerImpl. The functions calls would return right away anyway and GpuDataManagerImpl and GpuProcessHost would then be out of sync. BUG=630728 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 Review-Url: https://codereview.chromium.org/2781993002 Cr-Commit-Position: refs/heads/master@{#461322} Committed: https://chromium.googlesource.com/chromium/src/+/4b4a2c65e6ef9e8609a6a7b3bcd3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4b4a2c65e6ef9e8609a6a7b3bcd3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
