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

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: dri-devel@lists.freedesktop.org Acked-by: Ander Conselvan de Oliveira ander.conselvan.de.oliveira@intel.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

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 dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel