gpu: Add EGL fence support.

gpu: Add EGL fence support.

Adds fence support for EGL.

BUG=

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

[epenner, 2013-04-23 22:18:29]

We recently saw from Grace's trace that we can get 100ms latency when we are GPU
bound with expensive frames. This also showed we hit 3 frames of latency in GPU
buffering.

This patch uses fences to limit our frame latency. We were worried about fences
working, but it looks like in this scenario they work quite reliably (as seen
with the new "adb shell setprop debug.egl.traceGpuCompletion").

This will need to be enabled, but just putting the basic idea up for review.

[apatrick_chromium, 2013-04-23 22:32:56]

How about making an ImageTransportSurfaceAndroid that delegates to a
NativeViewGLSurfaceEGL via a GLSurfaceAdapter and do what Windows does in
PbufferImageTransportSurface::SwapBuffers to deschedule the command buffer until
the fence is signaled and without blocking the thread?

[epennerAtGoogle, 2013-04-23 23:30:38]

On 2013/04/23 22:32:56, apatrick_chromium wrote:
> How about making an ImageTransportSurfaceAndroid that delegates to a
> NativeViewGLSurfaceEGL via a GLSurfaceAdapter and do what Windows does in
> PbufferImageTransportSurface::SwapBuffers to deschedule the command buffer
until
> the fence is signaled and without blocking the thread?

I was initially thinking it wouldn't matter since the native SwapBuffers blocks
the real thread anyway... However, if we can deschedule before SwapBuffers
blocks, it will be a lot nicer. I'll look into that!

[epennerAtGoogle, 2013-04-24 00:17:59]

Oh... apatrick@, does the windows solution rely on polling to schedule the
thread again?  I like the idea of descheduling before we make a blocking swap,
but I'm a bit hesitant to reply on polling... Can you exaplain a bit how the
polling works?

[epennerAtGoogle, 2013-04-24 00:24:46]

'reply on polling' -> rely on polling
'expalain' -> explain
:)

[Sami, 2013-04-24 10:44:18]

I like the idea of doing this in the EGL surface. Maybe later the same fences
could be exposed to async uploads too.

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

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:376: EGLSyncKHR fence = eglCreateSyncKHR(GetDisplay(),
EGL_SYNC_FENCE_KHR, NULL);
I remember one driver where sync object creation was expensive. Might be worth
taking a quick trace to see how long this takes.

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:396: eglClientWaitSyncKHR(GetDisplay(), fence, flags,
time);
Call eglDestroySyncKHR after waiting here to avoid leaking the fence.

For extra paranoia please also add a DCHECK() for the result of the wait.

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h#newcode70
ui/gl/gl_surface_egl.h:70: virtual void SetMaximumFrameLatency(unsigned int
frames) OVERRIDE;
We spoke with Daniel that maybe something like SetMaximumPendingSwaps would be a
better name for this since that's also used elsewhere in the code and the
current name sounds like this gives a hard guarantee when it doesn't really know
what's happening downstream.

[no sievers, 2013-04-24 12:23:53]

see PbufferImageTransportSurface::SwapBuffers() calling helper_->DeferToFence().
It looks like the polling is already efficiently integrated with the gpu msg
loop. 

Sounded like mobile vendors might not have interrupt-driven fence completion, so
might be polling in the driver anyways.

You can add the EGL fence implementation in gl_fence.cc which already has
NV_fence and GL_ARB_sync (see GLFence::Create()).

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h#newcode70
ui/gl/gl_surface_egl.h:70: virtual void SetMaximumFrameLatency(unsigned int
frames) OVERRIDE;
On 2013/04/24 10:44:18, Sami wrote:
> We spoke with Daniel that maybe something like SetMaximumPendingSwaps would be
a
> better name for this since that's also used elsewhere in the code and the
> current name sounds like this gives a hard guarantee when it doesn't really
know
> what's happening downstream.

In gles2_implementation.cc we call it 'MaxSwapBuffers'.

[apatrick_chromium, 2013-04-24 18:04:40]

On 2013/04/24 00:17:59, epennerAtGoogle wrote:
> Oh... apatrick@, does the windows solution rely on polling to schedule the
> thread again?  I like the idea of descheduling before we make a blocking swap,
> but I'm a bit hesitant to reply on polling... Can you exaplain a bit how the
> polling works?

On Windows, while a command buffer is descheduled, it polls a GLFence every few
milliseconds using repeated delayed tasks. So it can run other tasks and handle
IPC messages in between polling the fence.

[epennerAtGoogle, 2013-04-24 19:33:38]

So, I like the idea of implementing it like windows which uses polling. Or
perhaps even using an extra thread that can block on completion and reply to
wake up the GPU thread. The fact that the driver might do polling doesn't make
our own polling any better.

In general I'm worried about polling causing too much drift due to the timer
delay other GPU tasks getting in front of the swap.

So I kind of feel inclined to do it here first, since it's similar to how we
already block on SwapBuffers, but long term it does sound better to deschedule.

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

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:376: EGLSyncKHR fence = eglCreateSyncKHR(GetDisplay(),
EGL_SYNC_FENCE_KHR, NULL);
On 2013/04/24 10:44:18, Sami wrote:
> I remember one driver where sync object creation was expensive. Might be worth
> taking a quick trace to see how long this takes.

