ozone/drm: Only reuse ScanoutBuffers with compatible modifiers

ozone/drm: Only reuse ScanoutBuffers with compatible modifiers

Now that we have modifiers, we need to make sure that not just the
format, but also the format modifer is compatible with the
HardwareDisplayController where we want to use the buffer.

BUG=b/38217940
TEST=Plug in external monitor that supports same resolution as internal
  panel with mirror mode enabled

Review-Url: https://codereview.chromium.org/2919533003
Cr-Commit-Position: refs/heads/master@{#479813}
Committed: https://chromium.googlesource.com/chromium/src/+/9947afe90294ff5ad902e2cc02f8064506f7f0a4

[hoegsberg1, 2017-05-31 22:29:03]

Description was changed from

==========
ozone/drm: Only reuse ScanoutBuffers with compatible modifiers

Now that we have modifiers, we need to make sure that not just the
format, but also the format modifer is compatible with the
HardwareDisplayController where we want to use the buffer.

BUG=b/38217940
TEST=Plug in external monitor that supports same resolution as internal
  panel with mirror mode enabled
==========

to

==========
ozone/drm: Only reuse ScanoutBuffers with compatible modifiers

Now that we have modifiers, we need to make sure that not just the
format, but also the format modifer is compatible with the
HardwareDisplayController where we want to use the buffer.

BUG=b/38217940
TEST=Plug in external monitor that supports same resolution as internal
  panel with mirror mode enabled
==========

[hoegsberg1, 2017-05-31 22:29:03]

hoegsberg@google.com changed reviewers:
+ dcastagna@chromium.org

[hoegsberg1, 2017-06-01 17:14:46]

hoegsberg@google.com changed reviewers:
+ reveman@chromium.org

[Daniele Castagna, 2017-06-03 19:53:06]

LGTM, thanks!

https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (right):

https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:366: if (modifiers.size() == 0)
nit: !modifiers.size()

https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:368: for (const uint64_t modifier :
modifiers) {
You could use std::find here. I have no preference.

https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:369: if (modifier ==
primary->buffer->GetFormatModifier()) {
nit: curly braces not needed.

[dnicoara, 2017-06-05 15:54:58]

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

[dnicoara, 2017-06-05 15:54:58]

Could you add some ScreenManager unittests to cover modifier flags?

https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
File ui/ozone/platform/drm/gpu/screen_manager.cc (right):

https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
ui/ozone/platform/drm/gpu/screen_manager.cc:366: if (modifiers.size() == 0)
On 2017/06/03 19:53:06, Daniele Castagna wrote:
> nit: !modifiers.size()

Actually, use: modifiers.empty()

[hoegsberg1, 2017-06-14 22:01:06]

On 2017/06/05 15:54:58, dnicoara wrote:
> Could you add some ScreenManager unittests to cover modifier flags?
> 
>
https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
> File ui/ozone/platform/drm/gpu/screen_manager.cc (right):
> 
>
https://codereview.chromium.org/2919533003/diff/1/ui/ozone/platform/drm/gpu/s...
> ui/ozone/platform/drm/gpu/screen_manager.cc:366: if (modifiers.size() == 0)
> On 2017/06/03 19:53:06, Daniele Castagna wrote:
> > nit: !modifiers.size()
> 
> Actually, use: modifiers.empty()

Here's an update with the couple of nits fixed and a unit test for the error
condition that we're fixing.

[dnicoara, 2017-06-15 14:24:43]

lgtm with minor fixes.

Thanks for the unittest!

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

https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/mock_scanout_buffer.cc:8: namespace {
nit: Nest anonymous namespace in the ui namespace.

https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/mock_scanout_buffer.cc:10: uint32_t
currentFramebufferId;
Global declarations follow the "g_current_framebuffer_id" style.

Also, initialize it to 1.

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

https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/mock_scanout_buffer.h:40: uint32_t id;
id_

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

https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/screen_manager_unittest.cc:531: LOG(ERROR) << "Create
buffer " << buffer.get();
Remove log.

[hoegsberg1, 2017-06-15 16:57:35]

On 2017/06/15 14:24:43, dnicoara wrote:
> lgtm with minor fixes.
> 
> Thanks for the unittest!

Thanks for the feedback and for taking the time to help review this.

> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/mock_scanout_buffer.cc (right):
> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/mock_scanout_buffer.cc:8: namespace {
> nit: Nest anonymous namespace in the ui namespace.
> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/mock_scanout_buffer.cc:10: uint32_t
> currentFramebufferId;
> Global declarations follow the "g_current_framebuffer_id" style.
> 
> Also, initialize it to 1.
> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/mock_scanout_buffer.h (right):
> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/mock_scanout_buffer.h:40: uint32_t id;
> id_
> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/screen_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2919533003/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/screen_manager_unittest.cc:531: LOG(ERROR) <<
"Create
> buffer " << buffer.get();
> Remove log.

Good points, all done.

[hoegsberg1, 2017-06-15 18:34:02]

The CQ bit was checked by hoegsberg@google.com

[hoegsberg1, 2017-06-15 18:34:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/2919533003/#ps40001
(title: "ozone/drm: Only reuse ScanoutBuffers with compatible modifiers")

[commit-bot: I haz the power, 2017-06-15 18:34:49]

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-06-15 19:15:58]

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

[commit-bot: I haz the power, 2017-06-15 19:15:59]

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_...)

[hoegsberg1, 2017-06-15 19:41:13]

On 2017/06/15 at 19:15:59, commit-bot wrote:
> 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_...)

Fixed DrmBuffer::GetFormatModifier() to not be inline.

[hoegsberg1, 2017-06-15 19:41:21]

The CQ bit was checked by hoegsberg@google.com

[hoegsberg1, 2017-06-15 19:41:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/2919533003/#ps60001
(title: "ozone/drm: Only reuse ScanoutBuffers with compatible modifiers")

[commit-bot: I haz the power, 2017-06-15 19:41:56]

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-06-15 20:30:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497555681248130,
"parent_rev": "6d5a279e4cd796ec67b9997e2c7912e5ee8f5868", "commit_rev":
"9947afe90294ff5ad902e2cc02f8064506f7f0a4"}

[commit-bot: I haz the power, 2017-06-15 20:30:28]

Description was changed from

==========
ozone/drm: Only reuse ScanoutBuffers with compatible modifiers

Now that we have modifiers, we need to make sure that not just the
format, but also the format modifer is compatible with the
HardwareDisplayController where we want to use the buffer.

BUG=b/38217940
TEST=Plug in external monitor that supports same resolution as internal
  panel with mirror mode enabled
==========

to

==========
ozone/drm: Only reuse ScanoutBuffers with compatible modifiers

Now that we have modifiers, we need to make sure that not just the
format, but also the format modifer is compatible with the
HardwareDisplayController where we want to use the buffer.

BUG=b/38217940
TEST=Plug in external monitor that supports same resolution as internal
  panel with mirror mode enabled

Review-Url: https://codereview.chromium.org/2919533003
Cr-Commit-Position: refs/heads/master@{#479813}
Committed:
https://chromium.googlesource.com/chromium/src/+/9947afe90294ff5ad902e2cc02f8...
==========

[commit-bot: I haz the power, 2017-06-15 20:30:29]

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

[marcheu1, 2017-06-19 23:40:43]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2948513003/ by marcheu@google.com.

The reason for reverting is: This breaks nyan (see crbug.com/734074) so let's
revert to hopefully have a green PFQ run today..

[hoegsberg1, 2017-06-20 00:24:06]

On 2017/06/19 at 23:40:43, marcheu wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2948513003/ by marcheu@google.com.
> 
> The reason for reverting is: This breaks nyan (see crbug.com/734074) so let's
revert to hopefully have a green PFQ run today..

Fix here:

 https://chromium-review.googlesource.com/c/540817/?polygerrit=1