[Ozone-DRM] Remove bool return value for SchedulePageFlip call

[Ozone-DRM] Remove bool return value for SchedulePageFlip call

The HardwareDisplayController::SchedulePageFlip call is expected to
always call the swap callback it is provided. Having an additional bool
return code has no value and promotes confusion as to which cases the
HardwareDisplayController should handle.

This change also splits a HDC::TestPageFlip() method since it is
currently used in a synchronous fashion.

Also fix the case where a failed page flip is supposed to be treated as
a successful one.

BUG=none

Committed: https://crrev.com/4d792fca0847afa0b5b406154ad2da91a83f50e4
Cr-Commit-Position: refs/heads/master@{#357376}

[dnicoara, 2015-10-29 19:37:13]

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

[commit-bot: I haz the power, 2015-10-29 19:37:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418503012/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418503012/1

[dnicoara, 2015-10-29 19:40:31]

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

[dnicoara, 2015-10-29 19:40:32]

PTAL

FYI: I split the TestPageFlip() as a separate function since it is sufficiently
complex that it wasn't trivial to simply convert it to use a callback.

[commit-bot: I haz the power, 2015-10-29 19:54:31]

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

[commit-bot: I haz the power, 2015-10-29 19:54:32]

Dry run: This issue passed the CQ dry run.

[spang, 2015-10-29 20:59:21]

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (left):

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:75: bool
CrtcController::SchedulePageFlip(
Still two statuses being returned here.

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:129:
current_planes_.swap(pending_planes_);
I think it should be moved to SignalPageFlipRequest().

void CrtcController::SignalPageFlipRequest(gfx::SwapResult result) {
  if (result == gfx::SwapResult::SWAP_ACK)
    current_planes_ = pending_planes_;
  pending_planes_.clear();

  if (page_flip_request_.get()) {
    // ...
  }
}

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:167: if (page_flip_request_.get())
{
When is this false?

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/h...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:93: bool
HardwareDisplayController::ActualSchedulePageFlip(
This only moves the problem to ActualSchedulePageFlip.

It should be solvable, CheckOverlayCapabilities at least takes a callback.

[dnicoara, 2015-10-30 14:59:31]

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:129:
current_planes_.swap(pending_planes_);
On 2015/10/29 20:59:20, spang wrote:
> I think it should be moved to SignalPageFlipRequest().
> 
> void CrtcController::SignalPageFlipRequest(gfx::SwapResult result) {
>   if (result == gfx::SwapResult::SWAP_ACK)
>     current_planes_ = pending_planes_;
>   pending_planes_.clear();
> 
>   if (page_flip_request_.get()) {
>     // ...
>   }
> }

Acknowledged.

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:167: if (page_flip_request_.get())
{
On 2015/10/29 20:59:20, spang wrote:
> When is this false?

This was needed when HDC::SchedulePageFlip() was re-entrant. Shouldn't be
possible anymore. I'll remove this in a separate change just in case.

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/h...
File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:93: bool
HardwareDisplayController::ActualSchedulePageFlip(
On 2015/10/29 20:59:20, spang wrote:
> This only moves the problem to ActualSchedulePageFlip.
> 
> It should be solvable, CheckOverlayCapabilities at least takes a callback.

Yes, though DrmWindow::TestPageFlip() has some allocation optimizations that I
wanted to take another look at before committing to a solution. My plan is to
have a followup that refactors TestPageFlip().

[spang, 2015-10-30 17:07:16]

On 2015/10/30 14:59:31, dnicoara wrote:
>
https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
> File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):
> 
>
https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
> ui/ozone/platform/drm/gpu/crtc_controller.cc:129:
> current_planes_.swap(pending_planes_);
> On 2015/10/29 20:59:20, spang wrote:
> > I think it should be moved to SignalPageFlipRequest().
> > 
> > void CrtcController::SignalPageFlipRequest(gfx::SwapResult result) {
> >   if (result == gfx::SwapResult::SWAP_ACK)
> >     current_planes_ = pending_planes_;
> >   pending_planes_.clear();
> > 
> >   if (page_flip_request_.get()) {
> >     // ...
> >   }
> > }
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
> ui/ozone/platform/drm/gpu/crtc_controller.cc:167: if
(page_flip_request_.get())
> {
> On 2015/10/29 20:59:20, spang wrote:
> > When is this false?
> 
> This was needed when HDC::SchedulePageFlip() was re-entrant. Shouldn't be
> possible anymore. I'll remove this in a separate change just in case.

Can you describe the sequence of events where we could have gotten here with
nullptr? I don't see how reentrancy could do it either, unless we ended up never
acking one of the flips.

> 
>
https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/h...
> File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right):
> 
>
https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/h...
> ui/ozone/platform/drm/gpu/hardware_display_controller.cc:93: bool
> HardwareDisplayController::ActualSchedulePageFlip(
> On 2015/10/29 20:59:20, spang wrote:
> > This only moves the problem to ActualSchedulePageFlip.
> > 
> > It should be solvable, CheckOverlayCapabilities at least takes a callback.
> 
> Yes, though DrmWindow::TestPageFlip() has some allocation optimizations that I
> wanted to take another look at before committing to a solution. My plan is to
> have a followup that refactors TestPageFlip().

[dnicoara, 2015-10-30 17:21:59]

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1418503012/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:167: if (page_flip_request_.get())
{
On 2015/10/30 14:59:31, dnicoara wrote:
> On 2015/10/29 20:59:20, spang wrote:
> > When is this false?
> 
> This was needed when HDC::SchedulePageFlip() was re-entrant. Shouldn't be
> possible anymore. I'll remove this in a separate change just in case.

Actually, sorry, you're right, it shouldn't be possible due to re-entrancy. I
guess it was just the call from the dtor that needed this.

[spang, 2015-10-30 20:27:02]

lgtm

https://codereview.chromium.org/1418503012/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/crtc_controller.h (right):

https://codereview.chromium.org/1418503012/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.h:62: // Called if the page flip event
wasn't scheduled and will never be. This will
Update comment.

[dnicoara, 2015-11-02 17:04:04]

https://codereview.chromium.org/1418503012/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/crtc_controller.h (right):

https://codereview.chromium.org/1418503012/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.h:62: // Called if the page flip event
wasn't scheduled and will never be. This will
On 2015/10/30 20:27:02, spang wrote:
> Update comment.

Done.

[dnicoara, 2015-11-02 17:04:08]

The CQ bit was checked by dnicoara@chromium.org

[dnicoara, 2015-11-02 17:04:08]

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/1418503012/#ps40001
(title: "Rebased & updated")

[commit-bot: I haz the power, 2015-11-02 17:04:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418503012/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418503012/40001

[commit-bot: I haz the power, 2015-11-02 17:20:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-02 17:21:33]

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