[RFCv1,02/12] drm: add object property type

Commit Message

Rob Clark Oct. 6, 2013, 12:45 a.m. UTC

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.
---
 drivers/gpu/drm/drm_crtc.c  | 34 ++++++++++++++++++++++++++++++----
 include/drm/drm_crtc.h      | 10 ++++++++++
 include/uapi/drm/drm_mode.h |  1 +
 3 files changed, 41 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Oct. 7, 2013, 1:43 p.m. UTC | #1

On Sat, Oct 05, 2013 at 08:45:40PM -0400, Rob Clark wrote:
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 5508117..35921ba 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -231,6 +231,7 @@ struct drm_mode_get_connector {
>  #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
>  #define DRM_MODE_PROP_BLOB	(1<<4)
>  #define DRM_MODE_PROP_BITMASK	(1<<5) /* bitmask of enumerated types */
> +#define DRM_MODE_PROP_OBJECT	(1<<6) /* drm mode object */

This way to using up one bit for each type sucks big time. IIRC we
discussed this at Fosdem and one idea was to leave the current bits as
sort of base types, and reserve a bunch of the other bits to indicate a
sub-type. For instance the new signed range and object ID prop types
could be sub-types of the current range type.

Maybe we should reserve a few more bits for new base types in case we
need them in the future, or just add sometime king DRM_MODE_PROP_MISC,
which is where we'd stick every sub-type that doesn't fit the current
base types.

Rob Clark Oct. 7, 2013, 2:44 p.m. UTC | #2

On Mon, Oct 7, 2013 at 9:43 AM, Ville Syrjälä
<[e-mail]> wrote:
> On Sat, Oct 05, 2013 at 08:45:40PM -0400, Rob Clark wrote:
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 5508117..35921ba 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -231,6 +231,7 @@ struct drm_mode_get_connector {
>>  #define DRM_MODE_PROP_ENUM   (1<<3) /* enumerated type with text strings */
>>  #define DRM_MODE_PROP_BLOB   (1<<4)
>>  #define DRM_MODE_PROP_BITMASK        (1<<5) /* bitmask of enumerated types */
>> +#define DRM_MODE_PROP_OBJECT (1<<6) /* drm mode object */
>
> This way to using up one bit for each type sucks big time. IIRC we
> discussed this at Fosdem and one idea was to leave the current bits as
> sort of base types, and reserve a bunch of the other bits to indicate a
> sub-type. For instance the new signed range and object ID prop types
> could be sub-types of the current range type.

oh, right..  current object-prop is just a straight rebase of the
original patch, so I didn't change this yet.

We probably don't want to use sub-type in cases where old userspace
could misinterpret things, so not sure about signed-range to be a
sub-type of range (if that is what you meant)... but maybe a good idea
for PROP_MISC + PROP_SUBTYPE_OBJECT.  Otoh, at the moment, the only
other prop type I have in mind is ARRAY (although not sure if we can
do single PROP_ARRAY or if we end up needing PROP_ARRAY_foo, for
everything we might want an array of..)

BR,
-R

> Maybe we should reserve a few more bits for new base types in case we
> need them in the future, or just add sometime king DRM_MODE_PROP_MISC,
> which is where we'd stick every sub-type that doesn't fit the current
> base types.
>
> --
> Ville Syrjälä
> Intel OTC

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f2a6d72..471cf16 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2776,6 +2776,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)
@@ -2879,6 +2881,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);
+
 int drm_property_add_enum(struct drm_property *property, int index,
 			  uint64_t value, const char *name)
 {
@@ -3207,6 +3226,11 @@  static bool drm_property_change_is_valid(struct drm_property *property,
 	} else if (property->flags & DRM_MODE_PROP_BLOB) {
 		/* Only the driver knows */
 		return true;
+	} else if (property->flags & 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;
 	} else {
 		int i;
 		for (i = 0; i < property->num_values; i++)
@@ -3283,9 +3307,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, void *state,
-		struct drm_property *property, uint64_t value, void *blob_data)
+static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
+		void *state, struct drm_property *property, 
+		uint64_t value, void *blob_data)
 {
 	if (drm_property_change_is_valid(property, value)) {
 		switch (obj->type) {
@@ -3299,6 +3323,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;
@@ -3333,7 +3359,7 @@  static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
 		return -EINVAL;
 	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 ad4e1ce..77c8f11 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -297,6 +297,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;
 };
@@ -1030,6 +1031,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);
@@ -1048,6 +1051,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 5508117..35921ba 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -231,6 +231,7 @@  struct drm_mode_get_connector {
 #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
 #define DRM_MODE_PROP_BLOB	(1<<4)
 #define DRM_MODE_PROP_BITMASK	(1<<5) /* bitmask of enumerated types */
+#define DRM_MODE_PROP_OBJECT	(1<<6) /* drm mode object */
 
 struct drm_mode_property_enum {
 	__u64 value;