Re: [PATCH 3/8] drm: Handle properties in the core for atomic drivers

[Archit Taneja <architt@codeaurora.org>]

On 07/25/2017 01:31 PM, Daniel Vetter wrote:
> The reason behind the original indirection through the helper
> functions was to allow existing drivers to overwrite how they handle
> properties. For example when a vendor-specific userspace had
> expectations that didn't match atomic. That seemed likely, since
> atomic is standardizing a _lot_ more of the behaviour of a kms driver.
> 
> But 20 drivers later there's no such need at all. Worse, this forces
> all drivers to hook up the default behaviour, breaking userspace if
> they forget to do that. And it forces us to export a bunch of core
> function just for those helpers.
> 
> And finally, these helpers are the last places using
> drm_atomic_legacy_backoff() and the implicit acquire_ctx.
> 
> This patch here just implements the new behaviour and updates the
> docs. Follow-up patches will garbage-collect all the dead code.
> 
> v2: Fixup docs even better!
> 
> v3: Make it actually work ...

Reviewed-by: Archit Taneja <architt@codeaurora.org>

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/drm_atomic.c        |  60 ++++++++++++++++++--
>   drivers/gpu/drm/drm_connector.c     |   6 +-
>   drivers/gpu/drm/drm_crtc_helper.c   |   3 +-
>   drivers/gpu/drm/drm_crtc_internal.h |   7 +++
>   drivers/gpu/drm/drm_mode_object.c   | 110 +++++++++++++++++++++++++++---------
>   include/drm/drm_connector.h         |  10 ++--
>   include/drm/drm_crtc.h              |   6 +-
>   include/drm/drm_plane.h             |   6 +-
>   8 files changed, 158 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 01192dd3ed79..0fd14aff7add 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1864,9 +1864,60 @@ static struct drm_pending_vblank_event *create_vblank_event(
>   	return e;
>   }
>   
> -static int atomic_set_prop(struct drm_atomic_state *state,
> -		struct drm_mode_object *obj, struct drm_property *prop,
> -		uint64_t prop_value)
> +int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
> +				     struct drm_connector *connector,
> +				     int mode)
> +{
> +	struct drm_connector *tmp_connector;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int i, ret, old_mode = connector->dpms;
> +	bool active = false;
> +
> +	ret = drm_modeset_lock(&state->dev->mode_config.connection_mutex,
> +			       state->acquire_ctx);
> +	if (ret)
> +		return ret;
> +
> +	if (mode != DRM_MODE_DPMS_ON)
> +		mode = DRM_MODE_DPMS_OFF;
> +	connector->dpms = mode;
> +
> +	crtc = connector->state->crtc;
> +	if (!crtc)
> +		goto out;
> +	ret = drm_atomic_add_affected_connectors(state, crtc);
> +	if (ret)
> +		goto out;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		goto out;
> +	}
> +
> +	for_each_new_connector_in_state(state, tmp_connector, new_conn_state, i) {
> +		if (new_conn_state->crtc != crtc)
> +			continue;
> +		if (tmp_connector->dpms == DRM_MODE_DPMS_ON) {
> +			active = true;
> +			break;
> +		}
> +	}
> +
> +	crtc_state->active = active;
> +	ret = drm_atomic_commit(state);
> +out:
> +	if (ret != 0)
> +		connector->dpms = old_mode;
> +	return ret;
> +}
> +
> +int drm_atomic_set_property(struct drm_atomic_state *state,
> +			    struct drm_mode_object *obj,
> +			    struct drm_property *prop,
> +			    uint64_t prop_value)
>   {
>   	struct drm_mode_object *ref;
>   	int ret;
> @@ -2286,7 +2337,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>   				goto out;
>   			}
>   
> -			ret = atomic_set_prop(state, obj, prop, prop_value);
> +			ret = drm_atomic_set_property(state, obj, prop,
> +						      prop_value);
>   			if (ret) {
>   				drm_mode_object_put(obj);
>   				goto out;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 349104eadefe..12371f184019 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -717,9 +717,9 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>    * 	drivers, it remaps to controlling the "ACTIVE" property on the CRTC the
>    * 	connector is linked to. Drivers should never set this property directly,
>    * 	it is handled by the DRM core by calling the &drm_connector_funcs.dpms
> - * 	callback. Atomic drivers should implement this hook using
> - * 	drm_atomic_helper_connector_dpms(). This is the only property standard
> - * 	connector property that userspace can change.
> + * 	callback. For atomic drivers the remapping to the "ACTIVE" property is
> + * 	implemented in the DRM core.  This is the only standard connector
> + * 	property that userspace can change.
>    * PATH:
>    * 	Connector path property to identify how this sink is physically
>    * 	connected. Used by DP MST. This should be set by calling
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 4afdf7902eda..eab36a460638 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -863,8 +863,7 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc)
>    * provided by the driver.
>    *
>    * This function is deprecated.  New drivers must implement atomic modeset
> - * support, for which this function is unsuitable. Instead drivers should use
> - * drm_atomic_helper_connector_dpms().
> + * support, where DPMS is handled in the DRM core.
>    *
>    * Returns:
>    * Always returns 0.
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index d077c5490041..a43582076b20 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -178,6 +178,13 @@ struct drm_minor;
>   int drm_atomic_debugfs_init(struct drm_minor *minor);
>   #endif
>   
> +int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
> +				     struct drm_connector *connector,
> +				     int mode);
> +int drm_atomic_set_property(struct drm_atomic_state *state,
> +			    struct drm_mode_object *obj,
> +			    struct drm_property *prop,
> +			    uint64_t prop_value);
>   int drm_atomic_get_property(struct drm_mode_object *obj,
>   			    struct drm_property *property, uint64_t *val);
>   int drm_mode_atomic_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 92743a796bf0..1055533792f3 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -392,6 +392,83 @@ struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
>   	return NULL;
>   }
>   
> +static int set_property_legacy(struct drm_mode_object *obj,
> +			       struct drm_property *prop,
> +			       uint64_t prop_value)
> +{
> +	struct drm_device *dev = prop->dev;
> +	struct drm_mode_object *ref;
> +	int ret = -EINVAL;
> +
> +	if (!drm_property_change_valid_get(prop, prop_value, &ref))
> +		return -EINVAL;
> +
> +	drm_modeset_lock_all(dev);
> +	switch (obj->type) {
> +	case DRM_MODE_OBJECT_CONNECTOR:
> +		ret = drm_mode_connector_set_obj_prop(obj, prop,
> +						      prop_value);
> +		break;
> +	case DRM_MODE_OBJECT_CRTC:
> +		ret = drm_mode_crtc_set_obj_prop(obj, prop, prop_value);
> +		break;
> +	case DRM_MODE_OBJECT_PLANE:
> +		ret = drm_mode_plane_set_obj_prop(obj_to_plane(obj),
> +						  prop, prop_value);
> +		break;
> +	}
> +	drm_property_change_valid_put(prop, ref);
> +	drm_modeset_unlock_all(dev);
> +
> +	return ret;
> +}
> +
> +static int set_property_atomic(struct drm_mode_object *obj,
> +			       struct drm_property *prop,
> +			       uint64_t prop_value)
> +{
> +	struct drm_device *dev = prop->dev;
> +	struct drm_atomic_state *state;
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +	state->acquire_ctx = &ctx;
> +retry:
> +	if (prop == state->dev->mode_config.dpms_property) {
> +		if (obj->type != DRM_MODE_OBJECT_CONNECTOR) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		ret = drm_atomic_connector_commit_dpms(state,
> +						       obj_to_connector(obj),
> +						       prop_value);
> +	} else {
> +		ret = drm_atomic_set_property(state, obj, prop, prop_value);
> +		if (ret)
> +			goto out;
> +		ret = drm_atomic_commit(state);
> +	}
> +out:
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}
> +
>   int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>   				    struct drm_file *file_priv)
>   {
> @@ -399,18 +476,13 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>   	struct drm_mode_object *arg_obj;
>   	struct drm_property *property;
>   	int ret = -EINVAL;
> -	struct drm_mode_object *ref;
>   
>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>   		return -EINVAL;
>   
> -	drm_modeset_lock_all(dev);
> -
>   	arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
> -	if (!arg_obj) {
> -		ret = -ENOENT;
> -		goto out;
> -	}
> +	if (!arg_obj)
> +		return -ENOENT;
>   
>   	if (!arg_obj->properties)
>   		goto out_unref;
> @@ -419,28 +491,12 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>   	if (!property)
>   		goto out_unref;
>   
> -	if (!drm_property_change_valid_get(property, arg->value, &ref))
> -		goto out_unref;
> -
> -	switch (arg_obj->type) {
> -	case DRM_MODE_OBJECT_CONNECTOR:
> -		ret = drm_mode_connector_set_obj_prop(arg_obj, property,
> -						      arg->value);
> -		break;
> -	case DRM_MODE_OBJECT_CRTC:
> -		ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);
> -		break;
> -	case DRM_MODE_OBJECT_PLANE:
> -		ret = drm_mode_plane_set_obj_prop(obj_to_plane(arg_obj),
> -						  property, arg->value);
> -		break;
> -	}
> -
> -	drm_property_change_valid_put(property, ref);
> +	if (drm_drv_uses_atomic_modeset(property->dev))
> +		ret = set_property_atomic(arg_obj, property, arg->value);
> +	else
> +		ret = set_property_legacy(arg_obj, property, arg->value);
>   
>   out_unref:
>   	drm_mode_object_put(arg_obj);
> -out:
> -	drm_modeset_unlock_all(dev);
>   	return ret;
>   }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 4bc088269d05..ea8da401c93c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -382,8 +382,8 @@ struct drm_connector_funcs {
>   	 * implement the 4 level DPMS support on the connector any more, but
>   	 * instead only have an on/off "ACTIVE" property on the CRTC object.
>   	 *
> -	 * Drivers implementing atomic modeset should use
> -	 * drm_atomic_helper_connector_dpms() to implement this hook.
> +	 * This hook is not used by atomic drivers, remapping of the legacy DPMS
> +	 * property is entirely handled in the DRM core.
>   	 *
>   	 * RETURNS:
>   	 *
> @@ -480,11 +480,9 @@ struct drm_connector_funcs {
>   	 * This is the legacy entry point to update a property attached to the
>   	 * connector.
>   	 *
> -	 * Drivers implementing atomic modeset should use
> -	 * drm_atomic_helper_connector_set_property() to implement this hook.
> -	 *
>   	 * This callback is optional if the driver does not support any legacy
> -	 * driver-private properties.
> +	 * driver-private properties. For atomic drivers it is not used because
> +	 * property handling is done entirely in the DRM core.
>   	 *
>   	 * RETURNS:
>   	 *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3a911a64c257..9c01f94b393c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -481,11 +481,9 @@ struct drm_crtc_funcs {
>   	 * This is the legacy entry point to update a property attached to the
>   	 * CRTC.
>   	 *
> -	 * Drivers implementing atomic modeset should use
> -	 * drm_atomic_helper_crtc_set_property() to implement this hook.
> -	 *
>   	 * This callback is optional if the driver does not support any legacy
> -	 * driver-private properties.
> +	 * driver-private properties. For atomic drivers it is not used because
> +	 * property handling is done entirely in the DRM core.
>   	 *
>   	 * RETURNS:
>   	 *
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9ab3e7044812..204c213810df 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -233,11 +233,9 @@ struct drm_plane_funcs {
>   	 * This is the legacy entry point to update a property attached to the
>   	 * plane.
>   	 *
> -	 * Drivers implementing atomic modeset should use
> -	 * drm_atomic_helper_plane_set_property() to implement this hook.
> -	 *
>   	 * This callback is optional if the driver does not support any legacy
> -	 * driver-private properties.
> +	 * driver-private properties. For atomic drivers it is not used because
> +	 * property handling is done entirely in the DRM core.
>   	 *
>   	 * RETURNS:
>   	 *
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel