Force GPU switch with CGLSetVirtualScreen only for compositor

Force GPU switch with CGLSetVirtualScreen only for compositor

Only compositor contexts are known to use only the subset of GL
that can be safely migrated between the iGPU and the dGPU. Mark
those contexts as safe to forcibly transition between the GPUs.

In particular, Intel GPUs support using a GL_STENCIL_INDEX8
attachment with no depth buffer, but NV GPUs don't. Skia will use
this FBO combo if it is listed as available.

If we use this combo on the iGPU and then forcibly transition the
GL context using this combo from the iGPU to the dGPU, corruption
occurs on NV, and a hang happens on AMD.

Further, if we cache complete FBO combos, then we will cache this
combo as valid when using the iGPU, and claim that it's valid when
using the dGPU, resulting in GL errors.

BUG=180876
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193302

[ccameron, 2013-04-06 01:42:12]

This was causing AMD-based Macbook Pros to hang in the driver -- only make the
call on NV-based systems (where it appears to be safe).

[jbauman, 2013-04-06 01:52:02]

https://codereview.chromium.org/13746002/diff/1/content/browser/gpu/gpu_drive...
File content/browser/gpu/gpu_driver_bug_list.json (right):

https://codereview.chromium.org/13746002/diff/1/content/browser/gpu/gpu_drive...
content/browser/gpu/gpu_driver_bug_list.json:61: //        
"use_current_program_after_successful_link".
Add the new value to this list.

https://codereview.chromium.org/13746002/diff/1/content/browser/gpu/gpu_drive...
content/browser/gpu/gpu_driver_bug_list.json:94: "version": "1.1",
Update the version number?

https://codereview.chromium.org/13746002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/13746002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:61: #if defined(OS_MACOSX)
Put this at the end of the list (with a blank line separating it from the
previous includes).

https://codereview.chromium.org/13746002/diff/1/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/13746002/diff/1/ui/gl/gl_context_cgl.cc#newco...
ui/gl/gl_context_cgl.cc:156: if (g_force_cgl_set_virtual_screen)
I'd recommend moving this check up to the beginning of the block (next to
g_support_renderer_switching), to avoid doing unnecessary
CGLDescribePixelFormats if it's disabled.

[ccameron, 2013-04-08 17:25:13]

Thanks!!

https://codereview.chromium.org/13746002/diff/1/content/browser/gpu/gpu_drive...
File content/browser/gpu/gpu_driver_bug_list.json (right):

https://codereview.chromium.org/13746002/diff/1/content/browser/gpu/gpu_drive...
content/browser/gpu/gpu_driver_bug_list.json:61: //        
"use_current_program_after_successful_link".
On 2013/04/06 01:52:02, jbauman wrote:
> Add the new value to this list.

Done.

https://codereview.chromium.org/13746002/diff/1/content/browser/gpu/gpu_drive...
content/browser/gpu/gpu_driver_bug_list.json:94: "version": "1.1",
On 2013/04/06 01:52:02, jbauman wrote:
> Update the version number?

Done.

https://codereview.chromium.org/13746002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/13746002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:61: #if defined(OS_MACOSX)
On 2013/04/06 01:52:02, jbauman wrote:
> Put this at the end of the list (with a blank line separating it from the
> previous includes).

Done. There were lots of instances of this in the #include list, I've merged
them at the bottom.

https://codereview.chromium.org/13746002/diff/1/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/13746002/diff/1/ui/gl/gl_context_cgl.cc#newco...
ui/gl/gl_context_cgl.cc:156: if (g_force_cgl_set_virtual_screen)
On 2013/04/06 01:52:02, jbauman wrote:
> I'd recommend moving this check up to the beginning of the block (next to
> g_support_renderer_switching), to avoid doing unnecessary
> CGLDescribePixelFormats if it's disabled.

I put the whole thing in an if-block. My initial inclination was to maintain
valid values for screen_ and renderer_id_ in case they were used elsewhere, but
this is indeed the only place where they are used.

[greggman, 2013-04-08 17:35:25]

