gpu: Workaround Adreno 420 bug with deleting current FBO.

gpu: Workaround Adreno 420 bug with deleting current FBO.

A bug is causing corrupt / missing tiles when we delete an FBO
immediately after drawing to it.

A workaround is to glFlush() before deleting the FBO, or to
unbind any attachments before deleting the FBO.  Opting for the latter
as it has a lower overhead.

BUG=457027
BUG=459557
Committed: https://crrev.com/7d08fee567ae107fc522b88a7b0ffb5ccba62a06
Cr-Commit-Position: refs/heads/master@{#316867}

[vmiura, 2015-02-10 02:33:41]

vmiura@chromium.org changed reviewers:
+ epenner@chromium.org, sievers@chromium.org

[vmiura, 2015-02-10 02:33:42]

PTAL.

[vmiura, 2015-02-10 02:46:06]

Patchset #1 (id:1) has been deleted

[no sievers, 2015-02-10 19:11:33]

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

https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.cc:638:
glFramebufferRenderbufferEXT(target, attachment_point, GL_RENDERBUFFER, 0);
What if the attachment is a texture?
Also, can you call AttachTexture/Renderbuffer(it->first, ...) like above?

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:22: "version": "7.15",
bump version

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:1143: "type": "android"
Can we deprecate this for the future in the hope that it gets fixed?
You could put version <= 5.0.1 or something.

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_workaround_type.h:105:
GPU_OP(UNBIND_ATTACHMENTS_ON_BOUND_RENDER_FBO_DELETE,      \
nit: put before line 103 :)

[boliu, 2015-02-10 21:03:56]

verified that fix works for webview

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:1143: "type": "android"
On 2015/02/10 19:11:33, sievers wrote:
> Can we deprecate this for the future in the hope that it gets fixed?
> You could put version <= 5.0.1 or something.

Ehh...currently that check would be >= 5.0.2 as this bug is a regression in a
future version :(

[boliu, 2015-02-10 21:25:51]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2015-02-10 21:25:52]

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:1143: "type": "android"
On 2015/02/10 21:03:56, boliu wrote:
> On 2015/02/10 19:11:33, sievers wrote:
> > Can we deprecate this for the future in the hope that it gets fixed?
> > You could put version <= 5.0.1 or something.
> 
> Ehh...currently that check would be >= 5.0.2 as this bug is a regression in a
> future version :(

Oops, should be:
"> 5.0.2"

[vmiura, 2015-02-11 05:07:43]

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

https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.cc:638:
glFramebufferRenderbufferEXT(target, attachment_point, GL_RENDERBUFFER, 0);
On 2015/02/10 19:11:33, sievers wrote:
> What if the attachment is a texture?

Isn't that OK?  This would replace either texture/renderbuffer attachment with
the default renderbuffer.

> Also, can you call AttachTexture/Renderbuffer(it->first, ...) like above?

Yes, I'll change it.

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:1143: "type": "android"
Yeah, it's actually a regression from > 5.0.2.

I think once we have an idea of what version ships, and what version it would be
fixed in then we can add the filter <= N case.

[vmiura, 2015-02-17 18:48:41]

PTAL

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

https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.cc:638:
glFramebufferRenderbufferEXT(target, attachment_point, GL_RENDERBUFFER, 0);
> Also, can you call AttachTexture/Renderbuffer(it->first, ...) like above?

Done.

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:22: "version": "7.15",
On 2015/02/10 19:11:33, sievers-OOOuntilFeb17 wrote:
> bump version

Done.

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

https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_workaround_type.h:105:
GPU_OP(UNBIND_ATTACHMENTS_ON_BOUND_RENDER_FBO_DELETE,      \
On 2015/02/10 19:11:33, sievers-OOOuntilFeb17 wrote:
> nit: put before line 103 :)

Done.

[no sievers, 2015-02-17 21:18:41]

On 2015/02/17 18:48:41, vmiura wrote:
> PTAL
> 
>
https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/framebuffer_manager.cc (right):
> 
>
https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/framebuffer_manager.cc:638:
> glFramebufferRenderbufferEXT(target, attachment_point, GL_RENDERBUFFER, 0);
> > Also, can you call AttachTexture/Renderbuffer(it->first, ...) like above?
> 
> Done.
> 
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> File gpu/config/gpu_driver_bug_list_json.cc (right):
> 
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> gpu/config/gpu_driver_bug_list_json.cc:22: "version": "7.15",
> On 2015/02/10 19:11:33, sievers-OOOuntilFeb17 wrote:
> > bump version
> 
> Done.
> 
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> File gpu/config/gpu_driver_bug_workaround_type.h (right):
> 
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> gpu/config/gpu_driver_bug_workaround_type.h:105:
> GPU_OP(UNBIND_ATTACHMENTS_ON_BOUND_RENDER_FBO_DELETE,      \
> On 2015/02/10 19:11:33, sievers-OOOuntilFeb17 wrote:
> > nit: put before line 103 :)
> 
> Done.

Did you update the latest patch?

[vmiura, 2015-02-17 21:44:33]

On 2015/02/17 21:18:41, sievers wrote:
> On 2015/02/17 18:48:41, vmiura wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/framebuffer_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/896303004/diff/20001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/framebuffer_manager.cc:638:
> > glFramebufferRenderbufferEXT(target, attachment_point, GL_RENDERBUFFER, 0);
> > > Also, can you call AttachTexture/Renderbuffer(it->first, ...) like above?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> > File gpu/config/gpu_driver_bug_list_json.cc (right):
> > 
> >
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> > gpu/config/gpu_driver_bug_list_json.cc:22: "version": "7.15",
> > On 2015/02/10 19:11:33, sievers-OOOuntilFeb17 wrote:
> > > bump version
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> > File gpu/config/gpu_driver_bug_workaround_type.h (right):
> > 
> >
>
https://codereview.chromium.org/896303004/diff/20001/gpu/config/gpu_driver_bu...
> > gpu/config/gpu_driver_bug_workaround_type.h:105:
> > GPU_OP(UNBIND_ATTACHMENTS_ON_BOUND_RENDER_FBO_DELETE,      \
> > On 2015/02/10 19:11:33, sievers-OOOuntilFeb17 wrote:
> > > nit: put before line 103 :)
> > 
> > Done.
> 
> Did you update the latest patch?

My bad.  PTAL again.

[vmiura, 2015-02-17 21:48:48]

https://codereview.chromium.org/896303004/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/896303004/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.cc:638:
AttachRenderbuffer(it->first, NULL);
Hmm, I'm getting some GL errors after doing this so this is probably not the
right thing to do.

[no sievers, 2015-02-17 22:49:54]

lgtm

https://codereview.chromium.org/896303004/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/896303004/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.h:79: void
UnbindAttachments(GLenum target);
Can you rename it to something so that it doesn't accidentally get used in the
wrong way, and since it leaves the object in an inconsistent state (compared
with UnbindRenderbuffer/Texture for example)?

DoUnbindGLAttachmentsForWorkaround() or something :)

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

https://codereview.chromium.org/896303004/diff/80001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:1168: "type": "android"
So can we do this?
"version": {
  "op": "=",
  "value": "5.0.2"
}

Is there an internal bug yet to get it fixed?

[vmiura, 2015-02-18 00:42:45]

On 2015/02/17 22:49:54, sievers wrote:
> lgtm
> 
>
https://codereview.chromium.org/896303004/diff/80001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/framebuffer_manager.h (right):
> 
>
https://codereview.chromium.org/896303004/diff/80001/gpu/command_buffer/servi...
> gpu/command_buffer/service/framebuffer_manager.h:79: void
> UnbindAttachments(GLenum target);
> Can you rename it to something so that it doesn't accidentally get used in the
> wrong way, and since it leaves the object in an inconsistent state (compared
> with UnbindRenderbuffer/Texture for example)?
> 
> DoUnbindGLAttachmentsForWorkaround() or something :)
> 
>
https://codereview.chromium.org/896303004/diff/80001/gpu/config/gpu_driver_bu...
> File gpu/config/gpu_driver_bug_list_json.cc (right):
> 
>
https://codereview.chromium.org/896303004/diff/80001/gpu/config/gpu_driver_bu...
> gpu/config/gpu_driver_bug_list_json.cc:1168: "type": "android"
> So can we do this?
> "version": {
>   "op": "=",
>   "value": "5.0.2"
> }
> 
> Is there an internal bug yet to get it fixed?

