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

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

 



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.

> +
>         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@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




[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