[ozone] Keep the queue of surfaceless buffers inside gl_surface_ozone

[ozone] Keep the queue of surfaceless buffers inside gl_surface_ozone and only dispatch them when the page flip ack callback for the previous one has been been called.

We previously had 2 queues, one in gl_surface_ozone one in the platform
display controller. Remove one of them since it shouldn't be necessary.

BUG=

Committed: https://crrev.com/9c97321ec9598a8b8af5eaa91cb9135b363efef0
Cr-Commit-Position: refs/heads/master@{#326791}

[alexst (slow to review), 2015-04-21 18:06:59]

Patchset #1 (id:1) has been deleted

[alexst (slow to review), 2015-04-21 18:07:09]

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

[alexst (slow to review), 2015-04-21 18:07:56]

Still stress testing, but it's mostly doing what I want.

[dnicoara, 2015-04-21 18:50:42]

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:60: // Because a page
flip is pending we need to leave some state for the
Could you please update the comment.

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:63:
ignore_page_flip_event_ = true;
nit: Simplity it to 'ignore_page_flip_event_ = HasPendingPageFlips();'

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:86: // A request is
being serviced right now.
Shouldn't this be reversed? When we schedule the page flip, there shouldn't be
any pending request, right?

[alexst (slow to review), 2015-04-21 19:19:55]

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:63:
ignore_page_flip_event_ = true;
On 2015/04/21 18:50:42, dnicoara wrote:
> nit: Simplity it to 'ignore_page_flip_event_ = HasPendingPageFlips();'

Done.

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:86: // A request is
being serviced right now.
On 2015/04/21 18:50:42, dnicoara wrote:
> Shouldn't this be reversed? When we schedule the page flip, there shouldn't be
> any pending request, right?

of course!

[dnicoara, 2015-04-21 19:37:58]

https://codereview.chromium.org/1091253003/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:86: // A request is
being serviced right now.
I'm finding this comment to be confusing. It sounds like there should be a
pending page flip. Probably just remove it since the DCHECKs are
self-explanatory.

https://codereview.chromium.org/1091253003/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:87:
DCHECK(!HasPendingPageFlips());
I think this may not always be true. For example:
 1) SchedulePageFlip()
 2) Modeset() before OnPageFlipCompleted()
 3) SchedulePageFlip()

When (3) is executed the CRTC will still have a pending page flip event.

[spang, 2015-04-21 19:39:35]

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

[spang, 2015-04-21 19:39:36]

drive by

https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (left):

