Skip to content

Commit

Permalink
On GPU process in Android, collect GPU info after init GL bindings.
Browse files Browse the repository at this point in the history
It doesn't make sense we open the lib and create a subset of bindings and use
our own code to do context related GPUInfo collection. We should just use the
same code on any other platforms.

We saw a surge of crashes related to this piece of code, my suspicion is the
current code path isn't doing the right thing. Let's see if we switch path and
order, situation changes.

BUG=787737,738414
TEST=bots
R=piman@chromium.org,kbr@chromium.org

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
Change-Id: If1e094faa38777c051d6e8fa559d133cce444c1a
Reviewed-on: https://chromium-review.googlesource.com/804659
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521190}
  • Loading branch information
zhenyao authored and Commit Bot committed Dec 2, 2017
1 parent b1abba9 commit f583865
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 22 deletions.
2 changes: 1 addition & 1 deletion gpu/config/gpu_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ const GPUInfo::GPUDevice& GPUInfo::active_gpu() const {
if (secondary_gpu.active)
return secondary_gpu;
}
DLOG(ERROR) << "No active GPU found, returning primary GPU.";
DLOG(WARNING) << "No active GPU found, returning primary GPU.";
return gpu;
}

Expand Down
12 changes: 12 additions & 0 deletions gpu/config/gpu_info_collector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@
#include "ui/gl/gl_version_info.h"
#include "ui/gl/init/gl_factory.h"

#if defined(OS_ANDROID)
#include "ui/gl/gl_surface_egl.h"
#endif // OS_ANDROID

