Allow switching to SwiftShader when GPU is blacklisted

Allow switching to SwiftShader when GPU is blacklisted

When a path to swiftshader is specified and the current GPU is blacklisted, tell the gpu process to use swiftshader to render it. If there already is a gpu process, kill it and make a new one that uses swiftshader

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109695

[Zhenyao Mo, 2011-11-05 00:39:19]

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:419:
flags.set_flags(GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas);
What about accelerated_compositing?  what about multisampling?

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:708: if (!GpuAccessAllowed()) {
Even if GpuAccessAllowed() returns true, but if webgl is blocked, you should
still enable software rendering.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:711: if (!swiftshader_path_.empty() &&
file_util::PathExists(eglpath))
PathExists should only be called on a thread that allows IO (not IO thread, but
say, FileThread).  So you need to make sure of that.  Maybe DCHECK thread in
RegisterSwiftShaderPath()?  Since that's the only caller of this function.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:718: return software_rendering_;
Accessor functions should match the variable name.

Say, the variable name is is_software_rendering_, the accessor function name
should be is_software_rendering().

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:607: software_rendering_ =
I feel like this logic should be in GpuDataManager, and we always check the
status from GpuDataManager instead of keep another cached status here.

See GpuDataManager::UserFlags().

[jbauman, 2011-11-05 00:49:34]

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:419:
flags.set_flags(GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas);
On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> What about accelerated_compositing?  what about multisampling?

We need accelerated compositing, or webgl will be even slower. Having support
for 3D css everywhere would also be nice. I'm not sure about multisampling, but
I might as well leave it on for now.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:708: if (!GpuAccessAllowed()) {
On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> Even if GpuAccessAllowed() returns true, but if webgl is blocked, you should
> still enable software rendering.

It's kind of annoying that this would slow down accelerated compositing on those
platforms, but you're right.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:711: if (!swiftshader_path_.empty() &&
file_util::PathExists(eglpath))
On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> PathExists should only be called on a thread that allows IO (not IO thread,
but
> say, FileThread).  So you need to make sure of that.  Maybe DCHECK thread in
> RegisterSwiftShaderPath()?  Since that's the only caller of this function.

Good point, I can remove this check.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:718: return software_rendering_;
On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> Accessor functions should match the variable name.
> 
> Say, the variable name is is_software_rendering_, the accessor function name
> should be is_software_rendering().

Ok.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:607: software_rendering_ =
On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> I feel like this logic should be in GpuDataManager, and we always check the
> status from GpuDataManager instead of keep another cached status here.
> 
> See GpuDataManager::UserFlags().

This is per-host, so we can check whether the gpu is blacklisted (and software
rendering should be turned on globally), but the gpu process is still using the
gpu and needs to be shut down.

[Zhenyao Mo, 2011-11-05 01:03:58]

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.cc (right):

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.cc:419:
flags.set_flags(GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas);
On 2011/11/05 00:49:34, jbauman wrote:
> On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> > What about accelerated_compositing?  what about multisampling?
> 
> We need accelerated compositing, or webgl will be even slower. Having support
> for 3D css everywhere would also be nice. I'm not sure about multisampling,
but
> I might as well leave it on for now.
> 

OK.  Multisampling on/off depends on performance.  If it turns out to be too
slow, we could turn it off later.

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/8480015/diff/2001/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:607: software_rendering_ =
On 2011/11/05 00:49:34, jbauman wrote:
> On 2011/11/05 00:39:19, Zhenyao Mo wrote:
> > I feel like this logic should be in GpuDataManager, and we always check the
> > status from GpuDataManager instead of keep another cached status here.
> > 
> > See GpuDataManager::UserFlags().
> 
> This is per-host, so we can check whether the gpu is blacklisted (and software
> rendering should be turned on globally), but the gpu process is still using
the
> gpu and needs to be shut down.

understood.  then you also need to make the accessor function name match the
variable in this class.

[jbauman, 2011-11-07 23:53:05]

Hopefully the issues are fixed. Also, the issue I was seeing, was due to the
first call to GpuDataManager::GetInstance() happening on a thread that wasn't a
file thread, causing the instance creation to behave oddly.

[Zhenyao Mo, 2011-11-10 18:34:33]

LGTM with style nits.  Please fix them before landing.

http://codereview.chromium.org/8480015/diff/7001/content/browser/gpu/gpu_data...
File content/browser/gpu/gpu_data_manager.h (right):

http://codereview.chromium.org/8480015/diff/7001/content/browser/gpu/gpu_data...
content/browser/gpu/gpu_data_manager.h:132: bool SoftwareRendering();
According to coding style, this should be software_rendering() instead of
SoftwareRendering().

http://codereview.chromium.org/8480015/diff/7001/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.h (right):

http://codereview.chromium.org/8480015/diff/7001/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.h:84: bool SoftwareRendering();
Here also: software_rendering().

[commit-bot: I haz the power, 2011-11-11 18:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/8480015/13001

[commit-bot: I haz the power, 2011-11-11 18:41:42]

Presubmit check for 8480015-13001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/gpu/gpu_data_manager.cc,content/browser/gpu/gpu_data_manager.h,content/browser/gpu/gpu_process_host.cc,content/browser/gpu/gpu_process_host.h

Presubmit checks took 1.1s to calculate.

[jbauman, 2011-11-11 19:15:56]

Al, could you review this?

[apatrick_chromium, 2011-11-11 19:33:04]

Cleanup discussed offline. LGTM.

[commit-bot: I haz the power, 2011-11-11 19:37:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/8480015/13001

[commit-bot: I haz the power, 2011-11-11 21:04:42]

Change committed as 109695