On Mon, Jul 03, 2017 at 04:43:37PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Make the atomic private object stuff less special by introducing proper
> base classes for the object and its state. Drivers can embed these in
> their own appropriate objects, after which these things will work
> exactly like the plane/crtc/connector states during atomic operations.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Won't apply without 1&4, but glossing over that detail I kinda like this.
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic.c | 78 +++++++++++++++------
> drivers/gpu/drm/drm_atomic_helper.c | 30 +++++++--
> drivers/gpu/drm/drm_dp_mst_topology.c | 63 +++++++++--------
> include/drm/drm_atomic.h | 123 +++++++++++++++++++++-------------
> include/drm/drm_atomic_helper.h | 4 ++
> include/drm/drm_dp_mst_helper.h | 10 +++
> 6 files changed, 206 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5eb14c73c0fb..da7752230e4c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -197,12 +197,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> for (i = 0; i < state->num_private_objs; i++) {
> struct __drm_private_objs_state *p =
> __drm_atomic_state_private_obj(state, i);
> - void *obj_state = p->obj_state;
> + struct drm_private_obj *obj = p->ptr;
>
> - p->funcs->destroy_state(obj_state);
> - p->obj = NULL;
> - p->obj_state = NULL;
> - p->funcs = NULL;
> + if (WARN_ON(!obj))
> + continue;
> +
> + obj->funcs->atomic_destroy_state(obj, p->state);
> + p->ptr = NULL;
> + p->state = NULL;
> }
> state->num_private_objs = 0;
>
> @@ -1000,11 +1002,44 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> }
>
> /**
> + * drm_atomic_private_obj_init - initialize private object
> + * @obj: private object
> + * @state: initial private object state
> + * @funcs: pointer to the struct of function pointers that identify the object
> + * type
> + *
> + * Initialize the private object, which can be embedded into any
> + * driver private object that needs its own atomic state.
> + */
> +void
> +drm_atomic_private_obj_init(struct drm_private_obj *obj,
> + struct drm_private_state *state,
> + const struct drm_private_state_funcs *funcs)
> +{
> + memset(obj, 0, sizeof(*obj));
> +
> + obj->state = state;
> + obj->funcs = funcs;
> +}
> +EXPORT_SYMBOL(drm_atomic_private_obj_init);
> +
> +/**
> + * drm_atomic_private_obj_fini - finalize private object
> + * @obj: private object
> + *
> + * Finalize the private object.
> + */
> +void
> +drm_atomic_private_obj_fini(struct drm_private_obj *obj)
> +{
> + obj->funcs->atomic_destroy_state(obj, obj->state);
> +}
> +EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> +
> +/**
> * drm_atomic_get_private_obj_state - get private object state
> * @state: global atomic state
> * @obj: private object to get the state for
> - * @funcs: pointer to the struct of function pointers that identify the object
> - * type
> *
> * This function returns the private object state for the given private object,
> * allocating the state if needed. It does not grab any locks as the caller is
> @@ -1014,19 +1049,21 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> *
> * Either the allocated state or the error code encoded into a pointer.
> */
> -void *
> -drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> - const struct drm_private_state_funcs *funcs)
> +struct drm_private_state *
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> + struct drm_private_obj *obj)
> {
> + const struct drm_private_state_funcs *funcs = obj->funcs;
> struct __drm_private_objs_state *p;
> + struct drm_private_state *obj_state;
> int index = state->num_private_objs;
> int ret, i;
>
> for (i = 0; i < state->num_private_objs; i++) {
> p = __drm_atomic_state_private_obj(state, i);
>
> - if (obj == p->obj)
> - return p->obj_state;
> + if (obj == p->ptr)
> + return p->state;
> }
>
> ret = drm_dynarray_reserve(&state->private_objs, index);
> @@ -1035,19 +1072,22 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
>
> p = __drm_atomic_state_private_obj(state, index);
>
> - p->obj_state = funcs->duplicate_state(state, obj);
> - if (!p->obj_state)
> + obj_state = funcs->atomic_duplicate_state(obj);
> + if (!obj_state)
> return ERR_PTR(-ENOMEM);
>
> - p->obj = obj;
> - p->funcs = funcs;
> + p->state = obj_state;
> + p->old_state = obj->state;
> + p->new_state = obj_state;
> + p->ptr = obj;
> + obj_state->state = state;
>
> state->num_private_objs = index + 1;
>
> - DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> - p->obj_state, state);
> + DRM_DEBUG_ATOMIC("Added new private object [%p] state %p to %p\n",
> + obj, obj_state, state);
>
> - return p->obj_state;
> + return obj_state;
> }
> EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5cd93c6d691e..78f0ff1ad982 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2267,8 +2267,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> struct drm_plane *plane;
> struct drm_plane_state *old_plane_state, *new_plane_state;
> struct drm_crtc_commit *commit;
> - void *obj, *obj_state;
> - const struct drm_private_state_funcs *funcs;
> + struct drm_private_obj *obj;
> + struct drm_private_state *old_obj_state, *new_obj_state;
>
> if (stall) {
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -2330,8 +2330,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> plane->state = new_plane_state;
> }
>
> - __for_each_private_obj(state, obj, obj_state, i, funcs)
> - funcs->swap_state(obj, &__drm_atomic_state_private_obj(state, i)->obj_state);
> + for_each_oldnew_private_obj_in_state(state, obj, old_obj_state, new_obj_state, i) {
> + WARN_ON(obj->state != old_obj_state);
> +
> + old_obj_state->state = state;
> + new_obj_state->state = NULL;
> +
> + __drm_atomic_state_private_obj(state, i)->state = old_obj_state;
> + obj->state = new_obj_state;
> + }
> }
> EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>
> @@ -3830,3 +3837,18 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> return ret;
> }
> EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
> +
> +/**
> + * __drm_atomic_helper_private_duplicate_state - copy atomic private state
> + * @obj: CRTC object
> + * @state: new private object state
> + *
> + * Copies atomic state from a private objects's current state and resets inferred values.
> + * This is useful for drivers that subclass the private state.
> + */
> +void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj,
> + struct drm_private_state *state)
> +{
> + memcpy(state, obj->state, sizeof(*state));
> +}
> +EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index bfd237c15e76..91510098f60e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -31,6 +31,8 @@
> #include <drm/drmP.h>
>
> #include <drm/drm_fixed.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
>
> /**
> * DOC: dp mst helper
> @@ -2992,41 +2994,32 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
> (*mgr->cbs->hotplug)(mgr);
> }
>
> -void *drm_dp_mst_duplicate_state(struct drm_atomic_state *state, void *obj)
> +static struct drm_private_state *
> +drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
> {
> - struct drm_dp_mst_topology_mgr *mgr = obj;
> - struct drm_dp_mst_topology_state *new_mst_state;
> + struct drm_dp_mst_topology_state *state;
>
> - if (WARN_ON(!mgr->state))
> + state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> + if (!state)
> return NULL;
>
> - new_mst_state = kmemdup(mgr->state, sizeof(*new_mst_state), GFP_KERNEL);
> - if (new_mst_state)
> - new_mst_state->state = state;
> - return new_mst_state;
> -}
> -
> -void drm_dp_mst_swap_state(void *obj, void **obj_state_ptr)
> -{
> - struct drm_dp_mst_topology_mgr *mgr = obj;
> - struct drm_dp_mst_topology_state **topology_state_ptr;
> -
> - topology_state_ptr = (struct drm_dp_mst_topology_state **)obj_state_ptr;
> + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
>
> - mgr->state->state = (*topology_state_ptr)->state;
> - swap(*topology_state_ptr, mgr->state);
> - mgr->state->state = NULL;
> + return &state->base;
> }
>
> -void drm_dp_mst_destroy_state(void *obj_state)
> +static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
> + struct drm_private_state *state)
> {
> - kfree(obj_state);
> + struct drm_dp_mst_topology_state *mst_state =
> + to_dp_mst_topology_state(state);
> +
> + kfree(mst_state);
> }
>
> static const struct drm_private_state_funcs mst_state_funcs = {
> - .duplicate_state = drm_dp_mst_duplicate_state,
> - .swap_state = drm_dp_mst_swap_state,
> - .destroy_state = drm_dp_mst_destroy_state,
> + .atomic_duplicate_state = drm_dp_mst_duplicate_state,
> + .atomic_destroy_state = drm_dp_mst_destroy_state,
> };
>
> /**
> @@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
> struct drm_device *dev = mgr->dev;
>
> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> - return drm_atomic_get_private_obj_state(state, mgr,
> - &mst_state_funcs);
> + return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
> }
> EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
>
> @@ -3071,6 +3063,8 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> int max_dpcd_transaction_bytes,
> int max_payloads, int conn_base_id)
> {
> + struct drm_dp_mst_topology_state *mst_state;
> +
> mutex_init(&mgr->lock);
> mutex_init(&mgr->qlock);
> mutex_init(&mgr->payload_lock);
> @@ -3099,14 +3093,18 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> if (test_calc_pbn_mode() < 0)
> DRM_ERROR("MST PBN self-test failed\n");
>
> - mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
> - if (mgr->state == NULL)
> + mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> + if (mst_state == NULL)
> return -ENOMEM;
> - mgr->state->mgr = mgr;
> +
> + mst_state->mgr = mgr;
>
> /* max. time slots - one slot for MTP header */
> - mgr->state->avail_slots = 63;
> - mgr->funcs = &mst_state_funcs;
> + mst_state->avail_slots = 63;
> +
> + drm_atomic_private_obj_init(&mgr->base,
> + &mst_state->base,
> + &mst_state_funcs);
>
> return 0;
> }
> @@ -3128,8 +3126,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
> mutex_unlock(&mgr->payload_lock);
> mgr->dev = NULL;
> mgr->aux = NULL;
> - kfree(mgr->state);
> - mgr->state = NULL;
> + drm_atomic_private_obj_fini(&mgr->base);
> mgr->funcs = NULL;
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 809e8b4c3719..addec49a14bf 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -155,6 +155,9 @@ struct __drm_connectors_state {
> struct drm_connector_state *state, *old_state, *new_state;
> };
>
> +struct drm_private_obj;
> +struct drm_private_state;
> +
> /**
> * struct drm_private_state_funcs - atomic state functions for private objects
> *
> @@ -167,7 +170,7 @@ struct __drm_connectors_state {
> */
> struct drm_private_state_funcs {
> /**
> - * @duplicate_state:
> + * @atomic_duplicate_state:
> *
> * Duplicate the current state of the private object and return it. It
> * is an error to call this before obj->state has been initialized.
> @@ -177,29 +180,30 @@ struct drm_private_state_funcs {
> * Duplicated atomic state or NULL when obj->state is not
> * initialized or allocation failed.
> */
> - void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> + struct drm_private_state *(*atomic_duplicate_state)(struct drm_private_obj *obj);
>
> /**
> - * @swap_state:
> + * @atomic_destroy_state:
> *
> - * This function swaps the existing state of a private object @obj with
> - * it's newly created state, the pointer to which is passed as
> - * @obj_state_ptr.
> + * Frees the private object state created with @atomic_duplicate_state.
> */
> - void (*swap_state)(void *obj, void **obj_state_ptr);
> + void (*atomic_destroy_state)(struct drm_private_obj *obj,
> + struct drm_private_state *state);
> +};
>
> - /**
> - * @destroy_state:
> - *
> - * Frees the private object state created with @duplicate_state.
> - */
> - void (*destroy_state)(void *obj_state);
> +struct drm_private_obj {
> + struct drm_private_state *state;
> +
> + const struct drm_private_state_funcs *funcs;
> +};
> +
> +struct drm_private_state {
> + struct drm_atomic_state *state;
> };
>
> struct __drm_private_objs_state {
> - void *obj;
> - void *obj_state;
> - const struct drm_private_state_funcs *funcs;
> + struct drm_private_obj *ptr;
> + struct drm_private_state *state, *old_state, *new_state;
> };
>
> /**
> @@ -336,10 +340,14 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> struct drm_connector_state *state, struct drm_property *property,
> uint64_t val);
>
> -void * __must_check
> +void drm_atomic_private_obj_init(struct drm_private_obj *obj,
> + struct drm_private_state *state,
> + const struct drm_private_state_funcs *funcs);
> +void drm_atomic_private_obj_fini(struct drm_private_obj *obj);
> +
> +struct drm_private_state * __must_check
> drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> - void *obj,
> - const struct drm_private_state_funcs *funcs);
> + struct drm_private_obj *obj);
>
> /**
> * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> @@ -826,43 +834,66 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> for_each_if (plane)
>
> /**
> - * __for_each_private_obj - iterate over all private objects
> + * for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
> * @__state: &struct drm_atomic_state pointer
> - * @obj: private object iteration cursor
> - * @obj_state: private object state iteration cursor
> + * @obj: &struct drm_private_obj iteration cursor
> + * @old_obj_state: &struct drm_private_state iteration cursor for the old state
> + * @new_obj_state: &struct drm_private_state iteration cursor for the new state
> * @__i: int iteration cursor, for macro-internal use
> - * @__funcs: &struct drm_private_state_funcs iteration cursor
> *
> - * This macro iterates over the array containing private object data in atomic
> - * state
> + * This iterates over all private objects in an atomic update, tracking both
> + * old and new state. This is useful in places where the state delta needs
> + * to be considered, for example in atomic check functions.
> */
> -#define __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \
> - for ((__i) = 0; \
> - (__i) < (__state)->num_private_objs && \
> - ((obj) = __drm_atomic_state_private_obj(__state, __i)->obj, \
> - (__funcs) = __drm_atomic_state_private_obj(__state, __i)->funcs, \
> - (obj_state) = __drm_atomic_state_private_obj(__state, __i)->obj_state, \
> - 1); \
> - (__i)++) \
> +#define for_each_oldnew_private_obj_in_state(__state, obj, old_obj_state, new_obj_state, __i) \
> + for ((__i) = 0; \
> + (__i) < (__state)->num_private_objs && \
> + ((obj) = __drm_atomic_state_private_obj(__state, __i)->ptr, \
> + (old_obj_state) = __drm_atomic_state_private_obj(__state, __i)->old_state, \
> + (new_obj_state) = __drm_atomic_state_private_obj(__state, __i)->new_state, 1); \
> + (__i)++) \
> + for_each_if (obj)
> +
>
> /**
> - * for_each_private_obj - iterate over a specify type of private object
> + * for_each_old_private_obj_in_state - iterate over all private objects in an atomic update
> * @__state: &struct drm_atomic_state pointer
> - * @obj_funcs: &struct drm_private_state_funcs function table to filter
> - * private objects
> - * @obj: private object iteration cursor
> - * @obj_state: private object state iteration cursor
> + * @obj: &struct drm_private_obj iteration cursor
> + * @old_obj_state: &struct drm_private_state iteration cursor for the old state
> * @__i: int iteration cursor, for macro-internal use
> - * @__funcs: &struct drm_private_state_funcs iteration cursor
> *
> - * This macro iterates over the private objects state array while filtering the
> - * objects based on the vfunc table that is passed as @obj_funcs. New macros
> - * can be created by passing in the vfunc table associated with a specific
> - * private object.
> + * This iterates over all private objects in an atomic update, tracking only
> + * the old state. This is useful in disable functions, where we need the old
> + * state the hardware is still in.
> */
> -#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs) \
> - __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \
> - for_each_if (__funcs == obj_funcs)
> +#define for_each_old_private_obj_in_state(__state, obj, old_obj_state, __i) \
> + for ((__i) = 0; \
> + (__i) < (__state)->num_private_objs && \
> + ((obj) = __drm_atomic_state_private_obj(__state, __i)->ptr, \
> + (old_obj_state) = __drm_atomic_state_private_obj(__state, __i)->old_state, 1); \
> + (__i)++) \
> + for_each_if (obj)
> +
> +
> +/**
> + * for_each_new_private_obj_in_state - iterate over all private objects in an atomic update
> + * @__state: &struct drm_atomic_state pointer
> + * @obj: &struct drm_private_obj iteration cursor
> + * @new_obj_state: &struct drm_private_state iteration cursor for the new state
> + * @__i: int iteration cursor, for macro-internal use
> + *
> + * This iterates over all private objects in an atomic update, tracking only
> + * the new state. This is useful in enable functions, where we need the new state the
> + * hardware should be in when the atomic commit operation has completed.
> + */
> +#define for_each_new_private_obj_in_state(__state, obj, new_obj_state, __i) \
> + for ((__i) = 0; \
> + (__i) < (__state)->num_private_objs && \
> + ((obj) = __drm_atomic_state_private_obj(__state, __i)->ptr, \
> + (new_obj_state) = __drm_atomic_state_private_obj(__state, __i)->new_state, 1); \
> + (__i)++) \
> + for_each_if (obj)
> +
>
> /**
> * drm_atomic_crtc_needs_modeset - compute combined modeset need
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index dd196cc0afd7..7db3438ff735 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -33,6 +33,8 @@
> #include <drm/drm_modeset_helper.h>
>
> struct drm_atomic_state;
> +struct drm_private_obj;
> +struct drm_private_state;
>
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> @@ -185,6 +187,8 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> u16 *red, u16 *green, u16 *blue,
> uint32_t size,
> struct drm_modeset_acquire_ctx *ctx);
> +void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj,
> + struct drm_private_state *state);
>
> /**
> * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 177ab6f86855..d55abb75f29a 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -404,12 +404,17 @@ struct drm_dp_payload {
> int vcpi;
> };
>
> +#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
> +
> struct drm_dp_mst_topology_state {
> + struct drm_private_state base;
> int avail_slots;
> struct drm_atomic_state *state;
> struct drm_dp_mst_topology_mgr *mgr;
> };
>
> +#define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base)
> +
> /**
> * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
> *
> @@ -419,6 +424,11 @@ struct drm_dp_mst_topology_state {
> */
> struct drm_dp_mst_topology_mgr {
> /**
> + * @base: Base private object for atomic
> + */
> + struct drm_private_obj base;
> +
> + /**
> * @dev: device pointer for adding i2c devices etc.
> */
> struct drm_device *dev;
> --
> 2.13.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
