Re: [PATCH 2/8] drm: Don't update property values for atomic drivers

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/8] drm: Don't update property values for atomic drivers
Date: Tue, 25 Jul 2017 10:32:56 +0200	[thread overview]
Message-ID: <442f5828-c67b-ce99-87a6-9a7e58c8607f@linux.intel.com> (raw)
In-Reply-To: <20170725080122.20548-3-daniel.vetter@ffwll.ch>

Op 25-07-17 om 10:01 schreef Daniel Vetter:
> Atomic drivers only use the property value store for immutable (i.e.
> can't be set by userspace, but the kernel can still adjust it)
> properties. The only tricky part is the removal of the update in
> drm_atomic_helper_update_legacy_modeset_state().
>
> This was added in
>
> commit 8c10342cb48f3140d9abeadcfd2fa6625d447282 (tag: topic/drm-misc-2015-07-28)
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Mon Jul 27 13:24:29 2015 +0200
>
>     drm/atomic: Update legacy DPMS state during modesets, v3.
>
> by copying it from the i915 code, where it was originally added in
>
> commit 68d3472047a572936551f8ff0b6f4016c5a1fdef
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Sep 6 22:08:35 2012 +0200
>
>     drm/i915: update dpms property in set_mode
>
> for the legacy modeset code. The reason we needed this hack was that
> i915 didn't yet set DRIVER_ATOMIC, and we checked for that instead of
> the newer-ish drm_drv_uses_atomic_modeset(), which avoids such
> troubles. With the correct feature checks this isn't needed anymore at
> all.
>
> Also make sure that drivers don't accidentally get this wrong by
> making the exported version of drm_object_property_get_value() only
> work for legacy drivers. Only gma500 uses it anyway.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 ---
>  drivers/gpu/drm/drm_connector.c     |  3 +--
>  drivers/gpu/drm/drm_crtc.c          |  2 +-
>  drivers/gpu/drm/drm_mode_object.c   | 49 +++++++++++++++++++++++--------------
>  drivers/gpu/drm/drm_plane.c         |  2 +-
>  5 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7582bbc5decc..4a960c741e35 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -921,16 +921,12 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>  		crtc = new_conn_state->crtc;
>  		if ((!crtc && old_conn_state->crtc) ||
>  		    (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) {
> -			struct drm_property *dpms_prop =
> -				dev->mode_config.dpms_property;
>  			int mode = DRM_MODE_DPMS_OFF;
>  
>  			if (crtc && crtc->state->active)
>  				mode = DRM_MODE_DPMS_ON;
>  
>  			connector->dpms = mode;
> -			drm_object_property_set_value(&connector->base,
> -						      dpms_prop, mode);
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8072e6e4c62c..349104eadefe 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1225,8 +1225,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  	} else if (connector->funcs->set_property)
>  		ret = connector->funcs->set_property(connector, property, value);
>  
> -	/* store the property value if successful */
> -	if (!ret)
> +	if (!ret && drm_drv_uses_atomic_modeset(property->dev))
>  		drm_object_property_set_value(&connector->base, property, value);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5af25ce5bf7c..7d4fcdd34342 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -736,7 +736,7 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
>  
>  	if (crtc->funcs->set_property)
>  		ret = crtc->funcs->set_property(crtc, property, value);
> -	if (!ret)
> +	if (!ret && drm_drv_uses_atomic_modeset(property->dev))
>  		drm_object_property_set_value(obj, property, value);
You've inverted the checks here for connector and crtc. :)

Otherwise looks sane, fix this and you can add my r-b.

On second thought, as penance you should add kms_properties to the fast-feedback testlist. ;)
>  	return ret;
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index da9a9adbcc98..92743a796bf0 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -233,6 +233,9 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
>  {
>  	int i;
>  
> +	WARN_ON(drm_drv_uses_atomic_modeset(property->dev) &&
> +		!(property->flags & DRM_MODE_PROP_IMMUTABLE));
> +
>  	for (i = 0; i < obj->properties->count; i++) {
>  		if (obj->properties->properties[i] == property) {
>  			obj->properties->values[i] = val;
> @@ -244,24 +247,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
>  }
>  EXPORT_SYMBOL(drm_object_property_set_value);
>  
> -/**
> - * drm_object_property_get_value - retrieve the value of a property
> - * @obj: drm mode object to get property value from
> - * @property: property to retrieve
> - * @val: storage for the property value
> - *
> - * This function retrieves the softare state of the given property for the given
> - * property. Since there is no driver callback to retrieve the current property
> - * value this might be out of sync with the hardware, depending upon the driver
> - * and property.
> - *
> - * Atomic drivers should never call this function directly, the core will read
> - * out property values through the various ->atomic_get_property callbacks.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_object_property_get_value(struct drm_mode_object *obj,
> +int __drm_object_property_get_value(struct drm_mode_object *obj,
>  				  struct drm_property *property, uint64_t *val)
>  {
>  	int i;
> @@ -284,6 +270,31 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
>  
>  	return -EINVAL;
>  }
> +
> +/**
> + * drm_object_property_get_value - retrieve the value of a property
> + * @obj: drm mode object to get property value from
> + * @property: property to retrieve
> + * @val: storage for the property value
> + *
> + * This function retrieves the softare state of the given property for the given
> + * property. Since there is no driver callback to retrieve the current property
> + * value this might be out of sync with the hardware, depending upon the driver
> + * and property.
> + *
> + * Atomic drivers should never call this function directly, the core will read
> + * out property values through the various ->atomic_get_property callbacks.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_object_property_get_value(struct drm_mode_object *obj,
> +				  struct drm_property *property, uint64_t *val)
> +{
> +	WARN_ON(drm_drv_uses_atomic_modeset(property->dev));
> +
> +	return __drm_object_property_get_value(obj, property, val);
> +}
>  EXPORT_SYMBOL(drm_object_property_get_value);
>  
>  /* helper for getconnector and getproperties ioctls */
> @@ -302,7 +313,7 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic,
>  			continue;
>  
>  		if (*arg_count_props > count) {
> -			ret = drm_object_property_get_value(obj, prop, &val);
> +			ret = __drm_object_property_get_value(obj, prop, &val);
>  			if (ret)
>  				return ret;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 5dc8c4350602..b4ffd0455200 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -330,7 +330,7 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  
>  	if (plane->funcs->set_property)
>  		ret = plane->funcs->set_property(plane, property, value);
> -	if (!ret)
> +	if (!ret && drm_drv_uses_atomic_modeset(property->dev))
>  		drm_object_property_set_value(obj, property, value);
>  
>  	return ret;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel