Message-ID: <20171016210315.stpau32hblgxfsgm@art_vandelay>
Date:   Mon, 16 Oct 2017 17:03:15 -0400
From:   Sean Paul <seanpaul@...omium.org>
To:     Keith Packard <keithp@...thp.com>
Cc:     linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 5/5] drm: Add four ioctls for managing drm mode object
 leases [v6]

On Thu, Oct 12, 2017 at 06:56:31PM -0700, Keith Packard wrote:
> drm_mode_create_lease
> 
> 	Creates a lease for a list of drm mode objects, returning an
> 	fd for the new drm_master and a 64-bit identifier for the lessee
> 
> drm_mode_list_lesees
> 
> 	List the identifiers of the lessees for a master file
> 
> drm_mode_get_lease
> 
> 	List the leased objects for a master file
> 
> drm_mode_revoke_lease
> 
> 	Erase the set of objects managed by a lease.
> 
> This should suffice to at least create and query leases.
> 
> Changes for v2 as suggested by Daniel Vetter <daniel.vetter@...ll.ch>:
> 
>  * query ioctls only query the master associated with
>    the provided file.
> 
>  * 'mask_lease' value has been removed
> 
>  * change ioctl has been removed.
> 
> Changes for v3 suggested in part by Dave Airlie <airlied@...il.com>
> 
>  * Add revoke ioctl.
> 
> Changes for v4 suggested by Dave Airlie <airlied@...il.com>
> 
>  * Expand on the comment about the magic use of &drm_lease_idr_object
>  * Pad lease ioctl structures to align on 64-bit boundaries
> 
> Changes for v5 suggested by Dave Airlie <airlied@...il.com>
> 
>  * Check for non-negative object_id in create_lease to avoid debug
>    output from the kernel.
> 
> Changes for v5 provided by Dave Airlie <airlied@...il.com>
> 
>  * For non-universal planes add primary/cursor planes to lease
> 
>    If we aren't exposing universal planes to this userspace client,
>    and it requests a lease on a crtc, we should implicitly export the
>    primary and cursor planes for the crtc.
> 
>    If the lessee doesn't request universal planes, it will just see
>    the crtc, but if it does request them it will then see the plane
>    objects as well.
> 
>    This also moves the object look ups earlier as a side effect, so
>    we'd exit the ioctl quicker for non-existant objects.
> 
>  * Restrict leases to crtc/connector/planes.
> 
>    This only allows leasing for objects we wish to allow.
> 
> Signed-off-by: Keith Packard <keithp@...thp.com>
> Signed-off-by: Dave Airlie <airlied@...hat.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c       |   4 +
>  drivers/gpu/drm/drm_lease.c       | 318 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_mode_object.c |   5 +-
>  include/drm/drm_lease.h           |  12 ++
>  include/drm/drm_mode_object.h     |   2 +
>  include/uapi/drm/drm.h            |   5 +
>  include/uapi/drm/drm_mode.h       |  66 ++++++++
>  7 files changed, 410 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 9c435a4c0c82..4aafe4802099 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -665,6 +665,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  };
>  
>  #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 6c3f2445254c..88c213f9c4ab 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -23,6 +23,8 @@
>  #define drm_for_each_lessee(lessee, lessor) \
>  	list_for_each_entry((lessee), &(lessor)->lessees, lessee_list)
>  
> +static uint64_t drm_lease_idr_object;
> +
>  /**
>   * drm_lease_owner - return ancestor owner drm_master
>   * @master: drm_master somewhere within tree of lessees and lessors
> @@ -337,3 +339,319 @@ void _drm_lease_revoke(struct drm_master *top)
>  		}
>  	}
>  }
> +
> +/**
> + * drm_mode_create_lease_ioctl - create a new lease
> + * @dev: the drm device
> + * @data: pointer to struct drm_mode_create_lease
> + * @file_priv: the file being manipulated
> + *
> + * The master associated with the specified file will have a lease
> + * created containing the objects specified in the ioctl structure.
> + * A file descriptor will be allocated for that and returned to the
> + * application.
> + */
> +int drm_mode_create_lease_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *lessor_priv)
> +{
> +	struct drm_mode_create_lease *cl = data;
> +	size_t object_count;
> +	size_t o;
> +	int ret = 0;
> +	struct idr leases;
> +	struct drm_master *lessor = lessor_priv->master;
> +	struct drm_master *lessee = NULL;
> +	struct file *lessee_file = NULL;
> +	struct file *lessor_file = lessor_priv->filp;
> +	struct drm_file *lessee_priv;
> +	int fd = -1;
> +
> +	/* Do not allow sub-leases */
> +	if (lessor->lessor)
> +		return -EINVAL;
> +
> +	object_count = cl->object_count;
> +	idr_init(&leases);
> +
> +	/* Allocate a file descriptor for the lease */
> +	fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
> +
> +	DRM_DEBUG_LEASE("Creating new lease\n");
> +
> +	/* Lookup the mode objects and add their IDs to the lease request */
> +	for (o = 0; o < object_count; o++) {
> +		__u32 object_id;
> +		struct drm_mode_object *obj;
> +
> +		if (copy_from_user(&object_id,
> +				   u64_to_user_ptr(cl->object_ids) + o * sizeof (__u32),
> +				   sizeof (__u32))) {
> +			ret = -EFAULT;
> +			goto out_leases;
> +		}
> +		DRM_DEBUG_LEASE("Adding object %d to lease\n", object_id);
> +
> +		if ((int) object_id < 0) {
> +			ret = -EINVAL;
> +			goto out_leases;
> +		}
> +
> +		obj = drm_mode_object_find(dev, lessor_priv, object_id,
> +					   DRM_MODE_OBJECT_ANY);
> +		if (!obj) {
> +			ret = -ENOENT;
> +			goto out_leases;
> +		}
> +
> +		/* only allow leasing on crtc/plane/connector objects */
> +		if (!drm_mode_object_lease_required(obj->type)) {
> +			ret = -EINVAL;
> +			drm_mode_object_put(obj);
> +			goto out_leases;
> +		}
> +
> +		/*
> +		 * We're using an IDR to hold the set of leased
> +		 * objects, but we don't need to point at the object's
> +		 * data structure from the lease as the main crtc_idr
> +		 * will be used to actually find that. Instead, all we
> +		 * really want is a 'leased/not-leased' result, for
> +		 * which any non-NULL pointer will work fine.
> +		 */
> +		ret = idr_alloc(&leases, &drm_lease_idr_object , object_id, object_id + 1, GFP_KERNEL);

nit: space before ,

> +		if (ret < 0) {
> +			DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n",
> +					object_id, ret);
> +			drm_mode_object_put(obj);
> +			goto out_leases;
> +		}
> +		if (obj->type == DRM_MODE_OBJECT_CRTC && !lessor_priv->universal_planes) {
> +			struct drm_crtc *crtc = obj_to_crtc(obj);
> +			ret = idr_alloc(&leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL);
> +			if (ret < 0) {
> +				DRM_DEBUG_LEASE("Object primary plane %d cannot be inserted into leases (%d)\n",
> +						object_id, ret);
> +				drm_mode_object_put(obj);
> +				goto out_leases;
> +			}
> +			if (crtc->cursor) {
> +				ret = idr_alloc(&leases, &drm_lease_idr_object, crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL);
> +				if (ret < 0) {
> +					DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n",
> +							object_id, ret);
> +					drm_mode_object_put(obj);
> +					goto out_leases;
> +				}
> +			}
> +		}
> +		drm_mode_object_put(obj);
> +	}
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	DRM_DEBUG_LEASE("Creating lease\n");
> +	lessee = drm_lease_create(lessor, &leases);
> +
> +	if (IS_ERR(lessee)) {
> +		ret = PTR_ERR(lessee);
> +		mutex_unlock(&dev->master_mutex);
> +		goto out_leases;
> +	}
> +
> +	/* Clone the lessor file to create a new file for us */
> +	DRM_DEBUG_LEASE("Allocating lease file\n");
> +	path_get(&lessor_file->f_path);

