Ozone: Avoid blocking in Swapbuffer Call.

Ozone: Avoid blocking in Swapbuffer Call.

Currently, we submit requests for page flip and wait for 
them to be handled in Main thread. This doesn't allow any 
other productive work to be done in GPU process while the 
pageflip events are being handled. This CL moves the wait to 
worker thread, so we don't block GPU main thread. 
SwapBufferAck is sent once the pageflip requests are handled.

[kalyank, 2014-12-05 15:17:54]

kalyan.kondapally@intel.com changed reviewers:
+ alexst@chromium.org, danakj@chromium.org, dnicoara@chromium.org,
marcheu@chromium.org

[kalyank, 2014-12-05 15:51:26]

Still need to check results on all hardware, looking for initial feedback.

[marcheu, 2014-12-05 17:05:44]

What I'd rather like to see is the page flip not block instead. triple buffering
tends to add latency

[alexst (slow to review), 2014-12-05 18:49:47]

On 2014/12/05 17:05:44, marcheu wrote:
> What I'd rather like to see is the page flip not block instead. triple
buffering
> tends to add latency

Thank you for the patch, Kalyan.

To elaborate on Steaphane's comment, there is a swapbuffers ack that goes from
the gpu process to the browser that is used to indicate that the front buffer is
not in use and that it's safe to paint into it again. The idea we were kicking
around was scheduling the pageflip, but instead of blocking gpu main thread, put
the wait on a separate thread and delay sending the ack until that wait
completes. The advantage here is that things like renderers and webgl which do
not paint into the front buffer can continue to get processed. Swap buffers ack
arrives same as it would've before, but we used the time we spent blocked doing
productive work.

[alexst (slow to review), 2014-12-05 18:50:08]

alexst@chromium.org changed reviewers:
+ achaulk@chromium.org

[alexst (slow to review), 2014-12-05 18:50:20]

+albert

[dnicoara, 2014-12-05 19:06:50]

I'm wondering if we can add that ack into VSyncProvider?

[achaulk, 2014-12-05 20:12:20]

https://codereview.chromium.org/781683005/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (left):

https://codereview.chromium.org/781683005/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:74:
available_surfaces_[i].damage.Union(damage);
This would have to stay to handle rare cases

https://codereview.chromium.org/781683005/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/781683005/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:117:
display_queue_surfaces_.pop_front();
I'm not sure what the point of this extra list (of at most one item?) is. It
seems to be the "current" surface - but that is already handled by the > 1 check
below, the last pushed in-flight surface is not available for re-use. This could
be accomplished by changing the check to > 2 to force always triple buffering

OTOH, it seems what you *really* want to accomplish here would be to trigger the
next frame when the current one is submitted and the previous one is flipped.

You would call into GpuBrowserCompositorOutputSurface::OnSwapBuffersComplete
from GpuSurfacelessBrowserCompositorOutputSurface::SwapBuffers IFF
in_flight_surfaces_.size == 1 (ie, one frame pending). If
BufferQueue::PageFlipComplete() reduces the size to 1, then you fire off a frame
too.

So, you would get this behaviour:
BindFramebuffer -> SwapBuffers (call for new frame)
-> PFC (does nothing, repeat)
-> BindFramebuffer (3rd FB bound) -> SwapBuffers (no new frame) -> PFC (for
first frame, call for new frame)

So, it will selectively triple buffer depending on the frame timings -
technically it will do so now anyway, if the compositor decides to draw a new
frame in advance of the PFC event; I'm not sure when of if this actually occurs
though. See CheckTripleBuffering in the unittest file for an example of a triple
buffering path

[kalyank, 2014-12-05 22:30:15]

On 2014/12/05 18:49:47, alexst wrote:
> On 2014/12/05 17:05:44, marcheu wrote:
> > What I'd rather like to see is the page flip not block instead. triple
> buffering
> > tends to add latency
> 
> Thank you for the patch, Kalyan.
> 
> To elaborate on Steaphane's comment, there is a swapbuffers ack that goes from
> the gpu process to the browser that is used to indicate that the front buffer
is
> not in use and that it's safe to paint into it again. The idea we were kicking
> around was scheduling the pageflip, but instead of blocking gpu main thread,
put
> the wait on a separate thread and delay sending the ack until that wait
> completes. The advantage here is that things like renderers and webgl which do
> not paint into the front buffer can continue to get processed. Swap buffers
ack
> arrives same as it would've before, but we used the time we spent blocked
doing
> productive work.

I started with that idea Alex, where in a worker thread I made sure drm fd_
handle is readable and 
then called drmhandleevent. I got stuck as to how I could cancel the
swapbufferAck call being sent 
after GLSurface handles partial swap or full swap and send it after actually
making sure the pageflip event
is done. PassThroughImageTransportSurface currently handles this. Any pointers ?

[kalyank, 2014-12-05 22:57:52]

On 2014/12/05 20:12:20, achaulk wrote:
>
https://codereview.chromium.org/781683005/diff/20001/content/browser/composit...
> File content/browser/compositor/buffer_queue.cc (left):

> below, the last pushed in-flight surface is not available for re-use. This
could
> be accomplished by changing the check to > 2 to force always triple buffering

Hmm, Ya. I guess I was overly complicating it.

> OTOH, it seems what you *really* want to accomplish here would be to trigger
the
> next frame when the current one is submitted and the previous one is flipped.

Ya and also make sure the submitted surface doesn't end up in Available list
before
Display is done with it. Sending SwapbufferAck after pageflip is done would
achieve 
that and dont have enforce tripple buffering.


> though. See CheckTripleBuffering in the unittest file for an example of a
triple
> buffering path

Will have a look.

[alexst (slow to review), 2014-12-05 23:07:19]

On 2014/12/05 22:30:15, kalyank wrote:
> On 2014/12/05 18:49:47, alexst wrote:
> > On 2014/12/05 17:05:44, marcheu wrote:
> > > What I'd rather like to see is the page flip not block instead. triple
> > buffering
> > > tends to add latency
> > 
> > Thank you for the patch, Kalyan.
> > 
> > To elaborate on Steaphane's comment, there is a swapbuffers ack that goes
from
> > the gpu process to the browser that is used to indicate that the front
buffer
> is
> > not in use and that it's safe to paint into it again. The idea we were
kicking
> > around was scheduling the pageflip, but instead of blocking gpu main thread,
> put
> > the wait on a separate thread and delay sending the ack until that wait
> > completes. The advantage here is that things like renderers and webgl which
do
> > not paint into the front buffer can continue to get processed. Swap buffers
> ack
> > arrives same as it would've before, but we used the time we spent blocked
> doing
> > productive work.
> 
> I started with that idea Alex, where in a worker thread I made sure drm fd_
> handle is readable and 
> then called drmhandleevent. I got stuck as to how I could cancel the
> swapbufferAck call being sent 
> after GLSurface handles partial swap or full swap and send it after actually
> making sure the pageflip event
> is done. PassThroughImageTransportSurface currently handles this. Any pointers
?

The idea I had in mind was along the lines of passing a SwapBuffersCompleted
callback to SwapBuffers in PassThroughImageTransportSurface. It gives you
ability to call it right away on some implementations, but delay it on others. I
haven't had a change to fully trace it through yet, I hope it's not insane.

[alexst (slow to review), 2014-12-10 02:21:51]

Kalyan, were you able to make any progress on this?

[kalyank, 2014-12-10 06:02:29]

On 2014/12/10 02:21:51, alexst wrote:
> Kalyan, were you able to make any progress on this?

Ya, I will update the patch by tomorrow.

[kalyank, 2014-12-10 19:00:11]

On 2014/12/10 02:21:51, alexst wrote:
> Kalyan, were you able to make any progress on this?

Can you PTAL ?

[brianderson, 2014-12-10 22:48:04]

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

[brianderson, 2014-12-10 22:48:05]

https://codereview.chromium.org/781683005/diff/60001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/781683005/diff/60001/cc/output/output_surface...
cc/output/output_surface.h:123: virtual void OnPageFlipComplete();
Does this need to be exposed for all OutputSurface types for this patch to work?

https://codereview.chromium.org/781683005/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/781683005/diff/60001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:550:
page_flip_completion_callback_.Run();
It would be really nice to pipe latency_info along this path too. Can you put a
TODO here?

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surface_factory.cc (right):

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surface_factory.cc:137:
scoped_ptr<PageFilpEventHandler>(new PageFilpEventHandler());
PageFilp -> PageFlip

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surfaceless.cc (right):

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surfaceless.cc:41:
flip_handler_->EnsurePreviousFlipHandled();
Looks like this is a blocking call. Is this on a critical thread?

If so, would it work for this to be a DCHECK that
flip_handler_->polling_.IsSignaled() is false and then make sure the rest of the
scheduling code doesn't hit this DCHECK.

I think Alex's suggestion to keep the swap ack associated with flip will be
enough to make sure we don't hit the DCHECK.

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/pa...
File ui/ozone/platform/dri/page_flip_event_handler.cc (right):

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/pa...
ui/ozone/platform/dri/page_flip_event_handler.cc:15: polling_(true, true) {
This should probably be polling_(true, false).

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/pa...
ui/ozone/platform/dri/page_flip_event_handler.cc:50: data->polling_.Reset();
This should be a DCHECK that IsSignaled is false. If it's not, there's likely a
race condition if EnsurePreviousFlipHandled is called before this task gets
scheduled.

[alexst (slow to review), 2014-12-10 23:19:25]

Some quick notes on the current code, but after chatting with Brian, who is our
scheduling expert, it seems like simply delaying the swapack until after
pageflip, while unblocking the gpu main would result in a simpler patch and
accomplish the same result.

Kalyan, was there a particular concern you had with that approach that
necessitated separating pageflip into a separate message? One of the byproducts
of this approach is that we may end up triple buffering.

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:42:
~GpuSurfacelessBrowserCompositorOutputSurface() {
Should the callback be reset here?

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:104:
page_flip_completion_callback_.callback());
Why not base::Bind(&BufferQueue::PageFlipComplete,
base::Unretained(output_surface_.get())) here instead?

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:42:
base::CancelableCallback<void()> page_flip_completion_callback_;
Any specific reason you chose CancelableCallback?

[jbauman, 2014-12-10 23:38:38]

On 2014/12/05 18:49:47, alexst wrote:
> On 2014/12/05 17:05:44, marcheu wrote:
> > What I'd rather like to see is the page flip not block instead. triple
> buffering
> > tends to add latency
> 
> Thank you for the patch, Kalyan.
> 
> To elaborate on Steaphane's comment, there is a swapbuffers ack that goes from
> the gpu process to the browser that is used to indicate that the front buffer
is
> not in use and that it's safe to paint into it again. The idea we were kicking
> around was scheduling the pageflip, but instead of blocking gpu main thread,
put
> the wait on a separate thread and delay sending the ack until that wait
> completes. The advantage here is that things like renderers and webgl which do
> not paint into the front buffer can continue to get processed. Swap buffers
ack
> arrives same as it would've before, but we used the time we spent blocked
doing
> productive work.

FYI, if GLES2DecoderImpl::ShouldDeferDraws() is set to true then the gpu process
will avoid doing draws that write to a GLSurface, but will allow all GL commands
that do other things to execute; it was designed for this sort of use-case. If
the presentation framebuffer isn't a GLSurface then it's trickier, but it could
be extended to work with that.

[kalyank, 2014-12-11 01:52:36]

On 2014/12/10 23:19:25, alexst wrote:
> Some quick notes on the current code, but after chatting with Brian, who is
our
> scheduling expert, it seems like simply delaying the swapack until after
> pageflip, while unblocking the gpu main would result in a simpler patch and
> accomplish the same result.
> 
> Kalyan, was there a particular concern you had with that approach that
> necessitated separating pageflip into a separate message? One of the
byproducts
> of this approach is that we may end up triple buffering.

I saw that in scheduler state machine, ShouldDraw checks to make sure
pending_swaps_ < max_pending_swaps_.
pending_swaps_ is only decremented if scheduler receives a swap buffer Ack (i.e.
DidSwapBuffersComplete) . Scheduler is notified 
only when OutputSurface receives the SwapBufferAck. As ShouldDraw returned
false, we don't try to prepare for a new frame.
Thus, I didn't see the value of sending SwapBufferAck after PageFlip is handled.
We will still wait till we get a swapbuffer Ack.
Perhaps, I am missing something ?

[kalyank, 2014-12-11 01:55:43]

On 2014/12/11 01:52:36, kalyank wrote:
> On 2014/12/10 23:19:25, alexst wrote:
> > Some quick notes on the current code, but after chatting with Brian, who is
> our
> > scheduling expert, it seems like simply delaying the swapack until after
> > pageflip, while unblocking the gpu main would result in a simpler patch and
> > accomplish the same result.
> > 
> > Kalyan, was there a particular concern you had with that approach that
> > necessitated separating pageflip into a separate message? One of the
> byproducts
> > of this approach is that we may end up triple buffering.
> 
> I saw that in scheduler state machine, ShouldDraw checks to make sure
> pending_swaps_ < max_pending_swaps_.
> pending_swaps_ is only decremented if scheduler receives a swap buffer Ack
(i.e.
> DidSwapBuffersComplete) . Scheduler is notified 
> only when OutputSurface receives the SwapBufferAck. As ShouldDraw returned
> false, we don't try to prepare for a new frame.
> Thus, I didn't see the value of sending SwapBufferAck after PageFlip is
handled.
> We will still wait till we get a swapbuffer Ack.

>>Thus, I didn't see the value of sending SwapBufferAck after PageFlip is
handled.
>>We will still wait till we get a swapbuffer Ack.
> Perhaps, I am missing something ?

I mean even though we unblock GPU main thread and wait in Worker thread, we
don't do any work
in compositor as scheduler is expecting the swapbufferAck before starting to
prepare for next frame.

[kalyank, 2014-12-11 03:24:26]

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surfaceless.cc (right):

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surfaceless.cc:41:
flip_handler_->EnsurePreviousFlipHandled();
On 2014/12/10 22:48:05, brianderson wrote:
> Looks like this is a blocking call. Is this on a critical thread?
> 
> If so, would it work for this to be a DCHECK that
> flip_handler_->polling_.IsSignaled() is false and then make sure the rest of
the
> scheduling code doesn't hit this DCHECK.
> 
> I think Alex's suggestion to keep the swap ack associated with flip will be
> enough to make sure we don't hit the DCHECK.

We swap, submit page flip requests and wait till the flip requests are handled.
The wait operation has been moved to worker thread now, making sure we don't
block the main thread unless really needed. In ideal case, by the next swap the
flip requests are handled and we issue new ones.How ever this might not be the
case depending on the work load. Issuing new flip requests before old ones are
handled is not well supported (Queuing Flips). This makes sure we handle this
case.

I am probably missing something here. I see scheduler has various criteria as to
when it can start drawing for next frame content. One of them is no of pending
swaps and max pending swaps allowed (capabilities of output surface). I see that
the max pending swaps allowed is one and thus scheduler doesn't start preparing
for next frame till it receives SwapBufferAck. Even though we move the wait
operation to a worker thread, Scheduler is left waiting for SwapBufferAck
request and thus not utilizing this wait time for some productive work.

[brianderson, 2014-12-11 09:27:44]

What's the timeline for getting this in?

Even though the Browser's compositor won't be drawing, we'll still have the
benefit that the GPU service won't block for swaps anymore, which will free the
GPU service up for GPU work from the Renderer.

If the actual flip takes a long time, we want to skip frames to avoid adding
latency in case we are GPU bound.

If we aren't actually GPU bound though and are still falling to 30fps, I can
imagine that could be due to cases where the swap just misses the vsync because
we started/finished drawing at a bad time.

Whatever we do, I would prefer to not have the swap ack return immediately
because we'll lose all information about how GPU bound we are.

Four more alternatives to help address the bad timing (if that is an issue), in
increasing order of preference:
  1) Try removing the swap throttling from
