| Message ID | 1401321445-31906-2-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, May 29, 2014 at 1:57 AM, Rob Clark <robdclark@gmail.com> 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@gmail.com> > --- > drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++++++++++++++++++++++-------- > include/drm/drm_crtc.h | 27 ++++++++++++++++++++++++ > include/uapi/drm/drm_mode.h | 14 +++++++++++++ > 3 files changed, 82 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 37a3e07..61ddc66 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3117,6 +3117,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, > if (!property) > return NULL; > > + property->dev = dev; > + Ugh.. embarrassing. > if (num_values) { > property->values = kzalloc(sizeof(uint64_t)*num_values, GFP_KERNEL); > if (!property->values) > @@ -3137,6 +3139,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)); WARN_ON(). There is no reason to panic.. I mean, it's obviously a bug, but we can continue just normally. > + > return property; > fail: > kfree(property->values); > @@ -3274,6 +3279,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 > @@ -3294,14 +3316,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)) { > @@ -3484,10 +3508,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++; > } > @@ -3509,7 +3534,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; > @@ -3531,7 +3557,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; > @@ -3687,19 +3713,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; > } else { > int i; > for (i = 0; i < property->num_values; i++) > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 5c1c31c..c1c243f 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -190,6 +190,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; > }; > @@ -931,6 +932,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); > +} I really don't get your naming scheme. I mean, both objects operate on "drm_property" objects, so the drm_property_* prefixes looks good. But as suffix, I'd expect a properly named function, so something like "drm_property_is_type()" or "*_is_of_type()" and "drm_property_is_valid_type()". Your naming-scheme looks more like these functions work on "drm_property_type" objects (which they don't). Just saying.. no reason to argue about taste here. > + > extern int drm_object_property_set_value(struct drm_mode_object *obj, > struct drm_property *property, > uint64_t val); > @@ -964,6 +982,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); > @@ -980,6 +1000,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 f104c26..5b530c7 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) > + Oh god, this is ugly. But I get your intention. Using bit-masks for types wasn't smart at all, and mixing it with flags even worse. But ok. With or without my nitpicks fixed, this is: Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Thanks David > struct drm_mode_property_enum { > __u64 value; > char name[DRM_PROP_NAME_LEN]; > -- > 1.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 29, 2014 at 4:01 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Thu, May 29, 2014 at 1:57 AM, Rob Clark <robdclark@gmail.com> 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@gmail.com> >> --- >> drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++++++++++++++++++++++-------- >> include/drm/drm_crtc.h | 27 ++++++++++++++++++++++++ >> include/uapi/drm/drm_mode.h | 14 +++++++++++++ >> 3 files changed, 82 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 37a3e07..61ddc66 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -3117,6 +3117,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, >> if (!property) >> return NULL; >> >> + property->dev = dev; >> + > > Ugh.. embarrassing. ?? >> if (num_values) { >> property->values = kzalloc(sizeof(uint64_t)*num_values, GFP_KERNEL); >> if (!property->values) >> @@ -3137,6 +3139,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)); > > WARN_ON(). There is no reason to panic.. I mean, it's obviously a bug, > but we can continue just normally. hmm, yeah, I keep forgetting not everyone has debug uart :-P >> + >> return property; >> fail: >> kfree(property->values); >> @@ -3274,6 +3279,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 >> @@ -3294,14 +3316,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)) { >> @@ -3484,10 +3508,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++; >> } >> @@ -3509,7 +3534,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; >> @@ -3531,7 +3557,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; >> @@ -3687,19 +3713,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; >> } else { >> int i; >> for (i = 0; i < property->num_values; i++) >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 5c1c31c..c1c243f 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -190,6 +190,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; >> }; >> @@ -931,6 +932,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); >> +} > > I really don't get your naming scheme. I mean, both objects operate on > "drm_property" objects, so the drm_property_* prefixes looks good. But > as suffix, I'd expect a properly named function, so something like > "drm_property_is_type()" or "*_is_of_type()" and > "drm_property_is_valid_type()". Your naming-scheme looks more like > these functions work on "drm_property_type" objects (which they > don't). _is_type() would have been kinda funny looking sitting next to _type_is() ;-) basically the name is _type_is_valid() with the _is drop to shorten it because my fingers already have too much to type > Just saying.. no reason to argue about taste here. > >> + >> extern int drm_object_property_set_value(struct drm_mode_object *obj, >> struct drm_property *property, >> uint64_t val); >> @@ -964,6 +982,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); >> @@ -980,6 +1000,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 f104c26..5b530c7 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) >> + > > Oh god, this is ugly. But I get your intention. Using bit-masks for > types wasn't smart at all, and mixing it with flags even worse. But > ok. hurray of keeping backwards compat w/ userspace! If it weren't part of the ABI we could do something much cleaner. I tried to wrap the uglyness up into those inline helpers as much as possible. But we kinda painted ourselves into a corner on that one with the existing ABI. BR, -R > > With or without my nitpicks fixed, this is: > > Reviewed-by: David Herrmann <dh.herrmann@gmail.com> > > Thanks > David > >> struct drm_mode_property_enum { >> __u64 value; >> char name[DRM_PROP_NAME_LEN]; >> -- >> 1.9.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 28, 2014 at 07:57:18PM -0400, Rob Clark wrote: [...] > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c [...] > +struct drm_property *drm_property_create_object(struct drm_device *dev, > + int flags, const char *name, uint32_t type) Indentation here is inconsistent with other functions in this file. > @@ -3294,14 +3316,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; The bulk of this patch seems to be this pattern of substituting explicit flag checks with drm_property_type_is() and that kind of hides the real purpose of this patch. Splitting that into a separate patch would make the addition of drm_property_create_object() more concise. > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h [...] > @@ -964,6 +982,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); > @@ -980,6 +1000,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) Perhaps for consistent naming with drm_property_create_object() this would be better called drm_property_get_object()? > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h [...] > +/* 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) Ugh, that's an unfortunate bit of userspace ABI... Could this perhaps be done in a more unified, yet backwards-compatible way? For example if we define legacy properties like this: #define DRM_MODE_PROP_TYPE(n) ((n) << 0) #define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE( 2) #define DRM_MODE_PROP_ENUM DRM_MODE_PROP_TYPE( 8) #define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE(16) #define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE(32) And define the type mask to be: #define DRM_MODE_PROP_TYPE 0x0000fffa Leaving out only the two real flags (PENDING and IMMUTABLE). This should allow things to be done without constantly having to special case legacy vs. extended types. It leaves a bunch of wholes in the number space and we need to be careful to introduce new types only in the upper range, but it has the advantage of not requiring special handling. Thierry
On Fri, May 30, 2014 at 3:57 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, May 28, 2014 at 07:57:18PM -0400, Rob Clark wrote: > [...] >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > [...] >> +struct drm_property *drm_property_create_object(struct drm_device *dev, >> + int flags, const char *name, uint32_t type) > > Indentation here is inconsistent with other functions in this file. > >> @@ -3294,14 +3316,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; > > The bulk of this patch seems to be this pattern of substituting explicit > flag checks with drm_property_type_is() and that kind of hides the real > purpose of this patch. Splitting that into a separate patch would make > the addition of drm_property_create_object() more concise. > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > [...] >> @@ -964,6 +982,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); >> @@ -980,6 +1000,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) > > Perhaps for consistent naming with drm_property_create_object() this > would be better called drm_property_get_object()? hmm, I thought I'd dropped the drm_property_get_obj() fxn and used _object_find() directly in the one call site. We don't actually want drm_property_get_obj() because of fb's. What that is supposed to be is _object_find() directly in drm_property_change_is_valid() and drm_mode_object_find() elsewhere. so apparently I lost some patch which was supposed to be squashed back into this one. :-/ >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > [...] >> +/* 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) > > Ugh, that's an unfortunate bit of userspace ABI... > > Could this perhaps be done in a more unified, yet backwards-compatible > way? For example if we define legacy properties like this: I mostly went for the keep-bitmask-comparision-for-old-types out of backwards compat paranoia.. at least if someone somehow managed to create an enum-blob-range property, it would follow the same code paths it did before ;-) quite possibly overkill. But it's hidden behind helper fxns/macros (and I was planning on doing the same on the userspace side). So meh. BR, -R > #define DRM_MODE_PROP_TYPE(n) ((n) << 0) > #define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE( 2) > #define DRM_MODE_PROP_ENUM DRM_MODE_PROP_TYPE( 8) > #define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE(16) > #define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE(32) > > And define the type mask to be: > > #define DRM_MODE_PROP_TYPE 0x0000fffa > > Leaving out only the two real flags (PENDING and IMMUTABLE). This should > allow things to be done without constantly having to special case legacy > vs. extended types. It leaves a bunch of wholes in the number space and > we need to be careful to introduce new types only in the upper range, > but it has the advantage of not requiring special handling. > > Thierry
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 37a3e07..61ddc66 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3117,6 +3117,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) @@ -3137,6 +3139,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); @@ -3274,6 +3279,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 @@ -3294,14 +3316,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)) { @@ -3484,10 +3508,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++; } @@ -3509,7 +3534,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; @@ -3531,7 +3557,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; @@ -3687,19 +3713,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; } else { int i; for (i = 0; i < property->num_values; i++) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5c1c31c..c1c243f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -190,6 +190,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; }; @@ -931,6 +932,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); @@ -964,6 +982,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); @@ -980,6 +1000,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 f104c26..5b530c7 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];
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@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++++++++++++++++++++++-------- include/drm/drm_crtc.h | 27 ++++++++++++++++++++++++ include/uapi/drm/drm_mode.h | 14 +++++++++++++ 3 files changed, 82 insertions(+), 9 deletions(-)