Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614559 > unrolled thread

[PATCH 0/4] drm: Add mode resource leasing

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] drm: Add mode resource leasing Keith Packard <keithp@keithp.com> - 2017-04-01 19:20 +0200
    [PATCH 1/4] drm: Add new LEASE debug level Keith Packard <keithp@keithp.com> - 2017-04-01 19:20 +0200
    [PATCH 2/4] drm: Add drm_object lease infrastructure Keith Packard <keithp@keithp.com> - 2017-04-01 19:20 +0200
      Re: [PATCH 2/4] drm: Add drm_object lease infrastructure Daniel Vetter <daniel@ffwll.ch> - 2017-04-02 15:40 +0200
        Re: [PATCH 2/4] drm: Add drm_object lease infrastructure Keith Packard <keithp@keithp.com> - 2017-04-02 18:40 +0200
          Re: [PATCH 2/4] drm: Add drm_object lease infrastructure Michel Dänzer <michel@daenzer.net> - 2017-04-10 07:20 +0200
      Re: [PATCH 2/4] drm: Add drm_object lease infrastructure Daniel Vetter <daniel@ffwll.ch> - 2017-04-02 20:00 +0200
        Re: [PATCH 2/4] drm: Add drm_object lease infrastructure Keith Packard <keithp@keithp.com> - 2017-04-02 22:10 +0200
    [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths Keith Packard <keithp@keithp.com> - 2017-04-01 19:20 +0200
      Re: [PATCH 3/4] drm: Check mode object lease status in all master  ioctl paths Daniel Vetter <daniel@ffwll.ch> - 2017-04-02 15:20 +0200
        Re: [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths Keith Packard <keithp@keithp.com> - 2017-04-02 18:40 +0200
          Re: [PATCH 3/4] drm: Check mode object lease status in all master  ioctl paths Daniel Vetter <daniel@ffwll.ch> - 2017-04-03 10:00 +0200
        Re: [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths Keith Packard <keithp@keithp.com> - 2017-04-10 06:50 +0200

#1614559 — [PATCH 0/4] drm: Add mode resource leasing

Here's a first cut of the proposed mode resource leasing code. What
this does is allow an application to create a new drm_master which
"leases" resources from an existing drm_master. This new drm_master
can do whatever it likes with the resources it was granted, including
setting modes, performing page_flip operations etc.

This can be used to run multiple compositors on the same GPU, each
driving its own set of outputs. Examples of this include multi-user
setups and virtual reality environments.

Because setting modes can consume 'hidden' resources within the GPU,
it isn't entirely clear that letting multiple processes perform mode
setting is a good idea or not. A process doing the usual
test/render/commit sequence may find that the commit fails because
some other process consumed necessary resources after the test was
invoked. Daniel Vetter suggested that perhaps some kind of locking
mechanism across this sequence might help, but that can lead to
problems when a process fails to unlock. If someone wants to come up
with a reasonable scheme here, that'd be great.

For now, I'll be working on the assumption that the lease holder will
not set any modes, which will avoid the problematic case described above.

The series is broken into four patches in an attempt to make review a
bit easier.

The trickiest bit of the code was in creating the new drm_master,
which involved allocating a new file and fd and getting those
initialized with the right reference counts on all of the related data
structures. It "seems" to work, but it would be nice if someone with
more experience in that part of the kernel could take a look at
it. That's in the fourth patch.

The third patch hooks the lease checks into the other ioctls; that
could use some review to make sure I didn't miss any needed checks.

-keith

[toc] | [next] | [standalone]

#1614560 — [PATCH 1/4] drm: Add new LEASE debug level

Separate out lease debugging from the core.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/drm_drv.c | 3 ++-
 include/drm/drmP.h        | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 6594b4088f11..d4a3612655e3 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -57,7 +57,8 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
 "\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n"
 "\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n"
 "\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n"
-"\t\tBit 5 (0x20) will enable VBL messages (vblank code)");
+"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n"
+"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)");
 module_param_named(debug, drm_debug, int, 0600);
 
 static DEFINE_SPINLOCK(drm_minor_lock);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9c4ee144b5f6..304a22c87999 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -137,6 +137,7 @@ struct dma_buf_attachment;
 #define DRM_UT_ATOMIC		0x10
 #define DRM_UT_VBL		0x20
 #define DRM_UT_STATE		0x40
+#define DRM_UT_LEASE		0x80
 
 /***********************************************************************/
 /** \name DRM template customization defaults */
@@ -251,6 +252,9 @@ struct dma_buf_attachment;
 #define DRM_DEBUG_VBL(fmt, ...)					\
 	drm_printk(KERN_DEBUG, DRM_UT_VBL, fmt, ##__VA_ARGS__)
 
+#define DRM_DEBUG_LEASE(fmt, ...)					\
+	drm_printk(KERN_DEBUG, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+
 #define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, level, fmt, args...)	\
 ({									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1614563 — [PATCH 2/4] drm: Add drm_object lease infrastructure

This provides new data structures to hold "lease" information about
drm mode setting objects, and provides for creating new drm_masters
which have access to a subset of the available drm resources.

An 'owner' is a drm_master which is not leasing the objects from
another drm_master, and hence 'owns' them. This sits at the top of a
tree of drm_masters.

A 'lessee' is a drm_master which is leasing objects from some other
drm_master. Each lessee holds the set of objects which it is leasing
from the lessor.

A 'lessor' is a drm_master which is leasing objects to another
drm_master.

The set of objects any drm_master 'controls' is limited to the set of
objects it leases (for lessees) or all objects (for owners),
optionally minus the set of objects it has leased to other
drm_masters.

Objects not controlled by a drm_master cannot be modified through the
various state manipulating ioctls, and any state reported back to user
space will be edited to make them appear idle and/or unusable. For
instance, connectors always report 'disconnected', while encoders
report no possible crtcs or clones.

The full list of lessees leasing objects from an owner (either
directly, or indirectly through another lessee), can be searched from
an idr in the drm_master of the owner.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/Makefile    |   3 +-
 drivers/gpu/drm/drm_auth.c  |  22 +-
 drivers/gpu/drm/drm_lease.c | 485 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h          |   1 +
 include/drm/drm_auth.h      |  28 +++
 include/drm/drm_lease.h     |  51 +++++
 6 files changed, 588 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_lease.c
 create mode 100644 include/drm/drm_lease.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index b9ae4280de9d..c2c6d61d30cf 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,7 +16,8 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_framebuffer.o drm_connector.o drm_blend.o \
 		drm_encoder.o drm_mode_object.o drm_property.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
-		drm_dumb_buffers.o drm_mode_config.o
+		drm_dumb_buffers.o drm_mode_config.o \
+		drm_lease.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 6b143514a566..1db4f63860d1 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -31,6 +31,7 @@
 #include <drm/drmP.h>
 #include "drm_internal.h"
 #include "drm_legacy.h"
+#include <drm/drm_lease.h>
 
 /**
  * DOC: master and authentication
@@ -93,7 +94,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
 	return file ? 0 : -EINVAL;
 }
 
-static struct drm_master *drm_master_create(struct drm_device *dev)
+struct drm_master *drm_master_create(struct drm_device *dev)
 {
 	struct drm_master *master;
 
@@ -107,6 +108,14 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
 	idr_init(&master->magic_map);
 	master->dev = dev;
 
+	/* initialize the tree of output resource lessees */
+	master->lessor = NULL;
+	master->lessee_id = 0;
+	INIT_LIST_HEAD(&master->lessees);
+	INIT_LIST_HEAD(&master->lessee_list);
+	idr_init(&master->leases);
+	idr_init(&master->lessee_idr);
+
 	return master;
 }
 
@@ -189,6 +198,12 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
+	if (file_priv->master->lessor != NULL) {
+		DRM_DEBUG_LEASE("Attempt to set lessee %d as master\n", file_priv->master->lessee_id);
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	ret = drm_set_master(dev, file_priv, false);
 out_unlock:
 	mutex_unlock(&dev->master_mutex);
@@ -310,12 +325,17 @@ static void drm_master_destroy(struct kref *kref)
 	struct drm_master *master = container_of(kref, struct drm_master, refcount);
 	struct drm_device *dev = master->dev;
 
+	drm_lease_destroy(master);
+
 	if (dev->driver->master_destroy)
 		dev->driver->master_destroy(dev, master);
 
 	drm_legacy_master_rmmaps(dev, master);
 
 	idr_destroy(&master->magic_map);
+
+	idr_destroy(&master->lessee_idr);
+
 	kfree(master->unique);
 	kfree(master);
 }
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
new file mode 100644
index 000000000000..782005c7706d
--- /dev/null
+++ b/drivers/gpu/drm/drm_lease.c
@@ -0,0 +1,485 @@
+/*
+ * Copyright © 2017 Keith Packard <keithp@keithp.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <drm/drmP.h>
+#include "drm_internal.h"
+#include "drm_legacy.h"
+#include "drm_crtc_internal.h"
+#include <drm/drm_lease.h>
+#include <drm/drm_auth.h>
+
+#define drm_for_each_lessee(lessee, lessor) \
+	list_for_each_entry((lessee), &(lessor)->lessees, lessee_list)
+
+/*
+ * drm_lease_owner - return ancestor owner drm_master
+ * @master: drm_master somewhere within tree of lessees and lessors
+ *
+ * RETURN:
+ *
+ * drm_master at the top of the tree (i.e, with lessor NULL
+ */
+struct drm_master *drm_lease_owner(struct drm_master *master) {
+	while (master->lessor != NULL)
+		master = master->lessor;
+	return master;
+}
+EXPORT_SYMBOL(drm_lease_owner);
+
+/**
+ * _drm_lease_find: Look up an object and check who holds the lease
+ * @master: the master to look in
+ * @id: the id to check
+ *
+ * Check if 'master' holds a lease (or owns) 'id' and has not
+ * sub-leased it to another master. Return value:
+ *
+ *	object		'master' holds lease and has not sub-leased
+ *	-EACCESS	Some other master holds the lease
+ *	-ENOENT		'id' is not a valid DRM object for this device
+ *	-EBUSY		'master' holds lease, but has sub-leased
+ *
+ * Must be called with mode_config.idr_mutex held
+ */
+static struct drm_mode_object *_drm_lease_find(struct drm_master *master, int id)
+{
+	struct drm_master *lessee;
+	struct drm_mode_object *object;
+
+	DRM_DEBUG_LEASE("master %d object %d\n", master->lessee_id, id);
+	BUG_ON(__mutex_owner(&master->dev->mode_config.idr_mutex) != current);
+
+	object = idr_find(&master->dev->mode_config.crtc_idr, id);
+	if (IS_ERR_OR_NULL(object)) {
+		DRM_DEBUG_LEASE("no such object %d\n", id);
+		return ERR_PTR(-ENOENT);
+	}
+
+	if (master->lessor) {
+		/*
+		 * If the lessor is not the owner, then make sure they
+		 * are the current lease holder for the object
+		 */
+		if (idr_find(&master->leases, id) == NULL) {
+			DRM_DEBUG_LEASE("%d is not lessee for %d\n", master->lessee_id, id);
+			return ERR_PTR(-EACCES);
+		}
+	}
+
+	/* Now make sure no other lessee already holds a lease which masks this object */
+	drm_for_each_lessee(lessee, master) {
+		if (lessee->mask_lease && idr_find(&lessee->leases, id) != NULL) {
+			DRM_DEBUG_LEASE("id %d is held by %d\n", id, lessee->lessee_id);
+			return ERR_PTR(-EBUSY);
+		}
+	}
+
+	DRM_DEBUG_LEASE("id %d is held by us\n", id);
+	return object;
+}
+
+struct drm_mode_object *drm_lease_find(struct drm_master *master, int id)
+{
+	struct drm_mode_object *object;
+
+	BUG_ON(__mutex_owner(&master->dev->mode_config.idr_mutex) == current);
+	mutex_lock(&master->dev->mode_config.idr_mutex);
+	object = _drm_lease_find(master, id);
+	mutex_unlock(&master->dev->mode_config.idr_mutex);
+	if (IS_ERR(object))
+		DRM_DEBUG_LEASE("find %d from master %d failed %ld\n", id, master->lessee_id, PTR_ERR(object));
+	else
+		DRM_DEBUG_LEASE("find %d from master %d %p\n", id, master->lessee_id, object);
+	return object;
+}
+EXPORT_SYMBOL(drm_lease_find);
+
+static inline int _drm_lease_check(struct drm_master *master, int id) {
+	struct drm_mode_object *object = _drm_lease_find(master, id);
+	if (IS_ERR(object))
+		return PTR_ERR(object);
+	return 0;
+}
+
+/**
+ * drm_lease_hoder - find current lease holder for a resource
+ * @master: Some drm_master for the device
+ * @id: the resource id
+ *
+ * Walks down the lease holder tree looking for the drm_master who holds
+ * a lease and is not sub-leasing to another drm_master.
+ *
+ * Returns the owner if no lessee holds the resource
+ */
+static struct drm_master *_drm_lease_holder(struct drm_master *master, int id)
+{
+	struct drm_master *lessor = drm_lease_owner(master);
+
+	for (;;) {
+		struct drm_master *lessee;
+		struct drm_master *found;
+
+		found = NULL;
+		drm_for_each_lessee(lessee, lessor) {
+			if (idr_find(&lessee->leases, id) != NULL) {
+				found = lessee;
+				break;
+			}
+		}
+		if (found == NULL)
+			return lessor;
+		lessor = found;
+	}
+}
+
+/**
+ * drm_lessee_is_descendant - check hierarchy structure
+ * @lessee: lessee
+ * @lessor: lessor
+ *
+ * Checks whether 'lessor' is in control of 'lessee' resources, which
+ * happens when lessee is lessor or lessee is a lessee of lessor or
+ * one of lessors lessees (recursively)
+ */
+static bool
+drm_lessee_is_descendant(struct drm_master *lessee, struct drm_master *lessor)
+{
+	/* Walk up the chain to the owner */
+	while (lessee) {
+
+		/* return true if we found lessor on the way up */
+		if (lessee == lessor)
+			return true;
+		lessee = lessee->lessor;
+	}
+
+	/* return false if lessor is not 'above' lessee */
+	return false;
+}
+
+/**
+ * drm_lessee_get - Get a reference on a lessee_id in a lease sub-tree
+ * @top: the top of the sub-tree to search in
+ * @id: the lessee_id to look for
+ */
+static struct drm_master *
+drm_lessee_get(struct drm_master *top, int lessee_id)
+{
+	struct drm_master *owner;
+	struct drm_master *lessee;
+
+	BUG_ON(__mutex_owner(&top->dev->master_mutex) != current);
+
+	DRM_DEBUG_LEASE("%d\n", lessee_id);
+
+	owner = drm_lease_owner(top);
+	if (lessee_id == 0)
+		lessee = owner;
+	else
+		lessee = idr_find(&owner->lessee_idr, lessee_id);
+
+	DRM_DEBUG_LEASE("%d -> %p (top %p)\n", lessee_id, lessee, top);
+	if (!lessee || !drm_lessee_is_descendant(lessee, top)) {
+		DRM_DEBUG_LEASE("null lessee or not descendant\n");
+		return NULL;
+	}
+
+	return drm_master_get(lessee);
+}
+
+enum drm_lease_walk {
+	drm_lease_walk_stop,
+	drm_lease_walk_dont_descend,
+	drm_lease_walk_descend
+};
+
+/**
+ * drm_lease_walk - walk a subtree of leases
+ * @master: the top of the sub-tree to walk
+ * @func: the function to call on each lease
+ * @closure: data to pass to the function
+ */
+static struct drm_master *
+drm_lease_walk(struct drm_master *top,
+	       enum drm_lease_walk (*func)(struct drm_master *lessee, void *closure),
+	       void *closure)
+{
+	struct drm_master *master = top;
+	enum drm_lease_walk result;
+
+	for (;;) {
+		result = (*func)(master, closure);
+		if (result == drm_lease_walk_stop)
+			return master;
+		if (result == drm_lease_walk_descend && !list_empty(&master->lessees)) {
+			master = list_first_entry(&master->lessees, struct drm_master, lessee_list);
+		} else {
+			while (master != top && list_is_last(&master->lessee_list,
+							     &master->lessor->lessees))
+				master = master->lessor;
+			if (master == top)
+				return NULL;
+			master = list_next_entry(master, lessee_list);
+		}
+	}
+}
+
+/**
+ * drm_lease_filter_crtcs - remove unleased crtcs from set
+ * @master: drm_master of requestor
+ * @crtcs: bitmask of crtcs to check
+ */
+uint32_t drm_lease_filter_crtcs(struct drm_master *master, uint32_t crtcs)
+{
+	struct drm_device *dev = master->dev;
+	struct drm_crtc *crtc;
+	int count;
+
+	BUG_ON(__mutex_owner(&master->dev->mode_config.idr_mutex) == current);
+	mutex_lock(&master->dev->mode_config.idr_mutex);
+	count = -1;
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		uint32_t	mask = 1ul << count;
+		count++;
+
+		if ((crtcs & mask) != 0)
+			if (IS_ERR(_drm_lease_find(master, crtc->base.id)))
+				crtcs &= ~mask;
+	}
+	mutex_unlock(&master->dev->mode_config.idr_mutex);
+	return crtcs;
+}
+EXPORT_SYMBOL(drm_lease_filter_crtcs);
+
+/**
+ * drm_lease_filter_encoders - remove unleased encoders from set
+ * @master: drm_master of requestor
+ * @encoders: bitmask of encoders to check
+ */
+uint32_t drm_lease_filter_encoders(struct drm_master *master, uint32_t encoders)
+{
+	struct drm_device *dev = master->dev;
+	struct drm_encoder *encoder;
+	int count;
+
+	BUG_ON(__mutex_owner(&master->dev->mode_config.idr_mutex) == current);
+	mutex_lock(&master->dev->mode_config.idr_mutex);
+	count = -1;
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		uint32_t	mask = 1ul << count;
+		count++;
+
+		if ((encoders & mask) != 0)
+			if (IS_ERR(_drm_lease_find(master, encoder->base.id)))
+				encoders &= ~mask;
+	}
+	mutex_unlock(&master->dev->mode_config.idr_mutex);
+	return encoders;
+}
+EXPORT_SYMBOL(drm_lease_filter_encoders);
+
+/*
+ * drm_lease_create - create a new drm_master with leased objects
+ * @lessor: lease holder (or owner) of objects
+ * @leases: objects to lease to the new drm_master
+ *
+ * Uses drm_master_create to allocate a new drm_master, then checks to
+ * make sure all of the desired objects can be leased, atomically
+ * leasing them to the new drmmaster.
+ *
+ * 	ERR_PTR(-EACCESS)	some other master holds the title to any object
+ * 	ERR_PTR(-ENOENT)	some object is not a valid DRM object for this device
+ * 	ERR_PTR(-EBUSY)		some other lessee holds title to this object
+ *	ERR_PTR(-EEXIST)	same object specified more than once in the provided list
+ *	ERR_PTR(-ENOMEM)	allocation failed
+ */
+static struct drm_master *drm_lease_create(struct drm_master *lessor, bool mask_lease, struct idr *leases)
+{
+	int error;
+	struct drm_master *lessee;
+	int object;
+	int id;
+	void *entry;
+
+	DRM_DEBUG_LEASE("lessor %d\n", lessor->lessee_id);
+
+	BUG_ON(__mutex_owner(&lessor->dev->master_mutex) != current);
+
+	lessee = drm_master_create(lessor->dev);
+	if (!lessee) {
+		DRM_DEBUG_LEASE("drm_master_create failed\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/* Insert the new lessee into the tree */
+
+	id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL);
+	if (id < 0) {
+		DRM_DEBUG_LEASE("lessee_id allocation failed %d\n", id);
+		drm_master_put(&lessee);
+		return ERR_PTR(id);
+	}
+
+	lessee->lessee_id = id;
+	lessee->lessor = drm_master_get(lessor);
+	list_add_tail(&lessee->lessee_list, &lessor->lessees);
+
+	/* Lock the mode object mutex to make the check and allocation atomic */
+
+	mutex_lock(&lessor->dev->mode_config.idr_mutex);
+	idr_for_each_entry(leases, entry, object) {
+		error = _drm_lease_check(lessor, object);
+		if (error) {
+			mutex_unlock(&lessor->dev->mode_config.idr_mutex);
+			drm_master_put(&lessee);
+			DRM_DEBUG_LEASE("_drm_lease_check %d failed %d\n", object, error);
+			return ERR_PTR(error);
+		}
+	}
+
+	/* Move the leases over */
+	lessee->leases = *leases;
+	lessee->mask_lease = mask_lease;
+	idr_init(leases);
+	mutex_unlock(&lessor->dev->mode_config.idr_mutex);
+
+	DRM_DEBUG_LEASE("new lessee %d %p, lessor %d %p\n", lessee->lessee_id, lessee, lessor->lessee_id, lessor);
+
+	return lessee;
+}
+
+static enum drm_lease_walk drm_lease_validate(struct drm_master *lessee, void *v_lessor)
+{
+	struct drm_master *lessor = v_lessor;
+	struct drm_mode_object *entry;
+	int object;
+	int to_remove;
+	bool any_removed = false;
+
+	if (lessee == lessor)
+		return drm_lease_walk_descend;
+
+	for (;;) {
+		to_remove = 0;
+
+		/* it's not safe to call idr_remove inside the idr_for_each_entry loop,
+		 * so find the first invalid object, remove that, and then try again
+		 */
+		idr_for_each_entry(&lessee->leases, entry, object) {
+			if (idr_find(&lessor->leases, object) == NULL) {
+				to_remove = object;
+				break;
+			}
+		}
+		if (to_remove == 0)
+			break;
+		idr_remove(&lessee->leases, to_remove);
+		any_removed = true;
+	}
+	if (any_removed)
+		return drm_lease_walk_descend;
+	else
+		return drm_lease_walk_dont_descend;
+}
+
+/**
+ * drm_lease_change - change the resources held in a specific lease
+ * @lessor: lessor of the resources
+ * @lessee_id: unique id of the lessee
+ * @leases: new set of resources to be held in the lease
+ */
+int
+drm_lease_change(struct drm_master *lessor, int lessee_id, bool mask_lease, struct idr *leases)
+{
+	struct drm_master *lessee;
+	int ret;
+	struct drm_mode_object *entry;
+	int object;
+
+	/* Find the lessee and hold a reference while we mess around */
+	lessee = drm_lessee_get(lessor, lessee_id);
+	if (!lessee)
+		return -ENOENT;
+	if (lessee == lessor)
+		return -EINVAL;
+
+	/*
+	 * Check to make sure the updated lease terms are valid, which
+	 * means that each resource is leased by the lessor and either
+	 * not sub-leased, or leased to the lessee
+	 */
+
+	mutex_lock(&lessor->dev->mode_config.idr_mutex);
+	idr_for_each_entry(leases, entry, object) {
+		entry = _drm_lease_find(lessor, object);
+		if (IS_ERR(entry)) {
+			ret = PTR_ERR(entry);
+			if (ret != -EBUSY ||
+			    _drm_lease_holder(lessor, object) != lessee)
+			{
+				mutex_unlock(&lessor->dev->mode_config.idr_mutex);
+				drm_master_put(&lessee);
+				return ret;
+			}
+		}
+	}
+
+	/* Free the existing leases and move the new ones in */
+	idr_destroy(&lessee->leases);
+	lessee->leases = *leases;
+	lessee->mask_lease = mask_lease;
+	idr_init(leases);
+
+	drm_lease_walk(lessee, drm_lease_validate, lessee);
+
+	mutex_unlock(&lessor->dev->mode_config.idr_mutex);
+
+	drm_master_put(&lessee);
+	return 0;
+}
+
+/**
+ * drm_lease_destroy - a master is going away
+ * @master: the drm_master being destroyed
+ *
+ * Revoke all leases held by this master from it and all lessees
+ * Orphan all lessees. They'll go away when closed.
+ */
+void drm_lease_destroy(struct drm_master *master)
+{
+	struct drm_device *dev = master->dev;
+
+	BUG_ON(__mutex_owner(&dev->master_mutex) != current);
+
+	DRM_DEBUG_LEASE("drm_lease_destroy %d\n", master->lessee_id);
+
+	/* This master is referenced by all lessees, hence it cannot be destroyed
+	 * until all of them have been
+	 */
+
+	BUG_ON(!list_empty(&master->lessees));
+
+	/* Remove this master from the lessee idr in the owner */
+	if (master->lessee_id != 0) {
+		DRM_DEBUG_LEASE("remove master %d from device list of lessees\n", master->lessee_id);
+		idr_remove(&(drm_lease_owner(master)->lessee_idr), master->lessee_id);
+	}
+
+	/* Remove this master from any lessee list it may be on */
+	list_del(&master->lessee_list);
+	if (master->lessor)
+		drm_master_put(&master->lessor);
+
+	DRM_DEBUG_LEASE("drm_lease_destroy done %d\n", master->lessee_id);
+}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 304a22c87999..b468bc9cf318 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -340,6 +340,7 @@ typedef int drm_ioctl_compat_t(struct file *filp, unsigned int cmd,
 #define DRM_CONTROL_ALLOW 0x8
 #define DRM_UNLOCKED	0x10
 #define DRM_RENDER_ALLOW 0x20
+#define DRM_ANY_MASTER	0x40
 
 struct drm_ioctl_desc {
 	unsigned int cmd;
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index 610223b0481b..e0e2af09d3af 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -50,10 +50,38 @@ struct drm_master {
 	struct idr magic_map;
 	struct drm_lock_data lock;
 	void *driver_priv;
+
+	/* Tree of display resource leases, each of which is a drm_master struct
+	 * All of these get activated simultaneously, so drm_device master points
+	 * at the top of the tree (for which lessor is NULL)
+	 */
+
+	/** Lease holder */
+	struct drm_master *lessor;
+
+	/** id for lessees. Owners always have id 0 */
+	int	lessee_id;
+
+	/** other lessees of the same master */
+	struct list_head lessee_list;
+
+	/** drm_masters leasing from this one */
+	struct list_head lessees;
+
+	/** Objects leased to this drm_master. */
+	struct idr leases;
+
+	/** All lessees under this owner (only used where lessor == NULL) */
+	struct idr lessee_idr;
+
+	/** Indicates that the leased objects should be hidden from the lessor */
+	bool mask_lease;
 };
 
 struct drm_master *drm_master_get(struct drm_master *master);
 void drm_master_put(struct drm_master **master);
 bool drm_is_current_master(struct drm_file *fpriv);
 
+struct drm_master *drm_master_create(struct drm_device *dev);
+
 #endif
diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
new file mode 100644
index 000000000000..e02adf3e42fd
--- /dev/null
+++ b/include/drm/drm_lease.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright © 2017 Keith Packard <keithp@keithp.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef _DRM_LEASE_H_
+#define _DRM_LEASE_H_
+
+struct drm_file;
+struct drm_device;
+
+struct drm_master *drm_lease_owner(struct drm_master *master);
+
+struct drm_master *drm_lessee_find(struct drm_master *top, int lessee_id);
+
+void drm_lease_destroy(struct drm_master *lessee);
+
+struct drm_mode_object *drm_lease_find(struct drm_master *master, int id);
+
+/**
+ * drm_lease_check - check drm_mode_object lease status
+ * @master: the drm_master
+ * @id: the object id
+ *
+ * Checks if the specified master holds a lease on the object
+ * and also whether it has been leased to some lessee of the
+ * specified master. Return value:
+ *
+ *	0		'master' holds a lease (or owns) and has not leased
+ *	-EACCESS	Some other master holds the lease
+ *	-ENOENT		'id' is not a valid DRM object for this device
+ *	-EBUSY		'master' holds lease, but has sub-leased
+ */
+
+static inline int drm_lease_check(struct drm_master *master, int id) {
+	struct drm_mode_object *object = drm_lease_find(master, id);
+	if (IS_ERR(object))
+		return PTR_ERR(object);
+	return 0;
+}
+
+#endif /* _DRM_LEASE_H_ */
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1614736 — Re: [PATCH 2/4] drm: Add drm_object lease infrastructure

On Sat, Apr 01, 2017 at 10:08:39AM -0700, Keith Packard wrote:
> This provides new data structures to hold "lease" information about
> drm mode setting objects, and provides for creating new drm_masters
> which have access to a subset of the available drm resources.
> 
> An 'owner' is a drm_master which is not leasing the objects from
> another drm_master, and hence 'owns' them. This sits at the top of a
> tree of drm_masters.
> 
> A 'lessee' is a drm_master which is leasing objects from some other
> drm_master. Each lessee holds the set of objects which it is leasing
> from the lessor.
> 
> A 'lessor' is a drm_master which is leasing objects to another
> drm_master.
> 
> The set of objects any drm_master 'controls' is limited to the set of
> objects it leases (for lessees) or all objects (for owners),
> optionally minus the set of objects it has leased to other
> drm_masters.

Still not sure we want to restrict objects on the lessor side. Feels like
unecessary complexity (i.e. more bugs in kernel, that's never good), and
at best only needed for lessors who can't keep track of stuff. I'm also
not sure whether we really want sub-leases in v1, that's easy to add later
on, but for now just complicates stuff. Main compositor should be a full
master, VR can be the first lease level, we don't need more I think for
now?

> Objects not controlled by a drm_master cannot be modified through the
> various state manipulating ioctls, and any state reported back to user
> space will be edited to make them appear idle and/or unusable. For
> instance, connectors always report 'disconnected', while encoders
> report no possible crtcs or clones.
> 
> The full list of lessees leasing objects from an owner (either
> directly, or indirectly through another lessee), can be searched from
> an idr in the drm_master of the owner.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

[snip]

> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index 610223b0481b..e0e2af09d3af 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -50,10 +50,38 @@ struct drm_master {
>  	struct idr magic_map;
>  	struct drm_lock_data lock;
>  	void *driver_priv;
> +
> +	/* Tree of display resource leases, each of which is a drm_master struct
> +	 * All of these get activated simultaneously, so drm_device master points

&drm_device.master to do a reference in kernel-doc. Please feed this to
kernel-doc in general and make sure the links all point at the right
stuff, and it's all parsed.

Cheers, Daniel


> +	 * at the top of the tree (for which lessor is NULL)
> +	 */
> +
> +	/** Lease holder */

	/** @lessor: Lease holder. */

> +	struct drm_master *lessor;
> +
> +	/** id for lessees. Owners always have id 0 */
> +	int	lessee_id;
> +
> +	/** other lessees of the same master */
> +	struct list_head lessee_list;
> +
> +	/** drm_masters leasing from this one */
> +	struct list_head lessees;
> +
> +	/** Objects leased to this drm_master. */
> +	struct idr leases;
> +
> +	/** All lessees under this owner (only used where lessor == NULL) */
> +	struct idr lessee_idr;
> +
> +	/** Indicates that the leased objects should be hidden from the lessor */
> +	bool mask_lease;
>  };
>  
>  struct drm_master *drm_master_get(struct drm_master *master);
>  void drm_master_put(struct drm_master **master);
>  bool drm_is_current_master(struct drm_file *fpriv);
>  
> +struct drm_master *drm_master_create(struct drm_device *dev);
> +
>  #endif
> diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
> new file mode 100644
> index 000000000000..e02adf3e42fd
> --- /dev/null
> +++ b/include/drm/drm_lease.h
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright © 2017 Keith Packard <keithp@keithp.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef _DRM_LEASE_H_
> +#define _DRM_LEASE_H_
> +
> +struct drm_file;
> +struct drm_device;
> +
> +struct drm_master *drm_lease_owner(struct drm_master *master);
> +
> +struct drm_master *drm_lessee_find(struct drm_master *top, int lessee_id);
> +
> +void drm_lease_destroy(struct drm_master *lessee);
> +
> +struct drm_mode_object *drm_lease_find(struct drm_master *master, int id);
> +
> +/**
> + * drm_lease_check - check drm_mode_object lease status
> + * @master: the drm_master
> + * @id: the object id
> + *
> + * Checks if the specified master holds a lease on the object
> + * and also whether it has been leased to some lessee of the
> + * specified master. Return value:
> + *
> + *	0		'master' holds a lease (or owns) and has not leased
> + *	-EACCESS	Some other master holds the lease
> + *	-ENOENT		'id' is not a valid DRM object for this device
> + *	-EBUSY		'master' holds lease, but has sub-leased
> + */
> +
> +static inline int drm_lease_check(struct drm_master *master, int id) {
> +	struct drm_mode_object *object = drm_lease_find(master, id);
> +	if (IS_ERR(object))
> +		return PTR_ERR(object);
> +	return 0;
> +}
> +
> +#endif /* _DRM_LEASE_H_ */
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[toc] | [prev] | [next] | [standalone]

#1614767 — Re: [PATCH 2/4] drm: Add drm_object lease infrastructure

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> writes:

> Still not sure we want to restrict objects on the lessor side. Feels like
> unecessary complexity (i.e. more bugs in kernel, that's never good), and
> at best only needed for lessors who can't keep track of stuff.

It's been useful when hacking existing code, and will help catch
application bugs. Limiting access to what you actually need always seems
like good practice to me.

> I'm also not sure whether we really want sub-leases in v1, that's easy
> to add later on, but for now just complicates stuff. Main compositor
> should be a full master, VR can be the first lease level, we don't
> need more I think for now?

We've discussed how leases might be used to implement multi-user
support, so offering sub-leases means that environment could also
support leasing resources out from the users session.

We also just don't know how useful it might be until we explore the
space a bit more. Given that it takes years to get new features into
distributions, I tend to error on the side of generality.

I think a key requirement for acceptance would be a set of robust tests,
something I haven't started writing yet.

>> +	/* Tree of display resource leases, each of which is a drm_master struct
>> +	 * All of these get activated simultaneously, so drm_device master points
>
> &drm_device.master to do a reference in kernel-doc. Please feed this to
> kernel-doc in general and make sure the links all point at the right
> stuff, and it's all parsed.

Thanks, will do.

-- 
-keith

[toc] | [prev] | [next] | [standalone]

#1619635 — Re: [PATCH 2/4] drm: Add drm_object lease infrastructure

[Multipart message — attachments visible in raw view] — view raw

On 03/04/17 01:31 AM, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
>> I'm also not sure whether we really want sub-leases in v1, that's easy
>> to add later on, but for now just complicates stuff. Main compositor
>> should be a full master, VR can be the first lease level, we don't
>> need more I think for now?
> 
> We've discussed how leases might be used to implement multi-user
> support, so offering sub-leases means that environment could also
> support leasing resources out from the users session.
> 
> We also just don't know how useful it might be until we explore the
> space a bit more.

It should only be added upstream once it's clear that it's useful.


> Given that it takes years to get new features into distributions, I
> tend to error on the side of generality.

Why would it take years? Distros seem to generally use the latest
upstream kernel release available at release/freeze.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

[toc] | [prev] | [next] | [standalone]

#1614781 — Re: [PATCH 2/4] drm: Add drm_object lease infrastructure

On Sat, Apr 01, 2017 at 10:08:39AM -0700, Keith Packard wrote:
> +	BUG_ON(__mutex_owner(&master->dev->mode_config.idr_mutex) != current);

Forgot to reply on this:

lockdep_assert_held + enable lockdep.

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

[toc] | [prev] | [next] | [standalone]

#1614795 — Re: [PATCH 2/4] drm: Add drm_object lease infrastructure

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> writes:

> On Sat, Apr 01, 2017 at 10:08:39AM -0700, Keith Packard wrote:
>> +	BUG_ON(__mutex_owner(&master->dev->mode_config.idr_mutex) != current);
>
> Forgot to reply on this:
>
> lockdep_assert_held + enable lockdep.

Thanks. Will fix.

-- 
-keith

[toc] | [prev] | [next] | [standalone]

#1614564 — [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths

Attempts to modify un-leased objects are rejected with an error.
Information returned about unleased objects is modified to make them
appear unusable and/or disconnected.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/drm_atomic.c      |  3 +++
 drivers/gpu/drm/drm_auth.c        |  2 +-
 drivers/gpu/drm/drm_color_mgmt.c  |  3 +++
 drivers/gpu/drm/drm_connector.c   | 52 ++++++++++++++++++++++++++-------------
 drivers/gpu/drm/drm_crtc.c        | 15 ++++++++---
 drivers/gpu/drm/drm_encoder.c     | 18 +++++++++++---
 drivers/gpu/drm/drm_mode_object.c |  3 +++
 drivers/gpu/drm/drm_plane.c       | 36 +++++++++++++++++++++++----
 include/drm/drm_lease.h           |  4 +++
 include/drm/drm_mode_object.h     |  1 +
 10 files changed, 108 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index fdfb1ec17e66..a3cb95f7f1c1 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2134,6 +2134,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			goto out;
 		}
 
+		if ((ret = drm_lease_check(file_priv->master, obj->id)) < 0)
+			goto out;
+
 		if (!obj->properties) {
 			drm_mode_object_unreference(obj);
 			ret = -ENOENT;
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 1db4f63860d1..44c99d12f4c1 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -303,7 +303,7 @@ void drm_master_release(struct drm_file *file_priv)
  */
 bool drm_is_current_master(struct drm_file *fpriv)
 {
-	return fpriv->is_master && fpriv->master == fpriv->minor->dev->master;
+	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
 }
 EXPORT_SYMBOL(drm_is_current_master);
 
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 6543ebde501a..f8d7a499cf95 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -206,6 +206,9 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
+	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0)
+		goto out;
+
 	if (crtc->funcs->gamma_set == NULL) {
 		ret = -ENOSYS;
 		goto out;
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 7a7019ac9388..a95db57dd966 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1079,6 +1079,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	struct drm_mode_modeinfo u_mode;
 	struct drm_mode_modeinfo __user *mode_ptr;
 	uint32_t __user *encoder_ptr;
+	bool leased;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -1093,9 +1094,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
-		if (connector->encoder_ids[i] != 0)
-			encoders_count++;
+	leased = drm_lease_check(file_priv->master, connector->base.id) == 0;
+
+	DRM_DEBUG_LEASE("connector %d leased %s\n", connector->base.id, leased ? "true" : "false");
+
+	if (leased) {
+		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
+			if (connector->encoder_ids[i] != 0 &&
+			    drm_lease_check(file_priv->master, connector->encoder_ids[i]) == 0)
+				encoders_count++;
+	}
 
 	if (out_resp->count_modes == 0) {
 		connector->funcs->fill_modes(connector,
@@ -1104,21 +1112,29 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	}
 
 	/* delayed so we get modes regardless of pre-fill_modes state */
-	list_for_each_entry(mode, &connector->modes, head)
-		if (drm_mode_expose_to_userspace(mode, file_priv))
-			mode_count++;
+	if (leased)
+		list_for_each_entry(mode, &connector->modes, head)
+			if (drm_mode_expose_to_userspace(mode, file_priv))
+				mode_count++;
 
 	out_resp->connector_id = connector->base.id;
 	out_resp->connector_type = connector->connector_type;
 	out_resp->connector_type_id = connector->connector_type_id;
-	out_resp->mm_width = connector->display_info.width_mm;
-	out_resp->mm_height = connector->display_info.height_mm;
-	out_resp->subpixel = connector->display_info.subpixel_order;
-	out_resp->connection = connector->status;
+	if (leased) {
+		out_resp->mm_width = connector->display_info.width_mm;
+		out_resp->mm_height = connector->display_info.height_mm;
+		out_resp->subpixel = connector->display_info.subpixel_order;
+		out_resp->connection = connector->status;
+	} else {
+		out_resp->mm_width = 0;
+		out_resp->mm_height = 0;
+		out_resp->subpixel = 0;
+		out_resp->connection = connector_status_disconnected;
+	}
 
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	encoder = drm_connector_get_encoder(connector);
-	if (encoder)
+	if (encoder && leased)
 		out_resp->encoder_id = encoder->base.id;
 	else
 		out_resp->encoder_id = 0;
@@ -1145,12 +1161,14 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	}
 	out_resp->count_modes = mode_count;
 
-	ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
-			(uint32_t __user *)(unsigned long)(out_resp->props_ptr),
-			(uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
-			&out_resp->count_props);
-	if (ret)
-		goto out;
+	if (leased) {
+		ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
+						     (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
+						     (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
+						     &out_resp->count_props);
+		if (ret)
+			goto out;
+	}
 
 	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
 		copied = 0;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e75f62cd8a65..95026ca74568 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -347,6 +347,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
 {
 	struct drm_mode_crtc *crtc_resp = data;
 	struct drm_crtc *crtc;
+	bool leased;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -355,9 +356,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
+	leased = drm_lease_check(file_priv->master, crtc->base.id) == 0;
+
+	DRM_DEBUG_LEASE("crtc %d leased %s\n", crtc->base.id, leased ? "true" : "false");
+
 	drm_modeset_lock_crtc(crtc, crtc->primary);
 	crtc_resp->gamma_size = crtc->gamma_size;
-	if (crtc->primary->fb)
+	if (crtc->primary->fb && leased)
 		crtc_resp->fb_id = crtc->primary->fb->base.id;
 	else
 		crtc_resp->fb_id = 0;
@@ -365,7 +370,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	if (crtc->state) {
 		crtc_resp->x = crtc->primary->state->src_x >> 16;
 		crtc_resp->y = crtc->primary->state->src_y >> 16;
-		if (crtc->state->enable) {
+		if (crtc->state->enable && leased) {
 			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
 			crtc_resp->mode_valid = 1;
 
@@ -375,7 +380,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	} else {
 		crtc_resp->x = crtc->x;
 		crtc_resp->y = crtc->y;
-		if (crtc->enabled) {
+		if (crtc->enabled && leased) {
 			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
 			crtc_resp->mode_valid = 1;
 
@@ -529,6 +534,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	}
 	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
 
+	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0) {
+		DRM_DEBUG_KMS("CRTC lease not held\n");
+		goto out;
+	}
 	if (crtc_req->mode_valid) {
 		/* If we have a mode we need a framebuffer. */
 		/* If we pass -1, set the mode with the currently bound fb */
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 992879f15f23..24d03e13f522 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -201,6 +201,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 	struct drm_mode_get_encoder *enc_resp = data;
 	struct drm_encoder *encoder;
 	struct drm_crtc *crtc;
+	bool leased;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -209,9 +210,13 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 	if (!encoder)
 		return -ENOENT;
 
+	leased = drm_lease_check(file_priv->master, encoder->base.id) == 0;
+
+	DRM_DEBUG_LEASE("encoder %d leased %s\n", encoder->base.id, leased ? "true" : "false");
+
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	crtc = drm_encoder_get_crtc(encoder);
-	if (crtc)
+	if (crtc && leased)
 		enc_resp->crtc_id = crtc->base.id;
 	else
 		enc_resp->crtc_id = 0;
@@ -219,8 +224,15 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 
 	enc_resp->encoder_type = encoder->encoder_type;
 	enc_resp->encoder_id = encoder->base.id;
-	enc_resp->possible_crtcs = encoder->possible_crtcs;
-	enc_resp->possible_clones = encoder->possible_clones;
+	if (leased) {
+		enc_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv->master,
+								  encoder->possible_crtcs);
+		enc_resp->possible_clones = drm_lease_filter_encoders(file_priv->master,
+								      encoder->possible_clones);
+	} else {
+		enc_resp->possible_crtcs = 0;
+		enc_resp->possible_clones = 0;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 9f17085b1fdd..9f8559d82a17 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -404,6 +404,9 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
+	if ((ret = drm_lease_check(file_priv->master, arg->obj_id)) != 0)
+		goto out;
+
 	if (!arg_obj->properties)
 		goto out_unref;
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 62b98f386fd1..df811869c1dd 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -383,6 +383,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	struct drm_mode_get_plane *plane_resp = data;
 	struct drm_plane *plane;
 	uint32_t __user *format_ptr;
+	bool leased;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -391,27 +392,34 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	if (!plane)
 		return -ENOENT;
 
+	leased = drm_lease_check(file_priv->master, plane->base.id) == 0;
+
 	drm_modeset_lock(&plane->mutex, NULL);
-	if (plane->crtc)
+	if (plane->crtc && leased)
 		plane_resp->crtc_id = plane->crtc->base.id;
 	else
 		plane_resp->crtc_id = 0;
 
-	if (plane->fb)
+	if (plane->fb && leased)
 		plane_resp->fb_id = plane->fb->base.id;
 	else
 		plane_resp->fb_id = 0;
 	drm_modeset_unlock(&plane->mutex);
 
 	plane_resp->plane_id = plane->base.id;
-	plane_resp->possible_crtcs = plane->possible_crtcs;
+	if (leased)
+		plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv->master,
+								    plane->possible_crtcs);
+	else
+		plane_resp->possible_crtcs = 0;
+
 	plane_resp->gamma_size = 0;
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
 	 * needed, and the 2nd time to fill it.
 	 */
-	if (plane->format_count &&
+	if (plane->format_count && leased &&
 	    (plane_resp->count_format_types >= plane->format_count)) {
 		format_ptr = (uint32_t __user *)(unsigned long)plane_resp->format_type_ptr;
 		if (copy_to_user(format_ptr,
@@ -420,7 +428,10 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 			return -EFAULT;
 		}
 	}
-	plane_resp->count_format_types = plane->format_count;
+	if (leased)
+		plane_resp->count_format_types = plane->format_count;
+	else
+		plane_resp->count_format_types = 0;
 
 	return 0;
 }
@@ -551,6 +562,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	struct drm_plane *plane;
 	struct drm_crtc *crtc = NULL;
 	struct drm_framebuffer *fb = NULL;
+	int ret;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -566,6 +578,12 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		return -ENOENT;
 	}
 
+	if ((ret = drm_lease_check(file_priv->master, plane->base.id)) < 0) {
+		DRM_DEBUG_KMS("Plane lease not held: %d error %d\n",
+			      plane->base.id, ret);
+		return ret;
+	}
+
 	if (plane_req->fb_id) {
 		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
 		if (!fb) {
@@ -687,6 +705,11 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id);
 		return -ENOENT;
 	}
+	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0) {
+		DRM_DEBUG_KMS("CRTC lease not held %d error %d\n",
+			      crtc->base.id, ret);
+		goto out;
+	}
 
 	/*
 	 * If this crtc has a universal cursor plane, call that plane's update
@@ -785,6 +808,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
+	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0)
+		return ret;
+
 	if (crtc->funcs->page_flip_target) {
 		u32 current_vblank;
 		int r;
diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
index e02adf3e42fd..8f91fc4226e3 100644
--- a/include/drm/drm_lease.h
+++ b/include/drm/drm_lease.h
@@ -48,4 +48,8 @@ static inline int drm_lease_check(struct drm_master *master, int id) {
 	return 0;
 }
 
+uint32_t drm_lease_filter_crtcs(struct drm_master *master, uint32_t crtcs);
+
+uint32_t drm_lease_filter_encoders(struct drm_master *master, uint32_t encoders);
+
 #endif /* _DRM_LEASE_H_ */
diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index 43460b21d112..07830182598b 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -24,6 +24,7 @@
 #define __DRM_MODESET_H__
 
 #include <linux/kref.h>
+#include <drm/drm_lease.h>
 struct drm_object_properties;
 struct drm_property;
 struct drm_device;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1614733 — Re: [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths

On Sat, Apr 01, 2017 at 10:08:40AM -0700, Keith Packard wrote:
> Attempts to modify un-leased objects are rejected with an error.
> Information returned about unleased objects is modified to make them
> appear unusable and/or disconnected.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Just a merge-technical idea to make this easier to change and less painful
to rebase until we've locked down the semantics properly. And also because
I expect we'll need v2 of this uapi soon or later anyway:

I think it'd be good if we could consolidate all the lease checking into
drm_mode_object_find (respectively __drm_mode_object_find). We'd need to
wire up the fpriv to be able to do that, but we could upstream that patch
right away before anything else. That should take care of most of the
checks in this patch here.

There's a few things on top:
- filtering the various bitmasks. I think you have most, but we could
  perhaps upstream the helpers for these.
- filtering object lists (essentially getresources and getplanes ioctls).
- filtering implicit objects in the legacy ioctl. E.g. page_flip done
  through atomic doesn't just need the CRTC id, but also the id of the
  primary plane plus of the FB_ID atomic property. Similarly for all the
  other legacy ioctls. I think we want to make sure there's no difference
  here in behaviour.

Especially for the last one it might be simplest to outright disallow all
legacy ioctl and require that sub-drm_master nodes only get access to the
read-only GET* ioctl (they get that anyway, even when they're not the
current master), plus atomic. Makes it a _lot_ easier to implement.
Downside is that amdgpu _really_ needs to land atomic asap :-)

> ---
>  drivers/gpu/drm/drm_atomic.c      |  3 +++
>  drivers/gpu/drm/drm_auth.c        |  2 +-
>  drivers/gpu/drm/drm_color_mgmt.c  |  3 +++
>  drivers/gpu/drm/drm_connector.c   | 52 ++++++++++++++++++++++++++-------------
>  drivers/gpu/drm/drm_crtc.c        | 15 ++++++++---
>  drivers/gpu/drm/drm_encoder.c     | 18 +++++++++++---
>  drivers/gpu/drm/drm_mode_object.c |  3 +++
>  drivers/gpu/drm/drm_plane.c       | 36 +++++++++++++++++++++++----
>  include/drm/drm_lease.h           |  4 +++
>  include/drm/drm_mode_object.h     |  1 +
>  10 files changed, 108 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fdfb1ec17e66..a3cb95f7f1c1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2134,6 +2134,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			goto out;
>  		}
>  
> +		if ((ret = drm_lease_check(file_priv->master, obj->id)) < 0)
> +			goto out;
> +
>  		if (!obj->properties) {
>  			drm_mode_object_unreference(obj);
>  			ret = -ENOENT;
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1db4f63860d1..44c99d12f4c1 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -303,7 +303,7 @@ void drm_master_release(struct drm_file *file_priv)
>   */
>  bool drm_is_current_master(struct drm_file *fpriv)
>  {
> -	return fpriv->is_master && fpriv->master == fpriv->minor->dev->master;
> +	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;

This feels like it should be part of the earlier patch to make
sub-drm_master objects for leasing. In that patch we should also make sure
that all the current docs about drm_master are updated correctly (but
maybe only later on, when we get towards merging this). This change here
is pretty important wrt set/drop_master ioctl behaviour and leases.

Cheers, Daniel

>  }
>  EXPORT_SYMBOL(drm_is_current_master);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 6543ebde501a..f8d7a499cf95 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -206,6 +206,9 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0)
> +		goto out;
> +
>  	if (crtc->funcs->gamma_set == NULL) {
>  		ret = -ENOSYS;
>  		goto out;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7a7019ac9388..a95db57dd966 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1079,6 +1079,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	struct drm_mode_modeinfo u_mode;
>  	struct drm_mode_modeinfo __user *mode_ptr;
>  	uint32_t __user *encoder_ptr;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -1093,9 +1094,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  		goto out_unlock;
>  	}
>  
> -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> -		if (connector->encoder_ids[i] != 0)
> -			encoders_count++;
> +	leased = drm_lease_check(file_priv->master, connector->base.id) == 0;
> +
> +	DRM_DEBUG_LEASE("connector %d leased %s\n", connector->base.id, leased ? "true" : "false");
> +
> +	if (leased) {
> +		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> +			if (connector->encoder_ids[i] != 0 &&
> +			    drm_lease_check(file_priv->master, connector->encoder_ids[i]) == 0)
> +				encoders_count++;
> +	}
>  
>  	if (out_resp->count_modes == 0) {
>  		connector->funcs->fill_modes(connector,
> @@ -1104,21 +1112,29 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	}
>  
>  	/* delayed so we get modes regardless of pre-fill_modes state */
> -	list_for_each_entry(mode, &connector->modes, head)
> -		if (drm_mode_expose_to_userspace(mode, file_priv))
> -			mode_count++;
> +	if (leased)
> +		list_for_each_entry(mode, &connector->modes, head)
> +			if (drm_mode_expose_to_userspace(mode, file_priv))
> +				mode_count++;
>  
>  	out_resp->connector_id = connector->base.id;
>  	out_resp->connector_type = connector->connector_type;
>  	out_resp->connector_type_id = connector->connector_type_id;
> -	out_resp->mm_width = connector->display_info.width_mm;
> -	out_resp->mm_height = connector->display_info.height_mm;
> -	out_resp->subpixel = connector->display_info.subpixel_order;
> -	out_resp->connection = connector->status;
> +	if (leased) {
> +		out_resp->mm_width = connector->display_info.width_mm;
> +		out_resp->mm_height = connector->display_info.height_mm;
> +		out_resp->subpixel = connector->display_info.subpixel_order;
> +		out_resp->connection = connector->status;
> +	} else {
> +		out_resp->mm_width = 0;
> +		out_resp->mm_height = 0;
> +		out_resp->subpixel = 0;
> +		out_resp->connection = connector_status_disconnected;
> +	}
>  
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	encoder = drm_connector_get_encoder(connector);
> -	if (encoder)
> +	if (encoder && leased)
>  		out_resp->encoder_id = encoder->base.id;
>  	else
>  		out_resp->encoder_id = 0;
> @@ -1145,12 +1161,14 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	}
>  	out_resp->count_modes = mode_count;
>  
> -	ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
> -			(uint32_t __user *)(unsigned long)(out_resp->props_ptr),
> -			(uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
> -			&out_resp->count_props);
> -	if (ret)
> -		goto out;
> +	if (leased) {
> +		ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
> +						     (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
> +						     (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
> +						     &out_resp->count_props);
> +		if (ret)
> +			goto out;
> +	}
>  
>  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
>  		copied = 0;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e75f62cd8a65..95026ca74568 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -347,6 +347,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  {
>  	struct drm_mode_crtc *crtc_resp = data;
>  	struct drm_crtc *crtc;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -355,9 +356,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> +	leased = drm_lease_check(file_priv->master, crtc->base.id) == 0;
> +
> +	DRM_DEBUG_LEASE("crtc %d leased %s\n", crtc->base.id, leased ? "true" : "false");
> +
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
>  	crtc_resp->gamma_size = crtc->gamma_size;
> -	if (crtc->primary->fb)
> +	if (crtc->primary->fb && leased)
>  		crtc_resp->fb_id = crtc->primary->fb->base.id;
>  	else
>  		crtc_resp->fb_id = 0;
> @@ -365,7 +370,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	if (crtc->state) {
>  		crtc_resp->x = crtc->primary->state->src_x >> 16;
>  		crtc_resp->y = crtc->primary->state->src_y >> 16;
> -		if (crtc->state->enable) {
> +		if (crtc->state->enable && leased) {
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>  			crtc_resp->mode_valid = 1;
>  
> @@ -375,7 +380,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	} else {
>  		crtc_resp->x = crtc->x;
>  		crtc_resp->y = crtc->y;
> -		if (crtc->enabled) {
> +		if (crtc->enabled && leased) {
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>  			crtc_resp->mode_valid = 1;
>  
> @@ -529,6 +534,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	}
>  	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
>  
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0) {
> +		DRM_DEBUG_KMS("CRTC lease not held\n");
> +		goto out;
> +	}
>  	if (crtc_req->mode_valid) {
>  		/* If we have a mode we need a framebuffer. */
>  		/* If we pass -1, set the mode with the currently bound fb */
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 992879f15f23..24d03e13f522 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -201,6 +201,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  	struct drm_mode_get_encoder *enc_resp = data;
>  	struct drm_encoder *encoder;
>  	struct drm_crtc *crtc;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -209,9 +210,13 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  	if (!encoder)
>  		return -ENOENT;
>  
> +	leased = drm_lease_check(file_priv->master, encoder->base.id) == 0;
> +
> +	DRM_DEBUG_LEASE("encoder %d leased %s\n", encoder->base.id, leased ? "true" : "false");
> +
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	crtc = drm_encoder_get_crtc(encoder);
> -	if (crtc)
> +	if (crtc && leased)
>  		enc_resp->crtc_id = crtc->base.id;
>  	else
>  		enc_resp->crtc_id = 0;
> @@ -219,8 +224,15 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  
>  	enc_resp->encoder_type = encoder->encoder_type;
>  	enc_resp->encoder_id = encoder->base.id;
> -	enc_resp->possible_crtcs = encoder->possible_crtcs;
> -	enc_resp->possible_clones = encoder->possible_clones;
> +	if (leased) {
> +		enc_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv->master,
> +								  encoder->possible_crtcs);
> +		enc_resp->possible_clones = drm_lease_filter_encoders(file_priv->master,
> +								      encoder->possible_clones);
> +	} else {
> +		enc_resp->possible_crtcs = 0;
> +		enc_resp->possible_clones = 0;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 9f17085b1fdd..9f8559d82a17 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -404,6 +404,9 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> +	if ((ret = drm_lease_check(file_priv->master, arg->obj_id)) != 0)
> +		goto out;
> +
>  	if (!arg_obj->properties)
>  		goto out_unref;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 62b98f386fd1..df811869c1dd 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -383,6 +383,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	struct drm_mode_get_plane *plane_resp = data;
>  	struct drm_plane *plane;
>  	uint32_t __user *format_ptr;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -391,27 +392,34 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	if (!plane)
>  		return -ENOENT;
>  
> +	leased = drm_lease_check(file_priv->master, plane->base.id) == 0;
> +
>  	drm_modeset_lock(&plane->mutex, NULL);
> -	if (plane->crtc)
> +	if (plane->crtc && leased)
>  		plane_resp->crtc_id = plane->crtc->base.id;
>  	else
>  		plane_resp->crtc_id = 0;
>  
> -	if (plane->fb)
> +	if (plane->fb && leased)
>  		plane_resp->fb_id = plane->fb->base.id;
>  	else
>  		plane_resp->fb_id = 0;
>  	drm_modeset_unlock(&plane->mutex);
>  
>  	plane_resp->plane_id = plane->base.id;
> -	plane_resp->possible_crtcs = plane->possible_crtcs;
> +	if (leased)
> +		plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv->master,
> +								    plane->possible_crtcs);
> +	else
> +		plane_resp->possible_crtcs = 0;
> +
>  	plane_resp->gamma_size = 0;
>  
>  	/*
>  	 * This ioctl is called twice, once to determine how much space is
>  	 * needed, and the 2nd time to fill it.
>  	 */
> -	if (plane->format_count &&
> +	if (plane->format_count && leased &&
>  	    (plane_resp->count_format_types >= plane->format_count)) {
>  		format_ptr = (uint32_t __user *)(unsigned long)plane_resp->format_type_ptr;
>  		if (copy_to_user(format_ptr,
> @@ -420,7 +428,10 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  			return -EFAULT;
>  		}
>  	}
> -	plane_resp->count_format_types = plane->format_count;
> +	if (leased)
> +		plane_resp->count_format_types = plane->format_count;
> +	else
> +		plane_resp->count_format_types = 0;
>  
>  	return 0;
>  }
> @@ -551,6 +562,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> +	int ret;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -566,6 +578,12 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> +	if ((ret = drm_lease_check(file_priv->master, plane->base.id)) < 0) {
> +		DRM_DEBUG_KMS("Plane lease not held: %d error %d\n",
> +			      plane->base.id, ret);
> +		return ret;
> +	}
> +
>  	if (plane_req->fb_id) {
>  		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
>  		if (!fb) {
> @@ -687,6 +705,11 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id);
>  		return -ENOENT;
>  	}
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0) {
> +		DRM_DEBUG_KMS("CRTC lease not held %d error %d\n",
> +			      crtc->base.id, ret);
> +		goto out;
> +	}
>  
>  	/*
>  	 * If this crtc has a universal cursor plane, call that plane's update
> @@ -785,6 +808,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0)
> +		return ret;
> +
>  	if (crtc->funcs->page_flip_target) {
>  		u32 current_vblank;
>  		int r;
> diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
> index e02adf3e42fd..8f91fc4226e3 100644
> --- a/include/drm/drm_lease.h
> +++ b/include/drm/drm_lease.h
> @@ -48,4 +48,8 @@ static inline int drm_lease_check(struct drm_master *master, int id) {
>  	return 0;
>  }
>  
> +uint32_t drm_lease_filter_crtcs(struct drm_master *master, uint32_t crtcs);
> +
> +uint32_t drm_lease_filter_encoders(struct drm_master *master, uint32_t encoders);
> +
>  #endif /* _DRM_LEASE_H_ */
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index 43460b21d112..07830182598b 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -24,6 +24,7 @@
>  #define __DRM_MODESET_H__
>  
>  #include <linux/kref.h>
> +#include <drm/drm_lease.h>
>  struct drm_object_properties;
>  struct drm_property;
>  struct drm_device;
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[toc] | [prev] | [next] | [standalone]

#1614766 — Re: [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> writes:

> I think it'd be good if we could consolidate all the lease checking into
> drm_mode_object_find (respectively __drm_mode_object_find). We'd need to
> wire up the fpriv to be able to do that, but we could upstream that patch
> right away before anything else. That should take care of most of the
> checks in this patch here.

That's a good idea.

> There's a few things on top:
> - filtering the various bitmasks. I think you have most, but we could
>   perhaps upstream the helpers for these.

Yeah, would be nice to get hooks in place soon to avoid rebase
adventures later. I guess that would involve shipping a stub drm_lease.h
for now?

> - filtering object lists (essentially getresources and getplanes ioctls).
> - filtering implicit objects in the legacy ioctl. E.g. page_flip done
>   through atomic doesn't just need the CRTC id, but also the id of the
>   primary plane plus of the FB_ID atomic property. Similarly for all the
>   other legacy ioctls. I think we want to make sure there's no difference
>   here in behaviour.

Oh, all of the implicit resource access from the legacy ioctls. Yeah,
that will take a bit of research to identify all of them.

> Especially for the last one it might be simplest to outright disallow all
> legacy ioctl and require that sub-drm_master nodes only get access to the
> read-only GET* ioctl (they get that anyway, even when they're not the
> current master), plus atomic. Makes it a _lot_ easier to implement.
> Downside is that amdgpu _really_ needs to land atomic asap :-)

I'd like to avoid that particular dependency as amdgpu is something of a
requirement for this particular project...

I'll get started fixing the lease checking stuff to try and centralize it.

-- 
-keith

[toc] | [prev] | [next] | [standalone]

#1614954 — Re: [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths

On Sun, Apr 02, 2017 at 09:37:16AM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > I think it'd be good if we could consolidate all the lease checking into
> > drm_mode_object_find (respectively __drm_mode_object_find). We'd need to
> > wire up the fpriv to be able to do that, but we could upstream that patch
> > right away before anything else. That should take care of most of the
> > checks in this patch here.
> 
> That's a good idea.
> 
> > There's a few things on top:
> > - filtering the various bitmasks. I think you have most, but we could
> >   perhaps upstream the helpers for these.
> 
> Yeah, would be nice to get hooks in place soon to avoid rebase
> adventures later. I guess that would involve shipping a stub drm_lease.h
> for now?

I think we should see how far wiring drm_file *fpriv through the lookup
functions gets us, but yeah we probably need a stub drm_lease.h for
filtering. For the interface maybe pass a drm_file *fpriv for all the
filtering functions, and deref into fpriv->master or whatever on the
implementation. That gives us all the freedom to filter however we want
really.

> > - filtering object lists (essentially getresources and getplanes ioctls).
> > - filtering implicit objects in the legacy ioctl. E.g. page_flip done
> >   through atomic doesn't just need the CRTC id, but also the id of the
> >   primary plane plus of the FB_ID atomic property. Similarly for all the
> >   other legacy ioctls. I think we want to make sure there's no difference
> >   here in behaviour.
> 
> Oh, all of the implicit resource access from the legacy ioctls. Yeah,
> that will take a bit of research to identify all of them.
> 
> > Especially for the last one it might be simplest to outright disallow all
> > legacy ioctl and require that sub-drm_master nodes only get access to the
> > read-only GET* ioctl (they get that anyway, even when they're not the
> > current master), plus atomic. Makes it a _lot_ easier to implement.
> > Downside is that amdgpu _really_ needs to land atomic asap :-)
> 
> I'd like to avoid that particular dependency as amdgpu is something of a
> requirement for this particular project...

Figured so :-) Otoh I've heard that atomic for amdgpu is happening for
real now. Although Harry told me I need to wait a few more weeks before
they're ready to present the rewritten stuff and get my feedback ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1619629 — Re: [PATCH 3/4] drm: Check mode object lease status in all master ioctl paths

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> writes:

> I think it'd be good if we could consolidate all the lease checking into
> drm_mode_object_find (respectively __drm_mode_object_find). We'd need to
> wire up the fpriv to be able to do that, but we could upstream that patch
> right away before anything else. That should take care of most of the
> checks in this patch here.

Hrm. Without some major changes, the effect of this will be to have the
lessee see only the objects which it holds a lease on. I think that's OK
as the lessor will be doing object filtering for its clients, however
it's not what Dave and I discussed, so I just want to make him aware
that the kernel is going to be simpler than originally planned. We had
originally discussed having the lessee see the un-leased objects, but to
have them reported with different status.

-- 
-keith

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web