[8/8][Ozone-Dri] Pass DRM FD to GPU process on hotplug event

[Ozone-Dri] Pass DRM FD to GPU process on hotplug event

Adds the ability to open a new DRM device in the GPU process as a result
of a hotplug event. This allows us to drive multiple displays using
multiple DRM devices.

BUG=427959
Committed: https://crrev.com/4795d133c6be918f8f6a1f43cabbbb4e9a3a472e
Cr-Commit-Position: refs/heads/master@{#317401}

[dnicoara, 2015-02-06 22:44:15]

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

[dnicoara, 2015-02-06 22:44:15]

PTAL

[spang, 2015-02-11 21:13:21]

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_gpu_platform_support.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_gpu_platform_support.cc:186:
ndd_->AddGraphicsDevice(drm);
Why are the open devices tracked both in DriGpuPlatformSupportHost and in
NativeDisplayDelegateDri?

When can we clean this code up to avoid using the browser process "display
configurator delegate" interface in the gpu process? At this point, it's just a
liability by holding the GPU implementation to an interface that isn't
appropriate for it.

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper_generator.h (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper_generator.h:15: class DriWrapperGenerator {
I'm sure you're tired of hearing it, but I think this is a bad name.

tl;dr: Please say device in the name. Like:

Dr[im]Device(Generator|Factory)(Delegate)?(Impl)?

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_display_delegate_proxy.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_display_delegate_proxy.cc:27: const char
kDefaultGraphicsCardPath[] = "/dev/dri/card0";
When do we get to remove this?

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_display_delegate_proxy.cc:235: true /* task_is_slow
*/);
Apparently task_is_slow = false should be used unless it's really, really slow
and needs a thread all to itself.

[dnicoara, 2015-02-11 22:21:14]

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_gpu_platform_support.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_gpu_platform_support.cc:186:
ndd_->AddGraphicsDevice(drm);
On 2015/02/11 21:13:21, spang wrote:
> Why are the open devices tracked both in DriGpuPlatformSupportHost and in
> NativeDisplayDelegateDri?
> 

Just to have the list handy for the operations. Would you like an accessor on
NativeDisplayDelegateDri?

I kinda hate the fact that we just have DriGpuPlatformSupport be a collection of
everything. I'd rather have NativeDisplayDelegateDri be an IPC message handler.
Then all the display configuration messages (and related state) can be isolated
from everything else.

> When can we clean this code up to avoid using the browser process "display
> configurator delegate" interface in the gpu process? At this point, it's just
a
> liability by holding the GPU implementation to an interface that isn't
> appropriate for it.

We can drop the interface anytime.

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper_generator.h (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper_generator.h:15: class DriWrapperGenerator {
On 2015/02/11 21:13:21, spang wrote:
> I'm sure you're tired of hearing it, but I think this is a bad name.
> 
> tl;dr: Please say device in the name. Like:
> 
> Dr[im]Device(Generator|Factory)(Delegate)?(Impl)?

Renamed as per our discussion yesterday.

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_display_delegate_proxy.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_display_delegate_proxy.cc:27: const char
kDefaultGraphicsCardPath[] = "/dev/dri/card0";
On 2015/02/11 21:13:21, spang wrote:
> When do we get to remove this?

I left a comment in NDDProxy::OnDeviceEvent() on this. The answer would be when
the render nodes patch is upstreamed in the DRM Mesa platform or when we don't
care about backwards compatibility anymore.

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_display_delegate_proxy.cc:235: true /* task_is_slow
*/);
On 2015/02/11 21:13:21, spang wrote:
> Apparently task_is_slow = false should be used unless it's really, really slow
> and needs a thread all to itself.

Done.

[spang, 2015-02-11 22:47:57]

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_gpu_platform_support.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_gpu_platform_support.cc:186:
ndd_->AddGraphicsDevice(drm);
On 2015/02/11 22:21:14, dnicoara wrote:
> On 2015/02/11 21:13:21, spang wrote:
> > Why are the open devices tracked both in DriGpuPlatformSupportHost and in
> > NativeDisplayDelegateDri?
> > 
> 
> Just to have the list handy for the operations. Would you like an accessor on
> NativeDisplayDelegateDri?
> 
> I kinda hate the fact that we just have DriGpuPlatformSupport be a collection
of
> everything. I'd rather have NativeDisplayDelegateDri be an IPC message
handler.
> Then all the display configuration messages (and related state) can be
isolated
> from everything else.
> 
> > When can we clean this code up to avoid using the browser process "display
> > configurator delegate" interface in the gpu process? At this point, it's
just
> a
> > liability by holding the GPU implementation to an interface that isn't
> > appropriate for it.
> 
> We can drop the interface anytime.

It's more than just dropping inheritance. We also need to rationalize the
relationship between "DriGpuPlatformSupport" and NativeDisplayDelegateDri. And
it wouldn't be appropriate to continue using the current name, since its
suggestive of the browser process delegate to DisplayConfigurator.

[spang, 2015-02-11 22:52:54]

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_gpu_platform_support.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_gpu_platform_support.cc:186:
ndd_->AddGraphicsDevice(drm);
On 2015/02/11 22:21:14, dnicoara wrote:
> On 2015/02/11 21:13:21, spang wrote:
> > Why are the open devices tracked both in DriGpuPlatformSupportHost and in
> > NativeDisplayDelegateDri?
> > 
> 
> Just to have the list handy for the operations. Would you like an accessor on
> NativeDisplayDelegateDri?

I would ideally hope these configuration tasks would be done here, or there, but
not a mix of the two.

There is no sensibly definable boundary currently.

> 
> I kinda hate the fact that we just have DriGpuPlatformSupport be a collection
of
> everything. I'd rather have NativeDisplayDelegateDri be an IPC message
handler.
> Then all the display configuration messages (and related state) can be
isolated
> from everything else.
> 
> > When can we clean this code up to avoid using the browser process "display
> > configurator delegate" interface in the gpu process? At this point, it's
just
> a
> > liability by holding the GPU implementation to an interface that isn't
> > appropriate for it.
> 
> We can drop the interface anytime.

[dnicoara, 2015-02-11 23:04:40]

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_gpu_platform_support.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_gpu_platform_support.cc:186:
ndd_->AddGraphicsDevice(drm);
On 2015/02/11 22:52:54, spang wrote:
> On 2015/02/11 22:21:14, dnicoara wrote:
> > On 2015/02/11 21:13:21, spang wrote:
> > > Why are the open devices tracked both in DriGpuPlatformSupportHost and in
> > > NativeDisplayDelegateDri?
> > > 
> > 
> > Just to have the list handy for the operations. Would you like an accessor
on
> > NativeDisplayDelegateDri?
> 
> I would ideally hope these configuration tasks would be done here, or there,
but
> not a mix of the two.
> 
> There is no sensibly definable boundary currently.

I'm sorry, I was too vague. The only configuration taking place here would be
the initialization of the IO task runner. But I can push all that into NDDDri
and just forward the task runner.

> 
> > 
> > I kinda hate the fact that we just have DriGpuPlatformSupport be a
collection
> of
> > everything. I'd rather have NativeDisplayDelegateDri be an IPC message
> handler.
> > Then all the display configuration messages (and related state) can be
> isolated
> > from everything else.
> > 
> > > When can we clean this code up to avoid using the browser process "display
> > > configurator delegate" interface in the gpu process? At this point, it's
> just
> > a
> > > liability by holding the GPU implementation to an interface that isn't
> > > appropriate for it.
> > 
> > We can drop the interface anytime.
>

[spang, 2015-02-11 23:11:56]

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_gpu_platform_support.cc (right):

https://codereview.chromium.org/905873003/diff/20001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_gpu_platform_support.cc:186:
ndd_->AddGraphicsDevice(drm);
On 2015/02/11 23:04:40, dnicoara wrote:
> On 2015/02/11 22:52:54, spang wrote:
> > On 2015/02/11 22:21:14, dnicoara wrote:
> > > On 2015/02/11 21:13:21, spang wrote:
> > > > Why are the open devices tracked both in DriGpuPlatformSupportHost and
in
> > > > NativeDisplayDelegateDri?
> > > > 
> > > 
> > > Just to have the list handy for the operations. Would you like an accessor
> on
> > > NativeDisplayDelegateDri?
> > 
> > I would ideally hope these configuration tasks would be done here, or there,
> but
> > not a mix of the two.
> > 
> > There is no sensibly definable boundary currently.
> 
> I'm sorry, I was too vague. The only configuration taking place here would be
> the initialization of the IO task runner. But I can push all that into NDDDri
> and just forward the task runner.
> 
> > 
> > > 
> > > I kinda hate the fact that we just have DriGpuPlatformSupport be a
> collection
> > of
> > > everything. I'd rather have NativeDisplayDelegateDri be an IPC message
> > handler.
> > > Then all the display configuration messages (and related state) can be
> > isolated
> > > from everything else.
> > > 
> > > > When can we clean this code up to avoid using the browser process
"display
> > > > configurator delegate" interface in the gpu process? At this point, it's
> > just
> > > a
> > > > liability by holding the GPU implementation to an interface that isn't
> > > > appropriate for it.
> > > 
> > > We can drop the interface anytime.
> > 

There's plenty of business logic for display configuration in here. Especially
in OnConfigureNativeDisplay.

This CL adds more, in OnAddGraphicsDevice.

Please define clear roles for these classes and stick to them.

[dnicoara, 2015-02-12 17:25:40]

Rebased, mind taking a look?

[spang, 2015-02-12 18:02:46]

lgtm

https://codereview.chromium.org/905873003/diff/60001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_display_delegate_dri.cc (right):

https://codereview.chromium.org/905873003/diff/60001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_display_delegate_dri.cc:221: if (it !=
devices_.end())
maybe warn?

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

New patchsets have been uploaded after l-g-t-m from spang@chromium.org

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

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

[dnicoara, 2015-02-12 18:11:19]

+jl@ for security review

This is the CR adding support for display link devices via hotplugging.

The hotplug events are processed on the browser side in
native_display_delegate_proxy.cc which will IPC the FD for the DRM device to the
GPU process.

The IPC is processed in dri_gpu_platform_support.cc on the GPU side and
native_display_delegate_dri.cc is where the FD is used to initialize the DRM
device and is used.

https://codereview.chromium.org/905873003/diff/60001/ui/ozone/platform/dri/na...
File ui/ozone/platform/dri/native_display_delegate_dri.cc (right):

https://codereview.chromium.org/905873003/diff/60001/ui/ozone/platform/dri/na...
ui/ozone/platform/dri/native_display_delegate_dri.cc:221: if (it !=
devices_.end())
On 2015/02/12 18:02:46, spang wrote:
> maybe warn?

Done.

[jln (very slow on Chromium), 2015-02-19 01:28:58]

Sorry, this slipped through.

I went through it fast (should have more time tomorrow):

- In general this looks good and I think it's fine
- Not knowing this code, I found it difficult to follow what gets the
notification that a new event happen, what is the first IPC sent, etc.. Could
you describe this somewhere?
- We want to make sure that the code on the "privileged" end of the IPC (which
does the actual open()) is paranoid about what it opens: anything that you can
think off is useful: make sure that we're not opening a directory and that we
can never get tricked to open arbitrary filenames.

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/common/g...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/common/g...
ui/ozone/common/gpu/ozone_gpu_messages.h:96: base::FilePath /* device_path */,
What is the reason for still sending the device path and not on the device_fd?

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
File ui/ozone/platform/dri/drm_device_generator.h (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
ui/ozone/platform/dri/drm_device_generator.h:20: virtual
scoped_refptr<DriWrapper> CreateDevice(
Could you add a comment here? (I'm sure this will become clear as I go further
in the review, but it we would be nice to understand on its own).

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
File ui/ozone/platform/dri/native_display_delegate_dri.cc (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
ui/ozone/platform/dri/native_display_delegate_dri.cc:90: };
Does STL require this to be copy-able?

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
File ui/ozone/platform/dri/native_display_delegate_proxy.cc (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
ui/ozone/platform/dri/native_display_delegate_proxy.cc:38: 
Could you please CHECK that the file is not a directory? (I know it shouldn't
happen, just being paranoid here).

[dnicoara, 2015-02-19 02:42:53]

To better understand the chain of events here's a breakdown:

On the browser side:
1) To provide hotplug events a DeviceManager (see ui/events/ozone/device/*; and
more specifically the UDEV implementation) is created in OzonePlatform{Dri,Gbm}
2) NativeDisplayDelegateProxy is an observer of DeviceManager which listens for
display hotplug events.
3) OnDeviceEvent() is called when a hotplug event is processed. In the case of
an add display event, we open the FD for the device and will IPC the FD to the
GPU process via OzoneGpuMsg_AddGraphicsDevice.

On the GPU side:
1) DriGpuPlatform handles the  OzoneGpuMsg_AddGraphicsDevice IPC and forward the
event to NativeDisplayDelegateDri
2) NativeDisplayDelegateDri keeps track of all the DRM devices and will use the
FD to create an appropriate wrapper (DriWrapper or GbmWrapper)
3) {Dri,Gbm}Wrapper are then used to configure displays.

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/common/g...
File ui/ozone/common/gpu/ozone_gpu_messages.h (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/common/g...
ui/ozone/common/gpu/ozone_gpu_messages.h:96: base::FilePath /* device_path */,
On 2015/02/19 01:28:58, jln wrote:
> What is the reason for still sending the device path and not on the device_fd?

To uniquely identify the device. These events are a result of hotplug events and
the event provides a file path (no FD).

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
File ui/ozone/platform/dri/drm_device_generator.h (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
ui/ozone/platform/dri/drm_device_generator.h:20: virtual
scoped_refptr<DriWrapper> CreateDevice(
On 2015/02/19 01:28:58, jln wrote:
> Could you add a comment here? (I'm sure this will become clear as I go further
> in the review, but it we would be nice to understand on its own).

Done.

Just to explain further: there are 2 platforms that share much of the code (DRM
and GBM). The device generator is here just to allow us to create share much of
the code, but create different devices depending on the platform used.

DRM uses the basic DRM API to allocate buffers and map them so that we can paint
into them using Skia. This is used for software rendering. GBM builds on top of
DRM and allows us to allocate buffers that will be used with EGL and GLES2 for
accelerated rendering.

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
File ui/ozone/platform/dri/native_display_delegate_dri.cc (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
ui/ozone/platform/dri/native_display_delegate_dri.cc:90: };
On 2015/02/19 01:28:58, jln wrote:
> Does STL require this to be copy-able?

Yes.

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
File ui/ozone/platform/dri/native_display_delegate_proxy.cc (right):

https://chromiumcodereview.appspot.com/905873003/diff/80001/ui/ozone/platform...
ui/ozone/platform/dri/native_display_delegate_proxy.cc:38: 
On 2015/02/19 01:28:58, jln wrote:
> Could you please CHECK that the file is not a directory? (I know it shouldn't
> happen, just being paranoid here).

Done. I've added another check to verify the path of the device. Let me know if
I should add more.

[jln (very slow on Chromium), 2015-02-20 19:46:07]

lgtm, thanks!

[hshi1, 2015-02-20 20:49:19]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2015-02-20 20:49:53]

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

[hshi1, 2015-02-20 20:50:00]

On 2015/02/20 19:46:07, jln wrote:
> lgtm, thanks!

Tested with UDL+i915 on top of TOT as of r317388 (30d4c2223ff74825)

[commit-bot: I haz the power, 2015-02-20 21:09:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-20 21:10:24]

Patchset 7 (id:??) landed as
https://crrev.com/4795d133c6be918f8f6a1f43cabbbb4e9a3a472e
Cr-Commit-Position: refs/heads/master@{#317401}

[gunsch, 2015-02-20 22:39:44]

On 2015/02/20 21:10:24, I haz the power (commit-bot) wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/4795d133c6be918f8f6a1f43cabbbb4e9a3a472e
> Cr-Commit-Position: refs/heads/master@{#317401}

Hey, we're seeing some failures on our Cast Linux bots from this CL:

../../base/file_descriptor_posix.h:21:1: error: 'FileDescriptor' defined as a
struct here but previously declared as a class [-Werror,-Wmismatched-tags]
struct FileDescriptor {
^
../../ui/ozone/platform/dri/native_display_delegate_dri.h:14:1: note: did you
mean struct here?
class FileDescriptor;
^~~~~
struct


../../ipc/ipc_message_utils.h:58:1: error: struct 'FileDescriptor' was
previously declared as a class [-Werror,-Wmismatched-tags]
struct FileDescriptor;
^
../../ui/ozone/platform/dri/dri_gpu_platform_support.h:18:7: note: previous use
is here
class FileDescriptor;
      ^

[hshi1, 2015-02-20 22:42:53]

On 2015/02/20 22:39:44, gunsch wrote:
> On 2015/02/20 21:10:24, I haz the power (commit-bot) wrote:
> > Patchset 7 (id:??) landed as
> > https://crrev.com/4795d133c6be918f8f6a1f43cabbbb4e9a3a472e
> > Cr-Commit-Position: refs/heads/master@{#317401}
> 
> Hey, we're seeing some failures on our Cast Linux bots from this CL:

re: gunsch: sorry it appears that this CL was not tested against the Cast Linux
bots in the pre-submit queue.

I think it suffices to replace the "class FileDescriptor" to "struct
FileDescriptor" in native_display_delegate_dri.h? Can you confirm if that fixes
the build?

[spang, 2015-02-20 22:43:50]

On 2015/02/20 22:42:53, hshi1 wrote:
> On 2015/02/20 22:39:44, gunsch wrote:
> > On 2015/02/20 21:10:24, I haz the power (commit-bot) wrote:
> > > Patchset 7 (id:??) landed as
> > > https://crrev.com/4795d133c6be918f8f6a1f43cabbbb4e9a3a472e
> > > Cr-Commit-Position: refs/heads/master@{#317401}
> > 
> > Hey, we're seeing some failures on our Cast Linux bots from this CL:
> 
> re: gunsch: sorry it appears that this CL was not tested against the Cast
Linux
> bots in the pre-submit queue.
> 
> I think it suffices to replace the "class FileDescriptor" to "struct
> FileDescriptor" in native_display_delegate_dri.h? Can you confirm if that
fixes
> the build?

alexst@ is compiling a fix now

[gunsch, 2015-02-20 23:18:39]

On 2015/02/20 22:43:50, spang wrote:
> On 2015/02/20 22:42:53, hshi1 wrote:
> > On 2015/02/20 22:39:44, gunsch wrote:
> > > On 2015/02/20 21:10:24, I haz the power (commit-bot) wrote:
> > > > Patchset 7 (id:??) landed as
> > > > https://crrev.com/4795d133c6be918f8f6a1f43cabbbb4e9a3a472e
> > > > Cr-Commit-Position: refs/heads/master@{#317401}
> > > 
> > > Hey, we're seeing some failures on our Cast Linux bots from this CL:
> > 
> > re: gunsch: sorry it appears that this CL was not tested against the Cast
> Linux
> > bots in the pre-submit queue.
> > 
> > I think it suffices to replace the "class FileDescriptor" to "struct
> > FileDescriptor" in native_display_delegate_dri.h? Can you confirm if that
> fixes
> > the build?
> 
> alexst@ is compiling a fix now

Our presubmit bots are WIP and should be running on actual CLs soon (probably as
advisory for awhile). Looks like the other Linux ozone bots immediately went red
on this CL as well but just aren't blocking yet.