drm: store property instead of id in obj attachment

Keep property pointer, instead of id, in per mode-object attachments. This will simplify things in later patches.

---
Does anyone care about the lifetime issue and dangling property ptrs?

Currently properties are destroyed during drm_mode_config_cleanup(), so it doesn't really matter.. if someone *did* see a use for dynamically destroying properties we'd have to do something a bit more clever and handle properly detaching from any mode object to which that property had been attached.

Just sending as an RFC for now, but later atomic properties/ioctl patches will depend on this.

 drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
 include/drm/drm_crtc.h     |  7 ++++++-
 2 files changed, 14 insertions(+), 10 deletions(-)

drivers/gpu/drm/drm_crtc.c +8 -9

index 3ba84df..3e6ef39 100644
@@ -2058,12 +2058,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
                prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
                prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
                for (i = 0; i < connector->properties.count; i++) {
-                       if (put_user(connector->properties.ids[i],
-                                    prop_ptr + copied)) {
+                       struct drm_property *prop = connector->properties.properties[i];
+                       if (put_user(prop->base.id, prop_ptr + copied)) {
                                ret = -EFAULT;
                                goto out;
                        }
-
                        if (put_user(connector->properties.values[i],
                                     prop_values + copied)) {
                                ret = -EFAULT;

@@ -3744,7 +3743,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
                return;
        }
 
-       obj->properties->ids[count] = property->base.id;
+       obj->properties->properties[count] = property;
        obj->properties->values[count] = init_val;
        obj->properties->count++;
 }

@@ -3769,7 +3768,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
        int i;
 
        for (i = 0; i < obj->properties->count; i++) {
-               if (obj->properties->ids[i] == property->base.id) {
+               if (obj->properties->properties[i] == property) {
                        obj->properties->values[i] = val;
                        return 0;
                }

@@ -3799,7 +3798,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
        int i;
 
        for (i = 0; i < obj->properties->count; i++) {
-               if (obj->properties->ids[i] == property->base.id) {
+               if (obj->properties->properties[i] == property) {
                        *val = obj->properties->values[i];
                        return 0;
                }

@@ -4263,8 +4262,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
                prop_values_ptr = (uint64_t __user *)(unsigned long)
                                  (arg->prop_values_ptr);
                for (i = 0; i < props_count; i++) {
-                       if (put_user(obj->properties->ids[i],
-                                    props_ptr + copied)) {
+                       struct drm_property *prop = obj->properties->properties[i];
+                       if (put_user(prop->base.id, props_ptr + copied)) {
                                ret = -EFAULT;
                                goto out;
                        }

@@ -4322,7 +4321,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
                goto out;
 
        for (i = 0; i < arg_obj->properties->count; i++)
-               if (arg_obj->properties->ids[i] == arg->prop_id)
+               if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
                        break;
 
        if (i == arg_obj->properties->count)

include/drm/drm_crtc.h +6 -2

index dd2c16e..017029d 100644
@@ -64,7 +64,12 @@ struct drm_mode_object {
 #define DRM_OBJECT_MAX_PROPERTY 24
 struct drm_object_properties {
        int count;
-       uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
+       /* NOTE: if we ever start dynamically destroying properties (ie.
+        * not at drm_mode_config_cleanup() time), then we'd have to do
+        * a better job of detaching property from mode objects to avoid
+        * dangling property pointers:
+        */
+       struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
        uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 
--

1.9.3

Quoted 3 lines · Rob Clark

Keep property pointer, instead of id, in per mode-object attachments. This will simplify things in later patches.

Quoted 2 lines

---
Does anyone care about the lifetime issue and dangling property ptrs?

Judging by the lack of response, it seems like the answer is probably no.

That said, it doesn't seem like a whole lot of work to add refcounting to drm_property so we don't have to worry about it. Maybe I'm missing something.

Sean

Quoted 97 lines

Currently properties are destroyed during drm_mode_config_cleanup(), so it doesn't really matter.. if someone *did* see a use for dynamically destroying properties we'd have to do something a bit more clever and handle properly detaching from any mode object to which that property had been attached.

Just sending as an RFC for now, but later atomic properties/ioctl patches will depend on this.

 drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
 include/drm/drm_crtc.h     |  7 ++++++-
 2 files changed, 14 insertions(+), 10 deletions(-)

drivers/gpu/drm/drm_crtc.c +8 -9

index 3ba84df..3e6ef39 100644
@@ -2058,12 +2058,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
                prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
                prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
                for (i = 0; i < connector->properties.count; i++) {
-                       if (put_user(connector->properties.ids[i],
-                                    prop_ptr + copied)) {
+                       struct drm_property *prop = connector->properties.properties[i];
+                       if (put_user(prop->base.id, prop_ptr + copied)) {
                                ret = -EFAULT;
                                goto out;
                        }
-
                        if (put_user(connector->properties.values[i],
                                     prop_values + copied)) {
                                ret = -EFAULT;

@@ -3744,7 +3743,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
                return;
        }
 
-       obj->properties->ids[count] = property->base.id;
+       obj->properties->properties[count] = property;
        obj->properties->values[count] = init_val;
        obj->properties->count++;
 }

@@ -3769,7 +3768,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
        int i;
 
        for (i = 0; i < obj->properties->count; i++) {
-               if (obj->properties->ids[i] == property->base.id) {
+               if (obj->properties->properties[i] == property) {
                        obj->properties->values[i] = val;
                        return 0;
                }

@@ -3799,7 +3798,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
        int i;
 
        for (i = 0; i < obj->properties->count; i++) {
-               if (obj->properties->ids[i] == property->base.id) {
+               if (obj->properties->properties[i] == property) {
                        *val = obj->properties->values[i];
                        return 0;
                }

@@ -4263,8 +4262,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
                prop_values_ptr = (uint64_t __user *)(unsigned long)
                                  (arg->prop_values_ptr);
                for (i = 0; i < props_count; i++) {
-                       if (put_user(obj->properties->ids[i],
-                                    props_ptr + copied)) {
+                       struct drm_property *prop = obj->properties->properties[i];
+                       if (put_user(prop->base.id, props_ptr + copied)) {
                                ret = -EFAULT;
                                goto out;
                        }

@@ -4322,7 +4321,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
                goto out;
 
        for (i = 0; i < arg_obj->properties->count; i++)
-               if (arg_obj->properties->ids[i] == arg->prop_id)
+               if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
                        break;
 
        if (i == arg_obj->properties->count)

include/drm/drm_crtc.h +6 -1

index dd2c16e..017029d 100644
@@ -64,7 +64,12 @@ struct drm_mode_object {
 #define DRM_OBJECT_MAX_PROPERTY 24
 struct drm_object_properties {
        int count;
-       uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
+       /* NOTE: if we ever start dynamically destroying properties (ie.
+        * not at drm_mode_config_cleanup() time), then we'd have to do
+        * a better job of detaching property from mode objects to avoid
+        * dangling property pointers:
+        */
+       struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
        uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 

Quoted 3 lines

--
1.9.3

Quoted 4 lines · Sean Paul

Quoted 3 lines · Rob Clark

Keep property pointer, instead of id, in per mode-object attachments. This will simplify things in later patches.

Quoted 8 lines

Quoted 2 lines

---
Does anyone care about the lifetime issue and dangling property ptrs?

Judging by the lack of response, it seems like the answer is probably no.

That said, it doesn't seem like a whole lot of work to add refcounting to drm_property so we don't have to worry about it. Maybe I'm missing something.

true.. but then the property would not actually go away then, it would be kept alive by the extra ref. That said, I can't think of a valid reason for dynamically creating/destroying properties, so I'm tempted to just add a comment somewhere to explain that detaching from mode objects would be required if someone did want to dynamically destroy a property and leave it at that..