On 5 September 2018 at 11:10, Thomas Hellstrom <thellstrom at vmware.com> wrote: > Hi, Emil, > > > On 09/05/2018 11:33 AM, Emil Velikov wrote: >> >> On 4 September 2018 at 23:33, Dave Airlie <airlied at gmail.com> wrote: >>> >>> On Tue, 4 Sep 2018 at 03:00, Thomas Hellstrom <thellstrom at vmware.com> >>> wrote: >>>> >>>> On 09/03/2018 06:33 PM, Daniel Vetter wrote: >>>>> >>>>> On Mon, Sep 03, 2018 at 11:16:29AM +0200, Thomas Hellstrom wrote: >>>>>> >>>>>> On 08/31/2018 05:30 PM, Thomas Hellstrom wrote: >>>>>>> >>>>>>> On 08/31/2018 05:27 PM, Emil Velikov wrote: >>>>>>>> >>>>>>>> On 31 August 2018 at 15:38, Michel Dänzer <michel at daenzer.net> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> [ Adding the amd-gfx list ] >>>>>>>>> >>>>>>>>> On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote: >>>>>>>>>> >>>>>>>>>> On 08/31/2018 02:30 PM, Emil Velikov wrote: >>>>>>>>>>> >>>>>>>>>>> On 31 August 2018 at 12:54, Thomas Hellstrom >>>>>>>>>>> <thellstrom at vmware.com> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> To determine whether a device node is a drm device >>>>>>>>>>>> node or not, the code >>>>>>>>>>>> currently compares the node's major number to the static drm >>>>>>>>>>>> major >>>>>>>>>>>> device >>>>>>>>>>>> number. >>>>>>>>>>>> >>>>>>>>>>>> This breaks the standalone vmwgfx driver on XWayland dri >>>>>>>>>>>> clients, >>>>>>>>>>>> >>>>>>>>>>> Any particular reason why the code doesn't use a fixed node >>>>>>>>>>> there? >>>>>>>>>>> It will make the diff vs the in-kernel driver a bit smaller. >>>>>>>>>> >>>>>>>>>> Because then it won't be able to interoperate with other in-tree >>>>>>>>>> drivers, like virtual drm drivers or passthrough usb drm drivers. >>>>>>>>>> There is no clean way to share the minor number allocation >>>>>>>>>> with in-tree >>>>>>>>>> drm, so standalone vmwgfx is using dynamic major allocation. >>>>>>>>> >>>>>>>>> I wonder why I haven't heard of any of these issues with the >>>>>>>>> standalone >>>>>>>>> version of amdgpu shipped in packaged AMD releases. Does that >>>>>>>>> also use a >>>>>>>>> different major number? If yes, maybe it's just that nobody has >>>>>>>>> tried >>>>>>>>> Xwayland clients with that driver. If no, how does it avoid the >>>>>>>>> other >>>>>>>>> issues described above? >>>>>>>>> >>>>>>>> AFAICT, the difference is that the standalone vmwgfx uses an >>>>>>>> internal >>>>>>>> copy of drm core. >>>>>>>> It doesn't reuse the in-kernel drm, hence it cannot know which minor >>>>>>>> it can use. >>>>>>>> >>>>>>>> -Emil >>>>>>> >>>>>>> Actually, standalone vmwgfx could perhaps also try to allocate minors >>>>>>> from 63 and downwards. That might work, but needs some verification. >>>>>>> >>>>>> So unfortuntately this doesn't work since the in-tree drm's file >>>>>> operations >>>>>> are registered with the DRM_MAJOR. >>>>>> So I still think the patch is the way to go. If people are concerned >>>>>> that >>>>>> also fbdev file descriptors are allowed, perhaps there are other sysfs >>>>>> traits we can look at? >>>>> >>>>> Somewhat out of curiosity, but why do you have to overwrite all of drm? >>>>> amdgpu seems to be able to pull their stunt off without ... >>>>> -Daniel >>>> >>>> At the time we launched the standalone vmwgfx, the DRM <-> driver >>>> interface was moving considerably more rapidly than the DRM <-> kernel >>>> interface. I think that's still the case. Hence less work for us. Also >>>> meant we can install the full driver stack with latest features on >>>> fairly old VMs without backported DRM functionality. >>>> >>> I think this should be fine for 99% of drm usage, there may be corner >>> cases in wierd places, but I can't point to any that really matter >>> (maybe strace?) >>> >> Having a closer look, I think this will break the Firefox/Chrome >> sandboxing :-\ >> I cannot see the path /sys/dev/char/%d:%d/device/drm in the allowed >> list [1] [2]. > > Thanks for pointing this out. > > The function drmGetMinorNameForFD() already opens this path, so any > user-space using that function would not work before either. > > Also mozilla/firefox adds /sys/dev/char/226:* Which means that while it > still won't work on standalone vmwgfx, there should at least be no > regression. > > For Chromium it seems they allow /sys/dev/char/ for AmdGpu, but only under > ChromOS, so I'll ping those to be safe. > > I also won't be doing an immediate release after pushing. > In that case, please give me 24h to do a libdrm release before pushing. I had to push some workarounds for the sandboxing mentioned earlier :-\ Thanks Emil