Preliminary atomic patch

ozone: Preliminary atomic patch

Adds atomic plane, plane manager and define to enable
Currently only in the GN build and disabled - no platforms support the ioctl so there's no reason to enable it anywhere

Committed: https://crrev.com/10660cc12bd27506a1353a9a04105c512f410eec
Cr-Commit-Position: refs/heads/master@{#321207}

[achaulk, 2015-03-09 19:29:20]

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

[dnicoara, 2015-03-09 20:10:41]

The GYP change is missing.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:157: virtual bool
CommitProperties(drmModePropertySet* properties,
This will need to be mocked in test/mock_drm_device.h

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:158: uint32_t flags,
|flags| and |user_ptr| are never set to different values. Can we just hide them
in the implementation and not pass them in?

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:14: namespace {
space above

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:35:
ScopedDrmPropertyPtr property(
Check that the returned property is a valid object.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:82: LOG(ERROR) <<
"Failed to set plane data";
Use PLOG since it outputs the errno value.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:93: LOG(ERROR) <<
"Unable to get plane properties.";
PLOG

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h:21: bool
Initialize(DrmDevice* drm) override;
// HardwareDisplayPlane:

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h:22: bool
SetPlaneData(drmModePropertySet* property_set,
Non-virtual should be above virtual declarations.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:7: //#include
"ui/gfx/geometry/rect_i.h"
remove

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:38:
LOG(ERROR) << "Failed to commit properties";
PLOG

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.h:14:
HardwareDisplayPlaneManagerAtomic();
virtual destructor

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.h:16: bool
AssignOverlayPlanes(HardwareDisplayPlaneList* plane_list,
// HardwareDisplayPlaneManager:

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/scoped_drm_types.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/scoped_drm_types.h:19: typedef struct
_drmModePropertySet drmModePropertySet;
struct _drmModePropertySet is never defined if we're not compiling with atomic
APIs. I think you'll need to wrap this in an #if def(OZONE_USE_ATOMIC).

[achaulk, 2015-03-09 20:23:49]

I'm hoping gyp goes away before we use this

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:157: virtual bool
CommitProperties(drmModePropertySet* properties,
On 2015/03/09 20:10:40, dnicoara wrote:
> This will need to be mocked in test/mock_drm_device.h

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:158: uint32_t flags,
On 2015/03/09 20:10:40, dnicoara wrote:
> |flags| and |user_ptr| are never set to different values. Can we just hide
them
> in the implementation and not pass them in?

They will likely be set to handle some sort of callbacks later on, once that's
figured out

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:35:
ScopedDrmPropertyPtr property(
On 2015/03/09 20:10:40, dnicoara wrote:
> Check that the returned property is a valid object.

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:82: LOG(ERROR) <<
"Failed to set plane data";
On 2015/03/09 20:10:40, dnicoara wrote:
> Use PLOG since it outputs the errno value.

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:93: LOG(ERROR) <<
"Unable to get plane properties.";
On 2015/03/09 20:10:40, dnicoara wrote:
> PLOG

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h:21: bool
Initialize(DrmDevice* drm) override;
On 2015/03/09 20:10:41, dnicoara wrote:
> // HardwareDisplayPlane: 

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h:22: bool
SetPlaneData(drmModePropertySet* property_set,
On 2015/03/09 20:10:41, dnicoara wrote:
> Non-virtual should be above virtual declarations.

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:38:
LOG(ERROR) << "Failed to commit properties";
On 2015/03/09 20:10:41, dnicoara wrote:
> PLOG

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.h:14:
HardwareDisplayPlaneManagerAtomic();
On 2015/03/09 20:10:41, dnicoara wrote:
> virtual destructor

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.h:16: bool
AssignOverlayPlanes(HardwareDisplayPlaneList* plane_list,
On 2015/03/09 20:10:41, dnicoara wrote:
> // HardwareDisplayPlaneManager:

Done.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/scoped_drm_types.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/scoped_drm_types.h:19: typedef struct
_drmModePropertySet drmModePropertySet;
On 2015/03/09 20:10:41, dnicoara wrote:
> struct _drmModePropertySet is never defined if we're not compiling with atomic
> APIs. I think you'll need to wrap this in an #if def(OZONE_USE_ATOMIC).

That's actually fine, the "struct foo" part also declares the struct

[dnicoara, 2015-03-09 20:42:58]

Soo, when is this expected to land?

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:158: uint32_t flags,
On 2015/03/09 20:23:48, achaulk wrote:
> On 2015/03/09 20:10:40, dnicoara wrote:
> > |flags| and |user_ptr| are never set to different values. Can we just hide
> them
> > in the implementation and not pass them in?
> 
> They will likely be set to handle some sort of callbacks later on, once that's
> figured out

Are you planing on adding more functionality part of this patch?

If not, we should add these flags later. It would be nice to have |user_ptr| of
some specific type.

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/scoped_drm_types.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/scoped_drm_types.h:19: typedef struct
_drmModePropertySet drmModePropertySet;
On 2015/03/09 20:23:49, achaulk wrote:
> On 2015/03/09 20:10:41, dnicoara wrote:
> > struct _drmModePropertySet is never defined if we're not compiling with
atomic
> > APIs. I think you'll need to wrap this in an #if def(OZONE_USE_ATOMIC).
> 
> That's actually fine, the "struct foo" part also declares the struct

Acknowledged.

https://codereview.chromium.org/994503004/diff/40001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h (right):

https://codereview.chromium.org/994503004/diff/40001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h:36: uint32_t id_;
Just |id| since that's how the style guide dictates this needs to be for
structs.

[achaulk, 2015-03-09 20:55:19]

We can land it whenever but I don't think we will turn it on for a couple
months. Even if we did the gyp changes we couldn't turn the define on because
nothing supports it yet

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:158: uint32_t flags,
On 2015/03/09 20:42:58, dnicoara wrote:
> On 2015/03/09 20:23:48, achaulk wrote:
> > On 2015/03/09 20:10:40, dnicoara wrote:
> > > |flags| and |user_ptr| are never set to different values. Can we just hide
> > them
> > > in the implementation and not pass them in?
> > 
> > They will likely be set to handle some sort of callbacks later on, once
that's
> > figured out
> 
> Are you planing on adding more functionality part of this patch?
> 
> If not, we should add these flags later. It would be nice to have |user_ptr|
of
> some specific type.

Not yet, since we don't even have a complete definition yet, but I suspect flags
will specify what events we want and the user_ptr will just be a generic
callback pointer, but the actual mechanism is unknown for now

[dnicoara, 2015-03-09 20:57:26]

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:158: uint32_t flags,
On 2015/03/09 20:55:18, achaulk wrote:
> On 2015/03/09 20:42:58, dnicoara wrote:
> > On 2015/03/09 20:23:48, achaulk wrote:
> > > On 2015/03/09 20:10:40, dnicoara wrote:
> > > > |flags| and |user_ptr| are never set to different values. Can we just
hide
> > > them
> > > > in the implementation and not pass them in?
> > > 
> > > They will likely be set to handle some sort of callbacks later on, once
> that's
> > > figured out
> > 
> > Are you planing on adding more functionality part of this patch?
> > 
> > If not, we should add these flags later. It would be nice to have |user_ptr|
> of
> > some specific type.
> 
> Not yet, since we don't even have a complete definition yet, but I suspect
flags
> will specify what events we want and the user_ptr will just be a generic
> callback pointer, but the actual mechanism is unknown for now

For the callback, we can do the same as SchedulePageFlip and provide a concrete
PageFlipCallback.

[achaulk, 2015-03-09 21:41:00]

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.h (right):

https://codereview.chromium.org/994503004/diff/20001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.h:158: uint32_t flags,
On 2015/03/09 20:57:26, dnicoara wrote:
> On 2015/03/09 20:55:18, achaulk wrote:
> > On 2015/03/09 20:42:58, dnicoara wrote:
> > > On 2015/03/09 20:23:48, achaulk wrote:
> > > > On 2015/03/09 20:10:40, dnicoara wrote:
> > > > > |flags| and |user_ptr| are never set to different values. Can we just
> hide
> > > > them
> > > > > in the implementation and not pass them in?
> > > > 
> > > > They will likely be set to handle some sort of callbacks later on, once
> > that's
> > > > figured out
> > > 
> > > Are you planing on adding more functionality part of this patch?
> > > 
> > > If not, we should add these flags later. It would be nice to have
|user_ptr|
> > of
> > > some specific type.
> > 
> > Not yet, since we don't even have a complete definition yet, but I suspect
> flags
> > will specify what events we want and the user_ptr will just be a generic
> > callback pointer, but the actual mechanism is unknown for now
> 
> For the callback, we can do the same as SchedulePageFlip and provide a
concrete
> PageFlipCallback.

Sure, I'll just reuse the same one until we know more about the implementation

https://codereview.chromium.org/994503004/diff/40001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h (right):

https://codereview.chromium.org/994503004/diff/40001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.h:36: uint32_t id_;
On 2015/03/09 20:42:58, dnicoara wrote:
> Just |id| since that's how the style guide dictates this needs to be for
> structs.

Done.

[achaulk, 2015-03-13 19:21:36]

ping

[dnicoara, 2015-03-13 19:27:18]

I thought we were waiting until we're getting actual support before landing? If
we're landing it now, you'll want to add the GYP changes as well. Otherwise it
looks OK.

Also, FYI, libdrm is being uprev-ed (and contains the atomic API), so it may be
a good chance to actually test this for  potential errors.

https://codereview.chromium.org/994503004/diff/60001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc (right):

https://codereview.chromium.org/994503004/diff/60001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:30:
plane_list->plane_list.begin(),
s/.begin/.end()/

[achaulk, 2015-03-13 19:51:42]

We already have the APIs in the headers, but we're missing kernel support, so
no-go there. The idea is to just check it in to prevent having to rebase the
common changes all the time and so we can point to it and say "hey we're ready
to go once you get the kernel stuff done"

I'd rather not plumb the gyp variable through until we need to though

[dnicoara, 2015-03-13 20:11:29]

On 2015/03/13 19:51:42, achaulk wrote:
> We already have the APIs in the headers, but we're missing kernel support, so
> no-go there. The idea is to just check it in to prevent having to rebase the
> common changes all the time and so we can point to it and say "hey we're ready
> to go once you get the kernel stuff done"
> 
> I'd rather not plumb the gyp variable through until we need to though

So why not just compile these files by default? We'd at least know that there
aren't any compile time errors?

[achaulk, 2015-03-13 20:13:11]

Hmm, previously there were some builds that broke, I can always try again now
though

[achaulk, 2015-03-13 20:15:46]

Oh right it would break the locally compiled X11 platform. Which never got in
and nobody has been complaining so maybe we don't care about it anymore?

[dnicoara, 2015-03-13 20:19:02]

Also, could you please update the CL title and description and be more explicit
to what this CL is doing :)

[dnicoara, 2015-03-14 02:07:44]

On 2015/03/13 20:15:46, achaulk wrote:
> Oh right it would break the locally compiled X11 platform. Which never got in
> and nobody has been complaining so maybe we don't care about it anymore?

It shouldn't break the an X11 Ozone platform since Ozone platforms should be
self contained. (unless we're leaking some configuration or implementation
detail)

Lets try and have it compiled. I prefer code that won't rot as fast.

[achaulk, 2015-03-16 20:47:35]

It will break the buildability of everything on X. Unless we can guarantee that
this only only ever build for cros

[achaulk, 2015-03-16 21:05:00]

https://codereview.chromium.org/994503004/diff/60001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc (right):

https://codereview.chromium.org/994503004/diff/60001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc:30:
plane_list->plane_list.begin(),
On 2015/03/13 19:27:18, dnicoara wrote:
> s/.begin/.end()/

Done.

[dnicoara, 2015-03-16 21:14:11]

Arr, sorry, missed one. lgtm with include change

https://codereview.chromium.org/994503004/diff/80001/ui/ozone/platform/drm/gp...
File ui/ozone/platform/drm/gpu/drm_device.cc (right):

https://codereview.chromium.org/994503004/diff/80001/ui/ozone/platform/drm/gp...
ui/ozone/platform/drm/gpu/drm_device.cc:22: #if defined(OZONE_USE_ATOMIC)
move #ifdef include block after the normal includes.

[spang, 2015-03-16 21:17:20]

Can you rename the build variable & ifdef to something like

  USE_DRM_ATOMIC

This is a dependency issue and there is no reason to say "ozone" here.

[achaulk, 2015-03-18 20:33:20]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-03-18 20:33:21]

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/994503004/#ps160001
(title: "change gyp var")

[commit-bot: I haz the power, 2015-03-18 20:33:52]

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

[commit-bot: I haz the power, 2015-03-18 21:24:37]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-18 21:25:17]

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