cc::SchedulerStateMachine::ShouldSendBeginMainFrame for the Browser's
compositor. This will let the Browser start creating a frame without necessarily
swapping it. (I've tried to remove this throttling from the Renderer, but kept
running into test flakiness. See https://codereview.chromium.org/222023010.
Doing this in the Browser only might not cause the same flakiness.)
  2) Switch max_pending_swaps_ to be 2 and add some logic to the cc::Scheduler
that attempts to throttle itself when the swap acks take more than 2 vsyncs to
return.
  3) Would it be possible to send the swap ack when the swap is "armed" and will
be flipped on the next vsync? (for example, by using a fence inserted before the
swap.) That way, the swap ack comes back late if we are actually GPU bound, but
not if we just had bad timing.
  4) A combination of all the above. Have a flip ack in addition to a swap ack
that makes it all the way to the cc::Scheduler. The swap ack comes back when the
swap is "armed", but the flip ack waits until the actual vsync. Throttle
SchedulerStateMachine::ShouldDraw() with a max_pending_flips_ of 1. Throttle
SchedulerStateMachine::ShouldSendBeginMainFrame with a max_pending_swaps_ of 2.

For 2 and 3, we'll need the logic you have to block the next swap request on the
previous flip. 1 and 4 should never block on a swap though.

[kalyank, 2014-12-11 12:55:59]

https://codereview.chromium.org/781683005/diff/60001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/781683005/diff/60001/cc/output/output_surface...
cc/output/output_surface.h:123: virtual void OnPageFlipComplete();
On 2014/12/10 22:48:05, brianderson wrote:
> Does this need to be exposed for all OutputSurface types for this patch to
work?

Not needed, removed.

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/781683005/diff/60001/content/browser/composit...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:104:
page_flip_completion_callback_.callback());
On 2014/12/10 23:19:25, alexst wrote:
> Why not base::Bind(&BufferQueue::PageFlipComplete,
> base::Unretained(output_surface_.get())) here instead?

Done.

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surface_factory.cc (right):

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surface_factory.cc:137:
scoped_ptr<PageFilpEventHandler>(new PageFilpEventHandler());
On 2014/12/10 22:48:05, brianderson wrote:
> PageFilp -> PageFlip

Done.

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surfaceless.cc (right):

https://codereview.chromium.org/781683005/diff/60001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surfaceless.cc:41:
flip_handler_->EnsurePreviousFlipHandled();
On 2014/12/11 03:24:26, kalyank wrote:
> On 2014/12/10 22:48:05, brianderson wrote:
> > Looks like this is a blocking call. Is this on a critical thread?
> > 
> > If so, would it work for this to be a DCHECK that
> > flip_handler_->polling_.IsSignaled() is false and then make sure the rest of
> the
> > scheduling code doesn't hit this DCHECK.
> > 
> > I think Alex's suggestion to keep the swap ack associated with flip will be
> > enough to make sure we don't hit the DCHECK.

Now, we don't block on Main thread at all. In GLSurfaceOzone, we track if we are
still handling the submitted page flip events, if so postpone the onSwapBuffer
call till we receive the callback from PageEventHandler. I see that after making
this change, when I open around 6-7 new windows or close and reopen windows
(Around 2-3 times), Page flip IPC call is not received in
CommandBufferProxyImpl, for some reason even though ImageTransportHelper sends
it. I am currently investigating that.

[kalyank, 2014-12-11 18:30:17]

On 2014/12/11 09:27:44, brianderson wrote:
Sorry missed this.

> What's the timeline for getting this in?

Preferably before M41.

> Even though the Browser's compositor won't be drawing, we'll still have the
> benefit that the GPU service won't block for swaps anymore, which will free
the
> GPU service up for GPU work from the Renderer.
> 
> If the actual flip takes a long time, we want to skip frames to avoid adding
> latency in case we are GPU bound.
> 
> If we aren't actually GPU bound though and are still falling to 30fps, I can
> imagine that could be due to cases where the swap just misses the vsync
because
> we started/finished drawing at a bad time.
> 
> Whatever we do, I would prefer to not have the swap ack return immediately
> because we'll lose all information about how GPU bound we are.

ok

