[2/4]Allow hotplugging of primary DRM device

Allow hotplugging of primary DRM device

Removes special initialization of the primary DRM device from
the GPU process. This allows us to work around sandboxing issues
on initialization on ARM devices since the DRM card devices will
all be sent to the sandboxed GPU via IPCs.

BUG=477792, 476728
Committed: https://crrev.com/a889b38d983fd85071dd5d5324e6d1450eaba2ed
Cr-Commit-Position: refs/heads/master@{#327518}

[dnicoara, 2015-04-22 17:47:26]

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

[alexst (slow to review), 2015-04-22 18:27:56]

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

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_device_manager.cc:60: if (!primary_device_)
Document this please, so we know that primary is the first one.

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_device_manager.h (right):

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_device_manager.h:59:
std::vector<scoped_refptr<DrmDevice>> devices_;
typedef DrmDeviceVector?

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

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:174:
std::vector<scoped_refptr<DrmDevice>> devices =
This is not thread safe. You return a vector here, but a new thing might be
getting added to it on a different thread.

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:187:
drm_device_manager_->GetDrmDevices();
ditto here.

[dnicoara, 2015-04-22 18:41:30]

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

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_device_manager.cc:60: if (!primary_device_)
On 2015/04/22 18:27:56, alexst wrote:
> Document this please, so we know that primary is the first one.

Done; added to the header file.

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_device_manager.h (right):

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_device_manager.h:59:
std::vector<scoped_refptr<DrmDevice>> devices_;
On 2015/04/22 18:27:56, alexst wrote:
> typedef DrmDeviceVector?

It is only used in a handful of places and doesn't cause line wraps, so I didn't
bother. If you feel strongly, I can add it.

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

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:174:
std::vector<scoped_refptr<DrmDevice>> devices =
On 2015/04/22 18:27:56, alexst wrote:
> This is not thread safe. You return a vector here, but a new thing might be
> getting added to it on a different thread.

This is fine. All the devices are added on the GPU main thread, so there aren't
any races. (We were previously keeping the list in here, I just changed the
location since I felt it was more appropriate to keep device management in one
place).

Added more comments to the DrmDeviceManager header.

[spang, 2015-04-22 18:50:58]

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

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_device_manager.cc:53: VLOG(2) << "Could not
initialize DRM device for " << path.value();
LOG(ERROR)

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_native_display_delegate.cc (right):

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:37: bool
IsAuthenticated(int fd) {
A function called IsBlah() should not have side effects like this.

s/IsAuthenticated/Authenticate/

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:39: memset(&magic, 0,
sizeof(magic));
Seems like you can do "= 0;" instead of memset..

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:64: break;
??

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:345: // graphics
state.
How does sync open in the browser help the GPU process?

(what goes wrong if you omit this part?)

[dnicoara, 2015-04-22 19:10:01]

Also added thread checking in DrmDeviceManager

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

https://codereview.chromium.org/1100973002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_device_manager.cc:53: VLOG(2) << "Could not
initialize DRM device for " << path.value();
On 2015/04/22 18:50:58, spang wrote:
> LOG(ERROR)

Done.

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_native_display_delegate.cc (right):

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:37: bool
IsAuthenticated(int fd) {
On 2015/04/22 18:50:58, spang wrote:
> A function called IsBlah() should not have side effects like this.
> 
> s/IsAuthenticated/Authenticate/

Done.

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:39: memset(&magic, 0,
sizeof(magic));
On 2015/04/22 18:50:58, spang wrote:
> Seems like you can do "= 0;" instead of memset..

Done.

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:64: break;
On 2015/04/22 18:50:58, spang wrote:
> ??

Oops, removed.

https://codereview.chromium.org/1100973002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_native_display_delegate.cc:345: // graphics
state.
On 2015/04/22 18:50:58, spang wrote:
> How does sync open in the browser help the GPU process?
> 
> (what goes wrong if you omit this part?)

The UI thread creates a window right away which creates a Compositor, which, in
turn, will IPC a surface creation message. We need to have the primary device
open and registered before the surface creation IPC arrives in the GPU process.

If we omit, the device is opened later via the worker thread and by that time
the Compositor will have already IPC-ed to the GPU process to create a surface
(and caused the GPU process to crash since we don't have a device to allocate
with).

[alexst (slow to review), 2015-04-23 00:01:20]

code lgtm, curious about boot time.

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

https://codereview.chromium.org/1100973002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:174:
std::vector<scoped_refptr<DrmDevice>> devices =
nit: typedef here as well.

[dnicoara, 2015-04-23 13:40:07]

bootperf results on a test image with "--noenable_rootfs_verification":

without this patch:
(on 10 cycles):
 time  s%     dt  s%  event
 1702  1%  +1702  1%  startup
 2031  1%   +329  5%  startup_done
 4992  0%  +2961  1%  chrome_exec
 5059  0%    +67 10%  chrome_main
 5173  1%   +114 33%  network
 5410  1%   +237 19%  signin_start
 5435  1%    +25 21%  signin_wait
 6265  0%   +830  4%  signin_seen
 6277  0%    +12 35%  login

with this patch:
(on 10 cycles):
 time  s%     dt  s%  event
 1696  1%  +1696  1%  startup
 2017  1%   +321  4%  startup_done
 4998  1%  +2981  1%  chrome_exec
 5060  1%    +62  7%  chrome_main
 5157  1%    +97 44%  network
 5416  1%   +259 19%  signin_start
 5437  1%    +21 15%  signin_wait
 6231  1%   +794  2%  signin_seen
 6241  1%    +10 67%  login

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

https://codereview.chromium.org/1100973002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc:174:
std::vector<scoped_refptr<DrmDevice>> devices =
On 2015/04/23 00:01:19, alexst wrote:
> nit: typedef here as well.

Done.

[alexst (slow to review), 2015-04-23 13:53:43]

okay, lgtm

https://codereview.chromium.org/1100973002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_device_manager.h (right):

https://codereview.chromium.org/1100973002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_device_manager.h:58: DrmDeviceVector
GetDrmDevices();
One more nit: do you need to make a copy here or could it be const
DrmDeviceVector& GetDrmDevices() instead?

[dnicoara, 2015-04-23 14:06:11]

https://codereview.chromium.org/1100973002/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_device_manager.h (right):

https://codereview.chromium.org/1100973002/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_device_manager.h:58: DrmDeviceVector
GetDrmDevices();
On 2015/04/23 13:53:43, alexst wrote:
> One more nit: do you need to make a copy here or could it be const
> DrmDeviceVector& GetDrmDevices() instead?

Nope, done.

[spang, 2015-04-27 22:18:30]

lgtm

[dnicoara, 2015-04-28 18:08:18]

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

[dnicoara, 2015-04-28 18:08:19]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1100973002/#ps120001
(title: "Fix rebase")

[commit-bot: I haz the power, 2015-04-28 18:08:38]

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

[commit-bot: I haz the power, 2015-04-28 18:52:59]

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

[commit-bot: I haz the power, 2015-04-28 18:52:59]

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

[dnicoara, 2015-04-28 21:08:03]

Realized the unittests needed updating. (Also found some mistakes in the
process) Could you take another look.

https://codereview.chromium.org/1100973002/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right):

https://codereview.chromium.org/1100973002/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_device_manager.cc:100: DCHECK_NE(it->second,
primary_device_);
The comment/DCHECK is wrong since this doesn't remove the primary device. This
is unmapping the window from the device.

[alexst (slow to review), 2015-04-29 17:33:29]

still lgtm

[dnicoara, 2015-04-29 17:33:54]

The CQ bit was checked by dnicoara@chromium.org

[dnicoara, 2015-04-29 17:33:54]

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/1100973002/#ps140001
(title: "unittests")

[commit-bot: I haz the power, 2015-04-29 17:34:53]

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

[commit-bot: I haz the power, 2015-04-29 17:40:15]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-29 17:41:13]

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