Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1124063003:
drm: GPU process manages VGEM fd. (Closed)
|
Created:
5 years, 7 months ago by dshwang Modified:
5 years, 6 months ago Reviewers:
dnicoara, reveman, kenrb, vignatti (out of this project), zachr, emircan, mcasas, kalyank, spang CC:
chromium-reviews, ozone-reviews_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondrm: GPU process manages VGEM fd.
Browser process clarifies which is vgem fd to GPU process,
and then GPU process manages VGEM fd and DRM fd separately.
Currently GPU process handles VGEM fd as DRM fd, so VGEM is
corrupted.
Introduce DrmDeviceBase to abstract both DrmDevice and VgemDevice.
It's part of "native GpuMemoryBuffer on ChromeOS Freon" implementation.
* Design doc: https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8
* dependent CLs:
vgem fd is used in https://codereview.chromium.org/1124063003/
vgem fd is transferred to Renderer in https://codereview.chromium.org/1128113011/
TEST=Run chromeos using amd64-generic_freon image
BUG=475633
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix crash #
Total comments: 6
Patch Set 3 : io code is moved to io thread #
Total comments: 22
Patch Set 4 : use LazyInstance #
Total comments: 8
Patch Set 5 : GemDevice abstract both drm and vgem devices #Patch Set 6 : rebase to ToT based on Patch Set 5 #Patch Set 7 : add new IPC msgs for vgem #Patch Set 8 : rebase to ToT based on Patch Set 7 #Patch Set 9 : remove OzoneGpuMsg_RemoveVgemDevice message #Patch Set 10 : rebase to ToT #Messages
Total messages: 47 (5 generated)
dnicoara, could you review this CL, in which browser process clarifies to gpu process which is vgem fd. It's prerequisite of zero/one-copy texture upload for cros; https://codereview.chromium.org/1134993003 I made this CL to seek minimal change, rather than generalizing for the future.
https://codereview.chromium.org/1124063003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_device_manager.h (right): https://codereview.chromium.org/1124063003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_device_manager.h:63: int GetVgemDevice() const; This method is needed to implement zero/one-copy texture. See https://codereview.chromium.org/1134993003/diff/1/ui/ozone/platform/drm/gbm_s...
dongseong.hwang@intel.com changed reviewers: + tiago.vignatti@intel.com
I'll need to go over https://codereview.chromium.org/1134993003/ to understand how this is used, but I thought I'd provide feedback on something that jumped out. https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:258: if (stat(vgem_path.c_str(), &vgem_stat) == -1) This is an IO operation that shouldn't be performed on the UI thread. You'll need to do this in OpenDeviceOnWorkerThread(). Also, look at ui/events/devices/device_util_linux.cc for a similar example. You could just enumerate the devices in there rather than try and find it by iterating from 1 to 16.
i concur. https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:259: continue; I wouldn't use stat() nor a hardcoded 16, have a look at media/video/capture/fake_video_capture_device.cc in PS [1]. [1] https://codereview.chromium.org/1106563002/ https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:321: bool is_vgem = handle->IsVgem(); const. Actually why the temp variable anyway?
thx for reviewing. I address your concern. could you review again? https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:258: if (stat(vgem_path.c_str(), &vgem_stat) == -1) On 2015/05/11 20:04:29, dnicoara wrote: > This is an IO operation that shouldn't be performed on the UI thread. You'll > need to do this in OpenDeviceOnWorkerThread(). > > Also, look at ui/events/devices/device_util_linux.cc for a similar example. You > could just enumerate the devices in there rather than try and find it by > iterating from 1 to 16. Done. https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:259: continue; On 2015/05/12 00:02:15, mcasas wrote: > I wouldn't use stat() nor a hardcoded 16, have a look at > media/video/capture/fake_video_capture_device.cc > in PS [1]. > > [1] https://codereview.chromium.org/1106563002/ thx for pointing out. I use FileEnumerator to avoid 16 hardcode. FileEnumerator uses stat() internally. https://codereview.chromium.org/1124063003/diff/20001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:321: bool is_vgem = handle->IsVgem(); On 2015/05/12 00:02:15, mcasas wrote: > const. > Actually why the temp variable anyway? drm_devices_.add(path, handle.Pass()); reset |handle|, so handle->IsVgem() cause seg fault if it's called below.
I'm failing to see why this is needed. Can you please explain a bit more how this is going to be used? Is it needed to support a mix of vgem/non-vgem devices at run-time? In that case, is that really something we need to worry about supporting at this stage? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:44: DCHECK(fd.auto_close); this check seems inappropriate. why should this not be left to the caller to decide? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { why is the code below not relevant in this case? Can a vgem device not be added twice? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.h (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.h:63: int GetVgemDevice() const; Why not use DrmDevice::get_fd? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:50: if (!vgem_card_path.empty()) Is this VgemCardPath really supposed to be a singleton? Please use LazyInstance instead to avoid a race here and make it clear how this works.
On 2015/05/12 13:47:02, reveman wrote: > I'm failing to see why this is needed. Can you please explain a bit more how > this is going to be used? Is it needed to support a mix of vgem/non-vgem devices > at run-time? In that case, is that really something we need to worry about > supporting at this stage? Thx for reviewing. I answered this question in below comment. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:44: DCHECK(fd.auto_close); On 2015/05/12 13:47:01, reveman wrote: > this check seems inappropriate. why should this not be left to the caller to > decide? DrmDeviceManager implementation always close the given fd, so I just add to clarify. Do you want to remove this dcheck? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 13:47:01, reveman wrote: > why is the code below not relevant in this case? Can a vgem device not be added > twice? 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. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.h (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.h:63: int GetVgemDevice() const; On 2015/05/12 13:47:01, reveman wrote: > Why not use DrmDevice::get_fd? ditto to above answer. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:50: if (!vgem_card_path.empty()) On 2015/05/12 13:47:01, reveman wrote: > Is this VgemCardPath really supposed to be a singleton? Please use LazyInstance > instead to avoid a race here and make it clear how this works. I believe it's singleton. +zachr However, this code's purpose is just to run below while statement only once. I see, I'll use LazyInstance.
dongseong.hwang@intel.com changed reviewers: + zachr@chromium.org
https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:78: if (vgem_path_ == path) { Should it be possible to remove the VGEM device? If yes, maybe we shouldn't cache the path of the VGEM device in DrmDisplayHostManager. https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:58: base::TrimString(name, "card", &name); Rather than extracting the interger, why not just append the base name to "/dev/dri/" to get the final path?
https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 14:11:42, dshwang wrote: > On 2015/05/12 13:47:01, reveman wrote: > > why is the code below not relevant in this case? Can a vgem device not be > added > > twice? > > 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. So should DrmDevice be doing something different in this case? It's confusing to be calling AddDrmDevice and not really add a DrmDevice.
thx for great feedback. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 15:49:24, reveman wrote: > On 2015/05/12 14:11:42, dshwang wrote: > > On 2015/05/12 13:47:01, reveman wrote: > > > why is the code below not relevant in this case? Can a vgem device not be > > added > > > twice? > > > > 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. > > So should DrmDevice be doing something different in this case? It's confusing to > be calling AddDrmDevice and not really add a DrmDevice. 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? https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:78: if (vgem_path_ == path) { On 2015/05/12 15:41:22, dnicoara wrote: > Should it be possible to remove the VGEM device? If yes, maybe we shouldn't > cache the path of the VGEM device in DrmDisplayHostManager. What's mean? IMO it's impossible for chromium to drop vgem kernel module. vgem path is immutable since chromebook booting. https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:58: base::TrimString(name, "card", &name); On 2015/05/12 15:41:22, dnicoara wrote: > Rather than extracting the interger, why not just append the base name to > "/dev/dri/" to get the final path? good point. I'll update.
https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:44: DCHECK(fd.auto_close); On 2015/05/12 14:11:42, dshwang wrote: > On 2015/05/12 13:47:01, reveman wrote: > > this check seems inappropriate. why should this not be left to the caller to > > decide? > > DrmDeviceManager implementation always close the given fd, so I just add to > clarify. Do you want to remove this dcheck? yes https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 16:18:57, dshwang wrote: > On 2015/05/12 15:49:24, reveman wrote: > > On 2015/05/12 14:11:42, dshwang wrote: > > > On 2015/05/12 13:47:01, reveman wrote: > > > > why is the code below not relevant in this case? Can a vgem device not be > > > added > > > > twice? > > > > > > 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. > > > > So should DrmDevice be doing something different in this case? It's confusing > to > > be calling AddDrmDevice and not really add a DrmDevice. > > 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? 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. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:48: vgem_path_ = path; what happens if 2 vgem devices are added?
spang@chromium.org changed reviewers: + spang@chromium.org
https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:36: base::LazyInstance<std::string> kVgemCardPath; Please don't add state to the global scope. Instead, find an appropriate object to own this.
zachr, could you feedback in below comment? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 17:19:00, reveman wrote: > On 2015/05/12 16:18:57, dshwang wrote: > > On 2015/05/12 15:49:24, reveman wrote: > > > On 2015/05/12 14:11:42, dshwang wrote: > > > > On 2015/05/12 13:47:01, reveman wrote: > > > > > why is the code below not relevant in this case? Can a vgem device not > be > > > > added > > > > > twice? > > > > > > > > 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. > > > > > > So should DrmDevice be doing something different in this case? It's > confusing > > to > > > be calling AddDrmDevice and not really add a DrmDevice. > > > > 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? > > 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. 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. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:48: vgem_path_ = path; On 2015/05/12 17:19:00, reveman wrote: > what happens if 2 vgem devices are added? 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. https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:36: base::LazyInstance<std::string> kVgemCardPath; On 2015/05/12 17:20:38, spang wrote: > Please don't add state to the global scope. Instead, find an appropriate object > to own this. I see. I move this to member of this class.
https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 17:48:02, dshwang wrote: > On 2015/05/12 17:19:00, reveman wrote: > > On 2015/05/12 16:18:57, dshwang wrote: > > > On 2015/05/12 15:49:24, reveman wrote: > > > > On 2015/05/12 14:11:42, dshwang wrote: > > > > > On 2015/05/12 13:47:01, reveman wrote: > > > > > > why is the code below not relevant in this case? Can a vgem device not > > be > > > > > added > > > > > > twice? > > > > > > > > > > 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. > > > > > > > > So should DrmDevice be doing something different in this case? It's > > confusing > > > to > > > > be calling AddDrmDevice and not really add a DrmDevice. > > > > > > 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? > > > > 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. > > 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. 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. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:48: vgem_path_ = path; On 2015/05/12 17:48:02, dshwang wrote: > On 2015/05/12 17:19:00, reveman wrote: > > what happens if 2 vgem devices are added? > > 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. 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.
https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 18:24:37, zachr wrote: > On 2015/05/12 17:48:02, dshwang wrote: > > On 2015/05/12 17:19:00, reveman wrote: > > > On 2015/05/12 16:18:57, dshwang wrote: > > > > On 2015/05/12 15:49:24, reveman wrote: > > > > > On 2015/05/12 14:11:42, dshwang wrote: > > > > > > On 2015/05/12 13:47:01, reveman wrote: > > > > > > > why is the code below not relevant in this case? Can a vgem device > not > > > be > > > > > > added > > > > > > > twice? > > > > > > > > > > > > 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. > > > > > > > > > > So should DrmDevice be doing something different in this case? It's > > > confusing > > > > to > > > > > be calling AddDrmDevice and not really add a DrmDevice. > > > > > > > > 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? > > > > > > 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. > > > > 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. > > 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. 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? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:48: vgem_path_ = path; On 2015/05/12 18:24:37, zachr wrote: > On 2015/05/12 17:48:02, dshwang wrote: > > On 2015/05/12 17:19:00, reveman wrote: > > > what happens if 2 vgem devices are added? > > > > 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. > > 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. 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.
I handles vgem device in DrmDevice abstraction. Could you review again? https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device_manager.cc (right): https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:44: DCHECK(fd.auto_close); On 2015/05/12 17:19:00, reveman wrote: > On 2015/05/12 14:11:42, dshwang wrote: > > On 2015/05/12 13:47:01, reveman wrote: > > > this check seems inappropriate. why should this not be left to the caller to > > > decide? > > > > DrmDeviceManager implementation always close the given fd, so I just add to > > clarify. Do you want to remove this dcheck? > > yes Done. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:46: if (is_vgem) { On 2015/05/12 18:24:37, zachr wrote: > On 2015/05/12 17:48:02, dshwang wrote: > > On 2015/05/12 17:19:00, reveman wrote: > > > On 2015/05/12 16:18:57, dshwang wrote: > > > > On 2015/05/12 15:49:24, reveman wrote: > > > > > On 2015/05/12 14:11:42, dshwang wrote: > > > > > > On 2015/05/12 13:47:01, reveman wrote: > > > > > > > why is the code below not relevant in this case? Can a vgem device > not > > > be > > > > > > added > > > > > > > twice? > > > > > > > > > > > > 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. > > > > > > > > > > So should DrmDevice be doing something different in this case? It's > > > confusing > > > > to > > > > > be calling AddDrmDevice and not really add a DrmDevice. > > > > > > > > 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? > > > > > > 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. > > > > 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. > > 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. Done. https://codereview.chromium.org/1124063003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device_manager.cc:48: vgem_path_ = path; On 2015/05/12 18:24:37, zachr wrote: > On 2015/05/12 17:48:02, dshwang wrote: > > On 2015/05/12 17:19:00, reveman wrote: > > > what happens if 2 vgem devices are added? > > > > 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. > > 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. Done. handle multiple add and remove in same way to drm devices. https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_display_host_manager.cc (right): https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:36: base::LazyInstance<std::string> kVgemCardPath; On 2015/05/12 17:20:38, spang wrote: > Please don't add state to the global scope. Instead, find an appropriate object > to own this. Done. https://codereview.chromium.org/1124063003/diff/60001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_display_host_manager.cc:58: base::TrimString(name, "card", &name); On 2015/05/12 15:41:22, dnicoara wrote: > Rather than extracting the interger, why not just append the base name to > "/dev/dri/" to get the final path? Done.
when using vgem, will we have both drm and vgem devices at the same time?
On 2015/05/13 19:47:02, reveman wrote: > when using vgem, will we have both drm and vgem devices at the same time? Yes, but first I'd like to say that vgem IS a drm device. The thing that makes it special is that it is not backed by any physical device and is therefore platform agnostic. This makes it primarily useful as something that works (via prime dmabuf sharing) with the platform dependent drm devices, backed by real hardware.
On 2015/05/13 at 19:58:04, zachr wrote:
> On 2015/05/13 19:47:02, reveman wrote:
> > when using vgem, will we have both drm and vgem devices at the same time?
>
> Yes, but first I'd like to say that vgem IS a drm device. The thing that makes
it special is that it is not backed by any physical device and is therefore
platform agnostic. This makes it primarily useful as something that works (via
prime dmabuf sharing) with the platform dependent drm devices, backed by real
hardware.
Ok, should each physical device instead be optionally associated with a vgem
device?
e.g.
IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice,
base::FilePath /* device_path */,
base::FileDescriptor /* device_fd */)
base::FileDescriptor /* vgem_device_fd (optional) */)
or is it necessary to handle these vgem devices as completely separate graphics
devices on the GPU process side?
On 2015/05/13 20:44:46, reveman wrote: > On 2015/05/13 at 19:58:04, zachr wrote: > > On 2015/05/13 19:47:02, reveman wrote: > > > when using vgem, will we have both drm and vgem devices at the same time? > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing that makes > it special is that it is not backed by any physical device and is therefore > platform agnostic. This makes it primarily useful as something that works (via > prime dmabuf sharing) with the platform dependent drm devices, backed by real > hardware. > > Ok, should each physical device instead be optionally associated with a vgem > device? > > e.g. > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > base::FilePath /* device_path */, > base::FileDescriptor /* device_fd */) > base::FileDescriptor /* vgem_device_fd (optional) */) > > or is it necessary to handle these vgem devices as completely separate graphics > devices on the GPU process side? I think the interesting categories here are (1) DRM devices with modesetting resources. There's a single primary device for the GPU & builtin outputs, but more can come and go dynamically (e.g. DisplayLink). (2) vgem. It's virtual, has no outputs / modesetting resources, but is needed for sharing buffers to renderers. There's only one for the whole system (not one per above device). Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type.
On 2015/05/13 20:50:36, spang wrote: > On 2015/05/13 20:44:46, reveman wrote: > > On 2015/05/13 at 19:58:04, zachr wrote: > > > On 2015/05/13 19:47:02, reveman wrote: > > > > when using vgem, will we have both drm and vgem devices at the same time? > > > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing that > makes > > it special is that it is not backed by any physical device and is therefore > > platform agnostic. This makes it primarily useful as something that works (via > > prime dmabuf sharing) with the platform dependent drm devices, backed by real > > hardware. > > > > Ok, should each physical device instead be optionally associated with a vgem > > device? > > > > e.g. > > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > > base::FilePath /* device_path */, > > base::FileDescriptor /* device_fd */) > > base::FileDescriptor /* vgem_device_fd (optional) */) > > > > or is it necessary to handle these vgem devices as completely separate > graphics > > devices on the GPU process side? > > I think the interesting categories here are > > (1) DRM devices with modesetting resources. There's a single primary device for > the GPU & builtin outputs, but more can come and go dynamically (e.g. > DisplayLink). > > (2) vgem. It's virtual, has no outputs / modesetting resources, but is needed > for sharing buffers to renderers. There's only one for the whole system (not one > per above device). > > Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type. I agree with spang.
On 2015/05/13 at 20:55:53, zachr wrote: > On 2015/05/13 20:50:36, spang wrote: > > On 2015/05/13 20:44:46, reveman wrote: > > > On 2015/05/13 at 19:58:04, zachr wrote: > > > > On 2015/05/13 19:47:02, reveman wrote: > > > > > when using vgem, will we have both drm and vgem devices at the same time? > > > > > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing that > > makes > > > it special is that it is not backed by any physical device and is therefore > > > platform agnostic. This makes it primarily useful as something that works (via > > > prime dmabuf sharing) with the platform dependent drm devices, backed by real > > > hardware. > > > > > > Ok, should each physical device instead be optionally associated with a vgem > > > device? > > > > > > e.g. > > > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > > > base::FilePath /* device_path */, > > > base::FileDescriptor /* device_fd */) > > > base::FileDescriptor /* vgem_device_fd (optional) */) > > > > > > or is it necessary to handle these vgem devices as completely separate > > graphics > > > devices on the GPU process side? > > > > I think the interesting categories here are > > > > (1) DRM devices with modesetting resources. There's a single primary device for > > the GPU & builtin outputs, but more can come and go dynamically (e.g. > > DisplayLink). > > > > (2) vgem. It's virtual, has no outputs / modesetting resources, but is needed > > for sharing buffers to renderers. There's only one for the whole system (not one > > per above device). > > > > Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type. > > I agree with spang. Thanks for explaining. Makes sense. I'm not sure it make sense to use OzoneGpuMsg_AddGraphicsDevice for passing the vgem device handle as that message is clearly meant for devices with modesetting that can come and go. Would it make more sense to add something like a OzoneGpuMsg_RegisterVGEMDevice message for this purpose? Or use some other existing message that is sent just once to the GPU process.
On 2015/05/13 21:22:35, reveman wrote: > On 2015/05/13 at 20:55:53, zachr wrote: > > On 2015/05/13 20:50:36, spang wrote: > > > On 2015/05/13 20:44:46, reveman wrote: > > > > On 2015/05/13 at 19:58:04, zachr wrote: > > > > > On 2015/05/13 19:47:02, reveman wrote: > > > > > > when using vgem, will we have both drm and vgem devices at the same > time? > > > > > > > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing that > > > makes > > > > it special is that it is not backed by any physical device and is > therefore > > > > platform agnostic. This makes it primarily useful as something that works > (via > > > > prime dmabuf sharing) with the platform dependent drm devices, backed by > real > > > > hardware. > > > > > > > > Ok, should each physical device instead be optionally associated with a > vgem > > > > device? > > > > > > > > e.g. > > > > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > > > > base::FilePath /* device_path */, > > > > base::FileDescriptor /* device_fd */) > > > > base::FileDescriptor /* vgem_device_fd (optional) */) > > > > > > > > or is it necessary to handle these vgem devices as completely separate > > > graphics > > > > devices on the GPU process side? > > > > > > I think the interesting categories here are > > > > > > (1) DRM devices with modesetting resources. There's a single primary device > for > > > the GPU & builtin outputs, but more can come and go dynamically (e.g. > > > DisplayLink). > > > > > > (2) vgem. It's virtual, has no outputs / modesetting resources, but is > needed > > > for sharing buffers to renderers. There's only one for the whole system (not > one > > > per above device). > > > > > > Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type. > > > > I agree with spang. > > Thanks for explaining. Makes sense. > > I'm not sure it make sense to use OzoneGpuMsg_AddGraphicsDevice for passing the > vgem device handle as that message is clearly meant for devices with modesetting > that can come and go. Would it make more sense to add something like a > OzoneGpuMsg_RegisterVGEMDevice message for this purpose? Or use some other > existing message that is sent just once to the GPU process. Yeah, I think I'd prefer a different message to an "is_vgem" boolean as well. Although vgem is also a DRM device, it's function is different from that of OzoneGpuMsg_AddGraphicsDevice. It doesn't need to be dynamic or removable, either.
Thank you for nice discussion. I couldn't participate due to different time zone. I add new messages: OzoneGpuMsg_AddVgemDevice, OzoneGpuMsg_RemoveVgemDevice On 2015/05/13 21:28:41, spang wrote: > On 2015/05/13 21:22:35, reveman wrote: > > On 2015/05/13 at 20:55:53, zachr wrote: > > > On 2015/05/13 20:50:36, spang wrote: > > > > On 2015/05/13 20:44:46, reveman wrote: > > > > > On 2015/05/13 at 19:58:04, zachr wrote: > > > > > > On 2015/05/13 19:47:02, reveman wrote: > > > > > > > when using vgem, will we have both drm and vgem devices at the same > > time? > > > > > > > > > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing > that > > > > makes > > > > > it special is that it is not backed by any physical device and is > > therefore > > > > > platform agnostic. This makes it primarily useful as something that > works > > (via > > > > > prime dmabuf sharing) with the platform dependent drm devices, backed by > > real > > > > > hardware. > > > > > > > > > > Ok, should each physical device instead be optionally associated with a > > vgem > > > > > device? > > > > > > > > > > e.g. > > > > > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > > > > > base::FilePath /* device_path */, > > > > > base::FileDescriptor /* device_fd */) > > > > > base::FileDescriptor /* vgem_device_fd (optional) > */) > > > > > > > > > > or is it necessary to handle these vgem devices as completely separate > > > > graphics > > > > > devices on the GPU process side? > > > > > > > > I think the interesting categories here are > > > > > > > > (1) DRM devices with modesetting resources. There's a single primary > device > > for > > > > the GPU & builtin outputs, but more can come and go dynamically (e.g. > > > > DisplayLink). > > > > > > > > (2) vgem. It's virtual, has no outputs / modesetting resources, but is > > needed > > > > for sharing buffers to renderers. There's only one for the whole system > (not > > one > > > > per above device). > > > > > > > > Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type. > > > > > > I agree with spang. > > > > Thanks for explaining. Makes sense. > > > > I'm not sure it make sense to use OzoneGpuMsg_AddGraphicsDevice for passing > the > > vgem device handle as that message is clearly meant for devices with > modesetting > > that can come and go. Would it make more sense to add something like a > > OzoneGpuMsg_RegisterVGEMDevice message for this purpose? Or use some other > > existing message that is sent just once to the GPU process. > > Yeah, I think I'd prefer a different message to an "is_vgem" boolean as well. > Although vgem is also a DRM device, it's function is different from that of > OzoneGpuMsg_AddGraphicsDevice. Yes, I add new messages. > It doesn't need to be dynamic or removable, either. How about drm device? Why drm device need to be dynamic or removable? In my opinion, vgem device should be treated in the same way to drm device, so I did it. If drm and vgem device don't need to be dynamic, we will refactor together in the future. On 2015/05/13 19:58:04, zachr wrote: > On 2015/05/13 19:47:02, reveman wrote: > > when using vgem, will we have both drm and vgem devices at the same time? > > Yes, but first I'd like to say that vgem IS a drm device. The thing that makes > it special is that it is not backed by any physical device and is therefore > platform agnostic. This makes it primarily useful as something that works (via > prime dmabuf sharing) with the platform dependent drm devices, backed by real > hardware. Thx for nice explanation. I rename GemDevice to DrmDeviceBase because vgem is drm.
On 2015/05/14 13:19:55, dshwang wrote: > Thank you for nice discussion. I couldn't participate due to different time > zone. > I add new messages: OzoneGpuMsg_AddVgemDevice, OzoneGpuMsg_RemoveVgemDevice > > On 2015/05/13 21:28:41, spang wrote: > > On 2015/05/13 21:22:35, reveman wrote: > > > On 2015/05/13 at 20:55:53, zachr wrote: > > > > On 2015/05/13 20:50:36, spang wrote: > > > > > On 2015/05/13 20:44:46, reveman wrote: > > > > > > On 2015/05/13 at 19:58:04, zachr wrote: > > > > > > > On 2015/05/13 19:47:02, reveman wrote: > > > > > > > > when using vgem, will we have both drm and vgem devices at the > same > > > time? > > > > > > > > > > > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing > > that > > > > > makes > > > > > > it special is that it is not backed by any physical device and is > > > therefore > > > > > > platform agnostic. This makes it primarily useful as something that > > works > > > (via > > > > > > prime dmabuf sharing) with the platform dependent drm devices, backed > by > > > real > > > > > > hardware. > > > > > > > > > > > > Ok, should each physical device instead be optionally associated with > a > > > vgem > > > > > > device? > > > > > > > > > > > > e.g. > > > > > > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > > > > > > base::FilePath /* device_path */, > > > > > > base::FileDescriptor /* device_fd */) > > > > > > base::FileDescriptor /* vgem_device_fd (optional) > > */) > > > > > > > > > > > > or is it necessary to handle these vgem devices as completely separate > > > > > graphics > > > > > > devices on the GPU process side? > > > > > > > > > > I think the interesting categories here are > > > > > > > > > > (1) DRM devices with modesetting resources. There's a single primary > > device > > > for > > > > > the GPU & builtin outputs, but more can come and go dynamically (e.g. > > > > > DisplayLink). > > > > > > > > > > (2) vgem. It's virtual, has no outputs / modesetting resources, but is > > > needed > > > > > for sharing buffers to renderers. There's only one for the whole system > > (not > > > one > > > > > per above device). > > > > > > > > > > Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type. > > > > > > > > I agree with spang. > > > > > > Thanks for explaining. Makes sense. > > > > > > I'm not sure it make sense to use OzoneGpuMsg_AddGraphicsDevice for passing > > the > > > vgem device handle as that message is clearly meant for devices with > > modesetting > > > that can come and go. Would it make more sense to add something like a > > > OzoneGpuMsg_RegisterVGEMDevice message for this purpose? Or use some other > > > existing message that is sent just once to the GPU process. > > > > Yeah, I think I'd prefer a different message to an "is_vgem" boolean as well. > > Although vgem is also a DRM device, it's function is different from that of > > OzoneGpuMsg_AddGraphicsDevice. > > Yes, I add new messages. > > > It doesn't need to be dynamic or removable, either. > > How about drm device? Why drm device need to be dynamic or removable? Because DisplayLink devices are dynamic and removable. > In my opinion, vgem device should be treated in the same way to drm device, so I did > it. If drm and vgem device don't need to be dynamic, we will refactor together in > the future. They can't be treated the same because they serve different functions. You either need your is_vgem boolean or a 2nd IPC to provide the vgem device. > On 2015/05/13 19:58:04, zachr wrote: > > On 2015/05/13 19:47:02, reveman wrote: > > > when using vgem, will we have both drm and vgem devices at the same time? > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing that makes > > it special is that it is not backed by any physical device and is therefore > > platform agnostic. This makes it primarily useful as something that works (via > > prime dmabuf sharing) with the platform dependent drm devices, backed by real > > hardware. > > Thx for nice explanation. I rename GemDevice to DrmDeviceBase because vgem is > drm.
On 2015/05/14 13:19:55, dshwang wrote: > Thank you for nice discussion. I couldn't participate due to different time > zone. > I add new messages: OzoneGpuMsg_AddVgemDevice, OzoneGpuMsg_RemoveVgemDevice > > On 2015/05/13 21:28:41, spang wrote: > > On 2015/05/13 21:22:35, reveman wrote: > > > On 2015/05/13 at 20:55:53, zachr wrote: > > > > On 2015/05/13 20:50:36, spang wrote: > > > > > On 2015/05/13 20:44:46, reveman wrote: > > > > > > On 2015/05/13 at 19:58:04, zachr wrote: > > > > > > > On 2015/05/13 19:47:02, reveman wrote: > > > > > > > > when using vgem, will we have both drm and vgem devices at the > same > > > time? > > > > > > > > > > > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing > > that > > > > > makes > > > > > > it special is that it is not backed by any physical device and is > > > therefore > > > > > > platform agnostic. This makes it primarily useful as something that > > works > > > (via > > > > > > prime dmabuf sharing) with the platform dependent drm devices, backed > by > > > real > > > > > > hardware. > > > > > > > > > > > > Ok, should each physical device instead be optionally associated with > a > > > vgem > > > > > > device? > > > > > > > > > > > > e.g. > > > > > > IPC_MESSAGE_CONTROL3(OzoneGpuMsg_AddGraphicsDevice, > > > > > > base::FilePath /* device_path */, > > > > > > base::FileDescriptor /* device_fd */) > > > > > > base::FileDescriptor /* vgem_device_fd (optional) > > */) > > > > > > > > > > > > or is it necessary to handle these vgem devices as completely separate > > > > > graphics > > > > > > devices on the GPU process side? > > > > > > > > > > I think the interesting categories here are > > > > > > > > > > (1) DRM devices with modesetting resources. There's a single primary > > device > > > for > > > > > the GPU & builtin outputs, but more can come and go dynamically (e.g. > > > > > DisplayLink). > > > > > > > > > > (2) vgem. It's virtual, has no outputs / modesetting resources, but is > > > needed > > > > > for sharing buffers to renderers. There's only one for the whole system > > (not > > > one > > > > > per above device). > > > > > > > > > > Currently OzoneGpuMsg_AddGraphicsDevice is only used for the first type. > > > > > > > > I agree with spang. > > > > > > Thanks for explaining. Makes sense. > > > > > > I'm not sure it make sense to use OzoneGpuMsg_AddGraphicsDevice for passing > > the > > > vgem device handle as that message is clearly meant for devices with > > modesetting > > > that can come and go. Would it make more sense to add something like a > > > OzoneGpuMsg_RegisterVGEMDevice message for this purpose? Or use some other > > > existing message that is sent just once to the GPU process. > > > > Yeah, I think I'd prefer a different message to an "is_vgem" boolean as well. > > Although vgem is also a DRM device, it's function is different from that of > > OzoneGpuMsg_AddGraphicsDevice. > > Yes, I add new messages. > > > It doesn't need to be dynamic or removable, either. > > How about drm device? Why drm device need to be dynamic or removable? Because DisplayLink devices are dynamic and removable. > In my opinion, vgem device should be treated in the same way to drm device, so I did > it. If drm and vgem device don't need to be dynamic, we will refactor together in > the future. They can't be treated the same because they serve different functions. You either need your is_vgem boolean or a 2nd IPC to provide the vgem device. > On 2015/05/13 19:58:04, zachr wrote: > > On 2015/05/13 19:47:02, reveman wrote: > > > when using vgem, will we have both drm and vgem devices at the same time? > > > > Yes, but first I'd like to say that vgem IS a drm device. The thing that makes > > it special is that it is not backed by any physical device and is therefore > > platform agnostic. This makes it primarily useful as something that works (via > > prime dmabuf sharing) with the platform dependent drm devices, backed by real > > hardware. > > Thx for nice explanation. I rename GemDevice to DrmDeviceBase because vgem is > drm.
On 2015/05/14 16:24:29, spang wrote: > On 2015/05/14 13:19:55, dshwang wrote: > > > > I'm not sure it make sense to use OzoneGpuMsg_AddGraphicsDevice for > passing > > > the > > > > vgem device handle as that message is clearly meant for devices with > > > modesetting > > > > that can come and go. Would it make more sense to add something like a > > > > OzoneGpuMsg_RegisterVGEMDevice message for this purpose? Or use some other > > > > existing message that is sent just once to the GPU process. > > > > > > Yeah, I think I'd prefer a different message to an "is_vgem" boolean as > well. > > > Although vgem is also a DRM device, it's function is different from that of > > > OzoneGpuMsg_AddGraphicsDevice. > > > > Yes, I add new messages. > > > > > It doesn't need to be dynamic or removable, either. I handle vgem in same way to primary device. DCHECK in both browser and gpu process if it's removed. > > How about drm device? Why drm device need to be dynamic or removable? > > Because DisplayLink devices are dynamic and removable. got it. thx. > They can't be treated the same because they serve different functions. You > either need your is_vgem boolean or a 2nd IPC to provide the vgem device. Add new OzoneGpuMsg_AddVgemDevice message to add vgem fd in gpu process.
Patchset #9 (id:160001) has been deleted
hi dnicoara, spang, could you review again?
Do we need to involve DrmDeviceManager and DrmDevice in this?
On 2015/05/18 20:10:32, reveman wrote: > Do we need to involve DrmDeviceManager and DrmDevice in this? I think it's natural for DrmDeviceManager to handle vgem because vgem is one of drm devices. I couldn't find better class to handle vgem. However, inheriting DrmDevice is not necessary. All DrmDeviceManager have to do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the Patch Set 1.
On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > On 2015/05/18 20:10:32, reveman wrote: > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > I think it's natural for DrmDeviceManager to handle vgem because vgem is one of drm devices. I couldn't find better class to handle vgem. > However, inheriting DrmDevice is not necessary. All DrmDeviceManager have to do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the Patch Set 1. DrmDeviceManager manages a set of removable DrmDevice instances. Does it really make sense for the vgem fd to live there? Could it just live in DrmGpuPlatformSupport?
On 2015/05/19 14:47:18, reveman wrote: > On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > > On 2015/05/18 20:10:32, reveman wrote: > > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > > > I think it's natural for DrmDeviceManager to handle vgem because vgem is one > of drm devices. I couldn't find better class to handle vgem. > > However, inheriting DrmDevice is not necessary. All DrmDeviceManager have to > do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the Patch > Set 1. > > DrmDeviceManager manages a set of removable DrmDevice instances. Does it really > make sense for the vgem fd to live there? Could it just live in > DrmGpuPlatformSupport? That's good point. It's not strange for DrmGpuPlatformSupport to handle vgem also. By the way, |primary_device_| is handled by DrmDeviceManager, which is not-removable DrmDevice. spang, what do you think?
On 2015/05/19 at 15:17:41, dongseong.hwang wrote: > On 2015/05/19 14:47:18, reveman wrote: > > On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > > > On 2015/05/18 20:10:32, reveman wrote: > > > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > > > > > I think it's natural for DrmDeviceManager to handle vgem because vgem is one > > of drm devices. I couldn't find better class to handle vgem. > > > However, inheriting DrmDevice is not necessary. All DrmDeviceManager have to > > do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the Patch > > Set 1. > > > > DrmDeviceManager manages a set of removable DrmDevice instances. Does it really > > make sense for the vgem fd to live there? Could it just live in > > DrmGpuPlatformSupport? > > That's good point. It's not strange for DrmGpuPlatformSupport to handle vgem also. > By the way, |primary_device_| is handled by DrmDeviceManager, which is not-removable DrmDevice. > spang, what do you think? How is this fd going to be used and who will need access it? I think that will influence the decision of where this should live.
On 2015/05/19 15:21:34, reveman wrote: > On 2015/05/19 at 15:17:41, dongseong.hwang wrote: > > On 2015/05/19 14:47:18, reveman wrote: > > > On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > > > > On 2015/05/18 20:10:32, reveman wrote: > > > > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > > > > > > > I think it's natural for DrmDeviceManager to handle vgem because vgem is > one > > > of drm devices. I couldn't find better class to handle vgem. > > > > However, inheriting DrmDevice is not necessary. All DrmDeviceManager have > to > > > do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the > Patch > > > Set 1. > > > > > > DrmDeviceManager manages a set of removable DrmDevice instances. Does it > really > > > make sense for the vgem fd to live there? Could it just live in > > > DrmGpuPlatformSupport? > > > > That's good point. It's not strange for DrmGpuPlatformSupport to handle vgem > also. > > By the way, |primary_device_| is handled by DrmDeviceManager, which is > not-removable DrmDevice. > > spang, what do you think? > > How is this fd going to be used and who will need access it? I think that will > influence the decision of where this should live. That's good question. I'm introducing OzoneClient in https://codereview.chromium.org/1128113011/ OzoneClient is kind of OzonePlatform for renderer. OzoneClient has SurfaceClientFactoryOzone interface, which provide surface sharing mechanism for renderer. SurfaceClientFactoryOzone will receive vgem fd lazily from GPU process via IPC in the same way how DrmDisplayHostManager sends drm device fd to DrmGpuPlatformSupport. Worth to note is that Browser process can manage vgem fd and send to Renderer. However, in my opinion, it's a bit confusing.
On 2015/05/19 at 15:38:17, dongseong.hwang wrote: > On 2015/05/19 15:21:34, reveman wrote: > > On 2015/05/19 at 15:17:41, dongseong.hwang wrote: > > > On 2015/05/19 14:47:18, reveman wrote: > > > > On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > > > > > On 2015/05/18 20:10:32, reveman wrote: > > > > > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > > > > > > > > > I think it's natural for DrmDeviceManager to handle vgem because vgem is > > one > > > > of drm devices. I couldn't find better class to handle vgem. > > > > > However, inheriting DrmDevice is not necessary. All DrmDeviceManager have > > to > > > > do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the > > Patch > > > > Set 1. > > > > > > > > DrmDeviceManager manages a set of removable DrmDevice instances. Does it > > really > > > > make sense for the vgem fd to live there? Could it just live in > > > > DrmGpuPlatformSupport? > > > > > > That's good point. It's not strange for DrmGpuPlatformSupport to handle vgem > > also. > > > By the way, |primary_device_| is handled by DrmDeviceManager, which is > > not-removable DrmDevice. > > > spang, what do you think? > > > > How is this fd going to be used and who will need access it? I think that will > > influence the decision of where this should live. > > That's good question. I'm introducing OzoneClient in https://codereview.chromium.org/1128113011/ > OzoneClient is kind of OzonePlatform for renderer. OzoneClient has SurfaceClientFactoryOzone interface, which provide surface sharing mechanism for renderer. > SurfaceClientFactoryOzone will receive vgem fd lazily from GPU process via IPC in the same way how DrmDisplayHostManager sends drm device fd to DrmGpuPlatformSupport. > > Worth to note is that Browser process can manage vgem fd and send to Renderer. However, in my opinion, it's a bit confusing. I'm having a hard time seeing how all this fits together. Maybe a diagram and some example use cases would help make it easier to understand. Do you have anything like that?
On 2015/05/19 15:59:20, reveman wrote: > On 2015/05/19 at 15:38:17, dongseong.hwang wrote: > > On 2015/05/19 15:21:34, reveman wrote: > > > On 2015/05/19 at 15:17:41, dongseong.hwang wrote: > > > > On 2015/05/19 14:47:18, reveman wrote: > > > > > On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > > > > > > On 2015/05/18 20:10:32, reveman wrote: > > > > > > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > > > > > > > > > > > I think it's natural for DrmDeviceManager to handle vgem because vgem > is > > > one > > > > > of drm devices. I couldn't find better class to handle vgem. > > > > > > However, inheriting DrmDevice is not necessary. All DrmDeviceManager > have > > > to > > > > > do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like > the > > > Patch > > > > > Set 1. > > > > > > > > > > DrmDeviceManager manages a set of removable DrmDevice instances. Does it > > > really > > > > > make sense for the vgem fd to live there? Could it just live in > > > > > DrmGpuPlatformSupport? > > > > > > > > That's good point. It's not strange for DrmGpuPlatformSupport to handle > vgem > > > also. > > > > By the way, |primary_device_| is handled by DrmDeviceManager, which is > > > not-removable DrmDevice. > > > > spang, what do you think? > > > > > > How is this fd going to be used and who will need access it? I think that > will > > > influence the decision of where this should live. > > > > That's good question. I'm introducing OzoneClient in > https://codereview.chromium.org/1128113011/ > > OzoneClient is kind of OzonePlatform for renderer. OzoneClient has > SurfaceClientFactoryOzone interface, which provide surface sharing mechanism for > renderer. > > SurfaceClientFactoryOzone will receive vgem fd lazily from GPU process via IPC > in the same way how DrmDisplayHostManager sends drm device fd to > DrmGpuPlatformSupport. > > > > Worth to note is that Browser process can manage vgem fd and send to Renderer. > However, in my opinion, it's a bit confusing. > > I'm having a hard time seeing how all this fits together. Maybe a diagram and > some example use cases would help make it easier to understand. Do you have > anything like that? Sorry for confusing. No, I don't have, but I'll write a document and share it soon.
> > > > I'm having a hard time seeing how all this fits together. Maybe a diagram and > > some example use cases would help make it easier to understand. Do you have > > anything like that? > > Sorry for confusing. No, I don't have, but I'll write a document and share it > soon. Please extend the current document we have, rather then creating a new one
On 2015/05/19 15:38:17, dshwang wrote: > On 2015/05/19 15:21:34, reveman wrote: > > On 2015/05/19 at 15:17:41, dongseong.hwang wrote: > > > On 2015/05/19 14:47:18, reveman wrote: > > > > On 2015/05/19 at 10:35:47, dongseong.hwang wrote: > > > > > On 2015/05/18 20:10:32, reveman wrote: > > > > > > Do we need to involve DrmDeviceManager and DrmDevice in this? > > > > > > > > > > I think it's natural for DrmDeviceManager to handle vgem because vgem is > > one > > > > of drm devices. I couldn't find better class to handle vgem. > > > > > However, inheriting DrmDevice is not necessary. All DrmDeviceManager > have > > to > > > > do is to keep vgem fd, so DrmDeviceManager can just keep vgem fd like the > > Patch > > > > Set 1. > > > > > > > > DrmDeviceManager manages a set of removable DrmDevice instances. Does it > > really > > > > make sense for the vgem fd to live there? Could it just live in > > > > DrmGpuPlatformSupport? > > > > > > That's good point. It's not strange for DrmGpuPlatformSupport to handle vgem > > also. > > > By the way, |primary_device_| is handled by DrmDeviceManager, which is > > not-removable DrmDevice. > > > spang, what do you think? > > > > How is this fd going to be used and who will need access it? I think that will > > influence the decision of where this should live. > > That's good question. I'm introducing OzoneClient in > https://codereview.chromium.org/1128113011/ > OzoneClient is kind of OzonePlatform for renderer. OzoneClient has > SurfaceClientFactoryOzone interface, which provide surface sharing mechanism for > renderer. > SurfaceClientFactoryOzone will receive vgem fd lazily from GPU process via IPC > in the same way how DrmDisplayHostManager sends drm device fd to > DrmGpuPlatformSupport. > > Worth to note is that Browser process can manage vgem fd and send to Renderer. > However, in my opinion, it's a bit confusing. Reading https://codereview.chromium.org/1134993003 it looks like the only place the VGEM device is used from is GbmSurfaceFactory. I'd say that's a good place to own it. IMO, I think we're spending too much time in trying to bike-shed where to place this and how to pipe the FD. I'd say it would be nice to have the dependent CLs marked as a patch series and we can focus on the real usage of the VGEM device first.
On 2015/05/19 17:33:04, dnicoara wrote: > IMO, I think we're spending too much time in trying to bike-shed where to place > this and how to pipe the FD. I'd say it would be nice to have the dependent CLs > marked as a patch series and we can focus on the real usage of the VGEM device > first. yup! I appreciate your words, dnicoara. Thanks!
On 2015/05/19 15:59:20, reveman wrote: > I'm having a hard time seeing how all this fits together. Maybe a diagram and > some example use cases would help make it easier to understand. Do you have > anything like that? I explain how Browser process, GPU process and Renderer use vgem fd and create a vgem bo by vgem fd. See 'Control flow' section in the "native GpuMemoryBuffer on ChromeOS" document; https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8 Please feedback by Google docs comment. Thank you.
On 2015/05/19 18:06:44, dshwang wrote: > On 2015/05/19 15:59:20, reveman wrote: > > I'm having a hard time seeing how all this fits together. Maybe a diagram and > > some example use cases would help make it easier to understand. Do you have > > anything like that? > > I explain how Browser process, GPU process and Renderer use vgem fd and create a > vgem bo by vgem fd. > See 'Control flow' section in the "native GpuMemoryBuffer on ChromeOS" document; > https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8 > > Please feedback by Google docs comment. Thank you. reveman raises concern about why GPU process should get VGEM fd. That's good point. It's not necessary for GPU process to handle vgem FD. I made GPU process handle VGEM fd in this CL because it looks supposed to be like DRM device FD. VGEM FD is finally used in Browser process or Renderer. GPU process doesn't need to do anything for VGEM fd. So I made experimental CL to make DrmDisplayHostManager in Browser process handle VGEM fd in https://codereview.chromium.org/1151533002 DrmDisplayHostManager will distrubute VGEM FD to Renderers. Does the another CL's approach is better?
close it because https://codereview.chromium.org/1151533002/ is more viable solution. |
