Re: [PATCH 4/4] drm: Add four ioctls for managing drm mode object leases

[Daniel Vetter]

On Sat, Apr 01, 2017 at 10:08:41AM -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 from a particular lessor
> 
> drm_mode_get_lease
> 
> 	List the leased objects for a particular lessee

Should we just use fd for this? Essentially lessors would be required to
keep track of all their pending leases, and just dup() them over to the
client. Would reduce the uapi scope a bit, and if a lessor can't keep
track of it's leases there's a problem.

> drm_mode_change_lease
> 
> 	Adjust the terms of a lease, adding or removing resources or
> 	switching from masking to non-masking.

For this one here we could pass the fd of the lease as the argument. I
also still think that for v1 we just want revoke and otherwise invariant
leases. Makes things simpler.

> 
> This should suffice to at least create, query and manage available
> leases.
> 
> Signed-off-by: Keith Packard <keithp@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_ioctl.c |   4 +
>  drivers/gpu/drm/drm_lease.c | 309 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_lease.h     |  12 ++
>  include/uapi/drm/drm.h      |   4 +
>  include/uapi/drm/drm_mode.h |  78 +++++++++++
>  5 files changed, 407 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index fed22c2b98b6..0f9e3c0fe2ac 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -636,6 +636,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_ANY_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_ANY_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_ANY_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CHANGE_LEASE, drm_mode_change_lease_ioctl, DRM_ANY_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 782005c7706d..39131860bcd3 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -483,3 +483,312 @@ void drm_lease_destroy(struct drm_master *master)
>  
>  	DRM_DEBUG_LEASE("drm_lease_destroy done %d\n", master->lessee_id);
>  }
> +
> +/**
> + * 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;
> +
> +	object_count = cl->object_count;
> +	idr_init(&leases);
> +
> +	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;
> +
> +		if (copy_from_user(&object_id,
> +				   u64_to_user_ptr(cl->object_ids) + o * sizeof (__u32),
> +				   sizeof (__u32))) {
> +			ret = -EFAULT;
> +			goto bail;
> +		}
> +		DRM_DEBUG_LEASE("Adding object %d to lease\n", object_id);
> +
> +		ret = idr_alloc(&leases, (void *) 1, object_id, object_id + 1, GFP_KERNEL);
> +		if (ret < 0) {
> +			DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n",
> +					object_id, ret);
> +			goto bail;
> +		}
> +	}
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	DRM_DEBUG_LEASE("Creating lease\n");
> +	lessee = drm_lease_create(lessor, cl->mask_lease != 0, &leases);
> +
> +	if (IS_ERR(lessee)) {
> +		ret = PTR_ERR(lessee);
> +		lessee = NULL;
> +		mutex_unlock(&dev->master_mutex);
> +		goto bail;
> +	}
> +
> +	/* Clone the lessor file to create a new file for us */
> +	DRM_DEBUG_LEASE("Allocating lease file\n");
> +	path_get(&lessor_file->f_path);
> +	lessee_file = alloc_file(&lessor_file->f_path,
> +				 lessor_file->f_mode,
> +				 fops_get(lessor_file->f_inode->i_fop));
> +
> +	if (IS_ERR(lessee_file)) {
> +		ret = PTR_ERR(lessee_file);
> +		lessee_file = NULL;
> +		mutex_unlock(&dev->master_mutex);
> +		goto bail;
> +	}
> +
> +	mutex_unlock(&dev->master_mutex);
> +
> +	/* 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 bail;
> +
> +	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;
> +
> +	/* and don't destroy our resources */
> +	fd = -1;
> +	lessee = NULL;
> +	lessee_file = NULL;
> +
> +	ret = 0;
> +
> +bail:
> +
> +	if (lessee_file) {
> +		DRM_DEBUG_LEASE("Freeing lessee file\n");
> +		fput(lessee_file);
> +	}
> +
> +	if (lessee) {
> +		DRM_DEBUG_LEASE("Freeing lessee drm_master\n");
> +		drm_master_put(&lessee);
> +	}
> +
> +	idr_destroy(&leases);
> +
> +	if (fd != -1) {
> +		DRM_DEBUG_LEASE("Freeing unused fd\n");
> +		put_unused_fd(fd);
> +	}
> +
> +	DRM_DEBUG_LEASE("Return %d from drm_mode_create_lease_ioctl\n", ret);
> +	return ret;
> +}
> +
> +/**
> + * drm_mode_list_lessees_ioctl - list lessee ids
> + * @dev: the drm device
> + * @data: pointer to struct drm_mode_create_lease
> + * @file_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 *file_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, *lessee;
> +	int count;
> +	int ret = 0;
> +
> +	DRM_DEBUG_LEASE("List lessees for %d\n", arg->lessor_id);
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	/* Search the tree for the requested drm_master */
> +	lessor = drm_lessee_get(file_priv->master, arg->lessor_id);
> +	if (!lessor) {
> +		DRM_DEBUG_LEASE("No such lessor %d\n", arg->lessor_id);
> +		mutex_unlock(&dev->master_mutex);
> +		return -ENOENT;
> +	}
> +
> +	count = 0;
> +	drm_for_each_lessee(lessee, lessor) {
> +		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;
> +
> +	drm_master_put(&lessor);
> +
> +	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_create_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 *file_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;
> +	struct idr *object_idr;
> +	int count;
> +	void *entry;
> +	int object;
> +	int ret = 0;
> +
> +	DRM_DEBUG_LEASE("get lease for %d\n", arg->lessee_id);
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	/* Search the tree for the requested drm_master */
> +	lessee = drm_lessee_get(file_priv->master, arg->lessee_id);
> +	if (!lessee) {
> +		mutex_unlock(&dev->master_mutex);
> +		DRM_DEBUG_LEASE("No such lessee %d\n", arg->lessee_id);
> +		return -ENOENT;
> +	}
> +
> +	if (lessee->lessor == NULL)
> +		object_idr = &lessee->dev->mode_config.crtc_idr;
> +	else
> +		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;
> +
> +	drm_master_put(&lessee);
> +
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return ret;
> +}
> +
> +/**
> + * drm_mode_change_lease_ioctl - change the objects in a 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_change_lease_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *file_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 = file_priv->master;
> +
> +	object_count = cl->object_count;
> +	idr_init(&leases);
> +
> +	DRM_DEBUG_LEASE("Changing existing lease\n");
> +
> +	/* Lookup the mode objects and add their IDs to the lease request */
> +	for (o = 0; o < object_count; o++) {
> +		__u32 object_id;
> +
> +		if (copy_from_user(&object_id,
> +				   u64_to_user_ptr(cl->object_ids) + o * sizeof (__u32),
> +				   sizeof (__u32))) {
> +			return -EFAULT;
> +		}
> +		DRM_DEBUG_LEASE("Adding object %d to lease\n", object_id);
> +
> +		ret = idr_alloc(&leases, (void *) 1, object_id, object_id + 1, GFP_KERNEL);
> +		if (ret < 0) {
> +			DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n",
> +					object_id, ret);
> +			return ret;
> +		}
> +	}
> +
> +	mutex_lock(&dev->master_mutex);
> +
> +	DRM_DEBUG_LEASE("Change lease\n");
> +
> +	ret = drm_lease_change(lessor, cl->lessee_id, cl->mask_lease != 0, &leases);
> +
> +	mutex_unlock(&dev->master_mutex);
> +
> +	idr_destroy(&leases);
> +
> +	DRM_DEBUG_LEASE("Return %d from drm_mode_change_lease_ioctl\n", ret);
> +	return ret;
> +}
> diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
> index 8f91fc4226e3..367b57698f53 100644
> --- a/include/drm/drm_lease.h
> +++ b/include/drm/drm_lease.h
> @@ -52,4 +52,16 @@ 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);
>  
> +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_change_lease_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *file_priv);

