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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 26, 2014 at 4:29 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> ---
>>  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.

Hmm, I think I'll just drop drm_property_get_obj() and call directly
_object_find() here, I think.  Unless we come up with other spots that
need a drm_mode_object_exists().  That would avoid potential future
misunderstandings.

BR,
-R

> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux