Re: [PATCH 5/6] drm: s/enum_blob_list/enum_list/ in drm_property

[Rob Clark <robdclark@gmail.com>]

On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> I guess for hysterical raisins this was meant to be the way to read
> blob properties. But that's done with the two-stage approach which
> uses separate blob kms object and the special-purpose get_blob ioctl.
>
> Shipping userspace seems to have never relied on this, and the kernel
> also never put any blob thing onto that property. And nowadays it
> would blow up, e.g. in drm_property_destroy. Also it makes no sense to
> return values in an ioctl that only returns metadata about everything.
>
> So let's ditch all the internal code for the blob list, rename the
> list to be unambiguous and sprinkle comments all over the place to
> explain this peculiar piece of api.
>
> v2: Squash in fixup from Rob to remove now unused variables.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_crtc.c  | 53 +++++++++++++++------------------------------
>  include/drm/drm_crtc.h      |  2 +-
>  include/uapi/drm/drm_mode.h |  2 ++
>  3 files changed, 20 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 8c550302a9ef..589a921d4313 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3457,7 +3457,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
>
>         property->flags = flags;
>         property->num_values = num_values;
> -       INIT_LIST_HEAD(&property->enum_blob_list);
> +       INIT_LIST_HEAD(&property->enum_list);
>
>         if (name) {
>                 strncpy(property->name, name, DRM_PROP_NAME_LEN);
> @@ -3679,8 +3679,8 @@ int drm_property_add_enum(struct drm_property *property, int index,
>                         (value > 63))
>                 return -EINVAL;
>
> -       if (!list_empty(&property->enum_blob_list)) {
> -               list_for_each_entry(prop_enum, &property->enum_blob_list, head) {
> +       if (!list_empty(&property->enum_list)) {
> +               list_for_each_entry(prop_enum, &property->enum_list, head) {
>                         if (prop_enum->value == value) {
>                                 strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN);
>                                 prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0';
> @@ -3698,7 +3698,7 @@ int drm_property_add_enum(struct drm_property *property, int index,
>         prop_enum->value = value;
>
>         property->values[index] = value;
> -       list_add_tail(&prop_enum->head, &property->enum_blob_list);
> +       list_add_tail(&prop_enum->head, &property->enum_list);
>         return 0;
>  }
>  EXPORT_SYMBOL(drm_property_add_enum);
> @@ -3715,7 +3715,7 @@ void drm_property_destroy(struct drm_device *dev, struct drm_property *property)
>  {
>         struct drm_property_enum *prop_enum, *pt;
>
> -       list_for_each_entry_safe(prop_enum, pt, &property->enum_blob_list, head) {
> +       list_for_each_entry_safe(prop_enum, pt, &property->enum_list, head) {
>                 list_del(&prop_enum->head);
>                 kfree(prop_enum);
>         }
> @@ -3839,16 +3839,12 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>         struct drm_mode_get_property *out_resp = data;
>         struct drm_property *property;
>         int enum_count = 0;
> -       int blob_count = 0;
>         int value_count = 0;
>         int ret = 0, i;
>         int copied;
>         struct drm_property_enum *prop_enum;
>         struct drm_mode_property_enum __user *enum_ptr;
> -       struct drm_property_blob *prop_blob;
> -       uint32_t __user *blob_id_ptr;
>         uint64_t __user *values_ptr;
> -       uint32_t __user *blob_length_ptr;
>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -3862,11 +3858,8 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>
>         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)
> +               list_for_each_entry(prop_enum, &property->enum_list, head)
>                         enum_count++;
> -       } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
> -               list_for_each_entry(prop_blob, &property->enum_blob_list, head)
> -                       blob_count++;
>         }
>
>         value_count = property->num_values;
> @@ -3891,7 +3884,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>                 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;
> -                       list_for_each_entry(prop_enum, &property->enum_blob_list, head) {
> +                       list_for_each_entry(prop_enum, &property->enum_list, head) {
>
>                                 if (copy_to_user(&enum_ptr[copied].value, &prop_enum->value, sizeof(uint64_t))) {
>                                         ret = -EFAULT;
> @@ -3909,28 +3902,16 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>                 out_resp->count_enum_blobs = enum_count;
>         }
>
> -       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;
> -                       blob_length_ptr = (uint32_t __user *)(unsigned long)out_resp->values_ptr;
> -
> -                       list_for_each_entry(prop_blob, &property->enum_blob_list, head) {
> -                               if (put_user(prop_blob->base.id, blob_id_ptr + copied)) {
> -                                       ret = -EFAULT;
> -                                       goto done;
> -                               }
> -
> -                               if (put_user(prop_blob->length, blob_length_ptr + copied)) {
> -                                       ret = -EFAULT;
> -                                       goto done;
> -                               }
> -
> -                               copied++;
> -                       }
> -               }
> -               out_resp->count_enum_blobs = blob_count;
> -       }
> +       /*
> +        * NOTE: The idea seems to have been to use this to read all the blob
> +        * property values. But nothing ever added them to the corresponding
> +        * list, userspace always used the special-purpose get_blob ioctl to
> +        * read the value for a blob property. It also doesn't make a lot of
> +        * sense to return values here when everything else is just metadata for
> +        * the property itself.
> +        */
> +       if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))
> +               out_resp->count_enum_blobs = 0;
>  done:
>         drm_modeset_unlock_all(dev);
>         return ret;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index cdbae7bdac70..3773fe7bc737 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -216,7 +216,7 @@ struct drm_property {
>         uint64_t *values;
>         struct drm_device *dev;
>
> -       struct list_head enum_blob_list;
> +       struct list_head enum_list;
>  };
>
>  struct drm_crtc;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a0db2d4aa5f0..86574b0005ff 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -286,6 +286,8 @@ struct drm_mode_get_property {
>         char name[DRM_PROP_NAME_LEN];
>
>         __u32 count_values;
> +       /* This is only used to count enum values, not blobs. The _blobs is
> +        * simply because of a historical reason, i.e. backwards compat. */
>         __u32 count_enum_blobs;
>  };
>
> --
> 2.1.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx