diff mbox

[6/7] drm: Add reference counting to blob properties

Message ID 1429554176-9865-7-git-send-email-daniels@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Stone April 20, 2015, 6:22 p.m. UTC 
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(-)

Comments

Daniel Vetter May 7, 2015, 9:14 a.m. UTC | #1 
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))
>  		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) \
> -- 
> 2.3.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

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) \