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 at lists.freedesktop.org
> Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at
> intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
