drm: Make DrmDisplayHostManager handle VGEM fd by itself.

drm: Make DrmDisplayHostManager handle VGEM fd by itself.

GPU process doesn't need VGEM fd, so DrmDisplayHostManager doesn't send it to
GPU process.

This CL fixes a subtle bug which is that VGEM device is corrupted because of
invalid ioctl call to VGEM device. After this CL, following error log disappears;
[ERROR:drm_device_manager.cc(54)] Could not initialize DRM device for /dev/dri/card1

It's part of "native GpuMemoryBuffer on ChromeOS Freon" implementation.
 * Design doc: https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8

TEST=Run chromeos using amd64-generic_freon image
BUG=475633
Committed: https://crrev.com/da2c3cd1997a44a54c4166835d62bb8b1cd30f11
Cr-Commit-Position: refs/heads/master@{#332822}

[dshwang, 2015-05-20 14:29:38]

dongseong.hwang@intel.com changed reviewers:
+ dnicoara@chromium.org, reveman@chromium.org, spang@chromium.org

[dshwang, 2015-05-20 14:29:38]

after discussion in https://codereview.chromium.org/1124063003/, I realize that
it might be better for GPU process to not know VGEM fd.
What do you think? DrmDisplayHostManager will distrubute VGEM FD to Renderers.

[reveman, 2015-05-26 16:38:31]

Does it really make sense to use DrmDeviceHandle for vgem? What's the benefit of
DrmDeviceHandle compared to using base::ScopedFD directly for vgem?

[dshwang, 2015-05-26 17:06:12]

On 2015/05/26 16:38:31, reveman wrote:
> Does it really make sense to use DrmDeviceHandle for vgem? What's the benefit
of
> DrmDeviceHandle compared to using base::ScopedFD directly for vgem?

That's good point. It's fine to use base::ScopedFD for vgem. I just use
DrmDeviceHandle because vgem is also drm device. I'll update vgem to use
base::ScopedFD.

[dshwang, 2015-05-26 18:00:29]

On 2015/05/26 17:06:12, dshwang wrote:
> On 2015/05/26 16:38:31, reveman wrote:
> > Does it really make sense to use DrmDeviceHandle for vgem? What's the
benefit
> of
> > DrmDeviceHandle compared to using base::ScopedFD directly for vgem?
> 
> That's good point. It's fine to use base::ScopedFD for vgem. I just use
> DrmDeviceHandle because vgem is also drm device. I'll update vgem to use
> base::ScopedFD.

Now vgem is handled by base::File. could you review again? After this CL,
following error log disappears;
[ERROR:drm_device_manager.cc(54)] Could not initialize DRM device for
/dev/dri/card1

[vignatti (out of this project), 2015-05-26 18:28:33]

tiago.vignatti@intel.com changed reviewers:
+ tiago.vignatti@intel.com

[vignatti (out of this project), 2015-05-26 18:28:34]

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

https://codereview.chromium.org/1151533002/diff/20001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:36: const char
kVgemSysCardPath[] = "/sys/bus/platform/devices/vgem/drm/";
note that this only works on CrOS. In other Linux variants, platform device is
not initialized and there won't be a sysfs exported information about vgem.

(I'm more and more having trouble to hack Ozone GBM in regular Linuxes... I
think it's time to make a  definitive move to CrOS :/)

[spang, 2015-05-26 19:31:45]

On 2015/05/26 18:28:34, vignatti wrote:
>
https://codereview.chromium.org/1151533002/diff/20001/ui/ozone/platform/drm/h...
> File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right):
> 
>
https://codereview.chromium.org/1151533002/diff/20001/ui/ozone/platform/drm/h...
> ui/ozone/platform/drm/host/drm_display_host_manager.cc:36: const char
> kVgemSysCardPath[] = "/sys/bus/platform/devices/vgem/drm/";
> note that this only works on CrOS. In other Linux variants, platform device is
> not initialized and there won't be a sysfs exported information about vgem.
> 
> (I'm more and more having trouble to hack Ozone GBM in regular Linuxes... I
> think it's time to make a  definitive move to CrOS :/)

I think we'd welcome patches that make things work on upstream kernels.. that's
a useful ability.

[vignatti (out of this project), 2015-05-26 19:35:12]

On 2015/05/26 19:31:45, spang wrote:
> I think we'd welcome patches that make things work on upstream kernels..
that's
> a useful ability.

I'm sure you will! But it's all about finding the most productive way to develop
CrOS. After all my intentions are only within CrOS lately.

[dshwang, 2015-06-03 10:07:35]

spang, could you review again?
It removes the error log in both chrome://gpu and /var/log/ui/ui.LATEST

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right):

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:136: if
(!vgem_card_path_.empty()) {
handle the platform which doesn't have vgem.

[spang, 2015-06-03 19:03:22]

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right):

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:84:
base::FilePath::FromUTF8Unsafe(kVgemSysCardPath), false,
I don't think you need the FromUTF8Unsafe(). Does
base::FilePath(kVgemSysCardPath) not work?

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:91:
file_iter.GetInfo().GetName().BaseName().MaybeAsASCII();
Use value() instead of MaybeAsASCII().

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:136: if
(!vgem_card_path_.empty()) {
On 2015/06/03 10:07:35, dshwang wrote:
> handle the platform which doesn't have vgem.

Can you just use open() and store it in a ScopedFD?

I don't think we should use base::File - it's an unnecessary abstraction. In
this case we can assume linux.

We should also log if open() fails:

  PLOG(ERROR) << "Failed to open vgem: " << vgem_card_path_.value();

[dshwang, 2015-06-04 08:32:07]

Thank you for nice review. I addressed. Could you review again?

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right):

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:91:
file_iter.GetInfo().GetName().BaseName().MaybeAsASCII();
On 2015/06/03 19:03:21, spang wrote:
> Use value() instead of MaybeAsASCII().

Done.

https://codereview.chromium.org/1151533002/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_display_host_manager.cc:136: if
(!vgem_card_path_.empty()) {
On 2015/06/03 19:03:21, spang wrote:
> On 2015/06/03 10:07:35, dshwang wrote:
> > handle the platform which doesn't have vgem.
> 
> Can you just use open() and store it in a ScopedFD?
> 
> I don't think we should use base::File - it's an unnecessary abstraction. In
> this case we can assume linux.
> 
> We should also log if open() fails:
> 
>   PLOG(ERROR) << "Failed to open vgem: " << vgem_card_path_.value();

Done.

[spang, 2015-06-04 11:18:24]

lgtm

[dshwang, 2015-06-04 12:35:36]

On 2015/06/04 11:18:24, spang wrote:
> lgtm

thx for reviewing!

[dshwang, 2015-06-04 12:35:41]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-06-04 12:35:52]

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

[commit-bot: I haz the power, 2015-06-04 12:39:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-04 12:40:16]

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