[Android] Add workaround to unbind gpu memory buffer only on NVIDIA

[Android] Add workaround to unbind gpu memory buffer only on NVIDIA

For gralloc backed gpu memory buffer on Android, it is undefined whether
locking for read/write while the buffer is bound to a texture is
allowed.

On nvidia devices (eg 2012 nexus 7), lock without unbind will lead to
deadlocks in the driver (crbug.com/264096).

However on other nexus gpu vendors (img, arm, qualcomm), unbind is very
expensive since it flushes the gpu pipeline. And vendors have advised
that lock while bound is allowed and is the right solution to this
slowness, as long as the texture is eventually recycled or deleted.

Add a workaround for nvidia to only unbind in GLImage::DidUseTexImage.

BUG=

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

[boliu, 2013-11-12 00:26:35]

This is the minimum change needed get the desired behavior. But I think it might
make sense on android to skip ReleaseTexImage as well (except on nvidia) to be
consistent. But that will affect other platforms as well which I know nothing
about. I could reverse the workaroudn and make the condition "android &&
!nvidia".

Thoughts?

[reveman, 2013-11-12 16:39:40]

https://codereview.chromium.org/66033009/diff/30001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/66033009/diff/30001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: virtual void DidUseTexImage(bool release_tex_image);
Bind/release as part of Will/DidUseTexImage is an implementation detail. This
API should not expose that.

How about we add a "virtual void SetUnbindImageOnDidUse()" to the GLImage class
with a comment that makes it clear that this is for an Android work-around?
Consistent with GLContext::SetUnbindFboOnMakeCurrent.

[boliu, 2013-11-12 16:47:47]

https://codereview.chromium.org/66033009/diff/30001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/66033009/diff/30001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: virtual void DidUseTexImage(bool release_tex_image);
On 2013/11/12 16:39:42, David Reveman wrote:
> Bind/release as part of Will/DidUseTexImage is an implementation detail. This
> API should not expose that.
> 
> How about we add a "virtual void SetUnbindImageOnDidUse()" to the GLImage
class
> with a comment that makes it clear that this is for an Android work-around?
> Consistent with GLContext::SetUnbindFboOnMakeCurrent.

Ahh, that's why this felt so weird. Yes, SetUnbindImageOnDidUse sounds more
reaonsable.

One more question, do you care what the default for workaround should be for
non-android platforms?

[reveman, 2013-11-12 16:55:45]

On 2013/11/12 16:47:47, boliu wrote:
> https://codereview.chromium.org/66033009/diff/30001/ui/gl/gl_image.h
> File ui/gl/gl_image.h (right):
> 
> https://codereview.chromium.org/66033009/diff/30001/ui/gl/gl_image.h#newcode42
> ui/gl/gl_image.h:42: virtual void DidUseTexImage(bool release_tex_image);
> On 2013/11/12 16:39:42, David Reveman wrote:
> > Bind/release as part of Will/DidUseTexImage is an implementation detail.
This
> > API should not expose that.
> > 
> > How about we add a "virtual void SetUnbindImageOnDidUse()" to the GLImage
> class
> > with a comment that makes it clear that this is for an Android work-around?
> > Consistent with GLContext::SetUnbindFboOnMakeCurrent.
> 
> Ahh, that's why this felt so weird. Yes, SetUnbindImageOnDidUse sounds more
> reaonsable.
> 
> One more question, do you care what the default for workaround should be for
> non-android platforms?

Not really. Maybe SetSkipUnbindImageOnDidUse makes more sense if the standard
behavior is to unbind.

[boliu, 2013-11-12 18:42:39]

Changed to SetReleaseAfterUse, which is plumbed through ImageManager.

Kept SetReleaseAfterUse (instead of the reverse) since on android, nvidia really
is the special case.

PTAL

[kaanb, 2013-11-12 18:49:57]