#if defined(USE_X11)
#include "ui/gl/gl_visual_picker_glx.h"
#endif
Expand Down Expand Up @@ -159,13 +163,21 @@ CollectInfoResult CollectGraphicsInfoGL(GPUInfo* gpu_info) {
gpu_info->max_msaa_samples = base::IntToString(max_samples);
UMA_HISTOGRAM_SPARSE_SLOWLY("GPU.MaxMSAASampleCount", max_samples);

#if defined(OS_ANDROID)
gpu_info->can_support_threaded_texture_mailbox =
gl::GLSurfaceEGL::HasEGLExtension("EGL_KHR_fence_sync") &&
gl::GLSurfaceEGL::HasEGLExtension("EGL_KHR_image_base") &&
gl::GLSurfaceEGL::HasEGLExtension("EGL_KHR_gl_texture_2D_image") &&
gl::HasExtension(extension_set, "GL_OES_EGL_image");
#else
gl::GLWindowSystemBindingInfo window_system_binding_info;
if (gl::init::GetGLWindowSystemBindingInfo(&window_system_binding_info)) {
gpu_info->gl_ws_vendor = window_system_binding_info.vendor;
gpu_info->gl_ws_version = window_system_binding_info.version;
gpu_info->gl_ws_extensions = window_system_binding_info.extensions;
gpu_info->direct_rendering = window_system_binding_info.direct_rendering;
}
#endif // OS_ANDROID

bool supports_robustness =
gl::HasExtension(extension_set, "GL_EXT_robustness") ||
Expand Down
17 changes: 12 additions & 5 deletions gpu/config/gpu_info_collector_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,18 @@ gpu::CollectInfoResult CollectDriverInfo(gpu::GPUInfo* gpu_info) {
namespace gpu {

CollectInfoResult CollectContextGraphicsInfo(GPUInfo* gpu_info) {
/// TODO(tobiasjs) Check if CollectGraphicsInfo in gpu_main.cc
/// really only needs basic graphics info on all platforms, and if
/// so switch it to using that and make this the NOP that it really
/// should be, to avoid potential double collection of info.
return CollectBasicGraphicsInfo(gpu_info);
// When command buffer is compiled as a standalone library, the process might
// not have a Java environment.
if (base::android::IsVMInitialized()) {
gpu_info->machine_model_name =
base::android::BuildInfo::GetInstance()->model();
}

// At this point GL bindings have been initialized already.
CollectInfoResult result = CollectGraphicsInfoGL(gpu_info);
gpu_info->basic_info_state = result;
gpu_info->context_info_state = result;
return result;
}

CollectInfoResult CollectBasicGraphicsInfo(GPUInfo* gpu_info) {
Expand Down
23 changes: 7 additions & 16 deletions gpu/ipc/service/gpu_init.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,13 @@ GpuInit::~GpuInit() {
bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line,
const GpuPreferences& gpu_preferences) {
gpu_preferences_ = gpu_preferences;
#if defined(OS_ANDROID)
// Android doesn't have PCI vendor/device IDs, so collecting GL strings early
// is necessary.
CollectGraphicsInfo(&gpu_info_);
if (gpu_info_.context_info_state == gpu::kCollectInfoFatalFailure)
return false;
#else
gpu_info_.in_process_gpu = false;
#if !defined(OS_ANDROID)
// Get vendor_id, device_id, driver_version from browser process through
// commandline switches.
// TODO(zmo): Collect basic GPU info (without a context) here instead of
// passing from browser process.
GetGpuInfoFromCommandLine(*command_line, &gpu_info_);
#endif // OS_ANDROID

// Set keys for crash logging based on preliminary gpu info, in case we
// crash during feature collection.
Expand All @@ -125,7 +119,6 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line,
gpu_info_.driver_vendor == "NVIDIA" && !CanAccessNvidiaDeviceFile())
return false;
#endif
gpu_info_.in_process_gpu = false;

// Compute blacklist and driver bug workaround decisions based on basic GPU
// info.
Expand All @@ -139,6 +132,7 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line,
.status_values[GPU_FEATURE_TYPE_ACCELERATED_VIDEO_DECODE]) {
gpu_preferences_.disable_accelerated_video_decode = true;
}
#endif // OS_ANDROID

// In addition to disabling the watchdog if the command line switch is
// present, disable the watchdog on valgrind because the code is expected
Expand Down Expand Up @@ -222,10 +216,11 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line,
// multiple seconds to finish, which in turn cause the GPU process to crash.
// By skipping the following code on Mac, we don't really lose anything,
// because the basic GPU information is passed down from the host process.
#if !defined(OS_MACOSX) && !defined(OS_ANDROID)
#if !defined(OS_MACOSX)
CollectGraphicsInfo(&gpu_info_);
if (gpu_info_.context_info_state == gpu::kCollectInfoFatalFailure)
return false;
gpu::SetKeysForCrashLogging(gpu_info_);
gpu_feature_info_ = gpu::ComputeGpuFeatureInfo(gpu_info_, command_line);
if (kGpuFeatureStatusEnabled !=
gpu_feature_info_
Expand Down Expand Up @@ -290,13 +285,11 @@ void GpuInit::InitializeInProcess(base::CommandLine* command_line,
gpu_info_ = *gpu_info;
gpu_feature_info_ = *gpu_feature_info;
} else {
#if defined(OS_ANDROID)
gpu::CollectContextGraphicsInfo(&gpu_info_);
#else
#if !defined(OS_ANDROID)
// TODO(zmo): Collect basic GPU info here instead.
gpu::GetGpuInfoFromCommandLine(*command_line, &gpu_info_);
#endif
gpu_feature_info_ = gpu::ComputeGpuFeatureInfo(gpu_info_, command_line);
#endif
}
if (gpu::SwitchableGPUsSupported(gpu_info_, *command_line)) {
gpu::InitializeSwitchableGPUs(
Expand All @@ -308,10 +301,8 @@ void GpuInit::InitializeInProcess(base::CommandLine* command_line,
return;
}

#if !defined(OS_ANDROID)
gpu::CollectContextGraphicsInfo(&gpu_info_);
gpu_feature_info_ = gpu::ComputeGpuFeatureInfo(gpu_info_, command_line);
#endif
if (!gpu_feature_info_.disabled_extensions.empty()) {
gl::init::SetDisabledExtensionsPlatform(
gpu_feature_info_.disabled_extensions);
Expand Down

0 comments on commit f583865

Please sign in to comment.