[1/2][Ozone-Drm] Refactor GPU display management

[Ozone-Drm] Refactor GPU display management

This change decouples the DisplaySnapshot from the DrmDisplay since the
snapshot is only needed to IPC the display information to the browser
process. Also, move all the display configuration calls into the
DrmDisplay object to simplify state and clarify the intent of the
operations.

This change also moves the logic to parse the DRM display state into
DisplaySnapshot objects into a common location intended to be used by
both GPU and Browser processes. The parsing logic will be used in a
subsequent change to allow proper parsing of display configuration on
the browser side during startup.

BUG=484294
Committed: https://crrev.com/f8d980302e5136fd9b25f8f2c891d1f3859c6ddb
Cr-Commit-Position: refs/heads/master@{#329462}

[dnicoara, 2015-05-11 18:46:39]

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

[spang, 2015-05-11 21:21:48]

Can you mention the patch dependencies when you send out the CR?

[dnicoara, 2015-05-11 21:23:08]

On 2015/05/11 21:21:48, spang wrote:
> Can you mention the patch dependencies when you send out the CR?

Sorry, I've updated their subject line to reflect order.

[spang, 2015-05-11 21:49:29]

On 2015/05/11 21:23:08, dnicoara wrote:
> On 2015/05/11 21:21:48, spang wrote:
> > Can you mention the patch dependencies when you send out the CR?
> 
> Sorry, I've updated their subject line to reflect order.

This says #1 and doesn't apply (multiple conflicts..).

[dnicoara, 2015-05-11 21:50:00]

On 2015/05/11 21:49:29, spang wrote:
> On 2015/05/11 21:23:08, dnicoara wrote:
> > On 2015/05/11 21:21:48, spang wrote:
> > > Can you mention the patch dependencies when you send out the CR?
> > 
> > Sorry, I've updated their subject line to reflect order.
> 
> This says #1 and doesn't apply (multiple conflicts..).

You may need to sync to ToT :)

[spang, 2015-05-11 22:21:32]

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:98: for (int i = 0; i <
connector->count_props; ++i) {
How come we don't remember the property ids anywhere?

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:118: if (name == property->name &&
property->flags & DRM_MODE_PROP_BLOB)
Can you add another pair of parenthesis here around the bitwise and?

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:131: for (int props_i = 0; props_i <
connector->count_props; ++props_i) {
Could GetDrmProperty return the index instead of finding it again?

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:135: for (int enums_i = 0; enums_i <
property->count_enums; ++enums_i) {
There's too much going on here. Please factor this search into a separate
function. It'll be much more readable.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:232: drmModeModeInfo& mode =
info->connector()->modes[i];
Google style guide only allows const & and *

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.h (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:43: DisplayMode_Params
ParseDrmMode(const drmModeModeInfo& mode);
I don't think "Parse" is the right verb here. The input is not a string. Maybe
"Convert".

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:47: size_t index);
What is 'index'? Needs more docs.

[dnicoara, 2015-05-12 15:17:11]

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:98: for (int i = 0; i <
connector->count_props; ++i) {
On 2015/05/11 22:21:32, spang wrote:
> How come we don't remember the property ids anywhere?

They can change between display events and we only need a small subset when we
refresh the display state, so it isn't worth caching them.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:118: if (name == property->name &&
property->flags & DRM_MODE_PROP_BLOB)
On 2015/05/11 22:21:32, spang wrote:
> Can you add another pair of parenthesis here around the bitwise and?

Simplified, so I don't think it should matter anymore.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:131: for (int props_i = 0; props_i <
connector->count_props; ++props_i) {
On 2015/05/11 22:21:32, spang wrote:
> Could GetDrmProperty return the index instead of finding it again?

Done.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:135: for (int enums_i = 0; enums_i <
property->count_enums; ++enums_i) {
On 2015/05/11 22:21:32, spang wrote:
> There's too much going on here. Please factor this search into a separate
> function. It'll be much more readable.

Now that the outer for-loop is gone this should be simpler.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:232: drmModeModeInfo& mode =
info->connector()->modes[i];
On 2015/05/11 22:21:32, spang wrote:
> Google style guide only allows const & and *

Oops, done.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.h (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:43: DisplayMode_Params
ParseDrmMode(const drmModeModeInfo& mode);
On 2015/05/11 22:21:32, spang wrote:
> I don't think "Parse" is the right verb here. The input is not a string. Maybe
> "Convert".

Actually, I don't like the "DrmMode" part too. Renamed it.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:47: size_t index);
On 2015/05/11 22:21:32, spang wrote:
> What is 'index'? Needs more docs. 

Done.

[spang, 2015-05-12 16:58:53]

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:98: for (int i = 0; i <
connector->count_props; ++i) {
On 2015/05/12 15:17:10, dnicoara wrote:
> On 2015/05/11 22:21:32, spang wrote:
> > How come we don't remember the property ids anywhere?
> 
> They can change between display events and we only need a small subset when we
> refresh the display state, so it isn't worth caching them.

I don't think the ids can change. It looks like the lifetime of the property is
the lifetime of the DRM device.

I'm not talking about the property values, but the property ids themselves.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:135: for (int enums_i = 0; enums_i <
property->count_enums; ++enums_i) {
On 2015/05/12 15:17:10, dnicoara wrote:
> On 2015/05/11 22:21:32, spang wrote:
> > There's too much going on here. Please factor this search into a separate
> > function. It'll be much more readable.
> 
> Now that the outer for-loop is gone this should be simpler.

I still think it would be more obvious what it does if we had
GetNameForEnumValue.

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:62: float GetRefreshRate(const
drmModeModeInfo& mode) {
Please describe why vrefresh is sometimes used and clock/htotal/vtotal other
times.

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

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:47: // display. |index| will be used to
create the display id.
s/will be used/may be used... if no other identifier is available/

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:51: size_t index);
display_index

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

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_display.cc:30: uint32_t
GetContentProtectionValue(drmModePropertyRes* property,
please document

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_display.cc:151: PLOG(ERROR) << "'" <<
kContentProtection << "' property doesn't exist.";
PLOG doesn't make sense here. You can't be sure drmModeGetProperty has actually
failed and set errno.

[dnicoara, 2015-05-12 18:05:02]

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:98: for (int i = 0; i <
connector->count_props; ++i) {
On 2015/05/12 16:58:53, spang wrote:
> On 2015/05/12 15:17:10, dnicoara wrote:
> > On 2015/05/11 22:21:32, spang wrote:
> > > How come we don't remember the property ids anywhere?
> > 
> > They can change between display events and we only need a small subset when
we
> > refresh the display state, so it isn't worth caching them.
> 
> I don't think the ids can change. It looks like the lifetime of the property
is
> the lifetime of the DRM device.
> 
> I'm not talking about the property values, but the property ids themselves.

From what I can tell (at least for the standard properties) the IDs don't change
during the device's lifetime. Though I haven't seen any documentation mentioning
this, so I didn't want to make any assumptions that may not hold down the road.

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:135: for (int enums_i = 0; enums_i <
property->count_enums; ++enums_i) {
On 2015/05/12 16:58:53, spang wrote:
> On 2015/05/12 15:17:10, dnicoara wrote:
> > On 2015/05/11 22:21:32, spang wrote:
> > > There's too much going on here. Please factor this search into a separate
> > > function. It'll be much more readable.
> > 
> > Now that the outer for-loop is gone this should be simpler.
> 
> I still think it would be more obvious what it does if we had
> GetNameForEnumValue.

Done.

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:62: float GetRefreshRate(const
drmModeModeInfo& mode) {
On 2015/05/12 16:58:53, spang wrote:
> Please describe why vrefresh is sometimes used and clock/htotal/vtotal other
> times.

Done.

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

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:47: // display. |index| will be used to
create the display id.
On 2015/05/12 16:58:53, spang wrote:
> s/will be used/may be used... if no other identifier is available/

Added more details. It actually is used even if we have a valid EDID. It allows
us to distinguish between 2 identical displays.

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.h:51: size_t index);
On 2015/05/12 16:58:53, spang wrote:
> display_index

Done.

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

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_display.cc:30: uint32_t
GetContentProtectionValue(drmModePropertyRes* property,
On 2015/05/12 16:58:53, spang wrote:
> please document

Done.

https://codereview.chromium.org/1129923004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_display.cc:151: PLOG(ERROR) << "'" <<
kContentProtection << "' property doesn't exist.";
On 2015/05/12 16:58:53, spang wrote:
> PLOG doesn't make sense here. You can't be sure drmModeGetProperty has
actually
> failed and set errno.

Done.

[spang, 2015-05-12 18:28:48]

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:98: for (int i = 0; i <
connector->count_props; ++i) {
On 2015/05/12 18:05:02, dnicoara wrote:
> On 2015/05/12 16:58:53, spang wrote:
> > On 2015/05/12 15:17:10, dnicoara wrote:
> > > On 2015/05/11 22:21:32, spang wrote:
> > > > How come we don't remember the property ids anywhere?
> > > 
> > > They can change between display events and we only need a small subset
when
> we
> > > refresh the display state, so it isn't worth caching them.
> > 
> > I don't think the ids can change. It looks like the lifetime of the property
> is
> > the lifetime of the DRM device.
> > 
> > I'm not talking about the property values, but the property ids themselves.
> 
> From what I can tell (at least for the standard properties) the IDs don't
change
> during the device's lifetime. Though I haven't seen any documentation
mentioning
> this, so I didn't want to make any assumptions that may not hold down the
road.

If it changes and chrome breaks for a device it worked on previously then that's
a kernel regression which would be fixed.

The kernel indexes the properties by id. It seems to me we are expected to cache
the ids. Reading half the properties every time we want to read any of them
seems really silly to me.

[spang, 2015-05-12 18:51:25]

lgtm

https://codereview.chromium.org/1129923004/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:66: // as an interger).
spelling typo

[dnicoara, 2015-05-12 18:53:19]

https://codereview.chromium.org/1129923004/diff/60001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/drm_util.cc (right):

https://codereview.chromium.org/1129923004/diff/60001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/drm_util.cc:66: // as an interger).
On 2015/05/12 18:51:25, spang wrote:
> spelling typo

Done.

[dnicoara, 2015-05-12 18:53:27]

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

[dnicoara, 2015-05-12 18:53:27]

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/1129923004/#ps80001
(title: ".")

[commit-bot: I haz the power, 2015-05-12 18:54:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129923004/80001

[commit-bot: I haz the power, 2015-05-12 19:37:55]

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

[commit-bot: I haz the power, 2015-05-12 19:37:56]

Dry run: This issue passed the CQ dry run.

[dnicoara, 2015-05-12 19:38:37]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2015-05-12 19:40:04]

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

[commit-bot: I haz the power, 2015-05-12 19:45:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-12 19:46:30]

Patchset 5 (id:??) landed as
https://crrev.com/f8d980302e5136fd9b25f8f2c891d1f3859c6ddb
Cr-Commit-Position: refs/heads/master@{#329462}