Re: [PATCH 1/8] drm: add object property type

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