[PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Thomas Hellstrom]

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
>