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

[Thomas Hellstrom <thellstrom-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>]

On 09/05/2018 03:07 PM, Emil Velikov wrote:
> On 5 September 2018 at 11:10, Thomas Hellstrom <thellstrom@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@gmail.com> wrote:
>>>> On Tue, 4 Sep 2018 at 03:00, Thomas Hellstrom <thellstrom@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@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@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


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx