[3/3] drm/atomic: Allow drivers to subclass drm_atomic_state, v2

Commit Message

Maarten Lankhorst May 13, 2015, 7:56 a.m. UTC

Drivers may need to store the state of shared resources, such as PLLs
or FIFO space, into the atomic state. Allow this by making it possible
to subclass drm_atomic_state.

Changes since v1:
- Change member names for functions to atomic_state_(alloc,clear)
- Change __drm_atomic_state_new to drm_atomic_state_init
- Allow free function to be overridden too, in case extra memory is
  allocated in alloc.

Cc: [e-mail]
Acked-by: Ander Conselvan de Oliveira <[e-mail]>
Signed-off-by: Maarten Lankhorst <[e-mail]>
---
 drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++-----------
 include/drm/drm_atomic.h     |   5 ++
 include/drm/drm_crtc.h       |   6 +++
 3 files changed, 99 insertions(+), 28 deletions(-)

Comments

Daniel Vetter May 13, 2015, 9:05 a.m. UTC | #1

On Wed, May 13, 2015 at 09:56:02AM +0200, Maarten Lankhorst wrote:
> Drivers may need to store the state of shared resources, such as PLLs
> or FIFO space, into the atomic state. Allow this by making it possible
> to subclass drm_atomic_state.
> 
> Changes since v1:
> - Change member names for functions to atomic_state_(alloc,clear)
> - Change __drm_atomic_state_new to drm_atomic_state_init
> - Allow free function to be overridden too, in case extra memory is
>   allocated in alloc.
> 
> Cc: [e-mail]
> Acked-by: Ander Conselvan de Oliveira <[e-mail]>
> Signed-off-by: Maarten Lankhorst <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++-----------
>  include/drm/drm_atomic.h     |   5 ++
>  include/drm/drm_crtc.h       |   6 +++
>  3 files changed, 99 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6e3b78ee7d16..88259057f87b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,7 +30,15 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_plane_helper.h>
>  
> -static void kfree_state(struct drm_atomic_state *state)
> +/**
> + * drm_atomic_state_default_free -
> + * free memory initialized by drm_atomic_state_init
> + * @state: atomic state
> + *
> + * Free all the memory allocated by drm_atomic_state_init.
> + * This is useful for drivers that subclass the atomic state.
> + */
> +void drm_atomic_state_default_free(struct drm_atomic_state *state)
>  {
>  	kfree(state->connectors);
>  	kfree(state->connector_states);
> @@ -38,24 +46,20 @@ static void kfree_state(struct drm_atomic_state *state)
>  	kfree(state->crtc_states);
>  	kfree(state->planes);
>  	kfree(state->plane_states);
> -	kfree(state);

Once more a naming thing: Since this doesn't free the state structure
itself I think we should call it _release. Or keep the name and also keep
the kfree(state) here and remove it from drm_atomic_state_free. I can do
that when applying if you're ok with that. lgtm otherwise.

I also merged the preceeding 2 patches to drm-misc, thanks.
-Daniel


>  }
> +EXPORT_SYMBOL(drm_atomic_state_default_free);
>  
>  /**
> - * drm_atomic_state_alloc - allocate atomic state
> + * drm_atomic_state_init - init new atomic state
>   * @dev: DRM device
> + * @state: atomic state
>   *
> - * This allocates an empty atomic state to track updates.
> + * Default implementation for filling in a new atomic state.
> + * This is useful for drivers that subclass the atomic state.
>   */
> -struct drm_atomic_state *
> -drm_atomic_state_alloc(struct drm_device *dev)
> +int
> +drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
>  {
> -	struct drm_atomic_state *state;
> -
> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> -	if (!state)
> -		return NULL;
> -
>  	/* TODO legacy paths should maybe do a better job about
>  	 * setting this appropriately?
>  	 */
> @@ -92,31 +96,50 @@ drm_atomic_state_alloc(struct drm_device *dev)
>  
>  	state->dev = dev;
>  
> -	DRM_DEBUG_ATOMIC("Allocate atomic state %p\n", state);
> +	DRM_DEBUG_ATOMIC("Allocated atomic state %p\n", state);
>  
> -	return state;
> +	return 0;
>  fail:
> -	kfree_state(state);
> +	drm_atomic_state_default_free(state);
> +	return -ENOMEM;
> +}
> +EXPORT_SYMBOL(drm_atomic_state_init);
> +
> +/**
> + * drm_atomic_state_alloc - allocate atomic state
> + * @dev: DRM device
> + *
> + * This allocates an empty atomic state to track updates.
> + */
> +struct drm_atomic_state *
> +drm_atomic_state_alloc(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_atomic_state *state;
> +
> +	if (!config->funcs->atomic_state_alloc) {
> +		state = kzalloc(sizeof(*state), GFP_KERNEL);
> +		if (!state)
> +			return NULL;
> +		if (drm_atomic_state_init(dev, state) < 0) {
> +			kfree(state);
> +			return NULL;
> +		}
> +		return state;
> +	}
>  
> -	return NULL;
> +	return config->funcs->atomic_state_alloc(dev);
>  }
>  EXPORT_SYMBOL(drm_atomic_state_alloc);
>  
>  /**
> - * drm_atomic_state_clear - clear state object
> + * drm_atomic_state_default_clear - clear base atomic state
>   * @state: atomic state
>   *
> - * When the w/w mutex algorithm detects a deadlock we need to back off and drop
> - * all locks. So someone else could sneak in and change the current modeset
> - * configuration. Which means that all the state assembled in @state is no
> - * longer an atomic update to the current state, but to some arbitrary earlier
> - * state. Which could break assumptions the driver's ->atomic_check likely
> - * relies on.
> - *
> - * Hence we must clear all cached state and completely start over, using this
> - * function.
> + * Default implementation for clearing atomic state.
> + * This is useful for drivers that subclass the atomic state.
>   */
> -void drm_atomic_state_clear(struct drm_atomic_state *state)
> +void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> @@ -162,6 +185,32 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
>  		state->plane_states[i] = NULL;
>  	}
>  }
> +EXPORT_SYMBOL(drm_atomic_state_default_clear);
> +
> +/**
> + * drm_atomic_state_clear - clear state object
> + * @state: atomic state
> + *
> + * When the w/w mutex algorithm detects a deadlock we need to back off and drop
> + * all locks. So someone else could sneak in and change the current modeset
> + * configuration. Which means that all the state assembled in @state is no
> + * longer an atomic update to the current state, but to some arbitrary earlier
> + * state. Which could break assumptions the driver's ->atomic_check likely
> + * relies on.
> + *
> + * Hence we must clear all cached state and completely start over, using this
> + * function.
> + */
> +void drm_atomic_state_clear(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (config->funcs->atomic_state_clear)
> +		config->funcs->atomic_state_clear(state);
> +	else
> +		drm_atomic_state_default_clear(state);
> +}
>  EXPORT_SYMBOL(drm_atomic_state_clear);
>  
>  /**
> @@ -173,14 +222,25 @@ EXPORT_SYMBOL(drm_atomic_state_clear);
>   */
>  void drm_atomic_state_free(struct drm_atomic_state *state)
>  {
> +	struct drm_device *dev;
> +	struct drm_mode_config *config;
> +
>  	if (!state)
>  		return;
>  
> +	dev = state->dev;
> +	config = &dev->mode_config;
> +
>  	drm_atomic_state_clear(state);
>  
>  	DRM_DEBUG_ATOMIC("Freeing atomic state %p\n", state);
>  
> -	kfree_state(state);
> +	if (config->funcs->atomic_state_free) {
> +		config->funcs->atomic_state_free(state);
> +	} else {
> +		drm_atomic_state_default_free(state);
> +		kfree(state);
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_state_free);
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index c157103492b0..953af6bd7430 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -35,6 +35,11 @@ drm_atomic_state_alloc(struct drm_device *dev);
>  void drm_atomic_state_clear(struct drm_atomic_state *state);
>  void drm_atomic_state_free(struct drm_atomic_state *state);
>  
> +int  __must_check
> +drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state);
> +void drm_atomic_state_default_clear(struct drm_atomic_state *state);
> +void drm_atomic_state_default_free(struct drm_atomic_state *state);
> +
>  struct drm_crtc_state * __must_check
>  drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  			  struct drm_crtc *crtc);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 0a4a040d6bb7..a1fce3a5e849 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -983,6 +983,9 @@ struct drm_mode_set {
>   * @atomic_check: check whether a given atomic state update is possible
>   * @atomic_commit: commit an atomic state update previously verified with
>   * 	atomic_check()
> + * @atomic_state_alloc: allocate a new atomic state
> + * @atomic_state_clear: clear the atomic state
> + * @atomic_state_free: free the atomic state
>   *
>   * Some global (i.e. not per-CRTC, connector, etc) mode setting functions that
>   * involve drivers.
> @@ -998,6 +1001,9 @@ struct drm_mode_config_funcs {
>  	int (*atomic_commit)(struct drm_device *dev,
>  			     struct drm_atomic_state *a,
>  			     bool async);
> +	struct drm_atomic_state *(*atomic_state_alloc)(struct drm_device *dev);
> +	void (*atomic_state_clear)(struct drm_atomic_state *state);
> +	void (*atomic_state_free)(struct drm_atomic_state *state);
>  };
>  
>  /**
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6e3b78ee7d16..88259057f87b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,7 +30,15 @@ 
 #include <drm/drm_atomic.h>
 #include <drm/drm_plane_helper.h>
 
-static void kfree_state(struct drm_atomic_state *state)
+/**
+ * drm_atomic_state_default_free -
+ * free memory initialized by drm_atomic_state_init
+ * @state: atomic state
+ *
+ * Free all the memory allocated by drm_atomic_state_init.
+ * This is useful for drivers that subclass the atomic state.
+ */
+void drm_atomic_state_default_free(struct drm_atomic_state *state)
 {
 	kfree(state->connectors);
 	kfree(state->connector_states);
@@ -38,24 +46,20 @@  static void kfree_state(struct drm_atomic_state *state)
 	kfree(state->crtc_states);
 	kfree(state->planes);
 	kfree(state->plane_states);
-	kfree(state);
 }
+EXPORT_SYMBOL(drm_atomic_state_default_free);
 
 /**
- * drm_atomic_state_alloc - allocate atomic state
+ * drm_atomic_state_init - init new atomic state
  * @dev: DRM device
+ * @state: atomic state
  *
- * This allocates an empty atomic state to track updates.
+ * Default implementation for filling in a new atomic state.
+ * This is useful for drivers that subclass the atomic state.
  */
-struct drm_atomic_state *
-drm_atomic_state_alloc(struct drm_device *dev)
+int
+drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	struct drm_atomic_state *state;
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (!state)
-		return NULL;
-
 	/* TODO legacy paths should maybe do a better job about
 	 * setting this appropriately?
 	 */
@@ -92,31 +96,50 @@  drm_atomic_state_alloc(struct drm_device *dev)
 
 	state->dev = dev;
 
-	DRM_DEBUG_ATOMIC("Allocate atomic state %p\n", state);
+	DRM_DEBUG_ATOMIC("Allocated atomic state %p\n", state);
 
-	return state;
+	return 0;
 fail:
-	kfree_state(state);
+	drm_atomic_state_default_free(state);
+	return -ENOMEM;
+}
+EXPORT_SYMBOL(drm_atomic_state_init);
+
+/**
+ * drm_atomic_state_alloc - allocate atomic state
+ * @dev: DRM device
+ *
+ * This allocates an empty atomic state to track updates.
+ */
+struct drm_atomic_state *
+drm_atomic_state_alloc(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_atomic_state *state;
+
+	if (!config->funcs->atomic_state_alloc) {
+		state = kzalloc(sizeof(*state), GFP_KERNEL);
+		if (!state)
+			return NULL;
+		if (drm_atomic_state_init(dev, state) < 0) {
+			kfree(state);
+			return NULL;
+		}
+		return state;
+	}
 
-	return NULL;
+	return config->funcs->atomic_state_alloc(dev);
 }
 EXPORT_SYMBOL(drm_atomic_state_alloc);
 
 /**
- * drm_atomic_state_clear - clear state object
+ * drm_atomic_state_default_clear - clear base atomic state
  * @state: atomic state
  *
- * When the w/w mutex algorithm detects a deadlock we need to back off and drop
- * all locks. So someone else could sneak in and change the current modeset
- * configuration. Which means that all the state assembled in @state is no
- * longer an atomic update to the current state, but to some arbitrary earlier
- * state. Which could break assumptions the driver's ->atomic_check likely
- * relies on.
- *
- * Hence we must clear all cached state and completely start over, using this
- * function.
+ * Default implementation for clearing atomic state.
+ * This is useful for drivers that subclass the atomic state.
  */
-void drm_atomic_state_clear(struct drm_atomic_state *state)
+void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_mode_config *config = &dev->mode_config;
@@ -162,6 +185,32 @@  void drm_atomic_state_clear(struct drm_atomic_state *state)
 		state->plane_states[i] = NULL;
 	}
 }
+EXPORT_SYMBOL(drm_atomic_state_default_clear);
+
+/**
+ * drm_atomic_state_clear - clear state object
+ * @state: atomic state
+ *
+ * When the w/w mutex algorithm detects a deadlock we need to back off and drop
+ * all locks. So someone else could sneak in and change the current modeset
+ * configuration. Which means that all the state assembled in @state is no
+ * longer an atomic update to the current state, but to some arbitrary earlier
+ * state. Which could break assumptions the driver's ->atomic_check likely
+ * relies on.
+ *
+ * Hence we must clear all cached state and completely start over, using this
+ * function.
+ */
+void drm_atomic_state_clear(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (config->funcs->atomic_state_clear)
+		config->funcs->atomic_state_clear(state);
+	else
+		drm_atomic_state_default_clear(state);
+}
 EXPORT_SYMBOL(drm_atomic_state_clear);
 
 /**
@@ -173,14 +222,25 @@  EXPORT_SYMBOL(drm_atomic_state_clear);
  */
 void drm_atomic_state_free(struct drm_atomic_state *state)
 {
+	struct drm_device *dev;
+	struct drm_mode_config *config;
+
 	if (!state)
 		return;
 
+	dev = state->dev;
+	config = &dev->mode_config;
+
 	drm_atomic_state_clear(state);
 
 	DRM_DEBUG_ATOMIC("Freeing atomic state %p\n", state);
 
-	kfree_state(state);
+	if (config->funcs->atomic_state_free) {
+		config->funcs->atomic_state_free(state);
+	} else {
+		drm_atomic_state_default_free(state);
+		kfree(state);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_state_free);
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index c157103492b0..953af6bd7430 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -35,6 +35,11 @@  drm_atomic_state_alloc(struct drm_device *dev);
 void drm_atomic_state_clear(struct drm_atomic_state *state);
 void drm_atomic_state_free(struct drm_atomic_state *state);
 
+int  __must_check
+drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state);
+void drm_atomic_state_default_clear(struct drm_atomic_state *state);
+void drm_atomic_state_default_free(struct drm_atomic_state *state);
+
 struct drm_crtc_state * __must_check
 drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 			  struct drm_crtc *crtc);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0a4a040d6bb7..a1fce3a5e849 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -983,6 +983,9 @@  struct drm_mode_set {
  * @atomic_check: check whether a given atomic state update is possible
  * @atomic_commit: commit an atomic state update previously verified with
  * 	atomic_check()
+ * @atomic_state_alloc: allocate a new atomic state
+ * @atomic_state_clear: clear the atomic state
+ * @atomic_state_free: free the atomic state
  *
  * Some global (i.e. not per-CRTC, connector, etc) mode setting functions that
  * involve drivers.
@@ -998,6 +1001,9 @@  struct drm_mode_config_funcs {
 	int (*atomic_commit)(struct drm_device *dev,
 			     struct drm_atomic_state *a,
 			     bool async);
+	struct drm_atomic_state *(*atomic_state_alloc)(struct drm_device *dev);
+	void (*atomic_state_clear)(struct drm_atomic_state *state);
+	void (*atomic_state_free)(struct drm_atomic_state *state);
 };
 
 /**