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

[Sean Paul]

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.

 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
>