Non-driver functions please into drm-internal.h, we don't want to let
drivers even see this :-)

Cheers, Daniel

> +
>  #endif /* _DRM_LEASE_H_ */
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c52843bc70..8c62da23d7a3 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -813,6 +813,10 @@ extern "C" {
>  #define DRM_IOCTL_MODE_ATOMIC		DRM_IOWR(0xBC, struct drm_mode_atomic)
>  #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
>  #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
> +#define DRM_IOCTL_MODE_CREATE_LEASE	DRM_IOWR(0xBF, struct drm_mode_create_lease)
> +#define DRM_IOCTL_MODE_LIST_LESSEES	DRM_IOWR(0xC0, struct drm_mode_list_lessees)
> +#define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC1, struct drm_mode_get_lease)
> +#define DRM_IOCTL_MODE_CHANGE_LEASE	DRM_IOWR(0xC2, struct drm_mode_change_lease)
>  
>  /**
>   * Device specific ioctls should only be in their respective headers
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2e8a5e..e6669ada3b10 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -681,6 +681,84 @@ 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;
> +	/** whether to hide the leased objects from the lessor */
> +	__u32 mask_lease;
> +
> +	/** 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 {
> +
> +	/** Identifier of the lessor's lessee_id (0 for owner) */
> +	__u32 lessor_id;
> +
> +	/** 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;
> +
> +	/** Pointer to lessees.
> +	 * pointer to __u64 array of lessee ids
> +	 */
> +	__u64 lessees_ptr;
> +};
> +
> +/**
> + * Get leased objects for a lessee
> + */
> +struct drm_mode_get_lease {
> +	/** Identifier of the lessee (0 for owner) */
> +	__u32 lessee_id;
> +
> +	/** 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;
> +
> +	/** Pointer to objects.
> +	 * pointer to __u32 array of object ids
> +	 */
> +	__u64 objects_ptr;
> +};
> +
> +/**
> + * Change resources leased to another drm_master
> + */
> +struct drm_mode_change_lease {
> +	/** Pointer to array of object ids (__u32) */
> +	__u64 object_ids;
> +	/** Number of object ids */
> +	__u32 object_count;
> +	/** unique identifier for lessee. */
> +	__u32 lessee_id;
> +	/** whether to hide the leased objects from the lessor */
> +	__u32 mask_lease;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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