Re: [Intel-gfx] [PATCH 01/42] drm/atomic: Allow drivers to subclass drm_atomic_state

On Mon, May 11, 2015 at 4:24 PM, Maarten Lankhorst maarten.lankhorst@linux.intel.com 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.

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

A few naming change suggestions below ...

---

drivers/gpu/drm/drm_atomic.c | 91 ++++++++++++++++++++++++++++++++------------ include/drm/drm_atomic.h | 4 ++ include/drm/drm_crtc.h | 4 ++ 3 files changed, 74 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6e3b78ee7d16..f0f914591f1d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -38,24 +38,19 @@ static void kfree_state(struct drm_atomic_state *state) kfree(state->crtc_states); kfree(state->planes); kfree(state->plane_states);

•   kfree(state);
  

}

/**

• • drm_atomic_state_alloc - allocate atomic state

• • __drm_atomic_new_state - 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_new_state(struct drm_device *dev,

•                      struct drm_atomic_state *state)
  

Generally the naming pattern is _object_create/alloc for allocating+initializing an object, and _object_init for just initializing it. So imo better to call this drm_atomic_state_init without any __ prefix or similar.

{

•   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 +87,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);

•   return -ENOMEM;
  

+} +EXPORT_SYMBOL(__drm_atomic_new_state);

•   return NULL;
  

+/**

• • 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_new_state) {
  

•           state = kzalloc(sizeof(*state), GFP_KERNEL);
  

•           if (!state)
  

•                   return NULL;
  

•           if (__drm_atomic_new_state(dev, state) < 0) {
  

•                   kfree(state);
  

•                   return NULL;
  

•           }
  

•           return state;
  

•   }
  

•   return config->funcs->atomic_new_state(dev);
  

Since this vfunc replaces drm_atomic_state_alloc completely I think it should be called atomic_state_alloc to match that. And not the init-only function above.

} EXPORT_SYMBOL(drm_atomic_state_alloc);

/**

• • drm_atomic_state_clear - clear state object

• • __drm_atomic_clear_state - clear 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_clear_state(struct drm_atomic_state *state)

We've gone with __ in the duplicate/destroy functions for lack of any good names, but here drm_atomic_state_default_clear looks imo good and is much clearer.

{ struct drm_device *dev = state->dev; struct drm_mode_config *config = &dev->mode_config; @@ -162,6 +176,32 @@ void drm_atomic_state_clear(struct drm_atomic_state *state) state->plane_states[i] = NULL; } } +EXPORT_SYMBOL(__drm_atomic_clear_state);

+/**

• • 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_clear_state)
  

•           config->funcs->atomic_clear_state(state);
  

Let's not mix up word order for no reason and call this hook atomic_state_clear.

•   else
  

•           __drm_atomic_clear_state(state);
  

+} EXPORT_SYMBOL(drm_atomic_state_clear);

/** @@ -181,6 +221,7 @@ void drm_atomic_state_free(struct drm_atomic_state *state) DRM_DEBUG_ATOMIC("Freeing atomic state %p\n", state);

    kfree_state(state);

•   kfree(state);
  

I think the kerneldoc somewhere must mention that if when subclassing struct drm_atomic_state must be the first member. Or we simply add an atomic_state_free hook for completeness - the kfree_state could still be done here.

Actually I think atomic_state_free is required, or how is the driver supposed to release additional allocations (e.g. for i915 shared dpll state we probably should just have pointers to the dpll state objects)?

Cheers, Daniel

} EXPORT_SYMBOL(drm_atomic_state_free);

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index c157103492b0..6125eec6ad79 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -35,6 +35,10 @@ 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_new_state(struct drm_device *dev, struct drm_atomic_state *state); +void __drm_atomic_clear_state(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..e5bea3a45484 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -983,6 +983,8 @@ 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_clear_state: allocate a new atomic state
• • @atomic_clear_state: clear the atomic state
  • Some global (i.e. not per-CRTC, connector, etc) mode setting functions that
  • involve drivers.

@@ -998,6 +1000,8 @@ struct drm_mode_config_funcs { int (*atomic_commit)(struct drm_device *dev, struct drm_atomic_state *a, bool async);

•   struct drm_atomic_state *(*atomic_new_state)(struct drm_device *dev);
  

•   void (*atomic_clear_state)(struct drm_atomic_state *state);
  

};

/**

2.1.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx