| Message ID | 1490221849-11073-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey, There are still 2 unnecessary NULL checks, afaict. Op 22-03-17 om 23:30 schreef Dhinakaran Pandiyan: > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > > 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. > > v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) > v3: Macro alignment (Chris) > v2: Added docs and new iterator to filter private objects (Daniel) > > Acked-by: Harry Wentland <harry.wentland@amd.com> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 5 ++ > include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 167 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 9b892af..e590148 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,21 @@ 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; ^This one. > + 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; > + } > + state->num_private_objs = 0; > + > } > EXPORT_SYMBOL(drm_atomic_state_default_clear); > > @@ -978,6 +994,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 4e26b73..1403334 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2001,6 +2001,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; > > if (stall) { > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > @@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, > state->planes[i].state = old_plane_state; > plane->state = new_plane_state; > } > + > + __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 0147a04..c17da39 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 > @@ -598,6 +654,43 @@ 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 > + * @__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) ^This. > +/** > + * 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 > * ~Maarten
On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote: > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > > 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. > > v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) > v3: Macro alignment (Chris) > v2: Added docs and new iterator to filter private objects (Daniel) > > Acked-by: Harry Wentland <harry.wentland@amd.com> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 5 ++ > include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 167 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 9b892af..e590148 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,21 @@ 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; > + } > + state->num_private_objs = 0; Here we set num_private_objs = 0; > + > } > EXPORT_SYMBOL(drm_atomic_state_default_clear); > > @@ -978,6 +994,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); But here we unconditionally realloc to a presumably smaller size. If you look at drm_atomic_state->num_connector (which also does dynamic array realloc), that one works a bit differently (and hence needs these NULL checks). I think aligning with how we do things with connectors, for consistency (no other reason really) would be good. Just noticed this while reading Maarten's review, which seems to go even farther away from how we handle this for connectors. -Daniel
Op 27-03-17 om 08:38 schreef Daniel Vetter: > On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote: >> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> >> >> 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. >> >> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) >> v3: Macro alignment (Chris) >> v2: Added docs and new iterator to filter private objects (Daniel) >> >> Acked-by: Harry Wentland <harry.wentland@amd.com> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_atomic_helper.c | 5 ++ >> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 167 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 9b892af..e590148 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,21 @@ 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; >> + } >> + state->num_private_objs = 0; > Here we set num_private_objs = 0; > >> + >> } >> EXPORT_SYMBOL(drm_atomic_state_default_clear); >> >> @@ -978,6 +994,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); > But here we unconditionally realloc to a presumably smaller size. If you > look at drm_atomic_state->num_connector (which also does dynamic array > realloc), that one works a bit differently (and hence needs these NULL > checks). > > I think aligning with how we do things with connectors, for consistency > (no other reason really) would be good. > > Just noticed this while reading Maarten's review, which seems to go even > farther away from how we handle this for connectors. > -Daniel Connectors are handled differently, because there's a fixed number of connectors and each connector is assigned to its slot at state->connectors[drm_connector_index]; For private objects this is not the case, there's no way to put them in a fixed index, so the array is resized and reallocated as needed. If you care about the realloc to a smaller size, add a separate variable max_private_objs and multiply its size by 2 every time it's not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)). I don't propose you should though, because N is small enough and the increased complexity isn't worth the decreased readability. So just set num to zero and don't worry about null checks. :) ~Maarten ~Maarten
On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote: > Op 27-03-17 om 08:38 schreef Daniel Vetter: > > On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote: > >> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > >> > >> 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. > >> > >> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) > >> v3: Macro alignment (Chris) > >> v2: Added docs and new iterator to filter private objects (Daniel) > >> > >> Acked-by: Harry Wentland <harry.wentland@amd.com> > >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >> --- > >> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ > >> drivers/gpu/drm/drm_atomic_helper.c | 5 ++ > >> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 167 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >> index 9b892af..e590148 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,21 @@ 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; > >> + } > >> + state->num_private_objs = 0; > > Here we set num_private_objs = 0; > > > >> + > >> } > >> EXPORT_SYMBOL(drm_atomic_state_default_clear); > >> > >> @@ -978,6 +994,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); > > But here we unconditionally realloc to a presumably smaller size. If you > > look at drm_atomic_state->num_connector (which also does dynamic array > > realloc), that one works a bit differently (and hence needs these NULL > > checks). > > > > I think aligning with how we do things with connectors, for consistency > > (no other reason really) would be good. > > > > Just noticed this while reading Maarten's review, which seems to go even > > farther away from how we handle this for connectors. > > -Daniel > > Connectors are handled differently, because there's a fixed number of connectors and each > connector is assigned to its slot at state->connectors[drm_connector_index]; > > For private objects this is not the case, there's no way to put them in a fixed index, > so the array is resized and reallocated as needed. If you care about the realloc to a smaller > size, add a separate variable max_private_objs and multiply its size by 2 every time it's > not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)). > > I don't propose you should though, because N is small enough and the increased complexity > isn't worth the decreased readability. So just set num to zero and don't worry about null > checks. :) Hm, in that case shouldn't we also kfree the allocation in default_clear? Makes no sense resetting to 0 and not freeing, when we do an unconditional krealloc afterwards. That's the part that confused me ... I'm not worried about the realloc overahead (and that's easy to fix indeed). -Daniel
Op 27-03-17 om 10:31 schreef Daniel Vetter: > On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote: >> Op 27-03-17 om 08:38 schreef Daniel Vetter: >>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote: >>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> >>>> >>>> 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. >>>> >>>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) >>>> v3: Macro alignment (Chris) >>>> v2: Added docs and new iterator to filter private objects (Daniel) >>>> >>>> Acked-by: Harry Wentland <harry.wentland@amd.com> >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ >>>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++ >>>> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 167 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>> index 9b892af..e590148 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,21 @@ 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; >>>> + } >>>> + state->num_private_objs = 0; >>> Here we set num_private_objs = 0; >>> >>>> + >>>> } >>>> EXPORT_SYMBOL(drm_atomic_state_default_clear); >>>> >>>> @@ -978,6 +994,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); >>> But here we unconditionally realloc to a presumably smaller size. If you >>> look at drm_atomic_state->num_connector (which also does dynamic array >>> realloc), that one works a bit differently (and hence needs these NULL >>> checks). >>> >>> I think aligning with how we do things with connectors, for consistency >>> (no other reason really) would be good. >>> >>> Just noticed this while reading Maarten's review, which seems to go even >>> farther away from how we handle this for connectors. >>> -Daniel >> Connectors are handled differently, because there's a fixed number of connectors and each >> connector is assigned to its slot at state->connectors[drm_connector_index]; >> >> For private objects this is not the case, there's no way to put them in a fixed index, >> so the array is resized and reallocated as needed. If you care about the realloc to a smaller >> size, add a separate variable max_private_objs and multiply its size by 2 every time it's >> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)). >> >> I don't propose you should though, because N is small enough and the increased complexity >> isn't worth the decreased readability. So just set num to zero and don't worry about null >> checks. :) > Hm, in that case shouldn't we also kfree the allocation in default_clear? > Makes no sense resetting to 0 and not freeing, when we do an > unconditional krealloc afterwards. That's the part that confused me ... > I'm not worried about the realloc overahead (and that's easy to fix > indeed). > -Daniel We might as well, strictly speaking not needed but probably reduces confusion. :)
On Mon, 2017-03-27 at 10:35 +0200, Maarten Lankhorst wrote: > Op 27-03-17 om 10:31 schreef Daniel Vetter: > > On Mon, Mar 27, 2017 at 10:28:42AM +0200, Maarten Lankhorst wrote: > >> Op 27-03-17 om 08:38 schreef Daniel Vetter: > >>> On Wed, Mar 22, 2017 at 03:30:49PM -0700, Dhinakaran Pandiyan wrote: > >>>> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > >>>> > >>>> 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. > >>>> > >>>> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten) > >>>> v3: Macro alignment (Chris) > >>>> v2: Added docs and new iterator to filter private objects (Daniel) > >>>> > >>>> Acked-by: Harry Wentland <harry.wentland@amd.com> > >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >>>> --- > >>>> drivers/gpu/drm/drm_atomic.c | 69 +++++++++++++++++++++++++++ > >>>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++ > >>>> include/drm/drm_atomic.h | 93 +++++++++++++++++++++++++++++++++++++ > >>>> 3 files changed, 167 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>>> index 9b892af..e590148 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,21 @@ 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; > >>>> + } > >>>> + state->num_private_objs = 0; > >>> Here we set num_private_objs = 0; > >>> > >>>> + > >>>> } > >>>> EXPORT_SYMBOL(drm_atomic_state_default_clear); > >>>> > >>>> @@ -978,6 +994,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); > >>> But here we unconditionally realloc to a presumably smaller size. If you > >>> look at drm_atomic_state->num_connector (which also does dynamic array > >>> realloc), that one works a bit differently (and hence needs these NULL > >>> checks). > >>> > >>> I think aligning with how we do things with connectors, for consistency > >>> (no other reason really) would be good. > >>> > >>> Just noticed this while reading Maarten's review, which seems to go even > >>> farther away from how we handle this for connectors. > >>> -Daniel > >> Connectors are handled differently, because there's a fixed number of connectors and each > >> connector is assigned to its slot at state->connectors[drm_connector_index]; > >> > >> For private objects this is not the case, there's no way to put them in a fixed index, > >> so the array is resized and reallocated as needed. If you care about the realloc to a smaller > >> size, add a separate variable max_private_objs and multiply its size by 2 every time it's > >> not big enough. This also changes get_private_obj_state from O(n²) to O(n log(n)). > >> > >> I don't propose you should though, because N is small enough and the increased complexity > >> isn't worth the decreased readability. So just set num to zero and don't worry about null > >> checks. :) > > Hm, in that case shouldn't we also kfree the allocation in default_clear? > > Makes no sense resetting to 0 and not freeing, when we do an > > unconditional krealloc afterwards. That's the part that confused me ... > > I'm not worried about the realloc overahead (and that's easy to fix > > indeed). > > -Daniel > > We might as well, strictly speaking not needed but probably reduces confusion. :) > kfree'ing the state->private_objs array in drm_atomic_state_default_clear() does not seem consistent with what we do for crtcs, planes and connectors. default_release() seems to be the function that frees memory for state arrays. We could just go with v4, where state->num_private_objs is not reset. As 'n' is small, the NULL checks shouldn't be costly. I can look at optimizing realloc's once we have more consumers for private_objs? -DK > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af..e590148 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,21 @@ 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; + } + state->num_private_objs = 0; + } EXPORT_SYMBOL(drm_atomic_state_default_clear); @@ -978,6 +994,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 4e26b73..1403334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2001,6 +2001,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; if (stall) { for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { @@ -2061,6 +2063,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, state->planes[i].state = old_plane_state; plane->state = new_plane_state; } + + __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 0147a04..c17da39 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 @@ -598,6 +654,43 @@ 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 + * @__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 *