> Four more alternatives to help address the bad timing (if that is an issue),
in
> increasing order of preference:
>   1) Try removing the swap throttling from
> cc::SchedulerStateMachine::ShouldSendBeginMainFrame for the Browser's
> compositor. This will let the Browser start creating a frame without
necessarily
> swapping it. (I've tried to remove this throttling from the Renderer, but kept
> running into test flakiness. See https://codereview.chromium.org/222023010.
> Doing this in the Browser only might not cause the same flakiness.)
>   2) Switch max_pending_swaps_ to be 2 and add some logic to the cc::Scheduler
> that attempts to throttle itself when the swap acks take more than 2 vsyncs to
> return.
>   3) Would it be possible to send the swap ack when the swap is "armed" and
will
> be flipped on the next vsync? (for example, by using a fence inserted before
the
> swap.) That way, the swap ack comes back late if we are actually GPU bound,
but
> not if we just had bad timing.
>   4) A combination of all the above. Have a flip ack in addition to a swap ack
> that makes it all the way to the cc::Scheduler. The swap ack comes back when
the
> swap is "armed", but the flip ack waits until the actual vsync. Throttle
> SchedulerStateMachine::ShouldDraw() with a max_pending_flips_ of 1. Throttle
> SchedulerStateMachine::ShouldSendBeginMainFrame with a max_pending_swaps_ of
2.
> 
> For 2 and 3, we'll need the logic you have to block the next swap request on
the
> previous flip. 1 and 4 should never block on a swap though.

K, will look into these options.

[marcheu, 2014-12-11 19:15:46]

On 2014/12/11 18:30:17, kalyank wrote:
> On 2014/12/11 09:27:44, brianderson wrote:
> Sorry missed this.
> 
> > What's the timeline for getting this in?
> 
> Preferably before M41.
> 
> > Even though the Browser's compositor won't be drawing, we'll still have the
> > benefit that the GPU service won't block for swaps anymore, which will free
> the
> > GPU service up for GPU work from the Renderer.
> > 
> > If the actual flip takes a long time, we want to skip frames to avoid adding
> > latency in case we are GPU bound.
> > 
> > If we aren't actually GPU bound though and are still falling to 30fps, I can
> > imagine that could be due to cases where the swap just misses the vsync
> because
> > we started/finished drawing at a bad time.
> > 
> > Whatever we do, I would prefer to not have the swap ack return immediately
> > because we'll lose all information about how GPU bound we are.
> 
> ok
> 
> > Four more alternatives to help address the bad timing (if that is an issue),
> in
> > increasing order of preference:
> >   1) Try removing the swap throttling from
> > cc::SchedulerStateMachine::ShouldSendBeginMainFrame for the Browser's
> > compositor. This will let the Browser start creating a frame without
> necessarily
> > swapping it. (I've tried to remove this throttling from the Renderer, but
kept
> > running into test flakiness. See https://codereview.chromium.org/222023010.
> > Doing this in the Browser only might not cause the same flakiness.)
> >   2) Switch max_pending_swaps_ to be 2 and add some logic to the
cc::Scheduler
> > that attempts to throttle itself when the swap acks take more than 2 vsyncs
to
> > return.
> >   3) Would it be possible to send the swap ack when the swap is "armed" and
> will
> > be flipped on the next vsync? (for example, by using a fence inserted before
> the
> > swap.) That way, the swap ack comes back late if we are actually GPU bound,
> but
> > not if we just had bad timing.
> >   4) A combination of all the above. Have a flip ack in addition to a swap
ack
> > that makes it all the way to the cc::Scheduler. The swap ack comes back when
> the
> > swap is "armed", but the flip ack waits until the actual vsync. Throttle
> > SchedulerStateMachine::ShouldDraw() with a max_pending_flips_ of 1. Throttle
> > SchedulerStateMachine::ShouldSendBeginMainFrame with a max_pending_swaps_ of
> 2.
> > 
> > For 2 and 3, we'll need the logic you have to block the next swap request on
> the
> > previous flip. 1 and 4 should never block on a swap though.
> 
> K, will look into these options.

For what it's worth, I don't think we need/want to do any of these. If you do
the following 2 things: don't block in swapbuffers and return the swap ack at
the right time (when the vblank happens) you should have something which is
equivalent to the current X-based case (you can schedule your frames correctly
based on the swap ack information, and you can also do texture uploads and webgl
until the swap ack comes back).

Or did you observe otherwise?

[brianderson, 2014-12-11 19:28:21]

