[PATCH v2 1/2] drm: document drm_mode_create_lease object requirements

[PATCH v2 1/2] drm: document drm_mode_create_lease object requirements

[Simon Ser]

validate_lease expects one CRTC, one connector and one plane.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Keith Packard <keithp@keithp.com>
---
 include/uapi/drm/drm_mode.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 90c55383f1ee..e4a2570a6058 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1110,6 +1110,9 @@ struct drm_mode_destroy_blob {
  * struct drm_mode_create_lease - Create lease
  *
  * Lease mode resources, creating another drm_master.
+ *
+ * The @object_ids array must reference at least one CRTC, one connector and
+ * one plane if &DRM_CLIENT_CAP_UNIVERSAL_PLANES is enabled.
  */
 struct drm_mode_create_lease {
 	/** @object_ids: Pointer to array of object ids (__u32) */
-- 
2.33.0

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

[Simon Ser]

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(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index dee4f24a1808..d72c2fac0ff1 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -489,12 +489,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	/* need some objects */
-	if (cl->object_count == 0) {
-		DRM_DEBUG_LEASE("no objects in lease\n");
-		return -EINVAL;
-	}
-
 	if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) {
 		DRM_DEBUG_LEASE("invalid flags\n");
 		return -EINVAL;
@@ -510,23 +504,26 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 
 	object_count = cl->object_count;
 
-	object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
-			array_size(object_count, sizeof(__u32)));
-	if (IS_ERR(object_ids)) {
-		ret = PTR_ERR(object_ids);
-		goto out_lessor;
-	}
-
+	/* Handle leased objects, if any */
 	idr_init(&leases);
+	if (object_count != 0) {
+		object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
+					 array_size(object_count, sizeof(__u32)));
+		if (IS_ERR(object_ids)) {
+			ret = PTR_ERR(object_ids);
+			idr_destroy(&leases);
+			goto out_lessor;
+		}
 
-	/* fill and validate the object idr */
-	ret = fill_object_idr(dev, lessor_priv, &leases,
-			      object_count, object_ids);
-	kfree(object_ids);
-	if (ret) {
-		DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
-		idr_destroy(&leases);
-		goto out_lessor;
+		/* fill and validate the object idr */
+		ret = fill_object_idr(dev, lessor_priv, &leases,
+				      object_count, object_ids);
+		kfree(object_ids);
+		if (ret) {
+			DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
+			idr_destroy(&leases);
+			goto out_lessor;
+		}
 	}
 
 	/* Allocate a file descriptor for the lease */
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index e4a2570a6058..e1e351682872 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1112,7 +1112,8 @@ struct drm_mode_destroy_blob {
  * Lease mode resources, creating another drm_master.
  *
  * The @object_ids array must reference at least one CRTC, one connector and
- * one plane if &DRM_CLIENT_CAP_UNIVERSAL_PLANES is enabled.
+ * one plane if &DRM_CLIENT_CAP_UNIVERSAL_PLANES is enabled. Alternatively,
+ * the lease can be completely empty.
  */
 struct drm_mode_create_lease {
 	/** @object_ids: Pointer to array of object ids (__u32) */
-- 
2.33.0

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

[Simon Ser]

Anyone up for a review on this one? Daniel Vetter has ack'ed but
doesn't have time for a full review.

CC Maarten Lankhorst, Maxime Ripard, Michel Dänzer

Who else should I CC?

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

[Pekka Paalanen]

[-- 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 --]

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

[Daniel Stone]

Hey,

On Wed, 22 Sept 2021 at 09:48, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> that is one awesome commit message! It explains everything I might have wanted
> to ask.

Yeah, what he said. An awesome explanation of a terrible problem.

My main worry is that we'd end up tripping over our own feet in either
the kernel or libdrm when GetResources got called against an
empty-lease file description, but that doesn't seem to be the case:
the worst I can see is that we'll return the list of all encoders,
which is useless but AFAIK harmless.

So this is:
Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

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

[Simon Ser]

Thanks a lot for the reviews!

Re: [PATCH v2 1/2] drm: document drm_mode_create_lease object requirements

[Daniel Vetter]

On Fri, Sep 03, 2021 at 01:00:16PM +0000, Simon Ser wrote:
> validate_lease expects one CRTC, one connector and one plane.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Cc: Keith Packard <keithp@keithp.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  include/uapi/drm/drm_mode.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 90c55383f1ee..e4a2570a6058 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1110,6 +1110,9 @@ struct drm_mode_destroy_blob {
>   * struct drm_mode_create_lease - Create lease
>   *
>   * Lease mode resources, creating another drm_master.
> + *
> + * The @object_ids array must reference at least one CRTC, one connector and
> + * one plane if &DRM_CLIENT_CAP_UNIVERSAL_PLANES is enabled.
>   */
>  struct drm_mode_create_lease {
>  	/** @object_ids: Pointer to array of object ids (__u32) */
> -- 
> 2.33.0
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch