Check if there is error on waiting for GL Fence.

Check if there is error on waiting for GL Fence.

DCHECK can catch a potential driver bug.

Committed: https://crrev.com/718fc603a04fe83d04947008b6a953b416fa314e
Cr-Commit-Position: refs/heads/master@{#302769}

[dshwang, 2014-10-31 13:56:00]

dongseong.hwang@intel.com changed reviewers:
+ sievers@chromium.org

[dshwang, 2014-10-31 13:56:01]

could you review this small CL.

[no sievers, 2014-10-31 18:49:32]

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

https://codereview.chromium.org/685003007/diff/1/ui/gl/gl_fence_arb.cc#newcode31
ui/gl/gl_fence_arb.cc:31: GLenum result = glClientWaitSync(sync_,
GL_SYNC_FLUSH_COMMANDS_BIT, 0);
GL_SYNC_FLUSH_COMMANDS_BIT doesn't work here because we might call this function
with a different context current than the one we created the fence in.
Also it's not needed since we call glFlush() in the constructor.

https://codereview.chromium.org/685003007/diff/1/ui/gl/gl_fence_arb.cc#newcode32
ui/gl/gl_fence_arb.cc:32: DCHECK_NE(GL_WAIT_FAILED, result);
The spec says it can only fail if you use an illegal flag, which we don't, or if
|sync| is not the name of a sync object which we handle in line 24.

So not sure why it would fail. Maybe some driver internal bug? I wonder if it
would also generate a GL error in that case which we should then clear here.

[dshwang, 2014-11-01 16:04:54]

On 2014/10/31 18:49:32, sievers wrote:
> https://codereview.chromium.org/685003007/diff/1/ui/gl/gl_fence_arb.cc
> File ui/gl/gl_fence_arb.cc (right):
> 
>
https://codereview.chromium.org/685003007/diff/1/ui/gl/gl_fence_arb.cc#newcode31
> ui/gl/gl_fence_arb.cc:31: GLenum result = glClientWaitSync(sync_,
> GL_SYNC_FLUSH_COMMANDS_BIT, 0);
> GL_SYNC_FLUSH_COMMANDS_BIT doesn't work here because we might call this
function
> with a different context current than the one we created the fence in.
> Also it's not needed since we call glFlush() in the constructor.

Thank you for explaining. I did misunderstand. rollback it.

> 
>
https://codereview.chromium.org/685003007/diff/1/ui/gl/gl_fence_arb.cc#newcode32
> ui/gl/gl_fence_arb.cc:32: DCHECK_NE(GL_WAIT_FAILED, result);
> The spec says it can only fail if you use an illegal flag, which we don't, or
if
> |sync| is not the name of a sync object which we handle in line 24.
> 
> So not sure why it would fail. Maybe some driver internal bug? I wonder if it
> would also generate a GL error in that case which we should then clear here.

Yes, I just concerned about some internal bug when I read code which check only
"!= GL_TIMEOUT_EXPIRED".
The spec indicates GL error is also genereated. The new patch set clears the GL
error.

[no sievers, 2014-11-03 19:00:31]

sievers@chromium.org changed reviewers:
+ reveman@chromium.org

[no sievers, 2014-11-03 19:05:03]

+reveman FYI, since this might uncover more driver bugs (in a good way I think)


Do you mind adding the similar logic to gl_fence_egl.cc?

You can do it for both eglClientWaitSyncKHR() and eglWaitSyncKHR() which also
returns a value, but in a slightly different way if I read the spec correctly:

eglClientWaitSyncKHR will return either
a) EGL_TIMEOUT_EXPIRED_KHR - shouldn't happen
b) EGL_CONDITION_SATISFIED_KHR
c) EGL_FALSE - EGL error is set

eglWaitSyncKHR will return either
a) EGL_TRUE
b) EGL_FALSE - EGl error is set

eglGetError() returns the error of the last function call, so you don't have to
loop but can simply call GetLastEGLErrorString() (egl_util.h).

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

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:33: NOTREACHED();
How about LOG(FATAL) instead (and removing NOTREACHED)?