On 2014/12/11 19:15:46, marcheu wrote:
> On 2014/12/11 18:30:17, kalyank wrote:
> > On 2014/12/11 09:27:44, brianderson wrote:
> > Sorry missed this.
> > 
> > > What's the timeline for getting this in?
> > 
> > Preferably before M41.
> > 
> > > Even though the Browser's compositor won't be drawing, we'll still have
the
> > > benefit that the GPU service won't block for swaps anymore, which will
free
> > the
> > > GPU service up for GPU work from the Renderer.
> > > 
> > > If the actual flip takes a long time, we want to skip frames to avoid
adding
> > > latency in case we are GPU bound.
> > > 
> > > If we aren't actually GPU bound though and are still falling to 30fps, I
can
> > > imagine that could be due to cases where the swap just misses the vsync
> > because
> > > we started/finished drawing at a bad time.
> > > 
> > > Whatever we do, I would prefer to not have the swap ack return immediately
> > > because we'll lose all information about how GPU bound we are.
> > 
> > ok
> > 
> > > Four more alternatives to help address the bad timing (if that is an
issue),
> > in
> > > increasing order of preference:
> > >   1) Try removing the swap throttling from
> > > cc::SchedulerStateMachine::ShouldSendBeginMainFrame for the Browser's
> > > compositor. This will let the Browser start creating a frame without
> > necessarily
> > > swapping it. (I've tried to remove this throttling from the Renderer, but
> kept
> > > running into test flakiness. See
https://codereview.chromium.org/222023010.
> > > Doing this in the Browser only might not cause the same flakiness.)
> > >   2) Switch max_pending_swaps_ to be 2 and add some logic to the
> cc::Scheduler
> > > that attempts to throttle itself when the swap acks take more than 2
vsyncs
> to
> > > return.
> > >   3) Would it be possible to send the swap ack when the swap is "armed"
and
> > will
> > > be flipped on the next vsync? (for example, by using a fence inserted
before
> > the
> > > swap.) That way, the swap ack comes back late if we are actually GPU
bound,
> > but
> > > not if we just had bad timing.
> > >   4) A combination of all the above. Have a flip ack in addition to a swap
> ack
> > > that makes it all the way to the cc::Scheduler. The swap ack comes back
when
> > the
> > > swap is "armed", but the flip ack waits until the actual vsync. Throttle
> > > SchedulerStateMachine::ShouldDraw() with a max_pending_flips_ of 1.
Throttle
> > > SchedulerStateMachine::ShouldSendBeginMainFrame with a max_pending_swaps_
of
> > 2.
> > > 
> > > For 2 and 3, we'll need the logic you have to block the next swap request
on
> > the
> > > previous flip. 1 and 4 should never block on a swap though.
> > 
> > K, will look into these options.
> 
> For what it's worth, I don't think we need/want to do any of these. If you do
> the following 2 things: don't block in swapbuffers and return the swap ack at
> the right time (when the vblank happens) you should have something which is
> equivalent to the current X-based case (you can schedule your frames correctly
> based on the swap ack information, and you can also do texture uploads and
webgl
> until the swap ack comes back).
> 
> Or did you observe otherwise?

Agree with marcheu, especially given the M41 target. If we aren't unexpectedly
falling to 30fps, don't worry about those alternatives I proposed. Even if we
are unexpectedly falling to 30fps, it sounds like there are other things to fix
if X already works well enough.

[kalyank, 2014-12-12 18:30:37]

On 2014/12/11 19:15:46, marcheu wrote:
> On 2014/12/11 18:30:17, kalyank wrote:
> > On 2014/12/11 09:27:44, brianderson wrote:
> > Sorry missed this.
> > 
> > > What's the timeline for getting this in?
> > 
> > Preferably before M41.
> > 
> > > Even though the Browser's compositor won't be drawing, we'll still have
the
> > > benefit that the GPU service won't block for swaps anymore, which will
free
> > the
> > > GPU service up for GPU work from the Renderer.
> > > 
> > > If the actual flip takes a long time, we want to skip frames to avoid
adding
> > > latency in case we are GPU bound.
> > > 
> > > If we aren't actually GPU bound though and are still falling to 30fps, I
can
> > > imagine that could be due to cases where the swap just misses the vsync
> > because
> > > we started/finished drawing at a bad time.
> > > 
> > > Whatever we do, I would prefer to not have the swap ack return immediately
> > > because we'll lose all information about how GPU bound we are.
> > 
> > ok
> > 
> > > Four more alternatives to help address the bad timing (if that is an
issue),
> > in
> > > increasing order of preference:
> > >   1) Try removing the swap throttling from
> > > cc::SchedulerStateMachine::ShouldSendBeginMainFrame for the Browser's
> > > compositor. This will let the Browser start creating a frame without
> > necessarily
> > > swapping it. (I've tried to remove this throttling from the Renderer, but
> kept
> > > running into test flakiness. See
https://codereview.chromium.org/222023010.
> > > Doing this in the Browser only might not cause the same flakiness.)
> > >   2) Switch max_pending_swaps_ to be 2 and add some logic to the
> cc::Scheduler
> > > that attempts to throttle itself when the swap acks take more than 2
vsyncs
> to
> > > return.
> > >   3) Would it be possible to send the swap ack when the swap is "armed"
and
> > will
> > > be flipped on the next vsync? (for example, by using a fence inserted
before
> > the
> > > swap.) That way, the swap ack comes back late if we are actually GPU
bound,
> > but
> > > not if we just had bad timing.
> > >   4) A combination of all the above. Have a flip ack in addition to a swap
> ack
> > > that makes it all the way to the cc::Scheduler. The swap ack comes back
when
> > the
> > > swap is "armed", but the flip ack waits until the actual vsync. Throttle
> > > SchedulerStateMachine::ShouldDraw() with a max_pending_flips_ of 1.
Throttle
> > > SchedulerStateMachine::ShouldSendBeginMainFrame with a max_pending_swaps_
of
> > 2.
> > > 
> > > For 2 and 3, we'll need the logic you have to block the next swap request
on
> > the
> > > previous flip. 1 and 4 should never block on a swap though.
> > 
> > K, will look into these options.
> 
> For what it's worth, I don't think we need/want to do any of these. If you do
> the following 2 things: don't block in swapbuffers and return the swap ack at
> the right time (when the vblank happens) you should have something which is
> equivalent to the current X-based case (you can schedule your frames correctly
> based on the swap ack information, and you can also do texture uploads and
webgl
> until the swap ack comes back).
> 
> Or did you observe otherwise?

