On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote:
> Hi,
>
> On 02/09/2017 12:08 PM, Dhinakaran Pandiyan wrote:
> > It is necessary to track states for objects other than connector, crtc
> > and plane for atomic modesets. But adding objects like DP MST link
> > bandwidth to drm_atomic_state would mean that a non-core object will be
> > modified by the core helper functions for swapping and clearing
> > it's state. So, lets add void * objects and helper functions that operate
> > on void * types to keep these objects and states private to the core.
> > Drivers can then implement specific functions to swap and clear states.
> > The other advantage having just void * for these objects in
> > drm_atomic_state is that objects of different types can be managed in the
> > same state array.
> >
> > v2: Added docs and new iterator to filter private objects (Daniel)
> >
> > Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 68 +++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_atomic_helper.c | 5 ++
> > include/drm/drm_atomic.h | 91 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 164 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index a567310..1a9ffe8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -57,6 +57,7 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > + kfree(state->private_objs);
> > }
> > EXPORT_SYMBOL(drm_atomic_state_default_release);
> >
> > @@ -184,6 +185,20 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > + for (i = 0; i < state->num_private_objs; i++) {
> > + void *private_obj = state->private_objs[i].obj;
> > + void *obj_state = state->private_objs[i].obj_state;
> > +
> > + if (!private_obj)
> > + continue;
> > +
> > + state->private_objs[i].funcs->destroy_state(obj_state);
> > + state->private_objs[i].obj = NULL;
> > + state->private_objs[i].obj_state = NULL;
> > + state->private_objs[i].funcs = NULL;
> > + }
> > +
> > }
> > EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >
> > @@ -974,6 +989,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> > }
> >
> > /**
> > + * 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
> > + * expected to care of any required locking.
> > + *
> > + * RETURNS:
> > + *
> > + * 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)
> > +{
> > + int index, num_objs, i;
> > + size_t size;
> > + struct __drm_private_objs_state *arr;
> > +
> > + for (i = 0; i < state->num_private_objs; i++)
> > + if (obj == state->private_objs[i].obj &&
> > + state->private_objs[i].obj_state)
> > + return state->private_objs[i].obj_state;
>
> Comparing this func to drm_atomic_get_plane_state/drm_atomic_get_crtc_state, it
> doesn't seem to call drm_modeset_lock if the obj_state doesn't already exist. I
> don't understand the locking stuff toowell, I just noticed this difference when
> comparing this approach with what is done in the msm kms driver (where we
> have subclassed drm_atomic_state to msm_kms_state).
>
> Thanks,
> Archit
>
The caller is expected to take care of any required locking. The
driver-private objects are opaque from core's pov, so the core is not
aware of necessary locks for that object type.
-DK
> > +
> > + num_objs = state->num_private_objs + 1;
> > + size = sizeof(*state->private_objs) * num_objs;
> > + arr = krealloc(state->private_objs, size, GFP_KERNEL);
> > + if (!arr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + state->private_objs = arr;
> > + index = state->num_private_objs;
> > + memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
> > +
> > + state->private_objs[index].obj_state = funcs->duplicate_state(state, obj);
> > + if (!state->private_objs[index].obj_state)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + state->private_objs[index].obj = obj;
> > + state->private_objs[index].funcs = funcs;
> > + state->num_private_objs = num_objs;
> > +
> > + DRM_DEBUG_ATOMIC("Added new private object state %p to %p\n",
> > + state->private_objs[index].obj_state, state);
> > +
> > + return state->private_objs[index].obj_state;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
> > +
> > +/**
> > * drm_atomic_get_connector_state - get connector state
> > * @state: global atomic state object
> > * @connector: connector to get state object for
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 3a4383f..8795088 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1983,6 +1983,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > struct drm_plane *plane;
> > struct drm_plane_state *plane_state;
> > struct drm_crtc_commit *commit;
> > + void *obj, *obj_state;
> > + const struct drm_private_state_funcs *funcs;
> >
> > if (stall) {
> > for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > @@ -2031,6 +2033,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > swap(state->planes[i].state, plane->state);
> > plane->state->state = NULL;
> > }
> > +
> > + __for_each_private_obj(state, obj, obj_state, i, funcs)
> > + funcs->swap_state(obj, &state->private_objs[i].obj_state);
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_swap_state);
> >
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 052ab16..cafa404 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -155,6 +155,53 @@ struct __drm_connnectors_state {
> > };
> >
> > /**
> > + * struct drm_private_state_funcs - atomic state functions for private objects
> > + *
> > + * These hooks are used by atomic helpers to create, swap and destroy states of
> > + * private objects. The structure itself is used as a vtable to identify the
> > + * associated private object type. Each private object type that needs to be
> > + * added to the atomic states is expected to have an implementation of these
> > + * hooks and pass a pointer to it's drm_private_state_funcs struct to
> > + * drm_atomic_get_private_obj_state().
> > + */
> > +struct drm_private_state_funcs {
> > + /**
> > + * @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.
> > + *
> > + * RETURNS:
> > + *
> > + * Duplicated atomic state or NULL when obj->state is not
> > + * initialized or allocation failed.
> > + */
> > + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> > +
> > + /**
> > + * @swap_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.
> > + */
> > + void (*swap_state)(void *obj, void **obj_state_ptr);
> > +
> > + /**
> > + * @destroy_state:
> > + *
> > + * Frees the private object state created with @duplicate_state.
> > + */
> > + void (*destroy_state)(void *obj_state);
> > +};
> > +
> > +struct __drm_private_objs_state {
> > + void *obj;
> > + void *obj_state;
> > + const struct drm_private_state_funcs *funcs;
> > +};
> > +
> > +/**
> > * struct drm_atomic_state - the global state object for atomic updates
> > * @ref: count of all references to this state (will not be freed until zero)
> > * @dev: parent DRM device
> > @@ -165,6 +212,8 @@ struct __drm_connnectors_state {
> > * @crtcs: pointer to array of CRTC pointers
> > * @num_connector: size of the @connectors and @connector_states arrays
> > * @connectors: pointer to array of structures with per-connector data
> > + * @num_private_objs: size of the @private_objs array
> > + * @private_objs: pointer to array of private object pointers
> > * @acquire_ctx: acquire context for this atomic modeset state update
> > */
> > struct drm_atomic_state {
> > @@ -178,6 +227,8 @@ struct drm_atomic_state {
> > struct __drm_crtcs_state *crtcs;
> > int num_connector;
> > struct __drm_connnectors_state *connectors;
> > + int num_private_objs;
> > + struct __drm_private_objs_state *private_objs;
> >
> > struct drm_modeset_acquire_ctx *acquire_ctx;
> >
> > @@ -270,6 +321,11 @@ 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
> > +drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> > + void *obj,
> > + const struct drm_private_state_funcs *funcs);
> > +
> > /**
> > * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> > * @state: global atomic state object
> > @@ -415,6 +471,41 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> > for_each_if (plane_state)
> >
> > /**
> > + * __for_each_private_obj - iterate over all private objects
> > + * @__state: the atomic state
> > + *
> > + * This macro iterates over the array containing private object data in atomic
> > + * state
> > + */
> > +#define __for_each_private_obj(__state, obj, obj_state, __i, __funcs) \
> > + for ((__i) = 0; \
> > + (__i) < (__state)->num_private_objs && \
> > + ((obj) = (__state)->private_objs[__i].obj, \
> > + (__funcs) = (__state)->private_objs[__i].funcs, \
> > + (obj_state) = (__state)->private_objs[__i].obj_state, 1); \
> > + (__i)++) \
> > + for_each_if (__funcs)
> > +
> > +/**
> > + * for_each_private_obj - iterate over a specify type of private object
> > + * @__state: the atomic state
> > + * @obj_funcs: function table to filter the private objects
> > + *
> > + * 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.
> > + */
> > +#define for_each_private_obj(__state, obj_funcs, obj, obj_state, __i, __funcs) \
> > + for ((__i) = 0; \
> > + (__i) < (__state)->num_private_objs && \
> > + ((obj) = (__state)->private_objs[__i].obj, \
> > + (__funcs) = (__state)->private_objs[__i].funcs, \
> > + (obj_state) = (__state)->private_objs[__i].obj_state, 1); \
> > + (__i)++) \
> > + for_each_if (__funcs == obj_funcs)
> > +
> > +/**
> > * drm_atomic_crtc_needs_modeset - compute combined modeset need
> > * @state: &drm_crtc_state for the CRTC
> > *
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
