Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 06/13] drm: add atomic hook to read property plus helper
Date: Wed, 17 Dec 2014 15:02:41 +0100	[thread overview]
Message-ID: <20141217140241.GJ2711@phenom.ffwll.local> (raw)
In-Reply-To: <1418771141-16954-7-git-send-email-robdclark@gmail.com>

On Tue, Dec 16, 2014 at 06:05:34PM -0500, Rob Clark wrote:
> Once a driver is using atomic helpers for modeset, the next step is to
> switch over to atomic properties.  To do this, plug in the helper
> function to your modeconfig funcs and be sure to implement the plane/
> crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
> objects which have driver custom properties.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  include/drm/drm_crtc.h              |  3 +++
>  4 files changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d42fdb1..1a1ab34 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1703,6 +1703,52 @@ backoff:
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>  
>  /**
> + * drm_atomic_helper_get_property - helper to read atomic property
> + * @obj: drm mode object whose property to read
> + * @property: the property to read
> + * @val: the read value, returned by reference
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	struct drm_device *dev = property->dev;
> +	int ret;
> +
> +	switch (obj->type) {
> +	case DRM_MODE_OBJECT_CONNECTOR: {
> +		struct drm_connector *connector = obj_to_connector(obj);
> +		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +		ret = connector->funcs->atomic_get_property(connector,
> +				connector->state, property, val);

Shouldn't we use the get helpers introduced in previous patches here? For
legacy support we definitely want old core stuff like dpms to keep
working. Of course that means all the new atomic properties won't get
magically filtered out. But I think we need to hide them explicitly
anyway, e.g. with a new DRM_MODE_PROP_ATOMIC and checking for that in
legacy paths.
-Daniel

> +		break;
> +	}
> +	case DRM_MODE_OBJECT_CRTC: {
> +		struct drm_crtc *crtc = obj_to_crtc(obj);
> +		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
> +		ret = crtc->funcs->atomic_get_property(crtc,
> +				crtc->state, property, val);
> +		break;
> +	}
> +	case DRM_MODE_OBJECT_PLANE: {
> +		struct drm_plane *plane = obj_to_plane(obj);
> +		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
> +		ret = plane->funcs->atomic_get_property(plane,
> +				plane->state, property, val);
> +		break;
> +	}
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_get_property);
> +
> +/**
>   * drm_atomic_helper_page_flip - execute a legacy page flip
>   * @crtc: DRM crtc
>   * @fb: DRM framebuffer
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 481bb25..57cd950 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3878,8 +3878,17 @@ EXPORT_SYMBOL(drm_object_property_set_value);
>  int drm_object_property_get_value(struct drm_mode_object *obj,
>  				  struct drm_property *property, uint64_t *val)
>  {
> +	struct drm_mode_config *config = &property->dev->mode_config;
>  	int i;
>  
> +	/* read-only properties bypass atomic mechanism and still store
> +	 * their value in obj->properties->values[].. mostly to avoid
> +	 * having to deal w/ EDID and similar props in atomic paths:
> +	 */
> +	if (config->funcs->atomic_get_property &&
> +			!(property->flags & DRM_MODE_PROP_IMMUTABLE))
> +		return config->funcs->atomic_get_property(obj, property, val);
> +
>  	for (i = 0; i < obj->properties->count; i++) {
>  		if (obj->properties->properties[i] == property) {
>  			*val = obj->properties->values[i];
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index f956b41..5e72b82 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -74,6 +74,8 @@ int drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>  int drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>  					struct drm_property *property,
>  					uint64_t val);
> +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
> +			   struct drm_property *property, uint64_t *val);
>  int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e6cfdaf..ca8ba72 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -941,6 +941,7 @@ struct drm_mode_set {
>   * struct drm_mode_config_funcs - basic driver provided mode setting functions
>   * @fb_create: create a new framebuffer object
>   * @output_poll_changed: function to handle output configuration changes
> + * @atomic_get_property: atomic way to read property value
>   * @atomic_check: check whether a give atomic state update is possible
>   * @atomic_commit: commit an atomic state update previously verified with
>   * 	atomic_check()
> @@ -954,6 +955,8 @@ struct drm_mode_config_funcs {
>  					     struct drm_mode_fb_cmd2 *mode_cmd);
>  	void (*output_poll_changed)(struct drm_device *dev);
>  
> +	int (*atomic_get_property)(struct drm_mode_object *obj,
> +				struct drm_property *property, uint64_t *val);
>  	int (*atomic_check)(struct drm_device *dev,
>  			    struct drm_atomic_state *a);
>  	int (*atomic_commit)(struct drm_device *dev,
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel