gpu: Fix Vivante's "hisilicon" GPUs

gpu: Fix Vivante's "hisilicon" GPUs

This fixes "Hisilicon" GPUs, which is an instance of a
Vivante's GPU design. This involves enabling virtual
contexts, since they don't support share-groups, and further
adds a work-around for switching surfaces. Without the work-
around the view surface "inherits" the size of the last
bound surface (which for Chrome tends to be a 1x1 pbuffer)
resulting in a black screen. The following steps "repair"
the view surface every time it is made current:

    - Make it current.
    - Bind the default frame-buffer.
    - Make it not current.
    - Destroy/recreate it from the native handle.
    - Make it current again.

NOTE: The "Recreate" function is added to surface because
the Destroy/Initialize may be intercepted or modified by
wrapper surface classes. Recreate is similar to resize,
which after being forwarded by a wrapper can call
Destroy/Initialize on the 'real' surface.

BUG=179250, 229643
NOTRY=true

No try, since it's Android only and already tested.

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

[epenner, 2013-03-28 05:23:09]

Ptal.

[no sievers, 2013-03-28 21:51:47]

On 2013/03/28 05:23:09, epenner wrote:
> Ptal.

lgtm. I think it's fine to enable virtual contexts on any Android device that it
works fine on (and hopefully doesn't affect performance negatively). Just be
sure that it's no red herring when it comes to the underlying bug :)

[epennerAtGoogle, 2013-03-28 21:56:53]

On 2013/03/28 21:51:47, Daniel Sievers wrote:
> On 2013/03/28 05:23:09, epenner wrote:
> > Ptal.
> 
> lgtm. I think it's fine to enable virtual contexts on any Android device that
it
> works fine on (and hopefully doesn't affect performance negatively). Just be
> sure that it's no red herring when it comes to the underlying bug :)

Yeah. I'll hold off until I have the whole picture. I think we'll want to enable
it, but let's wait and see what else comes up :)

[epenner, 2013-04-16 00:49:22]

PTAL. This makes this into a full work-around which fixes Huawie tablets with
"Hisilicon" GPUs. We still need to find the other vendors or Vivanti's GPUs, but
that can be added later.

[epenner, 2013-04-16 01:02:27]

https://codereview.chromium.org/13140006/diff/5001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/13140006/diff/5001/ui/gl/gl_context_egl.cc#ne...
ui/gl/gl_context_egl.cc:115: if (!RecreateSurfaceIfNeeded(surface))
Just one extra note on this. I did think about trying to do the recreate in the
surface itself, inside "OnMakeCurrent" called below. But there are two problems:
- First, the surface would need to call back into the context to make it
not-current, which is not nice given we are already in the context ::MakeCurrent
function.
- Second, the OnMakeCurrent may be called loosely after making a virtual context
current etc, whereas the real surface should be recreated only when absolutely
needed.

[no sievers, 2013-04-16 01:23:52]

Oh man, this driver bug is something :)

Can you make it check somewhere that the workaround is only used with virtual
contexts, where we optimize MakeCurrent() to be skipped for perf-critical paths
(i.e. when we are only switching back and forth between contexts)? It otherwise
certainly looks horrific to recreate the surface every time MakeCurrent()
happens.

Question:
If the very first context already gets created with the shared view surface,
does the workaround still need to be applied? This is only because we made it
current with an offscreen context first, right?

I'm just wondering if there should still be a bug and TODO to try to fix this
the right way, although it would be hard, by leaving it to the app to explicitly
share the view GLSurface and making sure offscreen contexts are not made current
until we have that surface available. (similarly a bit hard for tear-down of the
shared surface).

https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface_android.cc (right):

https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface_android.cc:36:
.makecurrent_recreates_surfaces) {
Hmm, this is flaky as it depends on some other context in the share group
(ContextGroup) having been created before we create this surface for another
context, because the order is (from when I checked last):
1) create surface
2) create context
3) make context current for the first time
4) init decoder (which queries gl_vendor and stuff and sets workaround bits)

And this function is 1).

Can you call SetRecreateOnMakeCurrent() in GLES2DecoderImpl::Initialize?

[no sievers, 2013-04-16 01:39:53]

@owners: FWIU, we are looking for some isolated workaround for m27 as this issue
affects many new low-cost devices, and has somehow bubbled up towards the top of
the list.

Eric can give more info on under what circumstances the bug exactly happens
(gl_tests maybe?) and we could also, in parallel, evaluate how we could do this
better later on trunk (but I think it will be more complicated and intrusive,
also see my comment below. I think what we would want to try there is more
explicitly share a surface, driven by the app, i.e. sharing a single GLSurface.)


