| Message ID | 1486622291-3524-5-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote: > +#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); \ Align to ( and put the trailing 1 on its own line so it stands out. (__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 > *
On Thu, 2017-02-09 at 08:08 +0000, Chris Wilson wrote: > On Wed, Feb 08, 2017 at 10:38:07PM -0800, Dhinakaran Pandiyan wrote: > > +#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); \ > > Align to ( and put the trailing 1 on its own line so it stands out. Sure, will do that. Looks like I have to change other macros in that file too. -DK > > (__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 > > * >
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@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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 > + > + 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 > * >
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@ffwll.ch> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > 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 > > * > > >
On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote: > 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@ffwll.ch> >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> --- >>> 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. I had a look at the rest of the series, and I couldn't easily understand whether the caller code protects the MST related driver private state. Is it expected to be protect via the drm_mode_config.connection_mutex lock? Thanks, Archit
On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote: > > On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote: > > 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@ffwll.ch> > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >>> --- > >>> 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. > > I had a look at the rest of the series, and I couldn't easily understand > whether the caller code protects the MST related driver private state. Is > it expected to be protect via the drm_mode_config.connection_mutex lock? > > Thanks, > Archit > That's right, the connection_mutex takes care of the locking for the MST private state. I can add that as a comment to the caller's (MST helper) kernel doc with a WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); -DK
On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote: > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote: >> >> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote: >>> 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@ffwll.ch> >>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>>>> --- >>>>> 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. >> >> I had a look at the rest of the series, and I couldn't easily understand >> whether the caller code protects the MST related driver private state. Is >> it expected to be protect via the drm_mode_config.connection_mutex lock? >> >> Thanks, >> Archit >> > > That's right, the connection_mutex takes care of the locking for the MST > private state. I can add that as a comment to the caller's (MST helper) > kernel doc with a > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); That would be nice to have. In the comment: "It does not grab any locks as the caller is expected to care of any required locking.", you could maybe be a bit more specific and rephrase it as "the caller needs to grab the &drm_modeset_lock responsible for protecting the private object state" Thanks, Archit > > > -DK >
On Wed, 2017-02-22 at 09:59 +0530, Archit Taneja wrote: > > On 02/22/2017 05:31 AM, Pandiyan, Dhinakaran wrote: > > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote: > >> > >> On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote: > >>> 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@ffwll.ch> > >>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >>>>> --- > >>>>> 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. > >> > >> I had a look at the rest of the series, and I couldn't easily understand > >> whether the caller code protects the MST related driver private state. Is > >> it expected to be protect via the drm_mode_config.connection_mutex lock? > >> > >> Thanks, > >> Archit > >> > > > > That's right, the connection_mutex takes care of the locking for the MST > > private state. I can add that as a comment to the caller's (MST helper) > > kernel doc with a > > > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > That would be nice to have. > > In the comment: "It does not grab any locks as the caller is expected to > care of any required locking.", you could maybe be a bit more specific > and rephrase it as "the caller needs to grab the &drm_modeset_lock > responsible for protecting the private object state" > > Thanks, > Archit > The core leaves it to the drivers to choose the private-object types to add. So, I believe the core should do not mandate the use of modeset_locks. MST happens to be one example where connection_mutex is the appropriate lock. -DK > > > > > > -DK > > >
On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote: > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote: > > > > On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote: > > > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote: > > >> 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. > > > > I had a look at the rest of the series, and I couldn't easily understand > > whether the caller code protects the MST related driver private state. Is > > it expected to be protect via the drm_mode_config.connection_mutex lock? > > > > Thanks, > > Archit > > > > That's right, the connection_mutex takes care of the locking for the MST > private state. I can add that as a comment to the caller's (MST helper) > kernel doc with a > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); Please don't add this as a comment, but as real code so it is checked at runtime :-) Personally I wouldn't mention locking rules in kernel-doc, that part tends to get outdated fast. Better to enforce with runtime-checks. -Daniel
On Sun, 2017-02-26 at 20:57 +0100, Daniel Vetter wrote: > On Wed, Feb 22, 2017 at 12:01:12AM +0000, Pandiyan, Dhinakaran wrote: > > On Fri, 2017-02-17 at 15:37 +0530, Archit Taneja wrote: > > > > > > On 02/16/2017 05:43 AM, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2017-02-15 at 16:53 +0530, Archit Taneja wrote: > > > >> 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. > > > > > > I had a look at the rest of the series, and I couldn't easily understand > > > whether the caller code protects the MST related driver private state. Is > > > it expected to be protect via the drm_mode_config.connection_mutex lock? > > > > > > Thanks, > > > Archit > > > > > > > That's right, the connection_mutex takes care of the locking for the MST > > private state. I can add that as a comment to the caller's (MST helper) > > kernel doc with a > > > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > Please don't add this as a comment, but as real code so it is checked at > runtime :-) Personally I wouldn't mention locking rules in kernel-doc, > that part tends to get outdated fast. Better to enforce with > runtime-checks. > -Daniel That's what I meant but evidently didn't type it that way:) I was going to add that the connection_mutex does the locking for MST state as a comment and put the WARN_ON() in code. -DK
IRC acked by Harry Wentland "<hwentlan> dhnkrn, the patch for driver-private atomic state object makes sense to me. Didn't realize that's the same one from early February. Feel free to add my Acked-by" -DK On Wed, 2017-02-08 at 22:38 -0800, 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@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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; > + > + 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 > *
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; + + 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 *
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@ffwll.ch> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- 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(+)