Hey Mo, Can you comment on what the id means in the driver bug list?

https://codereview.chromium.org/13746002/diff/7001/content/browser/gpu/gpu_dr...
File content/browser/gpu/gpu_driver_bug_list.json (right):

https://codereview.chromium.org/13746002/diff/7001/content/browser/gpu/gpu_dr...
content/browser/gpu/gpu_driver_bug_list.json:324: "id": 18,
I don't know why these have ids :-(  But shouldn't this be 19?

[greggman, 2013-04-08 17:36:12]

Let's try that again with mo in the list.

He Mo, can you common on what the 'id' field means in the driver bug list json

[ccameron, 2013-04-08 17:44:46]

Thanks!

https://codereview.chromium.org/13746002/diff/7001/content/browser/gpu/gpu_dr...
File content/browser/gpu/gpu_driver_bug_list.json (right):

https://codereview.chromium.org/13746002/diff/7001/content/browser/gpu/gpu_dr...
content/browser/gpu/gpu_driver_bug_list.json:324: "id": 18,
On 2013/04/08 17:35:26, greggman wrote:
> I don't know why these have ids :-(  But shouldn't this be 19?

Oops -- disable_ext_draw_buffers snuck in between the 1st and 2nd versions of
this patch!!

[Zhenyao Mo, 2013-04-08 17:46:16]

https://codereview.chromium.org/13746002/diff/7001/content/browser/gpu/gpu_dr...
File content/browser/gpu/gpu_driver_bug_list.json (right):

https://codereview.chromium.org/13746002/diff/7001/content/browser/gpu/gpu_dr...
content/browser/gpu/gpu_driver_bug_list.json:324: "id": 18,
On 2013/04/08 17:35:26, greggman wrote:
> I don't know why these have ids :-(  But shouldn't this be 19?

IDs are for histograms in blacklist. And yes, duplicate IDs are not OK.  I
should probably dcheck in the code to make sure loading will fail.

https://codereview.chromium.org/13746002/diff/7001/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/13746002/diff/7001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_driver_bug_workaround_type.h:54:
force_cgl_set_virtual_screen)
Please insert this in the right alphabetic order.

[ccameron, 2013-04-08 17:50:15]

Thanks!

https://codereview.chromium.org/13746002/diff/7001/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/13746002/diff/7001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_driver_bug_workaround_type.h:54:
force_cgl_set_virtual_screen)
On 2013/04/08 17:46:16, Zhenyao Mo wrote:
> Please insert this in the right alphabetic order.

Done. I had Force/Flush backwards in the json too -- fixed that.

[Zhenyao Mo, 2013-04-08 17:54:38]

On 2013/04/08 17:50:15, ccameron1 wrote:
> Thanks!
> 
>
https://codereview.chromium.org/13746002/diff/7001/gpu/command_buffer/service...
> File gpu/command_buffer/service/gpu_driver_bug_workaround_type.h (right):
> 
>
https://codereview.chromium.org/13746002/diff/7001/gpu/command_buffer/service...
> gpu/command_buffer/service/gpu_driver_bug_workaround_type.h:54:
> force_cgl_set_virtual_screen)
> On 2013/04/08 17:46:16, Zhenyao Mo wrote:
> > Please insert this in the right alphabetic order.
> 
> Done. I had Force/Flush backwards in the json too -- fixed that.

LGTM

