On 09/05/2018 03:07 PM, Emil Velikov wrote: > 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 Ouch, I just pushed the patch, but feel free to cut the release just before that commit. /Thomas