I observed the same. I also saw we dropped frames with Ozone more than x11
depending on the work load.
I haven't checked the difference between both yet, will do that after this.

[kalyank, 2014-12-12 18:32:26]

On 2014/12/10 23:19:25, alexst(OOTO until Dec 16) wrote:
> Some quick notes on the current code, but after chatting with Brian, who is
our
> scheduling expert, it seems like simply delaying the swapack until after
> pageflip, while unblocking the gpu main would result in a simpler patch and
> accomplish the same result.
> 

Updated the patch to do this now.

[marcheu, 2014-12-12 19:00:21]

On 2014/12/12 18:30:37, kalyank wrote:
> On 2014/12/11 19:15:46, marcheu wrote:
> > On 2014/12/11 18:30:17, kalyank wrote:
> > > On 2014/12/11 09:27:44, brianderson wrote:
> > > Sorry missed this.
> > > 
> > > > What's the timeline for getting this in?
> > > 
> > > Preferably before M41.
> > > 
> > > > Even though the Browser's compositor won't be drawing, we'll still have
> the
> > > > benefit that the GPU service won't block for swaps anymore, which will
> free
> > > the
> > > > GPU service up for GPU work from the Renderer.
> > > > 
> > > > If the actual flip takes a long time, we want to skip frames to avoid
> adding
> > > > latency in case we are GPU bound.
> > > > 
> > > > If we aren't actually GPU bound though and are still falling to 30fps, I
> can
> > > > imagine that could be due to cases where the swap just misses the vsync
> > > because
> > > > we started/finished drawing at a bad time.
> > > > 
> > > > Whatever we do, I would prefer to not have the swap ack return
immediately
> > > > because we'll lose all information about how GPU bound we are.
> > > 
> > > ok
> > > 
> > > > Four more alternatives to help address the bad timing (if that is an
> issue),
> > > in
> > > > increasing order of preference:
> > > >   1) Try removing the swap throttling from
> > > > cc::SchedulerStateMachine::ShouldSendBeginMainFrame for the Browser's
> > > > compositor. This will let the Browser start creating a frame without
> > > necessarily
> > > > swapping it. (I've tried to remove this throttling from the Renderer,
but
> > kept
> > > > running into test flakiness. See
> https://codereview.chromium.org/222023010.
> > > > Doing this in the Browser only might not cause the same flakiness.)
> > > >   2) Switch max_pending_swaps_ to be 2 and add some logic to the
> > cc::Scheduler
> > > > that attempts to throttle itself when the swap acks take more than 2
> vsyncs
> > to
> > > > return.
> > > >   3) Would it be possible to send the swap ack when the swap is "armed"
> and
> > > will
> > > > be flipped on the next vsync? (for example, by using a fence inserted
> before
> > > the
> > > > swap.) That way, the swap ack comes back late if we are actually GPU
> bound,
> > > but
> > > > not if we just had bad timing.
> > > >   4) A combination of all the above. Have a flip ack in addition to a
swap
> > ack
> > > > that makes it all the way to the cc::Scheduler. The swap ack comes back
> when
> > > the
> > > > swap is "armed", but the flip ack waits until the actual vsync. Throttle
> > > > SchedulerStateMachine::ShouldDraw() with a max_pending_flips_ of 1.
> Throttle
> > > > SchedulerStateMachine::ShouldSendBeginMainFrame with a
max_pending_swaps_
> of
> > > 2.
> > > > 
> > > > For 2 and 3, we'll need the logic you have to block the next swap
request
> on
> > > the
> > > > previous flip. 1 and 4 should never block on a swap though.
> > > 
> > > K, will look into these options.
> > 
> > For what it's worth, I don't think we need/want to do any of these. If you
do
> > the following 2 things: don't block in swapbuffers and return the swap ack
at
> > the right time (when the vblank happens) you should have something which is
> > equivalent to the current X-based case (you can schedule your frames
correctly
> > based on the swap ack information, and you can also do texture uploads and
> webgl
> > until the swap ack comes back).
> > 
> > Or did you observe otherwise?
> 
> I observed the same. I also saw we dropped frames with Ozone more than x11
> depending on the work load.
> I haven't checked the difference between both yet, will do that after this.

Hmm, rietveld ate my previous message and gave me an empty message instead...

Basically, if you observe the same ballpark performance, then the jittering
differences could be explained by the scheduling -- in particular the reporting
of the monitor refresh rate is pretty fragile and tends to break, so we should
check that this works first. This will be particularly visible with a trace if
that's the case.

[kalyank, 2014-12-12 21:36:29]

