Re: [PATCH v2 2/2] drm/lease: allow empty leases

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5216 bytes --]

On Fri, 03 Sep 2021 13:00:32 +0000
Simon Ser <contact@emersion.fr> wrote:

> This can be used to create a separate DRM file description, thus
> creating a new GEM handle namespace.
> 
> My use-case is wlroots. The library splits responsibilities between
> separate components: the GBM allocator creates buffers, the GLES2
> renderer uses EGL to import them and render to them, the DRM
> backend imports the buffers and displays them. wlroots has a
> modular architecture, and any of these components can be swapped
> and replaced with something else. For instance, the pipeline can
> be set up so that the DRM dumb buffer allocator is used instead of
> GBM and the Pixman renderer is used instead of GLES2. Library users
> can also replace any of these components with their own custom one.
> 
> DMA-BUFs are used to pass buffer references across components. We
> could use GEM handles instead, but this would result in pain if
> multiple GPUs are in use: wlroots copies buffers across GPUs as
> needed. Importing a GEM handle created on one GPU into a completely
> different GPU will blow up (fail at best, mix unrelated buffers
> otherwise).
> 
> Everything is fine if all components use Mesa. However, this isn't
> always desirable. For instance when running with DRM dumb buffers
> and the Pixman software renderer it's unfortunate to depend on GBM
> in the DRM backend just to turn DMA-BUFs into FB IDs. GBM loads
> Mesa drivers to perform an action which has nothing driver-specific.
> Additionally, drivers will fail the import if the 3D engine can't
> use the imported buffer, for instance amdgpu will refuse to import
> DRM dumb buffers [1]. We might also want to be running with a Vulkan
> renderer and a Vulkan allocator in the future, and GBM wouldn't be
> welcome in this setup.
> 
> To address this, GBM can be side-stepped in the DRM backend, and
> can be replaced with drmPrimeFDToHandle calls. However because of
> GEM handle reference counting issues, care must be taken to avoid
> double-closing the same GEM handle. In particular, it's not
> possible to share a DRM FD with GBM or EGL and perform some
> drmPrimeFDToHandle calls manually.
> 
> So wlroots needs to re-open the DRM FD to create a new GEM handle
> namespace. However there's no guarantee that the file-system
> permissions will be set up so that the primary FD can be opened
> by the compsoitor. On modern systems seatd or logind is a privileged
> process responsible for doing this, and other processes aren't
> expected to do it. For historical reasons systemd still allows
> physically logged in users to open primary DRM nodes, but this
> doesn't work on non-systemd setups and it's desirable to lock
> them down at some point.
> 
> Some might suggest to open the render node instead of re-opening
> the primary node. However some systems don't have a render node
> at all (e.g. no GPU, or a split render/display SoC).
> 
> Solutions to this issue have been discussed in [2]. One solution
> would be to open the magic /proc/self/fd/<fd> file, but it's a
> Linux-specific hack (wlroots supports BSDs too). Another solution
> is to add support for re-opening a DRM primary node to seatd/logind,
> but they don't support it now and really haven't been designed for
> this (logind would need to grow a completely new API, because it
> assumes unique dev_t IDs). Also this seems like pushing down a
> kernel limitation to user-space a bit too hard.
> 
> Another solution is to allow creating empty DRM leases. The lessee
> FD would have its own GEM handle namespace, so wouldn't conflict
> wth GBM/EGL. It would have the master bit set, but would be able
> to manage zero resources. wlroots doesn't intend to share this FD
> with any other process.
> 
> All in all IMHO that seems like a pretty reasonable solution to the
> issue at hand.
> 
> Note, I've discussed with Jonas Ådahl and Mutter plans to adopt a
> similar design in the future.
> 
> Example usage in wlroots is available at [3]. IGT test available
> at [4].
> 
> [1]: https://github.com/swaywm/wlroots/issues/2916
> [2]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/110
> [3]: https://github.com/swaywm/wlroots/pull/3158
> [4]: https://patchwork.freedesktop.org/series/94323/
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_lease.c | 39 +++++++++++++++++--------------------
>  include/uapi/drm/drm_mode.h |  3 ++-
>  2 files changed, 20 insertions(+), 22 deletions(-)

Hi Simon,

that is one awesome commit message!

It explains everything I might have wanted to ask. I also agree with
your analysis, so this is an easy:

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>

Of course, I can't say anything about the actual code.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]