Please forgive the stupid question, but where is this reference given up?

> +	lessee_file = alloc_file(&lessor_file->f_path,
> +				 lessor_file->f_mode,
> +				 fops_get(lessor_file->f_inode->i_fop));
> +	mutex_unlock(&dev->master_mutex);
> +
> +	if (IS_ERR(lessee_file)) {
> +		ret = PTR_ERR(lessee_file);
> +		goto out_lessee;
> +	}
> +
> +	/* Initialize the new file for DRM */
> +	DRM_DEBUG_LEASE("Initializing the file with %p\n", lessee_file->f_op->open);
> +	ret = lessee_file->f_op->open(lessee_file->f_inode, lessee_file);
> +	if (ret)
> +		goto out_lessee_file;
> +
> +	lessee_priv = lessee_file->private_data;
> +
> +	/* Change the file to a master one */
> +	drm_master_put(&lessee_priv->master);
> +	lessee_priv->master = lessee;
> +	lessee_priv->is_master = 1;
> +	lessee_priv->authenticated = 1;
> +
> +	/* Hook up the fd */
> +	fd_install(fd, lessee_file);
> +
> +	/* Pass fd back to userspace */
> +	DRM_DEBUG_LEASE("Returning fd %d id %d\n", fd, lessee->lessee_id);
> +	cl->fd = fd;
> +	cl->lessee_id = lessee->lessee_id;
> +
> +	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
> +	return 0;
> +
> +out_lessee_file:
> +	fput(lessee_file);
> +
> +out_lessee:
> +	drm_master_put(&lessee);
> +
> +out_leases:
> +	idr_destroy(&leases);
> +	put_unused_fd(fd);
> +
> +	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
> +	return ret;
> +}
> +
> +/**
> + * drm_mode_list_lessees_ioctl - list lessee ids
> + * @dev: the drm device
> + * @data: pointer to struct drm_mode_list_lessees
> + * @lessor_priv: the file being manipulated
> + *
> + * Starting from the master associated with the specified file,
> + * the master with the provided lessee_id is found, and then
> + * an array of lessee ids associated with leases from that master
> + * are returned.
> + */
> +
> +int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> +			       void *data, struct drm_file *lessor_priv)
> +{
> +	struct drm_mode_list_lessees *arg = data;
> +	__u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);
> +	__u32 count_lessees = arg->count_lessees;
> +	struct drm_master *lessor = lessor_priv->master, *lessee;
> +	int count;
> +	int ret = 0;
> +
> +	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	count = 0;
> +	drm_for_each_lessee(lessee, lessor) {
> +		/* Only list un-revoked leases */
> +		if (!idr_is_empty(&lessee->leases)) {
> +			if (count_lessees > count) {
> +				DRM_DEBUG_LEASE("Add lessee %d\n", lessee->lessee_id);
> +				ret = put_user(lessee->lessee_id, lessee_ids + count);
> +				if (ret)
> +					break;
> +			}
> +			count++;
> +		}
> +	}
> +
> +	DRM_DEBUG_LEASE("Lessor leases to %d\n", count);
> +	if (ret == 0)
> +		arg->count_lessees = count;
> +
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return ret;
> +}
> +
> +/**
> + * drm_mode_get_lease_ioctl - list leased objects
> + * @dev: the drm device
> + * @data: pointer to struct drm_mode_get_lease
> + * @file_priv: the file being manipulated
> + *
> + * Return the list of leased objects for the specified lessee
> + */
> +
> +int drm_mode_get_lease_ioctl(struct drm_device *dev,
> +			     void *data, struct drm_file *lessee_priv)
> +{
> +	struct drm_mode_get_lease *arg = data;
> +	__u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);
> +	__u32 count_objects = arg->count_objects;
> +	struct drm_master *lessee = lessee_priv->master;
> +	struct idr *object_idr;
> +	int count;
> +	void *entry;
> +	int object;
> +	int ret = 0;
> +
> +	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	if (lessee->lessor == NULL)
> +		/* owner can use all objects */
> +		object_idr = &lessee->dev->mode_config.crtc_idr;

