[RFC] drm: store property instead of id in obj attachment

[Rob Clark]

On Tue, Dec 2, 2014 at 1:47 PM, Sean Paul <seanpaul at chromium.org> wrote:
> On Wed, Nov 26, 2014 at 4:19 PM, Rob Clark <robdclark at gmail.com> wrote:
>> 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?
>
> 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..

BR,
-R

>  Sean
>
>
>> 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(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 3ba84df..3e6ef39 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -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)
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index dd2c16e..017029d 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -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
>>