Will do.

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:396: eglClientWaitSyncKHR(GetDisplay(), fence, flags,
time);
On 2013/04/24 10:44:18, Sami wrote:
> Call eglDestroySyncKHR after waiting here to avoid leaking the fence.
> 
> For extra paranoia please also add a DCHECK() for the result of the wait.

Oops! Thanks!

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h#newcode70
ui/gl/gl_surface_egl.h:70: virtual void SetMaximumFrameLatency(unsigned int
frames) OVERRIDE;
On 2013/04/24 12:23:53, Daniel Sievers wrote:
> On 2013/04/24 10:44:18, Sami wrote:
> > We spoke with Daniel that maybe something like SetMaximumPendingSwaps would
be
> a
> > better name for this since that's also used elsewhere in the code and the
> > current name sounds like this gives a hard guarantee when it doesn't really
> know
> > what's happening downstream.
> 
> In gles2_implementation.cc we call it 'MaxSwapBuffers'.  

Okay. I stole this name from D3D :P  I'll rename.

[brianderson, 2013-04-25 23:00:27]

On 2013/04/24 19:33:38, epennerAtGoogle wrote:
> So, I like the idea of implementing it like windows which uses polling. Or
> perhaps even using an extra thread that can block on completion and reply to
> wake up the GPU thread. The fact that the driver might do polling doesn't make
> our own polling any better.

+1 to an extra thread blocking on the sync; some drivers do implement interrupt
based sync points.

> 
> In general I'm worried about polling causing too much drift due to the timer
> delay other GPU tasks getting in front of the swap.
> 
> So I kind of feel inclined to do it here first, since it's similar to how we
> already block on SwapBuffers, but long term it does sound better to
deschedule.
> 
> https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
> ui/gl/gl_surface_egl.cc:376: EGLSyncKHR fence = eglCreateSyncKHR(GetDisplay(),
> EGL_SYNC_FENCE_KHR, NULL);
> On 2013/04/24 10:44:18, Sami wrote:
> > I remember one driver where sync object creation was expensive. Might be
worth
> > taking a quick trace to see how long this takes.
> 
> Will do.
> 
>
https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
> ui/gl/gl_surface_egl.cc:396: eglClientWaitSyncKHR(GetDisplay(), fence, flags,
> time);
> On 2013/04/24 10:44:18, Sami wrote:
> > Call eglDestroySyncKHR after waiting here to avoid leaking the fence.
> > 
> > For extra paranoia please also add a DCHECK() for the result of the wait.
> 
> Oops! Thanks!
> 
> https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h
> File ui/gl/gl_surface_egl.h (right):
> 
>
https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h#newcode70
> ui/gl/gl_surface_egl.h:70: virtual void SetMaximumFrameLatency(unsigned int
> frames) OVERRIDE;
> On 2013/04/24 12:23:53, Daniel Sievers wrote:
> > On 2013/04/24 10:44:18, Sami wrote:
> > > We spoke with Daniel that maybe something like SetMaximumPendingSwaps
would
> be
> > a
> > > better name for this since that's also used elsewhere in the code and the
> > > current name sounds like this gives a hard guarantee when it doesn't
really
> > know
> > > what's happening downstream.
> > 
> > In gles2_implementation.cc we call it 'MaxSwapBuffers'.  
> 
> Okay. I stole this name from D3D :P  I'll rename.

[no sievers, 2013-04-25 23:16:38]

On 2013/04/25 23:00:27, Brian Anderson wrote:
> On 2013/04/24 19:33:38, epennerAtGoogle wrote:
> > So, I like the idea of implementing it like windows which uses polling. Or
> > perhaps even using an extra thread that can block on completion and reply to
> > wake up the GPU thread. The fact that the driver might do polling doesn't
make
> > our own polling any better.
> 
> +1 to an extra thread blocking on the sync; some drivers do implement
interrupt
> based sync points.
> 

but please let's not do that on android. it will just expose driver bugs.