I'm not sure if it's better to return |true| or |false| from this function if
there was an error. Above we return |true| if the fence creation failed, but the
current code does not signal the fence as complete if there was an error. So
maybe LOG(FATAL) would help so that we could find out about drivers that have
issues and then blacklist the extension since it will break functionality either
way (we either skip synchronization or never signal the fence).

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:34: LOG(ERROR) << "Fail to wait for legal GLFence. error
code:" << glGetError();
You have to loop over it until it stops returning errors, since glGetError() is
not guaranteed to return the accumulated errors in any specific order. I.e.

LOG(ERROR) << "Failed to wait for GLFence. Clearing errors:";
GLenum error;
while ((error = glGetError()) != GL_NO_ERROR) {
  LOG(ERROR) << "GLES2Util::GetStringEnum(error)";
}

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:36: return result != GL_TIMEOUT_EXPIRED;
If you return explicitly above, you can DCHECK_NE(result, GL_TIMEOUT_EXPIRED)
here, since we don't set a timeout.

[dshwang, 2014-11-04 13:55:08]

On 2014/11/03 19:05:03, sievers wrote:
> +reveman FYI, since this might uncover more driver bugs (in a good way I
think)

Thank you for big review for this small CL.

> Do you mind adding the similar logic to gl_fence_egl.cc?

Yes, done.

> You can do it for both eglClientWaitSyncKHR() and eglWaitSyncKHR() which also
> returns a value, but in a slightly different way if I read the spec correctly:
> 
> eglClientWaitSyncKHR will return either
> a) EGL_TIMEOUT_EXPIRED_KHR - shouldn't happen
> b) EGL_CONDITION_SATISFIED_KHR
> c) EGL_FALSE - EGL error is set
> 
> eglWaitSyncKHR will return either
> a) EGL_TRUE
> b) EGL_FALSE - EGl error is set
> 
> eglGetError() returns the error of the last function call, so you don't have
to
> loop but can simply call GetLastEGLErrorString() (egl_util.h).

Thank you for good explanation.

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

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:33: NOTREACHED();
On 2014/11/03 19:05:02, sievers wrote:
> How about LOG(FATAL) instead (and removing NOTREACHED)?
> 
> I'm not sure if it's better to return |true| or |false| from this function if
> there was an error. Above we return |true| if the fence creation failed, but
the
> current code does not signal the fence as complete if there was an error. So
> maybe LOG(FATAL) would help so that we could find out about drivers that have
> issues and then blacklist the extension since it will break functionality
either
> way (we either skip synchronization or never signal the fence).

Yes, I used FATAL, but I'm afraid of it can crash release build.

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:34: LOG(ERROR) << "Fail to wait for legal GLFence. error
code:" << glGetError();
On 2014/11/03 19:05:03, sievers wrote:
> You have to loop over it until it stops returning errors, since glGetError()
is
> not guaranteed to return the accumulated errors in any specific order. I.e.
> 
> LOG(ERROR) << "Failed to wait for GLFence. Clearing errors:";
> GLenum error;
> while ((error = glGetError()) != GL_NO_ERROR) {
>   LOG(ERROR) << "GLES2Util::GetStringEnum(error)";
> }

Most existing code don't do loop-checking. I think this code should clean up its
own error only, not leakage of others. If it doesn't clean up all accumulated
errors, it's not mistake of this. It's mistake of code leaking the error.

In addition, I could not use GetStringEnum because it's defined in gpu/

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:36: return result != GL_TIMEOUT_EXPIRED;
On 2014/11/03 19:05:02, sievers wrote:
> If you return explicitly above, you can DCHECK_NE(result, GL_TIMEOUT_EXPIRED)
> here, since we don't set a timeout.

even if we don't set a timeout, GL_TIMEOUT_EXPIRED can be fired, if the fence is
not signaled.

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:42: glClientWaitSync(sync_, GL_SYNC_FLUSH_COMMANDS_BIT,
GL_TIMEOUT_IGNORED);
Added DCHECK_NE(result, GL_TIMEOUT_EXPIRED) here

[dshwang, 2014-11-04 13:55:17]

Patchset #3 (id:40001) has been deleted

[no sievers, 2014-11-04 19:12:56]

lgtm

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

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:33: NOTREACHED();
On 2014/11/04 13:55:08, dshwang wrote:
> On 2014/11/03 19:05:02, sievers wrote:
> > How about LOG(FATAL) instead (and removing NOTREACHED)?
> > 
> > I'm not sure if it's better to return |true| or |false| from this function
if
> > there was an error. Above we return |true| if the fence creation failed, but
> the
> > current code does not signal the fence as complete if there was an error. So
> > maybe LOG(FATAL) would help so that we could find out about drivers that
have
> > issues and then blacklist the extension since it will break functionality
> either
> > way (we either skip synchronization or never signal the fence).
> 
> Yes, I used FATAL, but I'm afraid of it can crash release build.

