On Thu, Sep 8, 2016 at 6:30 AM, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
> Property lifetimes are equal to the device lifetime, so the separate
> drm_property_find is not needed. The pointer can be retrieved from
> the properties member, which saves us some locking and a extra lookup.
> The lifetime for properties is until the device is destroyed, which
> happens late in the device unload path.
>
> kms_atomic is also testing for invalid properties which returns -ENOENT,
> to be consistent return -ENOENT for valid properties that don't appear
> on the object property list.
>
> Changes since v1:
> - Return -ENOENT for invalid properties to make kms_atomic pass.
> - Change commit message slightly to take this into account.
>
> Testcase: kms_atomic
> Testcase: kms_properties
> Fixes: 4e9951d96093 ("drm/atomic: Reject properties not part of the object.")
> Suggested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
Nice cleanup! applied to drm-misc
Sean
> ---
> drivers/gpu/drm/drm_atomic.c | 13 ++-----------
> drivers/gpu/drm/drm_crtc_internal.h | 2 ++
> drivers/gpu/drm/drm_mode_object.c | 31 ++++++++++++++++---------------
> 3 files changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fac156c43506..23739609427d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1609,7 +1609,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> struct drm_crtc_state *crtc_state;
> unsigned plane_mask;
> int ret = 0;
> - unsigned int i, j, k;
> + unsigned int i, j;
>
> /* disallow for drivers not supporting atomic: */
> if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1691,16 +1691,7 @@ retry:
> goto out;
> }
>
> - for (k = 0; k < obj->properties->count; k++)
> - if (obj->properties->properties[k]->base.id
> == prop_id)
> - break;
> -
> - if (k == obj->properties->count) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - prop = drm_property_find(dev, prop_id);
> + prop = drm_mode_obj_find_prop_id(obj, prop_id);
> if (!prop) {
> drm_mode_object_unreference(obj);
> ret = -ENOENT;
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> b/drivers/gpu/drm/drm_crtc_internal.h
> index a3622644bccf..444e609078cc 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -115,6 +115,8 @@ int drm_mode_object_get_properties(struct drm_mode_object
> *obj, bool atomic,
> uint32_t __user *prop_ptr,
> uint64_t __user *prop_values,
> uint32_t *arg_count_props);
> +struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
> + uint32_t prop_id);
>
> /* IOCTL */
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c
> b/drivers/gpu/drm/drm_mode_object.c
> index 6edda8382a4c..9f17085b1fdd 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -372,14 +372,25 @@ out:
> return ret;
> }
>
> +struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
> + uint32_t prop_id)
> +{
> + int i;
> +
> + for (i = 0; i < obj->properties->count; i++)
> + if (obj->properties->properties[i]->base.id == prop_id)
> + return obj->properties->properties[i];
> +
> + return NULL;
> +}
> +
> int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv)
> {
> struct drm_mode_obj_set_property *arg = data;
> struct drm_mode_object *arg_obj;
> - struct drm_mode_object *prop_obj;
> struct drm_property *property;
> - int i, ret = -EINVAL;
> + int ret = -EINVAL;
> struct drm_mode_object *ref;
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> @@ -392,23 +403,13 @@ int drm_mode_obj_set_property_ioctl(struct drm_device
> *dev, void *data,
> ret = -ENOENT;
> goto out;
> }
> - if (!arg_obj->properties)
> - goto out_unref;
> -
> - for (i = 0; i < arg_obj->properties->count; i++)
> - if (arg_obj->properties->properties[i]->base.id ==
> arg->prop_id)
> - break;
>
> - if (i == arg_obj->properties->count)
> + if (!arg_obj->properties)
> goto out_unref;
>
> - prop_obj = drm_mode_object_find(dev, arg->prop_id,
> - DRM_MODE_OBJECT_PROPERTY);
> - if (!prop_obj) {
> - ret = -ENOENT;
> + property = drm_mode_obj_find_prop_id(arg_obj, arg->prop_id);
> + if (!property)
> goto out_unref;
> - }
> - property = obj_to_property(prop_obj);
>
> if (!drm_property_change_valid_get(property, arg->value, &ref))
> goto out_unref;
> --
> 2.7.4
>
>
