drm: GPU process manages VGEM fd.

ui/ozone/platform/drm/gpu/drm_device_manager.cc

Index: ui/ozone/platform/drm/gpu/drm_device_manager.cc
diff --git a/ui/ozone/platform/drm/gpu/drm_device_manager.cc b/ui/ozone/platform/drm/gpu/drm_device_manager.cc
index b01664abb706586b1332aee3d22d82fa6f87a5f0..5c8ce8dd6e86455604e509f1544ef72fa6874683 100644
--- a/ui/ozone/platform/drm/gpu/drm_device_manager.cc
+++ b/ui/ozone/platform/drm/gpu/drm_device_manager.cc
@@ -38,8 +38,17 @@ DrmDeviceManager::~DrmDeviceManager() {
 }
 
 bool DrmDeviceManager::AddDrmDevice(const base::FilePath& path,
-                                    const base::FileDescriptor& fd) {
+                                    const base::FileDescriptor& fd,
+                                    bool is_vgem) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(fd.auto_close);

[reveman, 2015/05/12 13:47:01]

this check seems inappropriate. why should this not be left to the caller to
decide?

[dshwang, 2015/05/12 14:11:42]

DrmDeviceManager implementation always close the given fd, so I just add to
clarify. Do you want to remove this dcheck?

[reveman, 2015/05/12 17:19:00]

yes

[dshwang, 2015/05/13 13:09:00]

Done.

+
+  if (is_vgem) {

[reveman, 2015/05/12 13:47:01]

why is the code below not relevant in this case? Can a vgem device not be added
twice?

[dshwang, 2015/05/12 14:11:42]

That's good question. The answer is directly related to your above concern also;
"I'm failing to see why this is needed."

In the short answer, below code corrupt vgem device and then we can not use vgem
fd.

In the long answer, unfortunately drm device and vgem device has similar path
name; default drm device is "/dev/dri/card0", and vgem device is
"/dev/dri/card1"
However, both are totally different device. Chromium must handle vgem fd, and
drm fd separately.

below routine executes inside DrmDevice as follows:
create PageFlipManager
create HardwareDisplayPlaneManagerLegacy
drmIoctl(DRM_IOCTL_MODE_GETRESOURCES)
gbm_create_device(get_fd())

After this routine, if chromium uses vgem fd for vgem API, unexpected behavior
or crash happens.

[reveman, 2015/05/12 15:49:24]

So should DrmDevice be doing something different in this case? It's confusing to
be calling AddDrmDevice and not really add a DrmDevice.

[dshwang, 2015/05/12 16:18:57]

Yes, vgem fd must be handled differently. I just add |is_vgem| in
OzoneGpuMsg_AddGraphicsDevice message. Do you want to add new message like
OzoneGpuMsg_AddVgemDevice?

[reveman, 2015/05/12 17:19:00]

If vgem is supported, are all devices not vgem?

If you don't think vgem devices fall into the DrmDevice category then we
shouldn't pretend the they do and abuse this ::AddDrmDevice API. 

From what I can tell, it seems like DrmDevice category is fine and we just need
to support vgem in the code below properly.

[dshwang, 2015/05/12 17:48:02]

How about creating DriDevice pure virtual class? DrmDevice and VgemDevice
inherit DriDevice class. GbmDevice inherits DrmDevice like as-is.

I thought above way but I did this CL for minimal change.

[zachr, 2015/05/12 18:24:37]

All of these things wrap the same type of object: an fd under /dev/dri. Could
you use that as your abstraction? Also, I remember dnicoara@ having just purged
the name DRI from the Chrome codebase. It's historical that the device path for
the cards exists under /dev/dri as the name DRI is X specific. I would avoid
calling things DRI.

[dshwang, 2015/05/12 18:36:20]

Thank you for answering. I'll handle vgem in DrmDevice abstraction.

IMO the common part of drm and vgem is GEM, so I'll create GemDevice abstraction
class and then DrmDevice and VgemDevice inherit GemDevice class.
DrmDeviceManager will use GemDevice.
Could you suggest better naming?

[dshwang, 2015/05/13 13:09:00]

Done.

+    vgem_device_.reset(fd.fd);
+    vgem_path_ = path;

[reveman, 2015/05/12 17:19:00]

what happens if 2 vgem devices are added?

[dshwang, 2015/05/12 17:48:02]

I don't think vgem can be 2. zachr, could you confirm?

As far as I understand, /dev/dri/card%d represents gpu card and 1 virtual gem.
If the device has 3 gpu cards, /dev/dri/card0, /dev/dri/card1, and
/dev/dri/card2 represent gpu cards, and /dev/dri/card3 represents vgem.

[zachr, 2015/05/12 18:24:37]

card numbers are arbitrary and it is a mistake to assume any particular number
corresponds to any particular card. I've seen code all over that hardcodes
/dev/dri/card0 and it is a large source of errors as vgem gets added to new
platforms.

As for your question of "vgem can be 2" I'm not sure if you mean can vgem be
/dev/dri/card2, can vgem exist as two different cards in /dev/dri/card2, or can
vgem be opened twice. Vgem can be opened any number of times, there will only
ever be one vgem device, and the particular card # that vgem exists as is
arbitrary. It seems to me that reveman was asking what will happen to your code
of somebody calls AddDrmDevice with a vgem device twice for some reason.

[dshwang, 2015/05/12 18:36:20]

Great answer. I did ask that can device have 2 different vgem cards. About
reveman's question, as I use same abstraction to drm device, |devices_| map
skips existing path name like drm devices.

[dshwang, 2015/05/13 13:09:00]

Done. handle multiple add and remove in same way to drm devices.

+    return true;
+  }
+
   base::File file(fd.fd);
   auto it =
       std::find_if(devices_.begin(), devices_.end(), FindByDevicePath(path));
@@ -67,6 +76,12 @@ bool DrmDeviceManager::AddDrmDevice(const base::FilePath& path,
 
 void DrmDeviceManager::RemoveDrmDevice(const base::FilePath& path) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (vgem_path_ == path) {
+    vgem_device_.reset();
+    vgem_path_ = base::FilePath();
+    return;
+  }
+
   auto it =
       std::find_if(devices_.begin(), devices_.end(), FindByDevicePath(path));
   if (it == devices_.end()) {
@@ -123,4 +138,9 @@ const DrmDeviceVector& DrmDeviceManager::GetDrmDevices() const {
   return devices_;
 }
 
+int DrmDeviceManager::GetVgemDevice() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return vgem_device_.get();
+}
+
 }  // namespace ui