On 2013/04/16 01:23:52, Daniel Sievers wrote:
> Oh man, this driver bug is something :)
> 
> Can you make it check somewhere that the workaround is only used with virtual
> contexts, where we optimize MakeCurrent() to be skipped for perf-critical
paths
> (i.e. when we are only switching back and forth between contexts)? It
otherwise
> certainly looks horrific to recreate the surface every time MakeCurrent()
> happens.
> 
> Question:
> If the very first context already gets created with the shared view surface,
> does the workaround still need to be applied? This is only because we made it
> current with an offscreen context first, right?
> 
> I'm just wondering if there should still be a bug and TODO to try to fix this
> the right way, although it would be hard, by leaving it to the app to
explicitly
> share the view GLSurface and making sure offscreen contexts are not made
current
> until we have that surface available. (similarly a bit hard for tear-down of
the
> shared surface).
> 
>
https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
> File content/common/gpu/image_transport_surface_android.cc (right):
> 
>
https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
> content/common/gpu/image_transport_surface_android.cc:36:
> .makecurrent_recreates_surfaces) {
> Hmm, this is flaky as it depends on some other context in the share group
> (ContextGroup) having been created before we create this surface for another
> context, because the order is (from when I checked last):
> 1) create surface
> 2) create context
> 3) make context current for the first time
> 4) init decoder (which queries gl_vendor and stuff and sets workaround bits)
> 
> And this function is 1).
> 
> Can you call SetRecreateOnMakeCurrent() in GLES2DecoderImpl::Initialize?

[epennerAtGoogle, 2013-04-16 02:03:14]

I'll add a DCHECK that virtual contexts are enabled when I enable the work
around.

Regarding "never calling MakeCurrent", we can discuss that on this patch which
reduces MakeCurrent until the one remaining case.
https://codereview.chromium.org/14069008/

This patch is orthogonal, in that it still allows us to change surfaces, which
is still nice functionality to have.

https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface_android.cc (right):

https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface_android.cc:36:
.makecurrent_recreates_surfaces) {
On 2013/04/16 01:23:52, Daniel Sievers wrote:
> Hmm, this is flaky as it depends on some other context in the share group
> (ContextGroup) having been created before we create this surface for another
> context, because the order is (from when I checked last):
> 1) create surface
> 2) create context
> 3) make context current for the first time
> 4) init decoder (which queries gl_vendor and stuff and sets workaround bits)
> 
> And this function is 1).
> 
> Can you call SetRecreateOnMakeCurrent() in GLES2DecoderImpl::Initialize?
> 
> 

I thought about that and it didn't appear to be a problem since the ContextGroup
is clearly created before all steps 1-4 above. Do you think the context group is
not initialized then?  I'll double check.

It's certainly nicer to call it here, since we know the type of surface and can
safely static_cast to the NativeViewGLSurfaceEGL.

[epennerAtGoogle, 2013-04-16 02:30:10]

https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface_android.cc (right):

https://codereview.chromium.org/13140006/diff/5001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface_android.cc:36:
.makecurrent_recreates_surfaces) {
You are right, the context-group might not be initialized, I'll do this in lower
in GpuCommandBufferStub::OnInitialize, since that is the function that created
the surface.

[epennerAtGoogle, 2013-04-16 02:55:42]

Addressed feedback. Ptal.

[epennerAtGoogle, 2013-04-16 19:07:35]

On 2013/04/16 02:55:42, epennerAtGoogle wrote:
> Addressed feedback. Ptal.

Okay, I found that I liked applying the work around in the original location in
image_transport_surface_android.cc, so I kept it there and added a TODO in
gpu_command_buffer_stub.cc, where the work around is applied again for the very
first surface.

Ptal.

[greggman, 2013-04-16 19:51:19]

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

https://codereview.chromium.org/13140006/diff/20001/ui/gl/gl_context_egl.cc#n...
ui/gl/gl_context_egl.cc:136: glBindFramebufferEXT(GL_FRAMEBUFFER, 0);
Sorry if this is a dumb question. It's getting harder and harder for me to
remember what order things are called. But....

Does this make the wrong FB bound? Do you need to restore the user's FB binding?

[epennerAtGoogle, 2013-04-16 19:54:24]

On 2013/04/16 19:51:19, greggman wrote:
> https://codereview.chromium.org/13140006/diff/20001/ui/gl/gl_context_egl.cc
> File ui/gl/gl_context_egl.cc (right):
> 
>
https://codereview.chromium.org/13140006/diff/20001/ui/gl/gl_context_egl.cc#n...
> ui/gl/gl_context_egl.cc:136: glBindFramebufferEXT(GL_FRAMEBUFFER, 0);
> Sorry if this is a dumb question. It's getting harder and harder for me to
> remember what order things are called. But....
> 
> Does this make the wrong FB bound? Do you need to restore the user's FB
binding?

I think it's okay given the virtual context restore happens after this, and this
is only enabled for virtual contexts. I could possibly query the FBO though and
restore it manually, since this happens rarely enough.

[epennerAtGoogle, 2013-04-17 00:30:04]

I added an FBO restore just for good measure. Let me know if there is any other
concerns.

There is a TODO for needing to check the work-around twice, but removing that
will require bigger changes to how/when the work-arounds are initialized. I may
also use the 'GpuInfo' instead in a follow-up, since that may be available
earlier.

[greggman, 2013-04-17 00:34:21]

lgtm

[epennerAtGoogle, 2013-04-17 01:11:02]

On 2013/04/17 00:34:21, greggman wrote:
> lgtm

Another other concerns Daniel, before I land this?

If we can reduce MakeCurrent calls to never use a pbuffer on startup, we might
be able to get rid of this. But this supports the very normal use case: 1x1
pbuffer followed by native surface.

[commit-bot: I haz the power, 2013-04-18 00:24:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/13140006/25001

[commit-bot: I haz the power, 2013-04-18 00:25:06]

Change committed as 194737