[commit-bot: I haz the power, 2013-04-08 18:20:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/13746002/18001

[commit-bot: I haz the power, 2013-04-08 18:20:04]

Presubmit check for 13746002-18001 failed and returned exit status 1.

INFO:root:Found 5 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    gpu/command_buffer/service/gles2_cmd_decoder.cc
    ui/gl/gl_context_cgl.cc
    gpu/command_buffer/service/gpu_driver_bug_workaround_type.h
    ui/gl/gl_context_cgl.h

[ccameron, 2013-04-08 20:38:33]

gman, does this LG for the ui/ and gpu/ paths?

[Ken Russell (switch to Gerrit), 2013-04-08 20:45:29]

LGTM

[ccameron, 2013-04-09 00:33:27]

New version of this patch. This time
- only transition compositor contexts
- disable the FBO completeness cache on OS X because it will cause canvas to
think that combinations are complete that actually aren't

This appears to work with iGPU->dGPU transition and dGPU->iGPU transition on AMD
and NV.

IMO the long-term solution is to expose only the intersection of the two GPUs'
capabilities to the command buffer's user.

[jbauman, 2013-04-09 00:47:13]

lgtm

https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc#n...
ui/gl/gl_context_cgl.cc:22: bool g_safe_to_force_gpu_switch = false;
Not necessary anymore.

[Ken Russell (switch to Gerrit), 2013-04-09 00:48:58]

Looks mostly good to me but a couple of concerns.

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/framebuffer_manager.cc:20: // graphics switching.
Is this change absolutely necessary to return to correct behavior, or can it be
separated out? My understanding is the addition of
GLContext::SetSafeToForceGpuSwitch is a sufficient fix. Only the minimal fix
should be backported to earlier releases.

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:63: #include
"base/mac/scoped_cftyperef.h"
This change is not necessary for this CL; please undo it.

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2580:
context_->SetSafeToForceGpuSwitch();
Does this work when virtual contexts are being used? I don't know how the
service side code works in this configuration. Is the same context passed in to
multiple decoder instances?

https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc#n...
ui/gl/gl_context_cgl.cc:22: bool g_safe_to_force_gpu_switch = false;
This is left over from a previous version of the patch.

[jbauman, 2013-04-09 00:57:36]

Yeah, we discovered that if you first create a canvas context on the
integrated GPU, open some webgl in a page, then create a canvas context for
another page (on the discrete GPU) then the framebuffer completeness cache
would cause it to attempt to use separate stencil on the discrete GPU.


On Mon, Apr 8, 2013 at 5:48 PM, <kbr@chromium.org> wrote:

> Looks mostly good to me but a couple of concerns.
>
>
>
> https://codereview.chromium.**org/13746002/diff/20001/gpu/**
>
command_buffer/service/**framebuffer_manager.cc<https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/service/framebuffer_manager.cc>
> File gpu/command_buffer/service/**framebuffer_manager.cc (right):
>
> https://codereview.chromium.**org/13746002/diff/20001/gpu/**
>
command_buffer/service/**framebuffer_manager.cc#**newcode20<https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/service/framebuffer_manager.cc#newcode20>
> gpu/command_buffer/service/**framebuffer_manager.cc:20: // graphics
> switching.
> Is this change absolutely necessary to return to correct behavior, or
> can it be separated out? My understanding is the addition of
> GLContext::**SetSafeToForceGpuSwitch is a sufficient fix. Only the minimal
> fix should be backported to earlier releases.
>
> https://codereview.chromium.**org/13746002/diff/20001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc<https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/service/gles2_cmd_decoder.cc>
> File gpu/command_buffer/service/**gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.**org/13746002/diff/20001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode63<https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode63>
> gpu/command_buffer/service/**gles2_cmd_decoder.cc:63: #include
> "base/mac/scoped_cftyperef.h"
> This change is not necessary for this CL; please undo it.
>
> https://codereview.chromium.**org/13746002/diff/20001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode2580<https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode2580>
> gpu/command_buffer/service/**gles2_cmd_decoder.cc:2580:
> context_->**SetSafeToForceGpuSwitch();
> Does this work when virtual contexts are being used? I don't know how
> the service side code works in this configuration. Is the same context
> passed in to multiple decoder instances?
>
>
> https://codereview.chromium.**org/13746002/diff/20001/ui/gl/**
>
gl_context_cgl.cc<https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc>
> File ui/gl/gl_context_cgl.cc (right):
>
> https://codereview.chromium.**org/13746002/diff/20001/ui/gl/**
>
gl_context_cgl.cc#newcode22<https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc#newcode22>
> ui/gl/gl_context_cgl.cc:22: bool g_safe_to_force_gpu_switch = false;
> This is left over from a previous version of the patch.
>
>
https://codereview.chromium.**org/13746002/<https://codereview.chromium.org/1...
>

[Ken Russell (switch to Gerrit), 2013-04-09 01:11:44]

On 2013/04/09 00:57:36, jbauman wrote:
> Yeah, we discovered that if you first create a canvas context on the
> integrated GPU, open some webgl in a page, then create a canvas context for
> another page (on the discrete GPU) then the framebuffer completeness cache
> would cause it to attempt to use separate stencil on the discrete GPU.

OK. In that case, thanks for testing both scenarios (these tests need to be
added to the GPU bots!), and LGTM modulo the other minor issues.

[ccameron, 2013-04-09 17:28:55]

Thanks!!

This should be simple to merge to M27 -- I'll probably want to do something more
complicated for tracking if a virtualized context's underlying context can be
forced to move at some point in the future.

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/framebuffer_manager.cc:20: // graphics switching.
On 2013/04/09 00:48:58, kbr wrote:
> Is this change absolutely necessary to return to correct behavior, or can it
be
> separated out? My understanding is the addition of
> GLContext::SetSafeToForceGpuSwitch is a sufficient fix. Only the minimal fix
> should be backported to earlier releases.

Yeah, this one is needed in M27 (John beat me to explaining this).

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:63: #include
"base/mac/scoped_cftyperef.h"
On 2013/04/09 00:48:58, kbr wrote:
> This change is not necessary for this CL; please undo it.

Done.

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2580:
context_->SetSafeToForceGpuSwitch();
On 2013/04/09 00:48:58, kbr wrote:
> Does this work when virtual contexts are being used? I don't know how the
> service side code works in this configuration. Is the same context passed in
to
> multiple decoder instances?

Good point -- I added support for virtual contexts. This will only work if all
virtualized contexts sharing a context agree about whether or not to allow
force-switching. It so-happens that they do ATM (because we only virtualize
compositor contexts on the Mac), but we may want to make this more stringent.

https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/13746002/diff/20001/ui/gl/gl_context_cgl.cc#n...
ui/gl/gl_context_cgl.cc:22: bool g_safe_to_force_gpu_switch = false;
On 2013/04/09 00:47:13, jbauman wrote:
> Not necessary anymore.

Done.

[Ken Russell (switch to Gerrit), 2013-04-09 17:42:03]

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/13746002/diff/20001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2580:
context_->SetSafeToForceGpuSwitch();
On 2013/04/09 17:28:55, ccameron1 wrote:
> On 2013/04/09 00:48:58, kbr wrote:
> > Does this work when virtual contexts are being used? I don't know how the
> > service side code works in this configuration. Is the same context passed in
> to
> > multiple decoder instances?
> 
> Good point -- I added support for virtual contexts. This will only work if all
> virtualized contexts sharing a context agree about whether or not to allow
> force-switching. It so-happens that they do ATM (because we only virtualize
> compositor contexts on the Mac), but we may want to make this more stringent.

A TODO is needed. It is clear that if we virtualize accelerated 2D canvas's
context in with the compositor's context at some point in the future, things
will break. It's OK to merge the fix in its current form to M27.

[ccameron, 2013-04-09 18:46:02]

Thanks -- added TODO. Need gman for gpu/OWNER.

IMO the post-M27 solution should be to be enforce that the capabilities exposed
(in particular, FBO completeness) by the iGPU and dGPU are the same (or at least
that the iGPU is a subset of the dGPU). At that point, virtualization, FBO
completeness caching, etc, all go back to working.

[ccameron, 2013-04-09 20:45:13]

Adding apatrick for OWNER review.

[apatrick_chromium, 2013-04-09 20:55:10]

lgtm

[ccameron, 2013-04-09 20:56:15]

Thanks!!

[commit-bot: I haz the power, 2013-04-09 20:57:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/13746002/36001

[commit-bot: I haz the power, 2013-04-09 22:21:54]

Retried try job too often on mac_rel for step(s) gpu_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-10 00:11:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/13746002/51001

[commit-bot: I haz the power, 2013-04-10 02:52:39]

Change committed as 193302