On 2014/12/12 19:00:21, marcheu wrote:
> On 2014/12/12 18:30:37, kalyank wrote:
> > On 2014/12/11 19:15:46, marcheu wrote:
> Hmm, rietveld ate my previous message and gave me an empty message instead...
> 
> Basically, if you observe the same ballpark performance, then the jittering
> differences could be explained by the scheduling -- in particular the
reporting
> of the monitor refresh rate is pretty fragile and tends to break, so we should
> check that this works first. This will be particularly visible with a trace if
> that's the case.

K I think we still have a bit of catch up to do w.r.t Performance. Atleast on my
Haswell device, while X11 backend seems to hit 60 pretty consistently,
Ozone avgs around 45- 50 FPS with Aquarium demo. I will report a new bug to
check and fix anything found.

[alexst (slow to review), 2014-12-17 19:12:24]

https://codereview.chromium.org/781683005/diff/120001/ui/ozone/platform/dri/p...
File ui/ozone/platform/dri/page_flip_event_handler.cc (right):

https://codereview.chromium.org/781683005/diff/120001/ui/ozone/platform/dri/p...
ui/ozone/platform/dri/page_flip_event_handler.cc:45:
controller->WaitForPageFlipEvent();
Hi Kalyan, thank you for your help, this is very useful. Danny and I have been
discussing this internally and we think we can do this without introducing an
extra thread here. The gpu io message loop has the ability to watch file
descriptors and notify us when we can read without blocking. This would allow us
to get pageflip events directly from the drm fd asynchronously and dispatch them
to gpu main while avoiding some of the thread safety subtleties.

controller->WaitForPageFlipEvent() for example, is not thread safe and
controller's lifetime is not guaranteed (if a monitor gets unplugged, bad things
could happen here), so this will require addition of locks and subtle
crossthread lifetime management we'd like to avoid.

I like the content/ and ui/gl parts of this patch, let's split them out and land
them separately because they will enable this async behaviour. Meanwhile, we'll
get cranking on the ozone/platform/ part that uses IO thread.

[kalyank, 2014-12-17 20:49:15]

On 2014/12/17 19:12:24, alexst wrote:
>
https://codereview.chromium.org/781683005/diff/120001/ui/ozone/platform/dri/p...
> File ui/ozone/platform/dri/page_flip_event_handler.cc (right):
> 
>
https://codereview.chromium.org/781683005/diff/120001/ui/ozone/platform/dri/p...
> ui/ozone/platform/dri/page_flip_event_handler.cc:45:
> controller->WaitForPageFlipEvent();
> Hi Kalyan, thank you for your help, this is very useful. Danny and I have been
> discussing this internally and we think we can do this without introducing an
> extra thread here. The gpu io message loop has the ability to watch file
> descriptors and notify us when we can read without blocking. This would allow
us
> to get pageflip events directly from the drm fd asynchronously and dispatch
them
> to gpu main while avoiding some of the thread safety subtleties.
> 
> controller->WaitForPageFlipEvent() for example, is not thread safe and
> controller's lifetime is not guaranteed (if a monitor gets unplugged, bad
things
> could happen here), so this will require addition of locks and subtle
> crossthread lifetime management we'd like to avoid.
> 
> I like the content/ and ui/gl parts of this patch, let's split them out and
land
> them separately because they will enable this async behaviour. Meanwhile,
we'll
> get cranking on the ozone/platform/ part that uses IO thread.

Sure, I will split the ui/gl parts

[kalyank, 2014-12-18 10:27:28]

On 2014/12/17 20:49:15, kalyank wrote:
> On 2014/12/17 19:12:24, alexst wrote:
> >
>
https://codereview.chromium.org/781683005/diff/120001/ui/ozone/platform/dri/p...
> > File ui/ozone/platform/dri/page_flip_event_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/781683005/diff/120001/ui/ozone/platform/dri/p...
> > ui/ozone/platform/dri/page_flip_event_handler.cc:45:
> > controller->WaitForPageFlipEvent();
> > Hi Kalyan, thank you for your help, this is very useful. Danny and I have
been
> > discussing this internally and we think we can do this without introducing
an
> > extra thread here. The gpu io message loop has the ability to watch file
> > descriptors and notify us when we can read without blocking. This would
allow
> us
> > to get pageflip events directly from the drm fd asynchronously and dispatch
> them
> > to gpu main while avoiding some of the thread safety subtleties.
> > 
> > controller->WaitForPageFlipEvent() for example, is not thread safe and
> > controller's lifetime is not guaranteed (if a monitor gets unplugged, bad
> things
> > could happen here), so this will require addition of locks and subtle
> > crossthread lifetime management we'd like to avoid.
> > 
> > I like the content/ and ui/gl parts of this patch, let's split them out and
> land
> > them separately because they will enable this async behaviour. Meanwhile,
> we'll
> > get cranking on the ozone/platform/ part that uses IO thread.
> 
> Sure, I will split the ui/gl parts

https://codereview.chromium.org/797843005/  for the ui/gl parts.

Created a bug for this
https://code.google.com/p/chromium/issues/detail?id=443543
I  dont have access rights to edit and add all of you here as cc :(

[alexst (slow to review), 2015-01-20 23:05:56]

alexst@chromium.org changed reviewers:
- alexst@chromium.org