Re: [PATCH 04/17] drm: add object property type

From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 04/17] drm: add object property type
Date: Mon, 26 May 2014 10:29:47 +0200	[thread overview]
Message-ID: <20140526082947.GX14357@phenom.ffwll.local> (raw)
In-Reply-To: <1400956226-28053-5-git-send-email-robdclark@gmail.com>

On Sat, May 24, 2014 at 02:30:13PM -0400, Rob Clark wrote:
> An object property is an id (idr) for a drm mode object.  This
> will allow a property to be used set/get a framebuffer, CRTC, etc.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 60 +++++++++++++++++++++++++++++++++++----------
>  include/drm/drm_crtc.h      | 27 ++++++++++++++++++++
>  include/uapi/drm/drm_mode.h | 14 +++++++++++
>  3 files changed, 88 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index dd10e4c..c049ba7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3142,6 +3142,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
>  	if (!property)
>  		return NULL;
>  
> +	property->dev = dev;
> +
>  	if (num_values) {
>  		property->values = kzalloc(sizeof(uint64_t)*num_values, GFP_KERNEL);
>  		if (!property->values)
> @@ -3162,6 +3164,9 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
>  	}
>  
>  	list_add_tail(&property->head, &dev->mode_config.property_list);
> +
> +	BUG_ON(!drm_property_type_valid(property));
> +
>  	return property;
>  fail:
>  	kfree(property->values);
> @@ -3299,6 +3304,23 @@ struct drm_property *drm_property_create_range(struct drm_device *dev, int flags
>  }
>  EXPORT_SYMBOL(drm_property_create_range);
>  
> +struct drm_property *drm_property_create_object(struct drm_device *dev,
> +					 int flags, const char *name, uint32_t type)
> +{
> +	struct drm_property *property;
> +
> +	flags |= DRM_MODE_PROP_OBJECT;
> +
> +	property = drm_property_create(dev, flags, name, 1);
> +	if (!property)
> +		return NULL;
> +
> +	property->values[0] = type;
> +
> +	return property;
> +}
> +EXPORT_SYMBOL(drm_property_create_object);
> +
>  /**
>   * drm_property_add_enum - add a possible value to an enumeration property
>   * @property: enumeration property to change
> @@ -3319,14 +3341,16 @@ int drm_property_add_enum(struct drm_property *property, int index,
>  {
>  	struct drm_property_enum *prop_enum;
>  
> -	if (!(property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)))
> +	if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> +			drm_property_type_is(property, DRM_MODE_PROP_BITMASK)))
>  		return -EINVAL;
>  
>  	/*
>  	 * Bitmask enum properties have the additional constraint of values
>  	 * from 0 to 63
>  	 */
> -	if ((property->flags & DRM_MODE_PROP_BITMASK) && (value > 63))
> +	if (drm_property_type_is(property, DRM_MODE_PROP_BITMASK) &&
> +			(value > 63))
>  		return -EINVAL;
>  
>  	if (!list_empty(&property->enum_blob_list)) {
> @@ -3509,10 +3533,11 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>  	}
>  	property = obj_to_property(obj);
>  
> -	if (property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) {
> +	if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> +			drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
>  		list_for_each_entry(prop_enum, &property->enum_blob_list, head)
>  			enum_count++;
> -	} else if (property->flags & DRM_MODE_PROP_BLOB) {
> +	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
>  		list_for_each_entry(prop_blob, &property->enum_blob_list, head)
>  			blob_count++;
>  	}
> @@ -3534,7 +3559,8 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>  	}
>  	out_resp->count_values = value_count;
>  
> -	if (property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) {
> +	if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> +			drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
>  		if ((out_resp->count_enum_blobs >= enum_count) && enum_count) {
>  			copied = 0;
>  			enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr;
> @@ -3556,7 +3582,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>  		out_resp->count_enum_blobs = enum_count;
>  	}
>  
> -	if (property->flags & DRM_MODE_PROP_BLOB) {
> +	if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
>  		if ((out_resp->count_enum_blobs >= blob_count) && blob_count) {
>  			copied = 0;
>  			blob_id_ptr = (uint32_t __user *)(unsigned long)out_resp->enum_blob_ptr;
> @@ -3712,19 +3738,25 @@ static bool drm_property_change_is_valid(struct drm_property *property,
>  {
>  	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
>  		return false;
> -	if (property->flags & DRM_MODE_PROP_RANGE) {
> +
> +	if (drm_property_type_is(property, DRM_MODE_PROP_RANGE)) {
>  		if (value < property->values[0] || value > property->values[1])
>  			return false;
>  		return true;
> -	} else if (property->flags & DRM_MODE_PROP_BITMASK) {
> +	} else if (drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
>  		int i;
>  		uint64_t valid_mask = 0;
>  		for (i = 0; i < property->num_values; i++)
>  			valid_mask |= (1ULL << property->values[i]);
>  		return !(value & ~valid_mask);
> -	} else if (property->flags & DRM_MODE_PROP_BLOB) {
> +	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
>  		/* Only the driver knows */
>  		return true;
> +	} else if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
> +		/* a zero value for an object property translates to null: */
> +		if (value == 0)
> +			return true;
> +		return drm_property_get_obj(property, value) != NULL;

Ok, so this usage is indeed safe for framebuffers since you only compare
the pointer against NULL. But you can't ever access this pointer since it
might disappear any time after you've dropped the idr lock. I think what
you actually want here is a new interface drm_mode_object_exists which
just returns a bool.

That won't expose any risky things to callers and we can still keep the
restriction that calling drm_mode_object_get for framebuffers is a BUG.
-Daniel

>  	} else {
>  		int i;
>  		for (i = 0; i < property->num_values; i++)
> @@ -3815,9 +3847,9 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  	return ret;
>  }
>  
> -static int drm_mode_set_obj_prop(struct drm_device *dev,
> -		struct drm_mode_object *obj, struct drm_atomic_state *state,
> -		struct drm_property *property, uint64_t value, void *blob_data)
> +static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
> +		struct drm_atomic_state *state, struct drm_property *property,
> +		uint64_t value, void *blob_data)
>  {
>  	if (drm_property_change_is_valid(property, value)) {
>  		switch (obj->type) {
> @@ -3831,6 +3863,8 @@ static int drm_mode_set_obj_prop(struct drm_device *dev,
>  			return drm_mode_plane_set_obj_prop(obj_to_plane(obj),
>  					state, property, value, blob_data);
>  		}
> +	} else {
> +		DRM_DEBUG("invalid value: %s = %llx\n", property->name, value);
>  	}
>  
>  	return -EINVAL;
> @@ -3866,7 +3900,7 @@ static int drm_mode_set_obj_prop_id(struct drm_device *dev,
>  		return -ENOENT;
>  	property = obj_to_property(prop_obj);
>  
> -	return drm_mode_set_obj_prop(dev, arg_obj, state, property,
> +	return drm_mode_set_obj_prop(arg_obj, state, property, 
>  			value, blob_data);
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 83e0f88..65bfb8b 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -191,6 +191,7 @@ struct drm_property {
>  	char name[DRM_PROP_NAME_LEN];
>  	uint32_t num_values;
>  	uint64_t *values;
> +	struct drm_device *dev;
>  
>  	struct list_head enum_blob_list;
>  };
> @@ -941,6 +942,23 @@ extern void drm_mode_config_cleanup(struct drm_device *dev);
>  
>  extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  						struct edid *edid);
> +
> +static inline bool drm_property_type_is(struct drm_property *property,
> +		uint32_t type)
> +{
> +	/* instanceof for props.. handles extended type vs original types: */
> +	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
> +		return (property->flags & DRM_MODE_PROP_EXTENDED_TYPE) == type;
> +	return property->flags & type;
> +}
> +
> +static inline bool drm_property_type_valid(struct drm_property *property)
> +{
> +	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
> +		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
> +	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
> +}
> +
>  extern int drm_object_property_set_value(struct drm_mode_object *obj,
>  					 struct drm_property *property,
>  					 uint64_t val);
> @@ -974,6 +992,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
>  struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
>  					 const char *name,
>  					 uint64_t min, uint64_t max);
> +struct drm_property *drm_property_create_object(struct drm_device *dev,
> +					 int flags, const char *name, uint32_t type);
>  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);
> @@ -990,6 +1010,13 @@ extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
>  					 int gamma_size);
>  extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  		uint32_t id, uint32_t type);
> +
> +static inline struct drm_mode_object *
> +drm_property_get_obj(struct drm_property *property, uint64_t value)
> +{
> +	return drm_mode_object_find(property->dev, value, property->values[0]);
> +}
> +
>  /* IOCTLs */
>  extern int drm_mode_getresources(struct drm_device *dev,
>  				 void *data, struct drm_file *file_priv);
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 12e2139..516425d 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -251,6 +251,20 @@ struct drm_mode_get_connector {
>  #define DRM_MODE_PROP_BLOB	(1<<4)
>  #define DRM_MODE_PROP_BITMASK	(1<<5) /* bitmask of enumerated types */
>  
> +/* non-extended types: legacy bitmask, one bit per type: */
> +#define DRM_MODE_PROP_LEGACY_TYPE  ( \
> +		DRM_MODE_PROP_RANGE | \
> +		DRM_MODE_PROP_ENUM | \
> +		DRM_MODE_PROP_BLOB | \
> +		DRM_MODE_PROP_BITMASK)
> +
> +/* extended-types: rather than continue to consume a bit per type,
> + * grab a chunk of the bits to use as integer type id.
> + */
> +#define DRM_MODE_PROP_EXTENDED_TYPE	0x0000ffc0
> +#define DRM_MODE_PROP_TYPE(n)		((n) << 6)
> +#define DRM_MODE_PROP_OBJECT		DRM_MODE_PROP_TYPE(1)
> +
>  struct drm_mode_property_enum {
>  	__u64 value;
>  	char name[DRM_PROP_NAME_LEN];
> -- 
> 1.9.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch