Hi, This is the spritual successor to the modes-as-blob-properties patchset.
There are some fairly drastic differences though, namely: - the referenced object in this case is the blob property - being just a dumb chunk of data - rather than attempting to refcount modes; meaning that ... - userspace doesn't see blob mode IDs from the connector list, only from the current mode, so it really needs to create the mdoes again from the drm_mode_modeinfo it gets - the mode-constness series has been split out and will be sent separately
Actually exposing the mode property does largely seem to work, but need to fix i915 to not copy CRTC states by value before it does.
This series still retains the concept of a type within the create blob ioctl, on the grounds that otherwise we could allow userspace to create unbounded allocations in the kernel with no attribution. Other than matching size, the type has no meaning per se.
Cheers, Daniel
One failure path in crtc_helper had an open-coded CRTC state destroy which didn't actually call through to the driver's specified state destroy. Fix that.
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_crtc_helper.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index ab00286..d727b73 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -959,7 +959,12 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod if (crtc_funcs->atomic_check) { ret = crtc_funcs->atomic_check(crtc, crtc_state); if (ret) { - kfree(crtc_state); + if (crtc->funcs->atomic_destroy_state) { + crtc->funcs->atomic_destroy_state(crtc, + crtc_state); + } else { + kfree(crtc_state); + }
return ret; }
Just change an if (success) branch to if (fail) return;
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d536817..60eb505 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,12 +1983,13 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
state = kmemdup(crtc->state, sizeof(*crtc->state), GFP_KERNEL);
- if (state) { - state->mode_changed = false; - state->active_changed = false; - state->planes_changed = false; - state->event = NULL; - } + if (!state) + return NULL; + + state->mode_changed = false; + state->active_changed = false; + state->planes_changed = false; + state->event = NULL;
return state; }
On Mon, Apr 20, 2015 at 07:22:51PM +0100, Daniel Stone wrote:
Just change an if (success) branch to if (fail) return;
Signed-off-by: Daniel Stone daniels@collabora.com
I dropped this one since the code already changed a bit. -Daniel
drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d536817..60eb505 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1983,12 +1983,13 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
state = kmemdup(crtc->state, sizeof(*crtc->state), GFP_KERNEL);
- if (state) {
state->mode_changed = false;state->active_changed = false;state->planes_changed = false;state->event = NULL;- }
if (!state)
return NULL;state->mode_changed = false;
state->active_changed = false;
state->planes_changed = false;
state->event = NULL;
return state;
}
2.3.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Previously, when updating the path blob property, we would leak the existing one. Make this symmetrical with the tile and EDID blob pointers.
Signed-off-by: Daniel Stone daniels@collabora.com Cc: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/drm_crtc.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a497c08..aae0d84 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4302,6 +4302,9 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector, size_t size = strlen(path) + 1; int ret;
+ if (connector->path_blob_ptr) + drm_property_destroy_blob(dev, connector->path_blob_ptr); + connector->path_blob_ptr = drm_property_create_blob(connector->dev, size, path); if (!connector->path_blob_ptr)
Introduce a common helper for the pattern of: - allocate new blob property - potentially free old blob property - replace content of indicative property with new blob ID - change member pointer on modeset object
Signed-off-by: Daniel Stone daniels@collabora.com Cc: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/drm_crtc.c | 155 +++++++++++++++++++++++++++++++-------------- 1 file changed, 106 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index aae0d84..ed9341d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4237,6 +4237,83 @@ static void drm_property_destroy_blob(struct drm_device *dev, }
/** + * drm_property_replace_global_blob - atomically replace existing blob property + * @dev: drm device + * @replace: location of blob property pointer to be replaced + * @length: length of data for new blob, or 0 for no data + * @data: content for new blob, or NULL for no data + * @obj_holds_id: optional object for property holding blob ID + * @holds_id: optional property holding blob ID + * @return 0 on success or error on failure + * + * This function will atomically replace a global property in the blob list, + * optionally updating a property which holds the ID of that property. It is + * guaranteed to be atomic: no caller will be allowed to see intermediate + * results, and either the entire operation will succeed and clean up the + * previous property, or it will fail and the state will be unchanged. + * + * If length is 0 or data is NULL, no new blob will be created, and the holding + * property, if specified, will be set to 0. + * + * Access to the replace pointer is assumed to be protected by the caller, e.g. + * by holding the relevant modesetting object lock for its parent. + * + * For example, a drm_connector has a 'PATH' property, which contains the ID + * of a blob property with the value of the MST path information. Calling this + * function with replace pointing to the connector's path_blob_ptr, length and + * data set for the new path information, obj_holds_id set to the connector's + * base object, and prop_holds_id set to the path property name, will perform + * a completely atomic update. The access to path_blob_ptr is protected by the + * caller holding a lock on the connector. + */ +static int drm_property_replace_global_blob(struct drm_device *dev, + struct drm_property_blob **replace, + size_t length, + const void *data, + struct drm_mode_object *obj_holds_id, + struct drm_property *prop_holds_id) +{ + struct drm_property_blob *new_blob = NULL; + struct drm_property_blob *old_blob = NULL; + int ret; + + WARN_ON(replace == NULL); + + old_blob = *replace; + + if (length && data) { + new_blob = drm_property_create_blob(dev, length, data); + if (!new_blob) + return -EINVAL; + } + + /* This does not need to be synchronised with blob_lock, as the + * get_properties ioctl locks all modesetting objects, and + * obj_holds_id must be locked before calling here, so we cannot + * have its value out of sync with the list membership modified + * below under blob_lock. */ + if (obj_holds_id) { + ret = drm_object_property_set_value(obj_holds_id, + prop_holds_id, + new_blob ? + new_blob->base.id : 0); + if (ret != 0) + goto err_created; + } + + if (old_blob) + drm_property_destroy_blob(dev, old_blob); + + *replace = new_blob; + + return 0; + +err_created: + drm_property_destroy_blob(dev, new_blob); + return ret; +} + +/** * drm_mode_getblob_ioctl - get the contents of a blob property value * @dev: DRM device * @data: ioctl data @@ -4285,7 +4362,7 @@ done: /** * drm_mode_connector_set_path_property - set tile property on connector * @connector: connector to set property on. - * @path: path to use for property. + * @path: path to use for property; must not be NULL. * * This creates a property to expose to userspace to specify a * connector path. This is mainly used for DisplayPort MST where @@ -4299,20 +4376,14 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector, const char *path) { struct drm_device *dev = connector->dev; - size_t size = strlen(path) + 1; int ret;
- if (connector->path_blob_ptr) - drm_property_destroy_blob(dev, connector->path_blob_ptr); - - connector->path_blob_ptr = drm_property_create_blob(connector->dev, - size, path); - if (!connector->path_blob_ptr) - return -EINVAL; - - ret = drm_object_property_set_value(&connector->base, - dev->mode_config.path_property, - connector->path_blob_ptr->base.id); + ret = drm_property_replace_global_blob(dev, + &connector->path_blob_ptr, + strlen(path) + 1, + path, + &connector->base, + dev->mode_config.path_property); return ret; } EXPORT_SYMBOL(drm_mode_connector_set_path_property); @@ -4331,16 +4402,16 @@ EXPORT_SYMBOL(drm_mode_connector_set_path_property); int drm_mode_connector_set_tile_property(struct drm_connector *connector) { struct drm_device *dev = connector->dev; - int ret, size; char tile[256]; - - if (connector->tile_blob_ptr) - drm_property_destroy_blob(dev, connector->tile_blob_ptr); + int ret;
if (!connector->has_tile) { - connector->tile_blob_ptr = NULL; - ret = drm_object_property_set_value(&connector->base, - dev->mode_config.tile_property, 0); + ret = drm_property_replace_global_blob(dev, + &connector->tile_blob_ptr, + 0, + NULL, + &connector->base, + dev->mode_config.tile_property); return ret; }
@@ -4349,16 +4420,13 @@ int drm_mode_connector_set_tile_property(struct drm_connector *connector) connector->num_h_tile, connector->num_v_tile, connector->tile_h_loc, connector->tile_v_loc, connector->tile_h_size, connector->tile_v_size); - size = strlen(tile) + 1;
- connector->tile_blob_ptr = drm_property_create_blob(connector->dev, - size, tile); - if (!connector->tile_blob_ptr) - return -EINVAL; - - ret = drm_object_property_set_value(&connector->base, - dev->mode_config.tile_property, - connector->tile_blob_ptr->base.id); + ret = drm_property_replace_global_blob(dev, + &connector->tile_blob_ptr, + strlen(tile) + 1, + tile, + &connector->base, + dev->mode_config.tile_property); return ret; } EXPORT_SYMBOL(drm_mode_connector_set_tile_property); @@ -4378,33 +4446,22 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector, const struct edid *edid) { struct drm_device *dev = connector->dev; - size_t size; + size_t size = 0; int ret;
/* ignore requests to set edid when overridden */ if (connector->override_edid) return 0;
- if (connector->edid_blob_ptr) - drm_property_destroy_blob(dev, connector->edid_blob_ptr); - - /* Delete edid, when there is none. */ - if (!edid) { - connector->edid_blob_ptr = NULL; - ret = drm_object_property_set_value(&connector->base, dev->mode_config.edid_property, 0); - return ret; - } - - size = EDID_LENGTH * (1 + edid->extensions); - connector->edid_blob_ptr = drm_property_create_blob(connector->dev, - size, edid); - if (!connector->edid_blob_ptr) - return -EINVAL; - - ret = drm_object_property_set_value(&connector->base, - dev->mode_config.edid_property, - connector->edid_blob_ptr->base.id); + if (edid) + size = EDID_LENGTH + (1 + edid->extensions);
+ ret = drm_property_replace_global_blob(dev, + &connector->edid_blob_ptr, + size, + edid, + &connector->base, + dev->mode_config.edid_property); return ret; } EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
Create a new global blob_lock mutex, which protects the blob property list from insertion and/or deletion.
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_crtc.c | 18 +++++++++++++++--- include/drm/drm_crtc.h | 3 +++ 2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ed9341d..9947078 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4214,25 +4214,34 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (!blob) return NULL;
+ blob->length = length; + + memcpy(blob->data, data, length); + + mutex_lock(&dev->mode_config.blob_lock); + ret = drm_mode_object_get(dev, &blob->base, DRM_MODE_OBJECT_BLOB); if (ret) { kfree(blob); + mutex_unlock(&dev->mode_config.blob_lock); return NULL; }
- blob->length = length; + list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
- memcpy(blob->data, data, length); + mutex_unlock(&dev->mode_config.blob_lock);
- list_add_tail(&blob->head, &dev->mode_config.property_blob_list); return blob; }
static void drm_property_destroy_blob(struct drm_device *dev, struct drm_property_blob *blob) { + mutex_lock(&dev->mode_config.blob_lock); drm_mode_object_put(dev, &blob->base); list_del(&blob->head); + mutex_unlock(&dev->mode_config.blob_lock); + kfree(blob); }
@@ -4339,6 +4348,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev, return -EINVAL;
drm_modeset_lock_all(dev); + mutex_lock(&dev->mode_config.blob_lock); blob = drm_property_blob_find(dev, out_resp->blob_id); if (!blob) { ret = -ENOENT; @@ -4355,6 +4365,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev, out_resp->length = blob->length;
done: + mutex_unlock(&dev->mode_config.blob_lock); drm_modeset_unlock_all(dev); return ret; } @@ -5488,6 +5499,7 @@ void drm_mode_config_init(struct drm_device *dev) drm_modeset_lock_init(&dev->mode_config.connection_mutex); mutex_init(&dev->mode_config.idr_mutex); mutex_init(&dev->mode_config.fb_lock); + mutex_init(&dev->mode_config.blob_lock); INIT_LIST_HEAD(&dev->mode_config.fb_list); INIT_LIST_HEAD(&dev->mode_config.crtc_list); INIT_LIST_HEAD(&dev->mode_config.connector_list); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index b009d70..43a3758 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1049,6 +1049,7 @@ struct drm_mode_group { * @poll_running: track polling status for this device * @output_poll_work: delayed work for polling in process context * @property_blob_list: list of all the blob property objects + * @blob_lock: mutex for blob property allocation and management * @*_property: core property tracking * @preferred_depth: preferred RBG pixel depth, used by fb helpers * @prefer_shadow: hint to userspace to prefer shadow-fb rendering @@ -1104,6 +1105,8 @@ struct drm_mode_config { bool delayed_event; struct delayed_work output_poll_work;
+ struct mutex blob_lock; + /* pointers to standard properties */ struct list_head property_blob_list; struct drm_property *edid_property;
Reference-count drm_property_blob objects, changing the API to ref/unref.
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_crtc.c | 164 +++++++++++++++++++++++++++++++++++++++++---- include/drm/drm_crtc.h | 17 ++--- 2 files changed, 159 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9947078..03245fb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -352,7 +352,9 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, if (obj && obj->id != id) obj = NULL; /* don't leak out unref'd fb's */ - if (obj && (obj->type == DRM_MODE_OBJECT_FB)) + if (obj && + (obj->type == DRM_MODE_OBJECT_FB || + obj->type == DRM_MODE_OBJECT_BLOB)) obj = NULL; mutex_unlock(&dev->mode_config.idr_mutex);
@@ -377,7 +379,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
/* Framebuffers are reference counted and need their own lookup * function.*/ - WARN_ON(type == DRM_MODE_OBJECT_FB); + WARN_ON(type == DRM_MODE_OBJECT_FB || type == DRM_MODE_OBJECT_BLOB); obj = _object_find(dev, id, type); return obj; } @@ -4200,7 +4202,7 @@ done: return ret; }
-static struct drm_property_blob * +struct drm_property_blob * drm_property_create_blob(struct drm_device *dev, size_t length, const void *data) { @@ -4215,6 +4217,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, return NULL;
blob->length = length; + blob->dev = dev;
memcpy(blob->data, data, length);
@@ -4227,25 +4230,148 @@ drm_property_create_blob(struct drm_device *dev, size_t length, return NULL; }
+ kref_init(&blob->refcount); + list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
mutex_unlock(&dev->mode_config.blob_lock);
return blob; } +EXPORT_SYMBOL(drm_property_create_blob);
-static void drm_property_destroy_blob(struct drm_device *dev, - struct drm_property_blob *blob) +/** + * drm_property_free_blob - Blob property destructor + * + * Internal free function for blob properties; must not be used directly. + * + * @param kref Reference + */ +static void drm_property_free_blob(struct kref *kref) { - mutex_lock(&dev->mode_config.blob_lock); - drm_mode_object_put(dev, &blob->base); + struct drm_property_blob *blob = + container_of(kref, struct drm_property_blob, refcount); + + WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock)); + list_del(&blob->head); - mutex_unlock(&dev->mode_config.blob_lock); + drm_mode_object_put(blob->dev, &blob->base);
kfree(blob); }
/** + * drm_property_unreference_blob - Unreference a blob property + * + * Drop a reference on a blob property. May free the object. + * + * @param dev Device the blob was created on + * @param blob Pointer to blob property + */ +void drm_property_unreference_blob(struct drm_property_blob *blob) +{ + struct drm_device *dev; + + if (!blob) + return; + + dev = blob->dev; + + DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount)); + + if (kref_put_mutex(&blob->refcount, drm_property_free_blob, + &dev->mode_config.blob_lock)) + mutex_unlock(&blob->dev->mode_config.blob_lock); + else + might_lock(&dev->mode_config.blob_lock); + +} +EXPORT_SYMBOL(drm_property_unreference_blob); + +/** + * drm_property_unreference_blob_locked - Unreference a blob property with blob_lock held + * + * Drop a reference on a blob property. May free the object. This must be + * called with blob_lock held. + * + * @param dev Device the blob was created on + * @param blob Pointer to blob property + */ +static void drm_property_unreference_blob_locked(struct drm_property_blob *blob) +{ + if (!blob) + return; + + DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount)); + + kref_put(&blob->refcount, drm_property_free_blob); +} + +/** + * drm_property_reference_blob - Take a reference on an existing property + * + * Take a new reference on an existing blob property. + * + * @param blob Pointer to blob property + */ +struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob) +{ + DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount)); + kref_get(&blob->refcount); + return blob; +} +EXPORT_SYMBOL(drm_property_reference_blob); + +/* + * Like drm_property_lookup_blob, but does not return an additional reference. + * Must be called with blob_lock held. + */ +static struct drm_property_blob *__drm_property_lookup_blob(struct drm_device *dev, + uint32_t id) +{ + struct drm_mode_object *obj = NULL; + struct drm_property_blob *blob; + + WARN_ON(!mutex_is_locked(&dev->mode_config.blob_lock)); + + mutex_lock(&dev->mode_config.idr_mutex); + obj = idr_find(&dev->mode_config.crtc_idr, id); + if (!obj || (obj->type != DRM_MODE_OBJECT_BLOB) || (obj->id != id)) + blob = NULL; + else + blob = obj_to_blob(obj); + mutex_unlock(&dev->mode_config.idr_mutex); + + return blob; +} + +/** + * drm_property_lookup_blob - look up a blob property and take a reference + * @dev: drm device + * @id: id of the blob property + * + * If successful, this takes an additional reference to the blob property. + * callers need to make sure to eventually unreference the returned property + * again, using @drm_property_unreference_blob. + */ +struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev, + uint32_t id) +{ + struct drm_property_blob *blob; + + mutex_lock(&dev->mode_config.blob_lock); + blob = __drm_property_lookup_blob(dev, id); + if (blob) { + if (!kref_get_unless_zero(&blob->refcount)) + blob = NULL; + } + mutex_unlock(&dev->mode_config.blob_lock); + + return blob; +} +EXPORT_SYMBOL(drm_property_lookup_blob); + +/** * drm_property_replace_global_blob - atomically replace existing blob property * @dev: drm device * @replace: location of blob property pointer to be replaced @@ -4311,14 +4437,14 @@ static int drm_property_replace_global_blob(struct drm_device *dev, }
if (old_blob) - drm_property_destroy_blob(dev, old_blob); + drm_property_unreference_blob(old_blob);
*replace = new_blob;
return 0;
err_created: - drm_property_destroy_blob(dev, new_blob); + drm_property_unreference_blob(new_blob); return ret; }
@@ -4349,7 +4475,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
drm_modeset_lock_all(dev); mutex_lock(&dev->mode_config.blob_lock); - blob = drm_property_blob_find(dev, out_resp->blob_id); + blob = __drm_property_lookup_blob(dev, out_resp->blob_id); if (!blob) { ret = -ENOENT; goto done; @@ -4513,8 +4639,18 @@ bool drm_property_change_valid_get(struct drm_property *property, valid_mask |= (1ULL << property->values[i]); return !(value & ~valid_mask); } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) { - /* Only the driver knows */ - return true; + struct drm_property_blob *blob; + + if (value == 0) + return true; + + blob = drm_property_lookup_blob(property->dev, value); + if (blob) { + *ref = &blob->base; + return true; + } else { + return false; + } } else if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) { /* a zero value for an object property translates to null: */ if (value == 0) @@ -5564,7 +5700,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list, head) { - drm_property_destroy_blob(dev, blob); + drm_property_unreference_blob(blob); }
/* diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 43a3758..07c99f6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -217,6 +217,8 @@ struct drm_framebuffer {
struct drm_property_blob { struct drm_mode_object base; + struct drm_device *dev; + struct kref refcount; struct list_head head; size_t length; unsigned char data[]; @@ -1366,6 +1368,13 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, int flags, const char *name, uint32_t type); struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, const char *name); +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, + size_t length, + const void *data); +struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev, + uint32_t id); +struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob); +void drm_property_unreference_blob(struct drm_property_blob *blob); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); extern int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name); @@ -1529,14 +1538,6 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, return mo ? obj_to_property(mo) : NULL; }
-static inline struct drm_property_blob * -drm_property_blob_find(struct drm_device *dev, uint32_t id) -{ - struct drm_mode_object *mo; - mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_BLOB); - return mo ? obj_to_blob(mo) : NULL; -} - /* Plane list iterator for legacy (overlay only) planes. */ #define drm_for_each_legacy_plane(plane, planelist) \ list_for_each_entry(plane, planelist, head) \
On Mon, Apr 20, 2015 at 07:22:55PM +0100, Daniel Stone wrote:
Reference-count drm_property_blob objects, changing the API to ref/unref.
Signed-off-by: Daniel Stone daniels@collabora.com
Merged up to this on (except patch 2) to topic/drm-misc.
Thanks, Daniel
drivers/gpu/drm/drm_crtc.c | 164 +++++++++++++++++++++++++++++++++++++++++---- include/drm/drm_crtc.h | 17 ++--- 2 files changed, 159 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9947078..03245fb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -352,7 +352,9 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, if (obj && obj->id != id) obj = NULL; /* don't leak out unref'd fb's */
- if (obj && (obj->type == DRM_MODE_OBJECT_FB))
- if (obj &&
(obj->type == DRM_MODE_OBJECT_FB ||obj->type == DRM_MODE_OBJECT_BLOB))@@ -377,7 +379,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
/* Framebuffers are reference counted and need their own lookup * function.*/
- WARN_ON(type == DRM_MODE_OBJECT_FB);
- WARN_ON(type == DRM_MODE_OBJECT_FB || type == DRM_MODE_OBJECT_BLOB); obj = _object_find(dev, id, type); return obj;
} @@ -4200,7 +4202,7 @@ done: return ret; }
-static struct drm_property_blob * +struct drm_property_blob * drm_property_create_blob(struct drm_device *dev, size_t length, const void *data) { @@ -4215,6 +4217,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, return NULL;
blob->length = length;
blob->dev = dev;
memcpy(blob->data, data, length);
@@ -4227,25 +4230,148 @@ drm_property_create_blob(struct drm_device *dev, size_t length, return NULL; }
kref_init(&blob->refcount);
list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
mutex_unlock(&dev->mode_config.blob_lock);
return blob;
} +EXPORT_SYMBOL(drm_property_create_blob);
-static void drm_property_destroy_blob(struct drm_device *dev,
struct drm_property_blob *blob)+/**
- drm_property_free_blob - Blob property destructor
- Internal free function for blob properties; must not be used directly.
- @param kref Reference
- */
+static void drm_property_free_blob(struct kref *kref) {
- mutex_lock(&dev->mode_config.blob_lock);
- drm_mode_object_put(dev, &blob->base);
- struct drm_property_blob *blob =
container_of(kref, struct drm_property_blob, refcount);- WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock));
- list_del(&blob->head);
- mutex_unlock(&dev->mode_config.blob_lock);
drm_mode_object_put(blob->dev, &blob->base);
kfree(blob);
}
/**
- drm_property_unreference_blob - Unreference a blob property
- Drop a reference on a blob property. May free the object.
- @param dev Device the blob was created on
- @param blob Pointer to blob property
- */
+void drm_property_unreference_blob(struct drm_property_blob *blob) +{
- struct drm_device *dev;
- if (!blob)
return;- dev = blob->dev;
- DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount));
- if (kref_put_mutex(&blob->refcount, drm_property_free_blob,
&dev->mode_config.blob_lock))mutex_unlock(&blob->dev->mode_config.blob_lock);- else
might_lock(&dev->mode_config.blob_lock);+} +EXPORT_SYMBOL(drm_property_unreference_blob);
+/**
- drm_property_unreference_blob_locked - Unreference a blob property with blob_lock held
- Drop a reference on a blob property. May free the object. This must be
- called with blob_lock held.
- @param dev Device the blob was created on
- @param blob Pointer to blob property
- */
+static void drm_property_unreference_blob_locked(struct drm_property_blob *blob) +{
- if (!blob)
return;- DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount));
- kref_put(&blob->refcount, drm_property_free_blob);
+}
+/**
- drm_property_reference_blob - Take a reference on an existing property
- Take a new reference on an existing blob property.
- @param blob Pointer to blob property
- */
+struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob) +{
- DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount));
- kref_get(&blob->refcount);
- return blob;
+} +EXPORT_SYMBOL(drm_property_reference_blob);
+/*
- Like drm_property_lookup_blob, but does not return an additional reference.
- Must be called with blob_lock held.
- */
+static struct drm_property_blob *__drm_property_lookup_blob(struct drm_device *dev,
uint32_t id)+{
- struct drm_mode_object *obj = NULL;
- struct drm_property_blob *blob;
- WARN_ON(!mutex_is_locked(&dev->mode_config.blob_lock));
- mutex_lock(&dev->mode_config.idr_mutex);
- obj = idr_find(&dev->mode_config.crtc_idr, id);
- if (!obj || (obj->type != DRM_MODE_OBJECT_BLOB) || (obj->id != id))
blob = NULL;- else
blob = obj_to_blob(obj);- mutex_unlock(&dev->mode_config.idr_mutex);
- return blob;
+}
+/**
- drm_property_lookup_blob - look up a blob property and take a reference
- @dev: drm device
- @id: id of the blob property
- If successful, this takes an additional reference to the blob property.
- callers need to make sure to eventually unreference the returned property
- again, using @drm_property_unreference_blob.
- */
+struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev,
uint32_t id)+{
- struct drm_property_blob *blob;
- mutex_lock(&dev->mode_config.blob_lock);
- blob = __drm_property_lookup_blob(dev, id);
- if (blob) {
if (!kref_get_unless_zero(&blob->refcount))blob = NULL;- }
- mutex_unlock(&dev->mode_config.blob_lock);
- return blob;
+} +EXPORT_SYMBOL(drm_property_lookup_blob);
+/**
- drm_property_replace_global_blob - atomically replace existing blob property
- @dev: drm device
- @replace: location of blob property pointer to be replaced
@@ -4311,14 +4437,14 @@ static int drm_property_replace_global_blob(struct drm_device *dev, }
if (old_blob)
drm_property_destroy_blob(dev, old_blob);
drm_property_unreference_blob(old_blob);*replace = new_blob;
return 0;
err_created:
- drm_property_destroy_blob(dev, new_blob);
- drm_property_unreference_blob(new_blob); return ret;
}
@@ -4349,7 +4475,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
drm_modeset_lock_all(dev); mutex_lock(&dev->mode_config.blob_lock);
- blob = drm_property_blob_find(dev, out_resp->blob_id);
- blob = __drm_property_lookup_blob(dev, out_resp->blob_id); if (!blob) { ret = -ENOENT; goto done;
@@ -4513,8 +4639,18 @@ bool drm_property_change_valid_get(struct drm_property *property, valid_mask |= (1ULL << property->values[i]); return !(value & ~valid_mask); } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
/* Only the driver knows */return true;
struct drm_property_blob *blob;if (value == 0)return true;blob = drm_property_lookup_blob(property->dev, value);if (blob) {*ref = &blob->base;return true;} else {return false;}@@ -5564,7 +5700,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list, head) {
drm_property_destroy_blob(dev, blob);
drm_property_unreference_blob(blob);}
/*
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 43a3758..07c99f6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -217,6 +217,8 @@ struct drm_framebuffer {
struct drm_property_blob { struct drm_mode_object base;
- struct drm_device *dev;
- struct kref refcount; struct list_head head; size_t length; unsigned char data[];
@@ -1366,6 +1368,13 @@ struct drm_property *drm_property_create_object(struct drm_device *dev, int flags, const char *name, uint32_t type); struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags, const char *name); +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
size_t length,const void *data);+struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev,
uint32_t id);+struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob); +void drm_property_unreference_blob(struct drm_property_blob *blob); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); extern int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name); @@ -1529,14 +1538,6 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev, return mo ? obj_to_property(mo) : NULL; }
-static inline struct drm_property_blob * -drm_property_blob_find(struct drm_device *dev, uint32_t id) -{
- struct drm_mode_object *mo;
- mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_BLOB);
- return mo ? obj_to_blob(mo) : NULL;
-}
/* Plane list iterator for legacy (overlay only) planes. */ #define drm_for_each_legacy_plane(plane, planelist) \ list_for_each_entry(plane, planelist, head) \ -- 2.3.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
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.
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_crtc.c | 160 +++++++++++++++++++++++++++++++++++++++++++- 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 | 22 ++++++ 7 files changed, 199 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 03245fb..4737794 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4216,6 +4216,9 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (!blob) return NULL;
+ /* 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;
@@ -4232,7 +4235,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);
@@ -4254,7 +4258,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); @@ -4308,6 +4313,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. @@ -4497,6 +4522,135 @@ 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 *blob_data; + int ret = 0; + void __user *blob_ptr; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + switch (out_resp->type) { + case DRM_MODE_BLOB_TYPE_MODE: { + if (out_resp->length != sizeof(struct drm_mode_modeinfo)) { + ret = -E2BIG; + goto out_unlocked; + } + break; + } + default: + ret = -EINVAL; + goto out_unlocked; + } + + blob_data = kzalloc(out_resp->length, GFP_USER); + if (!blob_data) { + ret = -ENOMEM; + goto out_unlocked; + } + + blob_ptr = (void __user *)(unsigned long)out_resp->data; + if (copy_from_user(blob_data, blob_ptr, out_resp->length)) { + ret = -EFAULT; + goto out_data; + } + + blob = drm_property_create_blob(dev, out_resp->length, blob_data); + if (!blob) { + ret = -EINVAL; + goto out_data; + } + + /* 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); + +out_data: + kfree(blob_data); +out_unlocked: + 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. @@ -5699,7 +5853,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 076dd60..09846ed 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 */ @@ -408,8 +409,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 62c40777..c5c717e 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 07c99f6..a228614 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -219,7 +219,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[]; }; @@ -1289,6 +1290,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); @@ -1434,6 +1437,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..73ac925 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -558,4 +558,26 @@ struct drm_mode_atomic { __u64 user_data; };
+#define DRM_MODE_BLOB_TYPE_MODE 1 /**< drm_mode_modeinfo */ + +/** + * Create a new 'blob' data property, copying length bytes from data pointer, + * and returning new blob ID. + */ +struct drm_mode_create_blob { + __u32 type; + __u32 length; + /** Pointer to data to create blob. */ + __u64 data; + /** Return: new property ID. */ + __u32 blob_id; +}; + +/** + * Destroy a user-created blob property. + */ +struct drm_mode_destroy_blob { + __u32 blob_id; +}; + #endif
Op 20-04-15 om 20:22 schreef Daniel Stone:
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.
Signed-off-by: Daniel Stone daniels@collabora.com
<snip> +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 *blob_data; + int ret = 0; + void __user *blob_ptr; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + switch (out_resp->type) { + case DRM_MODE_BLOB_TYPE_MODE: { + if (out_resp->length != sizeof(struct drm_mode_modeinfo)) { + ret = -E2BIG;to length
You want -EINVAL here, -E2BIG means "argument list too long".
goto out_unlocked;}break;- }
- default:
ret = -EINVAL;goto out_unlocked;- }
- blob_data = kzalloc(out_resp->length, GFP_USER);
- if (!blob_data) {
ret = -ENOMEM;goto out_unlocked;- }
- blob_ptr = (void __user *)(unsigned long)out_resp->data;
- if (copy_from_user(blob_data, blob_ptr, out_resp->length)) {
ret = -EFAULT;goto out_data;- }
- blob = drm_property_create_blob(dev, out_resp->length, blob_data);
- if (!blob) {
ret = -EINVAL;
drm_property_create_blob can fail from -ENOMEM or -EINVAL here, so correctly return the error from drm_property_create_blob?
You're also doing a double allocation, one for blob_ptr and the other for blob. It might be better to do a single allocation of the blob and copy the data to blob->data directly.
For the whole series if this patch is fixed up: Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
On Mon, Apr 20, 2015 at 07:22:49PM +0100, Daniel Stone wrote:
Hi, This is the spritual successor to the modes-as-blob-properties patchset.
There are some fairly drastic differences though, namely:
- the referenced object in this case is the blob property - being just a dumb chunk of data - rather than attempting to refcount modes; meaning that ...
- userspace doesn't see blob mode IDs from the connector list, only from the current mode, so it really needs to create the mdoes again from the drm_mode_modeinfo it gets
- the mode-constness series has been split out and will be sent separately
Actually exposing the mode property does largely seem to work, but need to fix i915 to not copy CRTC states by value before it does.
This series still retains the concept of a type within the create blob ioctl, on the grounds that otherwise we could allow userspace to create unbounded allocations in the kernel with no attribution. Other than matching size, the type has no meaning per se.
Yeah I'm a bit late, but not sure whether trying to restrict the size is all that useful really. Userspace can still just create a bazillion of them and starve the kernel of memory. And as long as we use kmalloc there'll be a natural limit to how big a blob can be.
I'm bringing this up since if we go with encoding size limits we'll need to add a new type of blob for each use, and with gamma tables, csc matrices and other stuff there will be lots of them. -Daniel
Hi,
On Thursday, May 7, 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Apr 20, 2015 at 07:22:49PM +0100, Daniel Stone wrote:
This is the spritual successor to the modes-as-blob-properties patchset.
There are some fairly drastic differences though, namely:
- the referenced object in this case is the blob property - being just a dumb chunk of data - rather than attempting to refcount modes; meaning that ...
- userspace doesn't see blob mode IDs from the connector list, only from the current mode, so it really needs to create the mdoes again from the drm_mode_modeinfo it gets
- the mode-constness series has been split out and will be sent separately
Actually exposing the mode property does largely seem to work, but need to fix i915 to not copy CRTC states by value before it does.
This series still retains the concept of a type within the create blob ioctl, on the grounds that otherwise we could allow userspace to create unbounded allocations in the kernel with no attribution. Other than matching size, the type has no meaning per se.
Yeah I'm a bit late, but not sure whether trying to restrict the size is all that useful really. Userspace can still just create a bazillion of them and starve the kernel of memory. And as long as we use kmalloc there'll be a natural limit to how big a blob can be.
I'm bringing this up since if we go with encoding size limits we'll need to add a new type of blob for each use, and with gamma tables, csc matrices and other stuff there will be lots of them.
Fair enough. My thinking is that it would be easier to catch a userspace in the act of gradually starving memory with a billion ioctls than all in one big go, but see the argument.
Will drop that (nearly resolving Maarten's -E2BIG correction), and also dig out the single-alloc patch I had earlier, but must've lost during rebase.
Cheers, Daniel
Hi, With most of the supporting series already merged into drm-misc, this is a resend after Maarten and Daniel's review of the core creation ioctl.
Notably, size/type validation is now gone, as well as the double allocation. Errors from blob creation are also propagated down through ERR_PTR.
Cheers, Daniel
672cb1d6ae mistakenly added an extra parameter to the kerneldoc for drm_property_unreference_blob which wasn't actually present.
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_crtc.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4f922aa..6f3cafb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4267,7 +4267,6 @@ static void drm_property_free_blob(struct kref *kref) * * Drop a reference on a blob property. May free the object. * - * @param dev Device the blob was created on * @param blob Pointer to blob property */ void drm_property_unreference_blob(struct drm_property_blob *blob)
On Sat, May 09, 2015 at 03:52:10PM +0100, Daniel Stone wrote:
672cb1d6ae mistakenly added an extra parameter to the kerneldoc for drm_property_unreference_blob which wasn't actually present.
Signed-off-by: Daniel Stone daniels@collabora.com
drivers/gpu/drm/drm_crtc.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4f922aa..6f3cafb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4267,7 +4267,6 @@ static void drm_property_free_blob(struct kref *kref)
- Drop a reference on a blob property. May free the object.
- @param dev Device the blob was created on
Same issue with drm_property_unreference_blob_locked. Also this isn't kerneldoc syntax, the proper one is
* @blob: Pointer to blob property
$ make htmldocs complains about a bunch of your recently added kerneldoc still. I've squashed this one in, but can you pls follow up with more fixups (and respin the later patches in this series too)?
Thanks, Daniel
- @param blob Pointer to blob property
*/
void drm_property_unreference_blob(struct drm_property_blob *blob)
2.4.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On Monday, May 11, 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Sat, May 09, 2015 at 03:52:10PM +0100, Daniel Stone wrote:
672cb1d6ae mistakenly added an extra parameter to the kerneldoc for drm_property_unreference_blob which wasn't actually present.
Signed-off-by: Daniel Stone <daniels@collabora.com javascript:;>
drivers/gpu/drm/drm_crtc.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4f922aa..6f3cafb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4267,7 +4267,6 @@ static void drm_property_free_blob(struct kref
*kref)
- Drop a reference on a blob property. May free the object.
- @param dev Device the blob was created on
Same issue with drm_property_unreference_blob_locked. Also this isn't kerneldoc syntax, the proper one is
- @blob: Pointer to blob property
$ make htmldocs complains about a bunch of your recently added kerneldoc still. I've squashed this one in, but can you pls follow up with more fixups (and respin the later patches in this series too)?
Augh, had left my editor in Doxygen mode, and hadn't noticed thanks to the 802.11 doc breakage masking everything else.
New branch is up at git://git.collabora.co.uk/git/user/daniels/linux wip/4.1.x/drm-blob-props - will send it out to the list when I get in tomorrow morning. Got sidetracked fixing up more unify-flip-modeset issues instead :\
Cheers, Daniel
Make the data parameter to drm_property_create_blob optional; if omitted, the copy will be skipped and the data will be empty.
Signed-off-by: Daniel Stone daniels@collabora.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@intel.com --- drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 6f3cafb..f7b075b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4204,6 +4204,16 @@ done: return ret; }
+/** + * drm_property_create_blob - Create new blob property + * + * Creates a new blob property for a specified DRM device, optionally + * copying data. + * + * @param dev DRM device to create property for + * @param length Length to allocate for blob data + * @param data If specified, copies data into blob + */ struct drm_property_blob * drm_property_create_blob(struct drm_device *dev, size_t length, const void *data) @@ -4211,7 +4221,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, struct drm_property_blob *blob; int ret;
- if (!length || !data) + if (!length) return NULL;
blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); @@ -4221,7 +4231,8 @@ drm_property_create_blob(struct drm_device *dev, size_t length, blob->length = length; blob->dev = dev;
- memcpy(blob->data, data, length); + if (data) + memcpy(blob->data, data, length);
mutex_lock(&dev->mode_config.blob_lock);
Change drm_property_create_blob to return an ERR_PTR-encoded error on failure, so we can pass the failure reason down.
Signed-off-by: Daniel Stone daniels@collabora.com Cc: Maarten Lankhorst maarten.lankhorst@intel.com --- drivers/gpu/drm/drm_crtc.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f7b075b..df4a3fb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4213,6 +4213,10 @@ done: * @param dev DRM device to create property for * @param length Length to allocate for blob data * @param data If specified, copies data into blob + * + * Returns: + * New blob property with a single reference on success, or an ERR_PTR + * value on failure. */ struct drm_property_blob * drm_property_create_blob(struct drm_device *dev, size_t length, @@ -4222,11 +4226,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length, int ret;
if (!length) - return NULL; + return ERR_PTR(-EINVAL);
blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); if (!blob) - return NULL; + return ERR_PTR(-ENOMEM);
blob->length = length; blob->dev = dev; @@ -4240,7 +4244,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (ret) { kfree(blob); mutex_unlock(&dev->mode_config.blob_lock); - return NULL; + return ERR_PTR(-EINVAL); }
kref_init(&blob->refcount); @@ -4430,8 +4434,8 @@ static int drm_property_replace_global_blob(struct drm_device *dev,
if (length && data) { new_blob = drm_property_create_blob(dev, length, data); - if (!new_blob) - return -EINVAL; + if (IS_ERR(new_blob)) + return PTR_ERR(new_blob); }
/* This does not need to be synchronised with blob_lock, as the
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@collabora.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@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; + + 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
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@collabora.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@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) {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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On Monday, May 11, 2015, Daniel Vetter daniel@ffwll.ch wrote:
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.
+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?
I haven't actually got to that yet - spent today dragging back up to Maarten's branch and fixing a couple of new issues there. This all works now with my fairly rudimentary test, so I'll reshuffle the Weston patches as quickly as possible tomorrow and give it a spin.
Cheers, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Daniel Stone -
Daniel Vetter -
Maarten Lankhorst