On Thu, May 29, 2014 at 4:01 AM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
> Hi
>
> On Thu, May 29, 2014 at 1:57 AM, Rob Clark <robdclark@xxxxxxxxx> 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 | 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@xxxxxxxxx>
>
> Thanks
> David
>
>> struct drm_mode_property_enum {
>> __u64 value;
>> char name[DRM_PROP_NAME_LEN];
>> --
>> 1.9.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