What about other types of objects?

> +	else
> +		/* lessee can only use allowed object */
> +		object_idr = &lessee->leases;
> +
> +	count = 0;
> +	idr_for_each_entry(object_idr, entry, object) {
> +		if (count_objects > count) {
> +			DRM_DEBUG_LEASE("adding object %d\n", object);
> +			ret = put_user(object, object_ids + count);
> +			if (ret)
> +				break;
> +		}
> +		count++;
> +	}
> +
> +	DRM_DEBUG("lease holds %d objects\n", count);
> +	if (ret == 0)
> +		arg->count_objects = count;
> +
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return ret;
> +}
> +
> +/**
> + * drm_mode_revoke_lease_ioctl - revoke lease
> + * @dev: the drm device
> + * @data: pointer to struct drm_mode_revoke_lease
> + * @file_priv: the file being manipulated
> + *
> + * This removes all of the objects from the lease without
> + * actually getting rid of the lease itself; that way all
> + * references to it still work correctly
> + */
> +int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *lessor_priv)
> +{
> +	struct drm_mode_revoke_lease *arg = data;
> +	struct drm_master *lessor = lessor_priv->master;
> +	struct drm_master *lessee;
> +	int ret = 0;
> +
> +	DRM_DEBUG_LEASE("revoke lease for %d\n", arg->lessee_id);
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	lessee = _drm_find_lessee(lessor, arg->lessee_id);
> +
> +	/* No such lessee */
> +	if (!lessee) {
> +		ret = -ENOENT;
> +		goto fail;
> +	}
> +
> +	/* Lease is not held by lessor */
> +	if (lessee->lessor != lessor) {
> +		ret = -EACCES;
> +		goto fail;
> +	}
> +
> +	_drm_lease_revoke(lessee);
> +
> +fail:
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index d1599f36b605..7c8b2698c6a7 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -111,7 +111,7 @@ void drm_mode_object_unregister(struct drm_device *dev,
>   * Returns whether the provided type of drm_mode_object must
>   * be owned or leased to be used by a process.
>   */
> -static bool drm_lease_required(uint32_t type)
> +bool drm_mode_object_lease_required(uint32_t type)
>  {
>  	switch(type) {
>  	case DRM_MODE_OBJECT_CRTC:
> @@ -136,7 +136,8 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
>  	if (obj && obj->id != id)
>  		obj = NULL;
>  
> -	if (obj && drm_lease_required(obj->type) && !_drm_lease_held(file_priv, obj->id))
> +	if (obj && drm_mode_object_lease_required(obj->type) &&
> +	    !_drm_lease_held(file_priv, obj->id))
>  		obj = NULL;
>  
>  	if (obj && obj->free_cb) {
> diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
> index 0981631b9aed..d2bcd1ea6cdc 100644
> --- a/include/drm/drm_lease.h
> +++ b/include/drm/drm_lease.h
> @@ -31,4 +31,16 @@ void _drm_lease_revoke(struct drm_master *master);
>  
>  uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs);
>  
> +int drm_mode_create_lease_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *file_priv);
> +
> +int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *file_priv);
> +
> +int drm_mode_get_lease_ioctl(struct drm_device *dev,
> +			     void *data, struct drm_file *file_priv);
> +
> +int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *file_priv);
> +
>  #endif /* _DRM_LEASE_H_ */
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index c8155cb5a932..7ba3913f30b5 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -154,4 +154,6 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
>  void drm_object_attach_property(struct drm_mode_object *obj,
>  				struct drm_property *property,
>  				uint64_t init_val);
> +
> +bool drm_mode_object_lease_required(uint32_t type);
>  #endif
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index a106f6a7a0f9..9c02d2125d07 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -888,6 +888,11 @@ extern "C" {
>  #define DRM_IOCTL_SYNCOBJ_RESET		DRM_IOWR(0xC4, struct drm_syncobj_array)
>  #define DRM_IOCTL_SYNCOBJ_SIGNAL	DRM_IOWR(0xC5, struct drm_syncobj_array)
>  
> +#define DRM_IOCTL_MODE_CREATE_LEASE	DRM_IOWR(0xC6, struct drm_mode_create_lease)
> +#define DRM_IOCTL_MODE_LIST_LESSEES	DRM_IOWR(0xC7, struct drm_mode_list_lessees)
> +#define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC8, struct drm_mode_get_lease)
> +#define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
> +
>  /**
>   * Device specific ioctls should only be in their respective headers
>   * The device specific ioctl range is from 0x40 to 0x9f.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 34b6bb34b002..5597a87154e5 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -782,6 +782,72 @@ struct drm_mode_destroy_blob {
>  	__u32 blob_id;
>  };
>  
> +/**
> + * Lease mode resources, creating another drm_master.
> + */
> +struct drm_mode_create_lease {
> +	/** Pointer to array of object ids (__u32) */
> +	__u64 object_ids;
> +	/** Number of object ids */
> +	__u32 object_count;
> +	/** flags for new FD (O_CLOEXEC, etc) */
> +	__u32 flags;
> +
> +	/** Return: unique identifier for lessee. */
> +	__u32 lessee_id;
> +	/** Return: file descriptor to new drm_master file */
> +	__u32 fd;
> +};
> +
> +/**
> + * List lesses from a drm_master
> + */
> +struct drm_mode_list_lessees {
> +	/** Number of lessees.
> +	 * On input, provides length of the array.
> +	 * On output, provides total number. No
> +	 * more than the input number will be written
> +	 * back, so two calls can be used to get
> +	 * the size and then the data.
> +	 */
> +	__u32 count_lessees;
> +	__u32 pad;
> +
> +	/** Pointer to lessees.
> +	 * pointer to __u64 array of lessee ids
> +	 */
> +	__u64 lessees_ptr;
> +};
> +
> +/**
> + * Get leased objects
> + */
> +struct drm_mode_get_lease {
> +	/** Number of leased objects.
> +	 * On input, provides length of the array.
> +	 * On output, provides total number. No
> +	 * more than the input number will be written
> +	 * back, so two calls can be used to get
> +	 * the size and then the data.
> +	 */
> +	__u32 count_objects;
> +	__u32 pad;
> +
> +	/** Pointer to objects.
> +	 * pointer to __u32 array of object ids
> +	 */
> +	__u64 objects_ptr;
> +};
> +
> +/**
> + * Revoke lease
> + */
> +struct drm_mode_revoke_lease {
> +	/** Unique ID of lessee
> +	 */
> +	__u32 lessee_id;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.15.0.rc0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Powered by blists - more mailing lists