> > 
> > In general I'm worried about polling causing too much drift due to the timer
> > delay other GPU tasks getting in front of the swap.
> > 
> > So I kind of feel inclined to do it here first, since it's similar to how we
> > already block on SwapBuffers, but long term it does sound better to
> deschedule.
> > 
> > https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc
> > File ui/gl/gl_surface_egl.cc (right):
> > 
> >
>
https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
> > ui/gl/gl_surface_egl.cc:376: EGLSyncKHR fence =
eglCreateSyncKHR(GetDisplay(),
> > EGL_SYNC_FENCE_KHR, NULL);
> > On 2013/04/24 10:44:18, Sami wrote:
> > > I remember one driver where sync object creation was expensive. Might be
> worth
> > > taking a quick trace to see how long this takes.
> > 
> > Will do.
> > 
> >
>
https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.cc#newco...
> > ui/gl/gl_surface_egl.cc:396: eglClientWaitSyncKHR(GetDisplay(), fence,
flags,
> > time);
> > On 2013/04/24 10:44:18, Sami wrote:
> > > Call eglDestroySyncKHR after waiting here to avoid leaking the fence.
> > > 
> > > For extra paranoia please also add a DCHECK() for the result of the wait.
> > 
> > Oops! Thanks!
> > 
> > https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h
> > File ui/gl/gl_surface_egl.h (right):
> > 
> >
>
https://codereview.chromium.org/14358014/diff/1/ui/gl/gl_surface_egl.h#newcode70
> > ui/gl/gl_surface_egl.h:70: virtual void SetMaximumFrameLatency(unsigned int
> > frames) OVERRIDE;
> > On 2013/04/24 12:23:53, Daniel Sievers wrote:
> > > On 2013/04/24 10:44:18, Sami wrote:
> > > > We spoke with Daniel that maybe something like SetMaximumPendingSwaps
> would
> > be
> > > a
> > > > better name for this since that's also used elsewhere in the code and
the
> > > > current name sounds like this gives a hard guarantee when it doesn't
> really
> > > know
> > > > what's happening downstream.
> > > 
> > > In gles2_implementation.cc we call it 'MaxSwapBuffers'.  
> > 
> > Okay. I stole this name from D3D :P  I'll rename.

[epennerAtGoogle, 2013-04-25 23:46:19]

On 2013/04/25 23:16:38, Daniel Sievers wrote:
> On 2013/04/25 23:00:27, Brian Anderson wrote:
> > On 2013/04/24 19:33:38, epennerAtGoogle wrote:
> > > So, I like the idea of implementing it like windows which uses polling. Or
> > > perhaps even using an extra thread that can block on completion and reply
to
> > > wake up the GPU thread. The fact that the driver might do polling doesn't
> make
> > > our own polling any better.
> > 
> > +1 to an extra thread blocking on the sync; some drivers do implement
> interrupt
> > based sync points.
> > 
> 
> but please let's not do that on android. it will just expose driver bugs.

If we didn't do things because they caused driver bugs, we wouldn't do anything
at all, would we? :P

But more seriously, that's exactly what "adb shell setprop
debug.egl.traceGpuCompletion" does, so we can already see that it works.

Okay, if it's okay with all, I'm going to do implement both :) (No separate
thread yet though)

[epenner, 2013-06-18 22:06:33]

Ptal.

I got side tracked so this is just the fence implementation for EGL. I'll follow
up with more when I get a chance, in another CL.

[apatrick_chromium, 2013-06-18 22:15:01]

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

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode36
ui/gl/gl_fence.cc:36: NOTIMPLEMENTED();
glFinishFenceNV(fence_);

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode70
ui/gl/gl_fence.cc:70: NOTIMPLEMENTED();
glClientWaitSyncARB(sync_, SYNC_FLUSH_COMMANDS_BIT, TIMEOUT_IGNORED);

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode81
ui/gl/gl_fence.cc:81: class EglFenceSync : public gfx::GLFence {
We've been using "EGL" (all caps) everywhere else.

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode84
ui/gl/gl_fence.cc:84: sync_ = eglCreateSyncKHR(eglGetCurrentDisplay(),
nit: make first arg display_ and move the statement below up.

[epenner, 2013-06-18 22:37:25]

Thanks!

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

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode36
ui/gl/gl_fence.cc:36: NOTIMPLEMENTED();
On 2013/06/18 22:15:01, apatrick_chromium wrote:
> glFinishFenceNV(fence_);

Done.

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode70
ui/gl/gl_fence.cc:70: NOTIMPLEMENTED();
On 2013/06/18 22:15:01, apatrick_chromium wrote:
> glClientWaitSyncARB(sync_, SYNC_FLUSH_COMMANDS_BIT, TIMEOUT_IGNORED);

Done. Note there is no ARB, and added GL_ to constants.

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode81
ui/gl/gl_fence.cc:81: class EglFenceSync : public gfx::GLFence {
On 2013/06/18 22:15:01, apatrick_chromium wrote:
> We've been using "EGL" (all caps) everywhere else.

Done.

https://codereview.chromium.org/14358014/diff/20001/ui/gl/gl_fence.cc#newcode84
ui/gl/gl_fence.cc:84: sync_ = eglCreateSyncKHR(eglGetCurrentDisplay(),
On 2013/06/18 22:15:01, apatrick_chromium wrote:
> nit: make first arg display_ and move the statement below up.

Done.

[apatrick_chromium, 2013-06-18 22:42:01]

lgtm

[commit-bot: I haz the power, 2013-06-18 23:12:19]

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

[commit-bot: I haz the power, 2013-06-18 23:29:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
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-06-21 00:17:40]

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

[commit-bot: I haz the power, 2013-06-21 23:19:18]

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

[commit-bot: I haz the power, 2013-06-22 02:28:04]

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

[commit-bot: I haz the power, 2013-06-22 02:58:17]

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

[commit-bot: I haz the power, 2013-06-22 03:29:29]

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

[commit-bot: I haz the power, 2013-06-22 03:39:55]

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

[commit-bot: I haz the power, 2013-06-22 13:47:22]

Change committed as 208021