On Sat, May 09, 2015 at 03:52:13PM +0100, Daniel Stone wrote:
> Add an ioctl which allows users to create blob properties from supplied
> data. Currently this only supports modes, creating a drm_display_mode from
> the userspace drm_mode_modeinfo.
>
> v2: Removed size/type checks.
> Rebased on new patches to allow error propagation from create_blob,
> as well as avoiding double-allocation.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at intel.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 139
> +++++++++++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/drm_fops.c | 5 +-
> drivers/gpu/drm/drm_ioctl.c | 2 +
> include/drm/drmP.h | 4 ++
> include/drm/drm_crtc.h | 9 ++-
> include/uapi/drm/drm.h | 2 +
> include/uapi/drm/drm_mode.h | 20 +++++++
> 7 files changed, 176 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index df4a3fb..9546aed 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4232,6 +4232,9 @@ drm_property_create_blob(struct drm_device *dev, size_t
> length,
> if (!blob)
> return ERR_PTR(-ENOMEM);
>
> + /* This must be explicitly initialised, so we can safely call list_del
> + * on it in the removal handler, even if it isn't in a file list. */
> + INIT_LIST_HEAD(&blob->head_file);
> blob->length = length;
> blob->dev = dev;
>
> @@ -4249,7 +4252,8 @@ drm_property_create_blob(struct drm_device *dev, size_t
> length,
>
> kref_init(&blob->refcount);
>
> - list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
> + list_add_tail(&blob->head_global,
> + &dev->mode_config.property_blob_list);
>
> mutex_unlock(&dev->mode_config.blob_lock);
>
> @@ -4271,7 +4275,8 @@ static void drm_property_free_blob(struct kref *kref)
>
> WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock));
>
> - list_del(&blob->head);
> + list_del(&blob->head_global);
> + list_del(&blob->head_file);
> drm_mode_object_put(blob->dev, &blob->base);
>
> kfree(blob);
> @@ -4324,6 +4329,26 @@ static void
> drm_property_unreference_blob_locked(struct drm_property_blob *blob)
> }
>
> /**
> + * drm_property_destroy_user_blobs - destroy all blobs created by this client
> + * @dev: DRM device
> + * @file_priv: destroy all blobs owned by this file handle
> + */
> +void drm_property_destroy_user_blobs(struct drm_device *dev,
> + struct drm_file *file_priv)
> +{
> + struct drm_property_blob *blob, *bt;
> +
> + mutex_lock(&dev->mode_config.blob_lock);
> +
> + list_for_each_entry_safe(blob, bt, &file_priv->blobs, head_file) {
> + list_del_init(&blob->head_file);
> + drm_property_unreference_blob_locked(blob);
> + }
> +
> + mutex_unlock(&dev->mode_config.blob_lock);
> +}
> +
> +/**
> * drm_property_reference_blob - Take a reference on an existing property
> *
> * Take a new reference on an existing blob property.
> @@ -4513,6 +4538,114 @@ done:
> }
>
> /**
> + * drm_mode_createblob_ioctl - create a new blob property
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * This function creates a new blob property with user-defined values. In
> order
> + * to give us sensible validation and checking when creating, rather than at
> + * every potential use, we also require a type to be provided upfront.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_createblob_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + struct drm_mode_create_blob *out_resp = data;
> + struct drm_property_blob *blob;
> + void __user *blob_ptr;
> + int ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
Technicality, but without the userspace demonstration vehicle we'll need
to hide this behind the atomic knob. Unless the westen patches are all
done, at which point we can just pull things in right away I think?
-Daniel
> +
> + blob = drm_property_create_blob(dev, out_resp->length, NULL);
> + if (IS_ERR(blob))
> + return PTR_ERR(blob);
> +
> + blob_ptr = (void __user *)(unsigned long)out_resp->data;
> + if (copy_from_user(blob->data, blob_ptr, out_resp->length)) {
> + ret = -EFAULT;
> + goto out_blob;
> + }
> +
> + /* Dropping the lock between create_blob and our access here is safe
> + * as only the same file_priv can remove the blob; at this point, it is
> + * not associated with any file_priv. */
> + mutex_lock(&dev->mode_config.blob_lock);
> + out_resp->blob_id = blob->base.id;
> + list_add_tail(&file_priv->blobs, &blob->head_file);
> + mutex_unlock(&dev->mode_config.blob_lock);
> +
> + return 0;
> +
> +out_blob:
> + drm_property_unreference_blob(blob);
> + return ret;
> +}
> +
> +/**
> + * drm_mode_destroyblob_ioctl - destroy a user blob property
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Destroy an existing user-defined blob property.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_destroyblob_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + struct drm_mode_destroy_blob *out_resp = data;
> + struct drm_property_blob *blob = NULL, *bt;
> + bool found = false;
> + int ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + mutex_lock(&dev->mode_config.blob_lock);
> + blob = __drm_property_lookup_blob(dev, out_resp->blob_id);
> + if (!blob) {
> + ret = -ENOENT;
> + goto err;
> + }
> +
> + /* Ensure the property was actually created by this user. */
> + list_for_each_entry(bt, &file_priv->blobs, head_file) {
> + if (bt == blob) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + ret = -EPERM;
> + goto err;
> + }
> +
> + /* We must drop head_file here, because we may not be the last
> + * reference on the blob. */
> + list_del_init(&blob->head_file);
> + drm_property_unreference_blob_locked(blob);
> + mutex_unlock(&dev->mode_config.blob_lock);
> +
> + return 0;
> +
> +err:
> + mutex_unlock(&dev->mode_config.blob_lock);
> + return ret;
> +}
> +
> +/**
> * drm_mode_connector_set_path_property - set tile property on connector
> * @connector: connector to set property on.
> * @path: path to use for property; must not be NULL.
> @@ -5715,7 +5848,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> }
>
> list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
> - head) {
> + head_global) {
> drm_property_unreference_blob(blob);
> }
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 0f6a5c8..c59ce4d 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -167,6 +167,7 @@ static int drm_open_helper(struct file *filp, struct
> drm_minor *minor)
> INIT_LIST_HEAD(&priv->lhead);
> INIT_LIST_HEAD(&priv->fbs);
> mutex_init(&priv->fbs_lock);
> + INIT_LIST_HEAD(&priv->blobs);
> INIT_LIST_HEAD(&priv->event_list);
> init_waitqueue_head(&priv->event_wait);
> priv->event_space = 4096; /* set aside 4k for event buffer */
> @@ -405,8 +406,10 @@ int drm_release(struct inode *inode, struct file *filp)
>
> drm_events_release(file_priv);
>
> - if (drm_core_check_feature(dev, DRIVER_MODESET))
> + if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> drm_fb_release(file_priv);
> + drm_property_destroy_user_blobs(dev, file_priv);
> + }
>
> if (drm_core_check_feature(dev, DRIVER_GEM))
> drm_gem_release(dev, file_priv);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 266dcd6..9bac1b7 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -641,6 +641,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY,
> drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl,
> DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> 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),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index df6d997..9fa6366 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -326,6 +326,10 @@ struct drm_file {
> struct list_head fbs;
> struct mutex fbs_lock;
>
> + /** User-created blob properties; this retains a reference on the
> + * property. */
> + struct list_head blobs;
> +
> wait_queue_head_t event_wait;
> struct list_head event_list;
> int event_space;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 5626191..0ed63d9 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -218,7 +218,8 @@ struct drm_property_blob {
> struct drm_mode_object base;
> struct drm_device *dev;
> struct kref refcount;
> - struct list_head head;
> + struct list_head head_global;
> + struct list_head head_file;
> size_t length;
> unsigned char data[];
> };
> @@ -1288,6 +1289,8 @@ extern const char *drm_get_dvi_i_select_name(int val);
> extern const char *drm_get_tv_subconnector_name(int val);
> extern const char *drm_get_tv_select_name(int val);
> extern void drm_fb_release(struct drm_file *file_priv);
> +extern void drm_property_destroy_user_blobs(struct drm_device *dev,
> + struct drm_file *file_priv);
> extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct
> drm_mode_group *group);
> extern void drm_mode_group_destroy(struct drm_mode_group *group);
> extern void drm_reinit_primary_mode_group(struct drm_device *dev);
> @@ -1433,6 +1436,10 @@ extern int drm_mode_getproperty_ioctl(struct
> drm_device *dev,
> void *data, struct drm_file *file_priv);
> extern int drm_mode_getblob_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> +extern int drm_mode_createblob_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +extern int drm_mode_destroyblob_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> extern int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> void *data, struct drm_file
> *file_priv);
> extern int drm_mode_getencoder(struct drm_device *dev,
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index ff6ef62..3801584 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -786,6 +786,8 @@ struct drm_prime_handle {
> #define DRM_IOCTL_MODE_OBJ_SETPROPERTY DRM_IOWR(0xBA, struct
> drm_mode_obj_set_property)
> #define DRM_IOCTL_MODE_CURSOR2 DRM_IOWR(0xBB, struct
> drm_mode_cursor2)
> #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)
>
> /**
> * 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 dbeba94..359107a 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -558,4 +558,24 @@ struct drm_mode_atomic {
> __u64 user_data;
> };
>
> +/**
> + * Create a new 'blob' data property, copying length bytes from data pointer,
> + * and returning new blob ID.
> + */
> +struct drm_mode_create_blob {
> + /** Pointer to data to copy. */
> + __u64 data;
> + /** Length of data to copy. */
> + __u32 length;
> + /** Return: new property ID. */
> + __u32 blob_id;
> +};
> +
> +/**
> + * Destroy a user-created blob property.
> + */
> +struct drm_mode_destroy_blob {
> + __u32 blob_id;
> +};
> +
> #endif
> --
> 2.4.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
