[Ozone-DRI] Listen for swap events

[Ozone-DRI] Listen for swap events

Rather than block until the swap event has been acknowledged, listen for page
flip events allowing the GPU main thread to continue execution while the
page flip is pending.

BUG=438389
NOTRY=true

Committed: https://crrev.com/472031054bcc5389673a728e7ad9b3ebedbf6660
Cr-Commit-Position: refs/heads/master@{#310794}

[dnicoara, 2015-01-08 15:31:36]

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

[dnicoara, 2015-01-08 15:31:37]

WIP change.

https://codereview.chromium.org/821023003/diff/1/ui/ozone/platform/dri/crtc_c...
File ui/ozone/platform/dri/crtc_controller.h (right):

https://codereview.chromium.org/821023003/diff/1/ui/ozone/platform/dri/crtc_c...
ui/ozone/platform/dri/crtc_controller.h:70: void AddObserver(PageFlipObserver*
observer);
This seemed good at the beginning, but now I feel like a callback on
SchedulePageFlip() will be simpler.

[dnicoara, 2015-01-09 00:26:00]

I got it in a decent shape. Let me know what you think.

[alexst (slow to review), 2015-01-09 03:55:46]

Just some quick notes. Looks good overall. I am going to have another thorough
pass through with a fresh head tomorrow morning.

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

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:28: PageFlipPayload(const
scoped_refptr<base::TaskRunner>& task_runner,
You should mention which thread this task runner belongs to.

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:107: base::Bind(&IOWatcher::UnregisterOnIO,
this));
I am pretty sure this is not safe, we are in the destructor, once this leaves
the function, the object is invalid. Taking a ref once we go into the task
runner won't help.

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:117:
DCHECK(base::MessageLoopForIO::IsCurrent());
Is this checking that an IO type thread is current? What about the other system
crated io thread?

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

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:50: LOG(ERROR) <<
"modeset";
Debug message.

[dnicoara, 2015-01-09 15:08:24]

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

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:28: PageFlipPayload(const
scoped_refptr<base::TaskRunner>& task_runner,
On 2015/01/09 03:55:46, alexst wrote:
> You should mention which thread this task runner belongs to.

Added some comments. In principle it doesn't matter. If we decide to move it to
another thread it should still be the same.

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:107: base::Bind(&IOWatcher::UnregisterOnIO,
this));
On 2015/01/09 03:55:46, alexst wrote:
> I am pretty sure this is not safe, we are in the destructor, once this leaves
> the function, the object is invalid. Taking a ref once we go into the task
> runner won't help.

Doh! You are right, move the post in a Shutdown() function.

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:117:
DCHECK(base::MessageLoopForIO::IsCurrent());
On 2015/01/09 03:55:46, alexst wrote:
> Is this checking that an IO type thread is current? What about the other
system
> crated io thread?

Yes. It would be hard to check which IO message loop we're on (especially since
we're cannot access the other IO message loop).

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

https://codereview.chromium.org/821023003/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:50: LOG(ERROR) <<
"modeset";
On 2015/01/09 03:55:46, alexst wrote:
> Debug message.

Removed.

[alexst (slow to review), 2015-01-09 16:06:58]

lgtm

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

https://codereview.chromium.org/821023003/diff/60001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:286: PageFlipRequest
request = requests_.front();
nit: PageFlipRequest& request = requests_.front(); here and in
ClearPendingRequests because deque returns a ref, no need to copy the data here.

[dnicoara, 2015-01-09 16:08:40]

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

https://codereview.chromium.org/821023003/diff/60001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:286: PageFlipRequest
request = requests_.front();
On 2015/01/09 16:06:58, alexst wrote:
> nit: PageFlipRequest& request = requests_.front(); here and in
> ClearPendingRequests because deque returns a ref, no need to copy the data
here.

We do need to copy it since we're popping the element right after.

[alexst (slow to review), 2015-01-09 16:10:52]

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

https://codereview.chromium.org/821023003/diff/60001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:286: PageFlipRequest
request = requests_.front();
On 2015/01/09 16:08:40, dnicoara wrote:
> On 2015/01/09 16:06:58, alexst wrote:
> > nit: PageFlipRequest& request = requests_.front(); here and in
> > ClearPendingRequests because deque returns a ref, no need to copy the data
> here.
> 
> We do need to copy it since we're popping the element right after.

Acknowledged.

[dnicoara, 2015-01-09 17:57:34]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2015-01-09 17:58:32]

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

[commit-bot: I haz the power, 2015-01-09 18:00:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-09 18:01:31]

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