b/17584380

[vmiura, 2015-02-18 17:40:33]

New patchsets have been uploaded after l-g-t-m from sievers@chromium.org

[vmiura, 2015-02-18 17:43:18]

https://codereview.chromium.org/896303004/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/896303004/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.h:79: void
UnbindAttachments(GLenum target);
On 2015/02/17 22:49:54, sievers wrote:
> Can you rename it to something so that it doesn't accidentally get used in the
> wrong way, and since it leaves the object in an inconsistent state (compared
> with UnbindRenderbuffer/Texture for example)?
> 
> DoUnbindGLAttachmentsForWorkaround() or something :)

Done.

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

https://codereview.chromium.org/896303004/diff/80001/gpu/config/gpu_driver_bu...
gpu/config/gpu_driver_bug_list_json.cc:1168: "type": "android"
On 2015/02/17 22:49:54, sievers wrote:
> So can we do this?
> "version": {
>   "op": "=",
>   "value": "5.0.2"
> }
> 
> Is there an internal bug yet to get it fixed?

Set to the following until we get a fix:

"version": {
  "op": ">",
  "value": "5.0.2"
}

[vmiura, 2015-02-18 17:43:46]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-18 17:44:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896303004/100001

[commit-bot: I haz the power, 2015-02-18 18:39:43]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-02-18 18:40:30]

Patchset 5 (id:??) landed as
https://crrev.com/7d08fee567ae107fc522b88a7b0ffb5ccba62a06
Cr-Commit-Position: refs/heads/master@{#316867}