ozone: dri: add synchronous SwapBuffers support on surfaceless

ozone: dri: add synchronous SwapBuffers support on surfaceless

BUG=447798
TEST=ozone_unittests

Committed: https://crrev.com/210577c2e1b7bb0462ccbb7905c800568ba3888e
Cr-Commit-Position: refs/heads/master@{#318457}

[llandwerlin-old, 2015-02-26 17:36:49]

lionel.g.landwerlin@intel.com changed reviewers:
+ dnicoara@chromium.org

[llandwerlin-old, 2015-02-26 17:36:49]

Daniel, here is the split of https://codereview.chromium.org/940903002/

[dnicoara, 2015-02-26 18:22:22]

For more feedback:

Currently the DriWrapper changes to async only when running in surfaceless mode.
There are no expectations to switch between different modes dynamically at
run-time (other than once during initialization). As such I think the easiest
way to do this is to assume we start in synchronous mode and unless a
SchedulePageFlip() requests async mode we should stay synchronous.

We're maintaining 3 mode right now:
1) Software rendering - running with synchronous PageFlips
2) Hardware rendering with surface - running with synchronous PageFlips
3) Hardware rendering with surfaceless - running with asynchronous PageFlips

and now we're trying to add a 4th mode: Hardware rendering with surfaceless and
running with synchronous PageFlips.

TLDR: only switch to asynchronous if DriWrapper has an io_task_runner and
SchedulePageFlip requests async page flips.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:109: io_task_runner_->PostTask(FROM_HERE,
I think we should remove this since the call to SetPaused() will register this
if needed.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:113: void SetPaused(bool value) {
Just rename |value| to |paused| and remove the parameter bellow.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:124: void Shutdown() {
We should check if |paused_| is false and return in this case.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:306: watcher_->SetPaused(is_sync);
|watcher_| may not be initialized. you'll want to check that |watcher_| exists
here and the the if-statement on line 319. The idea is to fallback to
synchronous mode if we don't have a watcher.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/hardwa...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/hardwa...
ui/ozone/platform/dri/hardware_display_controller.cc:117: bool
HardwareDisplayController::PageFlip() {
I think you can just use SchedulePageFlip() for this and just add the |is_sync|
parameter. You can stash the |is_sync| parameter in the PageFlipRequest() as
well to deal with transitions between synchronous and asynchronous modes.

[llandwerlin-old, 2015-02-26 22:02:56]

I might be missing something here, but when you write :

"Software rendering - running with synchronous PageFlips" is it because we don't
set a task runner? Or do we just flip as fast as possible with a DoNothing
callback?

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:109: io_task_runner_->PostTask(FROM_HERE,
On 2015/02/26 18:22:21, dnicoara wrote:
> I think we should remove this since the call to SetPaused() will register this
> if needed.

Done.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:113: void SetPaused(bool value) {
On 2015/02/26 18:22:21, dnicoara wrote:
> Just rename |value| to |paused| and remove the parameter bellow.

Done.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:124: void Shutdown() {
On 2015/02/26 18:22:21, dnicoara wrote:
> We should check if |paused_| is false and return in this case.

Done.

https://codereview.chromium.org/960273003/diff/1/ui/ozone/platform/dri/dri_wr...
ui/ozone/platform/dri/dri_wrapper.cc:306: watcher_->SetPaused(is_sync);
On 2015/02/26 18:22:21, dnicoara wrote:
> |watcher_| may not be initialized. you'll want to check that |watcher_| exists
> here and the the if-statement on line 319. The idea is to fallback to
> synchronous mode if we don't have a watcher.

Adding the check on watcher_, but on line 319 I still need to leave the is_sync
check.

[dnicoara, 2015-02-26 22:16:29]

The software path is using synchronous behavior since the Chromium interface
currently doesn't support asynchronous updates.

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:112: paused = !!paused;
remove this

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

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surfaceless.cc:36: return
controller->SchedulePageFlip(true, base::Bind(&base::DoNothing)));
nit: Would you mind adding "true /* is_sync */," here and in other places. This
is to keep track of what the argument is about.

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:102:
requests_.push_back(PageFlipRequest(pending_planes_, callback));
You'll need to add the |is_sync| param to the PageFlipRequest otherwise queued
buffers will execute synchronously.

This is relevant to the asynchronous path where we are queuing requests on
startup.

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:250: bool status =
ActualSchedulePageFlip(false);
Then |is_sync| will be taken from the PageFlipRequest.

[llandwerlin-old, 2015-02-27 10:56:47]

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:112: paused = !!paused;
On 2015/02/26 22:16:29, dnicoara wrote:
> remove this

Done.

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

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surfaceless.cc:36: return
controller->SchedulePageFlip(true, base::Bind(&base::DoNothing)));
On 2015/02/26 22:16:29, dnicoara wrote:
> nit: Would you mind adding "true /* is_sync */," here and in other places.
This
> is to keep track of what the argument is about.

Done.

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:102:
requests_.push_back(PageFlipRequest(pending_planes_, callback));
On 2015/02/26 22:16:29, dnicoara wrote:
> You'll need to add the |is_sync| param to the PageFlipRequest otherwise queued
> buffers will execute synchronously.
> 
> This is relevant to the asynchronous path where we are queuing requests on
> startup.

Done.

https://codereview.chromium.org/960273003/diff/20001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:250: bool status =
ActualSchedulePageFlip(false);
On 2015/02/26 22:16:29, dnicoara wrote:
> Then |is_sync| will be taken from the PageFlipRequest.

Done, thanks a lot.

[dnicoara, 2015-02-27 13:58:44]

https://codereview.chromium.org/960273003/diff/40001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/960273003/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:250: PageFlipRequest
request = requests_.front();
Move this back in the if-statement.

https://codereview.chromium.org/960273003/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:288: OverlayPlaneList
pending_planes = requests_.front().planes;
You'll want to get the is_sync flag here, from |requests_.front().is_sync|. This
is more to keep ActualSchedulePageFlip self-contained, otherwise there are
assumptions on the link between |is_sync| and the pending processed request.

[llandwerlin-old, 2015-02-27 14:09:04]

https://codereview.chromium.org/960273003/diff/40001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/960273003/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:250: PageFlipRequest
request = requests_.front();
On 2015/02/27 13:58:44, dnicoara wrote:
> Move this back in the if-statement.

Done.

https://codereview.chromium.org/960273003/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:288: OverlayPlaneList
pending_planes = requests_.front().planes;
On 2015/02/27 13:58:44, dnicoara wrote:
> You'll want to get the is_sync flag here, from |requests_.front().is_sync|.
This
> is more to keep ActualSchedulePageFlip self-contained, otherwise there are
> assumptions on the link between |is_sync| and the pending processed request.

Ah right, got it.

[dnicoara, 2015-02-27 15:32:12]

lgtm, thank you for fixing this!

[llandwerlin-old, 2015-02-27 15:33:54]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[commit-bot: I haz the power, 2015-02-27 15:35:09]

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

[commit-bot: I haz the power, 2015-02-27 16:10:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-27 16:11:06]

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