https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:210: } else if
(ozone_surface_->IsUniversalDisplayLinkDevice()) {
Can you fix the style guide violation here?

No else {} after return.

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

https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:316: bool last_swap_buffers_result_;
Can someone explain what last_swap_buffers_result_ is for?

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:63:
ignore_page_flip_event_ = true;
On 2015/04/21 19:19:55, alexst wrote:
> On 2015/04/21 18:50:42, dnicoara wrote:
> > nit: Simplity it to 'ignore_page_flip_event_ = HasPendingPageFlips();'
> 
> Done.

This is funky.

Could we pass a callback w/ weak pointer to the PageFlipRequest instead? Then
cancellation will be transparent.

[alexst (slow to review), 2015-04-21 19:46:17]

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:63:
ignore_page_flip_event_ = true;
On 2015/04/21 19:39:36, spang wrote:
> On 2015/04/21 19:19:55, alexst wrote:
> > On 2015/04/21 18:50:42, dnicoara wrote:
> > > nit: Simplity it to 'ignore_page_flip_event_ = HasPendingPageFlips();'
> > 
> > Done.
> 
> This is funky.
> 
> Could we pass a callback w/ weak pointer to the PageFlipRequest instead? Then
> cancellation will be transparent.

Yes, that's a good idea.

[alexst (slow to review), 2015-04-22 17:49:14]

Patchset #6 (id:120001) has been deleted

[alexst (slow to review), 2015-04-22 17:49:21]

Patchset #5 (id:100001) has been deleted

[alexst (slow to review), 2015-04-22 18:50:50]

New draft is ready, please take a look. I am stress testing now to see if I
messed something up.

[dnicoara, 2015-04-23 15:34:57]

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/crtc_controller.cc:27:
CrtcController::~CrtcController() {
I think you need to ack the request here as well.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/crtc_controller.cc:74: DCHECK(!page_flip_pending_);
Lets just remove page_flip_pending_ and check against page_flip_request_.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/crtc_controller.cc:95: PLOG(ERROR) << "Failed to
assign overlay planes for crtc " << crtc_;
Need to call the page_flip_request here as well.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:66: // A request is
being serviced right now.
Remove comment since it's inconsistent.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:207: bool
HardwareDisplayController::HasPendingPageFlips() const {
Lets just remove this since it is being used for a DCHECK only and the DCHECK is
repeated in the CrtcControllers.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_controller.h (right):

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_controller.h:153:
scoped_refptr<ScanoutBuffer> cursor_buffer_;
nit: Unrelated, but could you just remove this as well. It isn't being used in
HDC anymore.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_controller.h:169: bool
ignore_page_flip_event_;
remove

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/page_flip_request.h:17: ~PageFlipRequest();
I believe you'll need to make the destructor private due to ref counting;
otherwise clang will complain.

https://codereview.chromium.org/1091253003/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/page_flip_request.h:19: base::Closure callback;
And for style guide consistency this should be private. Just add a
Signal()/OnPageflip() function to run the callback.

[alexst (slow to review), 2015-04-23 16:17:31]

done, ptal.

[spang, 2015-04-23 16:18:23]

On 2015/04/21 19:39:36, spang wrote:
> drive by
> 
>
https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone.cc
> File ui/gl/gl_surface_ozone.cc (left):
> 
>
https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone....
> ui/gl/gl_surface_ozone.cc:210: } else if
> (ozone_surface_->IsUniversalDisplayLinkDevice()) {
> Can you fix the style guide violation here?
> 
> No else {} after return.
> 
>
https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone.cc
> File ui/gl/gl_surface_ozone.cc (right):
> 
>
https://codereview.chromium.org/1091253003/diff/20001/ui/gl/gl_surface_ozone....
> ui/gl/gl_surface_ozone.cc:316: bool last_swap_buffers_result_;
> Can someone explain what last_swap_buffers_result_ is for?

Please?

> 
>
https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):
> 
>
https://codereview.chromium.org/1091253003/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/hardware_display_controller.cc:63:
> ignore_page_flip_event_ = true;
> On 2015/04/21 19:19:55, alexst wrote:
> > On 2015/04/21 18:50:42, dnicoara wrote:
> > > nit: Simplity it to 'ignore_page_flip_event_ = HasPendingPageFlips();'
> > 
> > Done.
> 
> This is funky.
> 
> Could we pass a callback w/ weak pointer to the PageFlipRequest instead? Then
> cancellation will be transparent.

[dnicoara, 2015-04-23 16:29:50]

nit: Could the title be of sane length (I don't remember what it should be 70
... 80 characters)?

Also, is there a bug number?

Otherwise looks fine pending unittests update.

https://codereview.chromium.org/1091253003/diff/180001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1091253003/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/page_flip_request.h:24: };
Add DISALLOW_COPY_AND_ASSIGN(PageFlipRequest);

[spang, 2015-04-23 16:32:53]

https://codereview.chromium.org/1091253003/diff/180001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1091253003/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/crtc_controller.cc:158:
last_request.swap(page_flip_request_);
Put a comment here for why to swap.

[spang, 2015-04-23 16:32:57]

lgtm

[alexst (slow to review), 2015-04-23 18:58:37]

The CQ bit was checked by alexst@chromium.org to run a CQ dry run

[alexst (slow to review), 2015-04-23 18:58:37]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1091253003/#ps220001
(title: "unittests")

[commit-bot: I haz the power, 2015-04-23 18:59:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091253003/220001

[dnicoara, 2015-04-23 19:02:51]

lgtm

[commit-bot: I haz the power, 2015-04-23 19:42:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 19:42:28]

Dry run: This issue passed the CQ dry run.

[alexst (slow to review), 2015-04-24 14:27:26]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2015-04-24 14:28:00]

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

[commit-bot: I haz the power, 2015-04-24 14:28:58]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2015-04-24 14:30:14]

Patchset 9 (id:??) landed as
https://crrev.com/9c97321ec9598a8b8af5eaa91cb9135b363efef0
Cr-Commit-Position: refs/heads/master@{#326791}