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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Tuesday 25 Jul 2017 10:01:16 Daniel Vetter wrote:
> 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);
> 
>  	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));

It would have been nice to remove the calls to drm_object_property_set_value() 
for the dpms property from drivers before adding this :-/ Three drivers (rcar-
du, shmobile and fsl-dcu) initialize the connector's DPMS property to OFF 
using this call (the default being ON).

Following the DPMS code paths always give me a headache, so if you know by 
heart how I should replace the set property call, I'm all ears :-)

>  	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;

-- 
Regards,

Laurent Pinchart

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