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. Thanks, Thomas > Thomas, can you please send a patch to the respective teams or give > them a heads up? > > Thanks > Emil >
