On Wed, Nov 19, 2014 at 12:38 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
