ozone: Reject overlays when a test buffer can't be allocated.

ozone: Reject overlays when a test buffer can't be allocated.

Creating a scanout buffer can fail.
One example is if the size of the buffer is too big. In that case
it won't be possible to add a framebuffer and GetBufferForPageFlipTest
will return null.

This CL makes sure that we handle that case, and we reject the
overlay candidate when that happens.

BUG=b/33247171

Review-Url: https://codereview.chromium.org/2712463002
Cr-Commit-Position: refs/heads/master@{#451907}
Committed: https://chromium.googlesource.com/chromium/src/+/b3d8f58c9c4070637f727fa46eec5c9a367c8dcd

[Daniele Castagna, 2017-02-21 18:37:27]

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

[commit-bot: I haz the power, 2017-02-21 18:38:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniele Castagna, 2017-02-21 18:39:17]

dcastagna@chromium.org changed reviewers:
+ dnicoara@chromium.org, reveman@chromium.org

[dnicoara, 2017-02-21 19:03:18]

https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc (right):

https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a buffer
is too big, buffer allocation for scanout will fail.
Since we're expecting the AddFramebuffer2() call to fail, perhaps use
MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the fail (and
not some arbitrary size). That should be the same scenario we're currently
experiencing on devices, right?

[Daniele Castagna, 2017-02-21 19:12:22]

https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc (right):

https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a buffer
is too big, buffer allocation for scanout will fail.
On 2017/02/21 at 19:03:18, dnicoara wrote:
> Since we're expecting the AddFramebuffer2() call to fail, perhaps use
MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the fail (and
not some arbitrary size). That should be the same scenario we're currently
experiencing on devices, right?

You're right that what you described is the scenario we're trying to deal with.

We're never calling AddFramebuffer in these tests though.
We just call MockScanoutBufferGenerator::Create that creates a mock buffer.

We could use something like
MockScanoutBufferGenerator::set_allocation_failure(true)?
What do you think?

[dnicoara, 2017-02-21 19:19:39]

https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc (right):

https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a buffer
is too big, buffer allocation for scanout will fail.
On 2017/02/21 19:12:21, Daniele Castagna wrote:
> On 2017/02/21 at 19:03:18, dnicoara wrote:
> > Since we're expecting the AddFramebuffer2() call to fail, perhaps use
> MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the fail (and
> not some arbitrary size). That should be the same scenario we're currently
> experiencing on devices, right?
> 
> You're right that what you described is the scenario we're trying to deal
with.
> 
> We're never calling AddFramebuffer in these tests though.
> We just call MockScanoutBufferGenerator::Create that creates a mock buffer.

Oh, you're right, that makes me feel a bit sad :(

> 
> We could use something like
> MockScanoutBufferGenerator::set_allocation_failure(true)?
> What do you think?

Yeah, this sounds better. Less chance of accidentally breaking other tests.

[Daniele Castagna, 2017-02-21 19:33:30]

On 2017/02/21 at 19:19:39, dnicoara wrote:
>
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a
buffer is too big, buffer allocation for scanout will fail.
> On 2017/02/21 19:12:21, Daniele Castagna wrote:
> > On 2017/02/21 at 19:03:18, dnicoara wrote:
> > > Since we're expecting the AddFramebuffer2() call to fail, perhaps use
> > MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the fail
(and
> > not some arbitrary size). That should be the same scenario we're currently
> > experiencing on devices, right?
> > 
> > You're right that what you described is the scenario we're trying to deal
with.
> > 
> > We're never calling AddFramebuffer in these tests though.
> > We just call MockScanoutBufferGenerator::Create that creates a mock buffer.
> 
> Oh, you're right, that makes me feel a bit sad :(

Unfortunately GbmBuffer calls directly into gbm. That makes it harder to unit
tests and AFAIK on ozone_gl_unittest is exercising that code right now.

> 
> > 
> > We could use something like
> > MockScanoutBufferGenerator::set_allocation_failure(true)?
> > What do you think?
> 
> Yeah, this sounds better. Less chance of accidentally breaking other tests.

Done. PTAL.

[Daniele Castagna, 2017-02-21 19:34:15]

On 2017/02/21 at 19:33:30, Daniele Castagna wrote:
> On 2017/02/21 at 19:19:39, dnicoara wrote:
> >
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc (right):
> > 
> >
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a
buffer is too big, buffer allocation for scanout will fail.
> > On 2017/02/21 19:12:21, Daniele Castagna wrote:
> > > On 2017/02/21 at 19:03:18, dnicoara wrote:
> > > > Since we're expecting the AddFramebuffer2() call to fail, perhaps use
> > > MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the fail
(and
> > > not some arbitrary size). That should be the same scenario we're currently
> > > experiencing on devices, right?
> > > 
> > > You're right that what you described is the scenario we're trying to deal
with.
> > > 
> > > We're never calling AddFramebuffer in these tests though.
> > > We just call MockScanoutBufferGenerator::Create that creates a mock
buffer.
> > 
> > Oh, you're right, that makes me feel a bit sad :(
> 
> Unfortunately GbmBuffer calls directly into gbm. That makes it harder to unit
tests and AFAIK on ozone_gl_unittest is exercising that code right now.

I meant only ozone_gl_unittest is exercising that code.

> 
> > 
> > > 
> > > We could use something like
> > > MockScanoutBufferGenerator::set_allocation_failure(true)?
> > > What do you think?
> > 
> > Yeah, this sounds better. Less chance of accidentally breaking other tests.
> 
> Done. PTAL.

[dnicoara, 2017-02-21 19:38:03]

On 2017/02/21 19:34:15, Daniele Castagna wrote:
> On 2017/02/21 at 19:33:30, Daniele Castagna wrote:
> > On 2017/02/21 at 19:19:39, dnicoara wrote:
> > >
>
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > > File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > > ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a
> buffer is too big, buffer allocation for scanout will fail.
> > > On 2017/02/21 19:12:21, Daniele Castagna wrote:
> > > > On 2017/02/21 at 19:03:18, dnicoara wrote:
> > > > > Since we're expecting the AddFramebuffer2() call to fail, perhaps use
> > > > MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the
fail
> (and
> > > > not some arbitrary size). That should be the same scenario we're
currently
> > > > experiencing on devices, right?
> > > > 
> > > > You're right that what you described is the scenario we're trying to
deal
> with.
> > > > 
> > > > We're never calling AddFramebuffer in these tests though.
> > > > We just call MockScanoutBufferGenerator::Create that creates a mock
> buffer.
> > > 
> > > Oh, you're right, that makes me feel a bit sad :(
> > 
> > Unfortunately GbmBuffer calls directly into gbm. That makes it harder to
unit
> tests and AFAIK on ozone_gl_unittest is exercising that code right now.
> 
> I meant only ozone_gl_unittest is exercising that code.
> 
> > 
> > > 
> > > > 
> > > > We could use something like
> > > > MockScanoutBufferGenerator::set_allocation_failure(true)?
> > > > What do you think?
> > > 
> > > Yeah, this sounds better. Less chance of accidentally breaking other
tests.
> > 
> > Done. PTAL.

lgtm

[Daniele Castagna, 2017-02-21 19:45:06]

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

[Daniele Castagna, 2017-02-21 19:45:45]

On 2017/02/21 at 19:38:03, dnicoara wrote:
> On 2017/02/21 19:34:15, Daniele Castagna wrote:
> > On 2017/02/21 at 19:33:30, Daniele Castagna wrote:
> > > On 2017/02/21 at 19:19:39, dnicoara wrote:
> > > >
> >
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > > > File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc
(right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > > > ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If a
> > buffer is too big, buffer allocation for scanout will fail.
> > > > On 2017/02/21 19:12:21, Daniele Castagna wrote:
> > > > > On 2017/02/21 at 19:03:18, dnicoara wrote:
> > > > > > Since we're expecting the AddFramebuffer2() call to fail, perhaps
use
> > > > > MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the
fail
> > (and
> > > > > not some arbitrary size). That should be the same scenario we're
currently
> > > > > experiencing on devices, right?
> > > > > 
> > > > > You're right that what you described is the scenario we're trying to
deal
> > with.
> > > > > 
> > > > > We're never calling AddFramebuffer in these tests though.
> > > > > We just call MockScanoutBufferGenerator::Create that creates a mock
> > buffer.
> > > > 
> > > > Oh, you're right, that makes me feel a bit sad :(
> > > 
> > > Unfortunately GbmBuffer calls directly into gbm. That makes it harder to
unit
> > tests and AFAIK on ozone_gl_unittest is exercising that code right now.
> > 
> > I meant only ozone_gl_unittest is exercising that code.
> > 
> > > 
> > > > 
> > > > > 
> > > > > We could use something like
> > > > > MockScanoutBufferGenerator::set_allocation_failure(true)?
> > > > > What do you think?
> > > > 
> > > > Yeah, this sounds better. Less chance of accidentally breaking other
tests.
> > > 
> > > Done. PTAL.
> 
> lgtm

Tnx.
I just cleaned up the test that can now behave like the other test cases
(doesn't need a big buffer anymore).
That surfaced a bug where we'd try to reuse a nullptr and crash. Now that is
fixed too.

[commit-bot: I haz the power, 2017-02-21 19:45:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dnicoara, 2017-02-21 19:50:17]

On 2017/02/21 19:45:45, Daniele Castagna wrote:
> On 2017/02/21 at 19:38:03, dnicoara wrote:
> > On 2017/02/21 19:34:15, Daniele Castagna wrote:
> > > On 2017/02/21 at 19:33:30, Daniele Castagna wrote:
> > > > On 2017/02/21 at 19:19:39, dnicoara wrote:
> > > > >
> > >
>
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > > > > File ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc
> (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/2712463002/diff/1/ui/ozone/platform/drm/gpu/d...
> > > > > ui/ozone/platform/drm/gpu/drm_overlay_validator_unittest.cc:633: // If
a
> > > buffer is too big, buffer allocation for scanout will fail.
> > > > > On 2017/02/21 19:12:21, Daniele Castagna wrote:
> > > > > > On 2017/02/21 at 19:03:18, dnicoara wrote:
> > > > > > > Since we're expecting the AddFramebuffer2() call to fail, perhaps
> use
> > > > > > MockDrmDevice::set_add_framebuffer_expectation(false) to trigger the
> fail
> > > (and
> > > > > > not some arbitrary size). That should be the same scenario we're
> currently
> > > > > > experiencing on devices, right?
> > > > > > 
> > > > > > You're right that what you described is the scenario we're trying to
> deal
> > > with.
> > > > > > 
> > > > > > We're never calling AddFramebuffer in these tests though.
> > > > > > We just call MockScanoutBufferGenerator::Create that creates a mock
> > > buffer.
> > > > > 
> > > > > Oh, you're right, that makes me feel a bit sad :(
> > > > 
> > > > Unfortunately GbmBuffer calls directly into gbm. That makes it harder to
> unit
> > > tests and AFAIK on ozone_gl_unittest is exercising that code right now.
> > > 
> > > I meant only ozone_gl_unittest is exercising that code.
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > We could use something like
> > > > > > MockScanoutBufferGenerator::set_allocation_failure(true)?
> > > > > > What do you think?
> > > > > 
> > > > > Yeah, this sounds better. Less chance of accidentally breaking other
> tests.
> > > > 
> > > > Done. PTAL.
> > 
> > lgtm
> 
> Tnx.
> I just cleaned up the test that can now behave like the other test cases
> (doesn't need a big buffer anymore).
> That surfaced a bug where we'd try to reuse a nullptr and crash. Now that is
> fixed too.

Thank you for the cleanup! :)

[Daniele Castagna, 2017-02-21 20:36:51]

The CQ bit was unchecked by dcastagna@chromium.org

[Daniele Castagna, 2017-02-21 20:36:59]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2017-02-21 20:36:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/2712463002/#ps40001
(title: "Clean up unittest. Fix nullptr bug.")

[commit-bot: I haz the power, 2017-02-21 20:37:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 21:50:00]

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

[commit-bot: I haz the power, 2017-02-21 21:50:01]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Daniele Castagna, 2017-02-21 21:50:29]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2017-02-21 21:52:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 23:26:01]

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

[commit-bot: I haz the power, 2017-02-21 23:26:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniele Castagna, 2017-02-21 23:36:41]

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

[commit-bot: I haz the power, 2017-02-21 23:38:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 01:42:49]

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

[commit-bot: I haz the power, 2017-02-22 01:42:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Daniele Castagna, 2017-02-22 02:21:19]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2017-02-22 02:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/2712463002/#ps60001
(title: "Remove DCHECK that one plane would always succeed.")

[commit-bot: I haz the power, 2017-02-22 02:22:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 04:24:58]

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

[commit-bot: I haz the power, 2017-02-22 04:24:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Daniele Castagna, 2017-02-22 05:13:13]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:15:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 07:09:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487740393740310,
"parent_rev": "07205c9be7b8fa35aa6079b6027f6261761b62c3", "commit_rev":
"b3d8f58c9c4070637f727fa46eec5c9a367c8dcd"}

[commit-bot: I haz the power, 2017-02-22 07:10:43]

Description was changed from

==========
ozone: Reject overlays when a test buffer can't be allocated.

Creating a scanout buffer can fail.
One example is if the size of the buffer is too big. In that case
it won't be possible to add a framebuffer and GetBufferForPageFlipTest
will return null.

This CL makes sure that we handle that case, and we reject the
overlay candidate when that happens.

BUG=b/33247171
==========

to

==========
ozone: Reject overlays when a test buffer can't be allocated.

Creating a scanout buffer can fail.
One example is if the size of the buffer is too big. In that case
it won't be possible to add a framebuffer and GetBufferForPageFlipTest
will return null.

This CL makes sure that we handle that case, and we reject the
overlay candidate when that happens.

BUG=b/33247171

Review-Url: https://codereview.chromium.org/2712463002
Cr-Commit-Position: refs/heads/master@{#451907}
Committed:
https://chromium.googlesource.com/chromium/src/+/b3d8f58c9c4070637f727fa46eec...
==========

[commit-bot: I haz the power, 2017-02-22 07:10:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b3d8f58c9c4070637f727fa46eec...