ozone: Allocate scanout buffer according to KMS modifiers

ozone: Allocate scanout buffer according to KMS modifiers

BUG=chrome-os-partner:56407

[hoegsberg1, 2016-12-15 01:08:12]

Description was changed from

==========
ozone: Allocate scanout buffer according to KMS modifiers

BUG=chrome-os-partner:56407
==========

to

==========
ozone: Allocate scanout buffer according to KMS modifiers

BUG=chrome-os-partner:56407
==========

[hoegsberg1, 2016-12-15 01:08:13]

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

[hoegsberg1, 2016-12-15 01:17:26]

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

[hoegsberg1, 2016-12-15 05:42:34]

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

[hoegsberg1, 2016-12-15 05:42:51]

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

[hoegsberg1, 2016-12-15 18:37:38]

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

[Daniele Castagna, 2016-12-16 00:27:42]

Thank you for looking into the Ozone integration too.

Do you mind expanding a little bit the CL description?

Did you run "git cl format" before uploading a patch.

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.h:61: uint32_t
GetFormatModifiers(uint32_t format,
Can we use a vector here?
Something like:
std::vector<uint64_t> GetFormatModifiers(uint32_t format);

If you prefer to append to a vector, since in
HardwareDisplayController::GetFormatModifiers is needed, I'd go with something
like:


void AppendFormatModifiers(uint32_t format, std::vector<uint64_t>* modifiers);

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_buffer.cc:74: handles, strides, offsets, NULL,
&framebuffer_, 0)) {
nit: nullptr

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:131: uint64_t modifiers[16];
It'd be nice if this could go away (for a vector maybe).

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:132: uint32_t nmodifiers = 0;
nit: s/nmodifiers/num_modifiers or modifiers_size

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:134: if (window &&
window->GetController()) {
Did we figure out when this is not the case?

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:139: if (nmodifiers > 0 && !(flags &
GBM_BO_USE_LINEAR))
Why do we need to special case for GBM_BO_USE_LINEAR?

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:140: *buffer =
GbmBuffer::CreateBufferWithModifiers(
nit: if you split the body of an if, you need to add brackets.

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:31: static scoped_refptr<GbmBuffer>
CreateBufferWithModifiers(
Why do we need CreateBufferWithModifiers? Can't we just add modifiers to
CreateBuffer?

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:66: for (auto& element :
supported_format_info_) {
Was this for debugging?

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:130: DCHECK(format_index <
supported_formats_.size());
nit: DCHECK_LT

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:32: const std::vector<struct
drm_format_info>& format_info,
I don't think struct is necessary here.

[hoegsberg1, 2016-12-17 01:10:48]

hoegsberg@google.com changed reviewers:
+ hoegsberg@google.com

[hoegsberg1, 2016-12-17 01:10:49]

Thanks for the review - most issues fixed, except for whether or not to create a
new CreateBufferWithModifiers()

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.h:61: uint32_t
GetFormatModifiers(uint32_t format,
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> Can we use a vector here?
> Something like:
> std::vector<uint64_t> GetFormatModifiers(uint32_t format);
> 
> If you prefer to append to a vector, since in
> HardwareDisplayController::GetFormatModifiers is needed, I'd go with something
> like:
> 
> 
> void AppendFormatModifiers(uint32_t format, std::vector<uint64_t>* modifiers);

I like the vector, will update. Don't like AppendFormatModifiers, since it's not
supposed to be used in a way where we append to modifiers already in the vector
- it expects an empty vector and sets it to the entire set of modifiers.

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_buffer.cc:74: handles, strides, offsets, NULL,
&framebuffer_, 0)) {
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> nit: nullptr

Acknowledged.

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:131: uint64_t modifiers[16];
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> It'd be nice if this could go away (for a vector maybe).

Acknowledged.

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:132: uint32_t nmodifiers = 0;
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> nit: s/nmodifiers/num_modifiers or modifiers_size

Ack, but it becomes modifers.size(), even better.

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:134: if (window &&
window->GetController()) {
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> Did we figure out when this is not the case?

No... maybe related to the logout button race? (crbug.com/671293)

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:139: if (nmodifiers > 0 && !(flags &
GBM_BO_USE_LINEAR))
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> Why do we need to special case for GBM_BO_USE_LINEAR?

If we need a linear buffer, we can either 1) go through the regular
gbm_bo_create() and pass the use flag or 2) filter the modifiers and leave only
DRM_FORMAT_MOD_NONE (= linear).  If we don't do anything, we end up allocating
an AFBC buffer.

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:140: *buffer =
GbmBuffer::CreateBufferWithModifiers(
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> nit: if you split the body of an if, you need to add brackets.

Acknowledged.

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:31: static scoped_refptr<GbmBuffer>
CreateBufferWithModifiers(
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> Why do we need CreateBufferWithModifiers? Can't we just add modifiers to
> CreateBuffer?

Hm, I suppose we could just do that. It's a little subtle though that in case it
receive a modifier list it completely ignores the flags. So you get different
semantics whether or not you pass in a modifier vector.

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:66: for (auto& element :
supported_format_info_) {
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> Was this for debugging?

Yeah, removed it.

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:130: DCHECK(format_index <
supported_formats_.size());
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> nit: DCHECK_LT

Done - what's better about DCHECK_LT though?

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:32: const std::vector<struct
drm_format_info>& format_info,
On 2016/12/16 00:27:41, Daniele Castagna wrote:
> I don't think struct is necessary here.

Acknowledged.

[hoegsberg1, 2016-12-19 19:18:46]

Updated patch, now shouldn't have any whitespace warnings. Switched to just
return a std::vector<uint64_t> of modifiers instead of passing a vector to be
filled in.

[dnicoara, 2016-12-19 19:44:46]

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

https://codereview.chromium.org/2576263002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:31: static scoped_refptr<GbmBuffer>
CreateBufferWithModifiers(
On 2016/12/17 01:10:49, hoegsberg1 wrote:
> On 2016/12/16 00:27:41, Daniele Castagna wrote:
> > Why do we need CreateBufferWithModifiers? Can't we just add modifiers to
> > CreateBuffer?
> 
> Hm, I suppose we could just do that. It's a little subtle though that in case
it
> receive a modifier list it completely ignores the flags. So you get different
> semantics whether or not you pass in a modifier vector.

Should |flags| be removed from the definition then?

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:127: DCHECK(screen_manager_);
nit: No need to DCHECK, the following access will just crash if screen_manager_
is nullptr. Also, the only reason this would be nullptr is if Init() wasn't run.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:142: scoped_refptr<GbmBuffer>
GbmBuffer::CreateBufferWithModifiers(
It seems that only the gbm_bo_create()/gbm_bo_create_with_modifiers() is
different between CreateBuffer() and CreateBufferWithModifiers(); would it be
possible to have a helper function to share most of the common code?

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:153: format, &modifiers[0],
modifiers.size());
nit: Prefer modifiers.data().

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:148: //
TODO(hoegsberg): We need to intersect modifiers from different
I'm wondering what are the implications of not implementing this TODO right now
on existing platforms? Could this cause regressions?

[marcheu1, 2016-12-20 01:25:30]

marcheu@google.com changed reviewers:
+ marcheu@google.com

[marcheu1, 2016-12-20 01:25:31]

we will also have to tackle the issue of external display resolution being
limited by being on the little vop

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:148: //
TODO(hoegsberg): We need to intersect modifiers from different
On 2016/12/19 19:44:46, dnicoara wrote:
> I'm wondering what are the implications of not implementing this TODO right
now
> on existing platforms? Could this cause regressions?

I thought right now there should be none since we are hopefully allocating
buffers for evdi or udl on a different instance?

[dnicoara, 2016-12-20 15:35:42]

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:148: //
TODO(hoegsberg): We need to intersect modifiers from different
On 2016/12/20 01:25:30, marcheu1 wrote:
> On 2016/12/19 19:44:46, dnicoara wrote:
> > I'm wondering what are the implications of not implementing this TODO right
> now
> > on existing platforms? Could this cause regressions?
> 
> I thought right now there should be none since we are hopefully allocating
> buffers for evdi or udl on a different instance?

Yes, I believe we do software mirror for evdi and udl.

Just wanted to confirm that this doesn't apply to existing devices. Thank you
for confirming.

[Daniele Castagna, 2016-12-20 18:03:29]

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/scoped_drm_types.h (right):

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/scoped_drm_types.h:21: struct drm_format_modifier;
nit: typedef to stay consistent so we don't need to type struct all the time we
need to declare it? We could also typedef it to a camel case name to keep naming
conventions consistent.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.h:61: std::vector<uint64_t>
GetFormatModifiers(uint32_t format);
nit: a comment explaining what a modifier for a format is would be nice. While
there, I'd also add that the format is a fourcc format. You can also change the
parameter name to fourcc_format to stay consistent with the method above.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_buffer.cc:74: handles, strides, offsets, nullptr,
I personally don't like passing a nullptr for an array parameter. It seems like
we're exploiting a deficiency of the type system.
I'm assuming we want to maintain the interface the same as  AddFramebuffer2
though. So nothing to do here.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_device.cc:484: return
!drmModeAddFB2WithModifiers(file_.GetPlatformFile(), width, height,
How does this compile on the trybot where we use an old precise sysroot and this
is not even defined?

crrev.com/2594483002 might turn out to be useful for this too.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:133: if (window &&
window->GetController())
It'd be nice to understand why this happens, or at least leave a comment
mentioning we need to investigate further.
I don't think it's related to the logout button race we observed, since that one
was about adding ui elements.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:142: scoped_refptr<GbmBuffer>
GbmBuffer::CreateBufferWithModifiers(
On 2016/12/19 at 19:44:46, dnicoara wrote:
> It seems that only the gbm_bo_create()/gbm_bo_create_with_modifiers() is
different between CreateBuffer() and CreateBufferWithModifiers(); would it be
possible to have a helper function to share most of the common code?

Maybe we could have just GbmBuffer::CreateBuffer that always takes a vector
modifiers.
We can then change behavior of this method based on modifiers.lenght here,
basically moving the logic that is now in DrmThread::CreateBuffer to this
method.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:150: for (const auto&
controller : crtc_controllers_)
nit: maybe
  if (!crtc_controllers_.empty())
    return crtc_controllers_->front()->GetFormatModifiers(format)

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.h:130: // Add GBM usage
flags to |flags| as needed for the |format| and
Does this comment need to be updated with the new GetFormatModifiers definition?

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:124: for (const auto& f :
supported_formats_) {
This is a vector of ints. In general, it doesn't make much sense to get a
reference to a int.

While you're there, you could probably just replace this with:

uint32_t format_index = std::find(supported_format_.begin(),
supported_format_.end(), format) - supported_format.begin();
The DCHECK would still apply.

Btw, what happens when this is a dummy plane?

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:131: for (const auto&
modifier : supported_format_modifiers_) {
No need to use a ref for built in types.

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:30: const std::vector<struct
drm_format_modifier>& format_info,
should we typedef drm_format modifier?

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc:113: for (uint32_t j
= 0; j < format_modifiers_size; j++)
nit: s/uint32_t/size_t

[Daniele Castagna, 2016-12-20 19:08:59]

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

[commit-bot: I haz the power, 2016-12-20 19:09:21]

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, 2016-12-20 20:09:18]

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

[commit-bot: I haz the power, 2016-12-20 20:09:18]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[hoegsberg1, 2016-12-22 23:05:21]

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/scoped_drm_types.h (right):

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/scoped_drm_types.h:21: struct drm_format_modifier;
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> nit: typedef to stay consistent so we don't need to type struct all the time
we
> need to declare it? We could also typedef it to a camel case name to keep
naming
> conventions consistent.

This was just working around a missing include of xf86drmMode.h in
hardware_display_plane.h.  I've added that instead.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.h:61: std::vector<uint64_t>
GetFormatModifiers(uint32_t format);
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> nit: a comment explaining what a modifier for a format is would be nice. While
> there, I'd also add that the format is a fourcc format. You can also change
the
> parameter name to fourcc_format to stay consistent with the method above.

Acknowledged.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_buffer.cc:74: handles, strides, offsets, nullptr,
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> I personally don't like passing a nullptr for an array parameter. It seems
like
> we're exploiting a deficiency of the type system.
> I'm assuming we want to maintain the interface the same as  AddFramebuffer2
> though. So nothing to do here.
> 

¯\_(ツ)_/¯

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_device.cc:484: return
!drmModeAddFB2WithModifiers(file_.GetPlatformFile(), width, height,
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> How does this compile on the trybot where we use an old precise sysroot and
this
> is not even defined?
> 
> crrev.com/2594483002 might turn out to be useful for this too.

I've been using cros sdk with the chromiumos buildroot where I have the right
libdrm version. Thanks for getting the sysroots updated.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:127: DCHECK(screen_manager_);
On 2016/12/19 19:44:46, dnicoara wrote:
> nit: No need to DCHECK, the following access will just crash if
screen_manager_
> is nullptr. Also, the only reason this would be nullptr is if Init() wasn't
run.

Acknowledged.

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.cc:133: if (window &&
window->GetController())
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> It'd be nice to understand why this happens, or at least leave a comment
> mentioning we need to investigate further.
> I don't think it's related to the logout button race we observed, since that
one
> was about adding ui elements.

I added a todo there.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:142: scoped_refptr<GbmBuffer>
GbmBuffer::CreateBufferWithModifiers(
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> On 2016/12/19 at 19:44:46, dnicoara wrote:
> > It seems that only the gbm_bo_create()/gbm_bo_create_with_modifiers() is
> different between CreateBuffer() and CreateBufferWithModifiers(); would it be
> possible to have a helper function to share most of the common code?
> 
> Maybe we could have just GbmBuffer::CreateBuffer that always takes a vector
> modifiers.
> We can then change behavior of this method based on modifiers.lenght here,
> basically moving the logic that is now in DrmThread::CreateBuffer to this
> method.

I wanted to keep the two paths separate: either you allocate with flags or with
modifiers. But since a few places check for (flags & SCANOUT) to see if a buffer
is created for scanout, I decided to take both flags and modifiers in the
modifier case.

Longer term we should move away from flags to modifiers, so I wanted to avoid
conflating the two code paths. I suppose we can refactor towards that later on,
so it's not too important.

The important thing that's missing here is that we can't actually create the FB
with modifiers if we didn't create the bo with modifiers. So I need to pass a
bool into the GbmBufferBase constructor to indicate whether or not to pass the
modifiers there...

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:153: format, &modifiers[0],
modifiers.size());
On 2016/12/19 19:44:46, dnicoara wrote:
> nit: Prefer modifiers.data().

Acknowledged.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:148: //
TODO(hoegsberg): We need to intersect modifiers from different
On 2016/12/20 15:35:42, dnicoara wrote:
> On 2016/12/20 01:25:30, marcheu1 wrote:
> > On 2016/12/19 19:44:46, dnicoara wrote:
> > > I'm wondering what are the implications of not implementing this TODO
right
> > now
> > > on existing platforms? Could this cause regressions?
> > 
> > I thought right now there should be none since we are hopefully allocating
> > buffers for evdi or udl on a different instance?
> 
> Yes, I believe we do software mirror for evdi and udl.
> 
> Just wanted to confirm that this doesn't apply to existing devices. Thank you
> for confirming.

It looks like a HardwareDisplayController will have multiple CrtcControllers
when we're doing mirror mode. In that case we need to make sure we allocate a
buffer that works for all actual CRTCs in the HardwareDisplayController. In case
of kevin, one CRTC will support AFBC while the other wont. Intersecting the sets
here means that we'll drop the AFBC modifier, since it's not in both sets.

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.cc:150: for (const auto&
controller : crtc_controllers_)
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> nit: maybe
>   if (!crtc_controllers_.empty())
>     return crtc_controllers_->front()->GetFormatModifiers(format)
> 

I added the intersection code instead.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_controller.h:130: // Add GBM usage
flags to |flags| as needed for the |format| and
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> Does this comment need to be updated with the new GetFormatModifiers
definition?

Acknowledged.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:124: for (const auto& f :
supported_formats_) {
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> This is a vector of ints. In general, it doesn't make much sense to get a
> reference to a int.
> 
> While you're there, you could probably just replace this with:
> 
> uint32_t format_index = std::find(supported_format_.begin(),
> supported_format_.end(), format) - supported_format.begin();
> The DCHECK would still apply.

Sure. I guess that's idiomatic, but it's a lot harder to read.

> Btw, what happens when this is a dummy plane?

We initialize with an empty vector for modifiers. This function returns an empty
vector and buffer allocation falls back to old-fashion use-flag entry point.

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:131: for (const auto&
modifier : supported_format_modifiers_) {
On 2016/12/20 18:03:28, Daniele Castagna wrote:
> No need to use a ref for built in types.

We're iterating through std:vector<struct drm_format_modifier> here.

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:30: const std::vector<struct
drm_format_modifier>& format_info,
On 2016/12/20 18:03:29, Daniele Castagna wrote:
> should we typedef drm_format modifier?

Fixed by including xf86drmMode.h in the right place.

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

https://codereview.chromium.org/2576263002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc:113: for (uint32_t j
= 0; j < format_modifiers_size; j++)
On 2016/12/20 18:03:29, Daniele Castagna wrote:
> nit: s/uint32_t/size_t

I picked uint32_t deliberately to match format_modifiers_size, which in turn
matches drm_plane->count_format_modifiers.

[Daniele Castagna, 2016-12-23 00:32:54]

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

[commit-bot: I haz the power, 2016-12-23 00:33:30]

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, 2016-12-23 01:09:03]

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

[commit-bot: I haz the power, 2016-12-23 01:09:04]

Dry run: 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-01-09 15:55:33]

It seems to fail to compile ozone_unittests. Can you try building and running
those unittests?

[Daniele Castagna, 2017-01-11 04:53:48]

On 2017/01/09 at 15:55:33, Daniele Castagna wrote:
> It seems to fail to compile ozone_unittests. Can you try building and running
those unittests?

As discussed with hoegsberg@, re-uploaded to crrev.com/2623963002 and fixed
unittests.