lg2m with some suggestions

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:22: "version": "3.2",
I think 3.11 would be more appropriate, as this is a minor revision

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:684: "description": "NVIDIA driver
requires unbind gralloc from TexImage before lock",
how about: "NVIDIA driver requires unbinding a GpuMemoryBuffer from the texture
before mapping it for rasterization"

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_workaround_type.h:83:
GPU_OP(UNBIND_IMAGE_TARGET_TEXTURE_BEFORE_LOCK,            \
UNBIND_IMAGE_BEFORE_MAP is more concise

https://codereview.chromium.org/66033009/diff/100001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/66033009/diff/100001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: // Indicate that the DidUse should release tex image in
DidUse.
s/tex image/image/ here and elsewhere

[boliu, 2013-11-12 19:07:20]

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:22: "version": "3.2",
On 2013/11/12 18:49:57, kaanb wrote:
> I think 3.11 would be more appropriate, as this is a minor revision

Err, there is no fast rule here. I'll keep 3.2 since I think it's more
consistent.

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:684: "description": "NVIDIA driver
requires unbind gralloc from TexImage before lock",
On 2013/11/12 18:49:57, kaanb wrote:
> how about: "NVIDIA driver requires unbinding a GpuMemoryBuffer from the
texture
> before mapping it for rasterization"

s/for rasterization/to main memory/

Sound good?

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/66033009/diff/100001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_workaround_type.h:83:
GPU_OP(UNBIND_IMAGE_TARGET_TEXTURE_BEFORE_LOCK,            \
On 2013/11/12 18:49:57, kaanb wrote:
> UNBIND_IMAGE_BEFORE_MAP is more concise

RELEASE_IMAGE_AFTER_USE? Matches bocabulary in command buffer server side.

https://codereview.chromium.org/66033009/diff/100001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/66033009/diff/100001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: // Indicate that the DidUse should release tex image in
DidUse.
On 2013/11/12 18:49:57, kaanb wrote:
> s/tex image/image/ here and elsewhere

Done.

[kaanb, 2013-11-12 19:34:12]

lgtm

https://codereview.chromium.org/66033009/diff/240001/gpu/command_buffer/servi...
File gpu/command_buffer/service/image_manager.h (right):

https://codereview.chromium.org/66033009/diff/240001/gpu/command_buffer/servi...
gpu/command_buffer/service/image_manager.h:50: 
nit: extra space

[reveman, 2013-11-12 20:56:54]

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.cc
File ui/gl/gl_image.cc (right):

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.cc#newcode31
ui/gl/gl_image.cc:31: NOTIMPLEMENTED();
Only a small subset of derived classes should have to implement this as it's a
workaround. How about having a valid default implementation here instead of
forcing all derived classes to implement this?

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: // Indicate that the DidUse should release image in DidUse.
Please rephrase this comment. Maybe "Indicate that image should be released
after use"?

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image_egl.h#new...
ui/gl/gl_image_egl.h:34: bool need_unbind_after_use_;
release_after_use_ instead for consistency?

[boliu, 2013-11-12 21:06:50]

https://codereview.chromium.org/66033009/diff/240001/gpu/command_buffer/servi...
File gpu/command_buffer/service/image_manager.h (right):

https://codereview.chromium.org/66033009/diff/240001/gpu/command_buffer/servi...
gpu/command_buffer/service/image_manager.h:50: 
On 2013/11/12 19:34:13, kaanb wrote:
> nit: extra space

Done.

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.cc
File ui/gl/gl_image.cc (right):

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.cc#newcode31
ui/gl/gl_image.cc:31: NOTIMPLEMENTED();
On 2013/11/12 20:56:54, David Reveman wrote:
> Only a small subset of derived classes should have to implement this as it's a
> workaround. How about having a valid default implementation here instead of
> forcing all derived classes to implement this?

Done.

Left a comment here it's for workaround, so default to no-op.

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: // Indicate that the DidUse should release image in DidUse.
On 2013/11/12 20:56:54, David Reveman wrote:
> Please rephrase this comment. Maybe "Indicate that image should be released
> after use"?

Done.

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/66033009/diff/240001/ui/gl/gl_image_egl.h#new...
ui/gl/gl_image_egl.h:34: bool need_unbind_after_use_;
On 2013/11/12 20:56:54, David Reveman wrote:
> release_after_use_ instead for consistency?

Done.

[reveman, 2013-11-12 21:12:19]

ui/gl/gl_image* lgtm

[boliu, 2013-11-12 21:15:18]

+piman for owners

[piman, 2013-11-12 21:19:46]

lgtm

[commit-bot: I haz the power, 2013-11-12 23:39:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/66033009/310001

[commit-bot: I haz the power, 2013-11-13 02:07:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-13 02:30:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/66033009/310001

[commit-bot: I haz the power, 2013-11-13 04:00:52]

Change committed as 234740