That's good in a way, because we will get crash reports from which we will be
able to form a blacklist entry (based on vendor and driver version or so).
Better to not use the extension if it doesn't work.

https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
ui/gl/gl_fence_arb.cc:34: LOG(ERROR) << "Fail to wait for legal GLFence. error
code:" << glGetError();
On 2014/11/04 13:55:08, dshwang wrote:
> On 2014/11/03 19:05:03, sievers wrote:
> > You have to loop over it until it stops returning errors, since glGetError()
> is
> > not guaranteed to return the accumulated errors in any specific order. I.e.
> > 
> > LOG(ERROR) << "Failed to wait for GLFence. Clearing errors:";
> > GLenum error;
> > while ((error = glGetError()) != GL_NO_ERROR) {
> >   LOG(ERROR) << "GLES2Util::GetStringEnum(error)";
> > }
> 
> Most existing code don't do loop-checking. I think this code should clean up
its
> own error only, not leakage of others. If it doesn't clean up all accumulated
> errors, it's not mistake of this. It's mistake of code leaking the error.
> 
> In addition, I could not use GetStringEnum because it's defined in gpu/

The problem is that you cannot clear the previous error only. glGetError()
returns all error flags set "in unspecified order".

So you'd have to clear all errors before making a GL call here, if you only
wanted to call glGetError() once. But that seems worse.

[dshwang, 2014-11-04 19:35:20]

Patchset #4 (id:80001) has been deleted

[dshwang, 2014-11-04 19:37:13]

On 2014/11/04 19:12:56, sievers wrote:
> lgtm

Thank you for review.

>
https://codereview.chromium.org/685003007/diff/20001/ui/gl/gl_fence_arb.cc#ne...
> ui/gl/gl_fence_arb.cc:34: LOG(ERROR) << "Fail to wait for legal GLFence. error
> code:" << glGetError();
> On 2014/11/04 13:55:08, dshwang wrote:
> > On 2014/11/03 19:05:03, sievers wrote:
> > > You have to loop over it until it stops returning errors, since
glGetError()
> > is
> > > not guaranteed to return the accumulated errors in any specific order.
I.e.
> > > 
> > > LOG(ERROR) << "Failed to wait for GLFence. Clearing errors:";
> > > GLenum error;
> > > while ((error = glGetError()) != GL_NO_ERROR) {
> > >   LOG(ERROR) << "GLES2Util::GetStringEnum(error)";
> > > }
> > 
> > Most existing code don't do loop-checking. I think this code should clean up
> its
> > own error only, not leakage of others. If it doesn't clean up all
accumulated
> > errors, it's not mistake of this. It's mistake of code leaking the error.
> > 
> > In addition, I could not use GetStringEnum because it's defined in gpu/
> 
> The problem is that you cannot clear the previous error only. glGetError()
> returns all error flags set "in unspecified order".
> 
> So you'd have to clear all errors before making a GL call here, if you only
> wanted to call glGetError() once. But that seems worse.

Ok, new patch set clear all errors. see below.

https://codereview.chromium.org/685003007/diff/100001/ui/gl/gl_fence_arb.cc
File ui/gl/gl_fence_arb.cc (right):

https://codereview.chromium.org/685003007/diff/100001/ui/gl/gl_fence_arb.cc#n...
ui/gl/gl_fence_arb.cc:23: }
Now it clears all gl errors.

https://codereview.chromium.org/685003007/diff/100001/ui/gl/gl_fence_arb.cc#n...
ui/gl/gl_fence_arb.cc:48: LOG(FATAL) << "Failed to wait for GLFence. error
code:" << GetGLErrors();
it reports like "Failed .... error code:0x123 0x234 0x345"

[dshwang, 2014-11-05 07:57:43]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-11-05 07:58:38]

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

[commit-bot: I haz the power, 2014-11-05 08:59:49]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-11-05 09:00:30]

Patchset 4 (id:??) landed as
https://crrev.com/718fc603a04fe83d04947008b6a953b416fa314e
Cr-Commit-Position: refs/heads/master@{#302769}