| Message ID | 1485301777-3465-5-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 24, 2017 at 03:49:32PM -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. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> I think an overview DOC: section to explain how to subclass atomic state and how to do private state objects would be great. But I can do that once your patch has landed, since it'd be much more about the overall design of atomic (and I promised to do that anyway). Looks pretty good, a few nits below still. I'll also ask amd folks to check this out, and I think Ville could use it for his cdclk stuff. > --- > drivers/gpu/drm/drm_atomic.c | 55 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 6 ++++ > include/drm/drm_atomic.h | 30 ++++++++++++++++++++ > 3 files changed, 91 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 723392f..f3a71cc 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 *priv_obj = state->private_objs[i].obj; > + void *obj_state = state->private_objs[i].obj_state; > + > + if (!priv_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); > > @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, > plane->funcs->atomic_print_state(p, state); > } > > + Needs kerneldoc here. > +void * > +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj, ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to one (I don't care which one). > + 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); I wondered whether we need to allow other error codes than ENOMEM, e.g. if duplicate_state needs to acquire a ww_mutex. But we can always acquire the necessary locks outside of drm_atomic_get_priv_obj_state in some helper/driver wrapper. So no big deal, but worth explaining that drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it doesn't know about them) in the kernel-doc. > + > + 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_priv_obj_state); > + > + > /** > * drm_atomic_get_connector_state - get connector state > * @state: global atomic state object > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 1f0cd7e..dd34d21 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1977,6 +1977,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) { > @@ -2025,6 +2027,10 @@ 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 f1cb2b0..80d6e21 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -153,6 +153,18 @@ struct __drm_connnectors_state { > struct drm_connector_state *state; > }; > This one needs kernel-doc too, since it's a struct that drivers/helpers will need to implement. Please use the inline style and document expected semantics in detail, like we do for other vfunc tables. > +struct drm_private_state_funcs { I also wonder whether we shouldn't give this a drm_atomic_ prefix ... > + void (*swap_state)(void *obj, void **obj_state_ptr); > + void (*destroy_state)(void *obj_state); > + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj); > +}; > + > +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) > @@ -164,6 +176,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 { > @@ -177,6 +191,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; > > @@ -269,6 +285,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_priv_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 > @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); > (__i)++) \ > for_each_if (plane_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) You are not filtering for the function table here, which is important to make sure that this can be used to only walk over objects with a given vtable. With that we can then #define specific macros for e.g. mst: struct drm_private_state_funcs mst_state_funcs; #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \ for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs) I'd place the vfunc right after the state, since those are both input parameters to the macro, and specify what exactly we're looping over. To make this work you need something like: #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) Note the check to compare __funcs == obj_funcs. With that other subsytem can the filter for their own objects only with e.g. #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \ for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs) Would be good to also then have kerneldoc for this iterator, to explain how to use it. -Daniel > /** > * drm_atomic_crtc_needs_modeset - compute combined modeset need > * @state: &drm_crtc_state for the CRTC > -- > 2.7.4 >
On Wed, 2017-01-25 at 06:59 +0100, Daniel Vetter wrote: > On Tue, Jan 24, 2017 at 03:49:32PM -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. > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > I think an overview DOC: section to explain how to subclass atomic state > and how to do private state objects would be great. But I can do that once > your patch has landed, since it'd be much more about the overall design of > atomic (and I promised to do that anyway). > > Looks pretty good, a few nits below still. I'll also ask amd folks to > check this out, and I think Ville could use it for his cdclk stuff. > > > --- > > drivers/gpu/drm/drm_atomic.c | 55 +++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/drm_atomic_helper.c | 6 ++++ > > include/drm/drm_atomic.h | 30 ++++++++++++++++++++ > > 3 files changed, 91 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 723392f..f3a71cc 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 *priv_obj = state->private_objs[i].obj; > > + void *obj_state = state->private_objs[i].obj_state; > > + > > + if (!priv_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); > > > > @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, > > plane->funcs->atomic_print_state(p, state); > > } > > > > + > > Needs kerneldoc here. > > > +void * > > +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj, > > ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to > one (I don't care which one). > > > + 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); > > I wondered whether we need to allow other error codes than ENOMEM, e.g. > if duplicate_state needs to acquire a ww_mutex. But we can always acquire > the necessary locks outside of drm_atomic_get_priv_obj_state in some > helper/driver wrapper. So no big deal, but worth explaining that > drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it > doesn't know about them) in the kernel-doc. > Got it, will include that. > > + > > + 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_priv_obj_state); > > + > > + > > /** > > * drm_atomic_get_connector_state - get connector state > > * @state: global atomic state object > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 1f0cd7e..dd34d21 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1977,6 +1977,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) { > > @@ -2025,6 +2027,10 @@ 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 f1cb2b0..80d6e21 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -153,6 +153,18 @@ struct __drm_connnectors_state { > > struct drm_connector_state *state; > > }; > > > > This one needs kernel-doc too, since it's a struct that drivers/helpers > will need to implement. Please use the inline style and document expected > semantics in detail, like we do for other vfunc tables. > > > +struct drm_private_state_funcs { > > I also wonder whether we shouldn't give this a drm_atomic_ prefix ... > > > + void (*swap_state)(void *obj, void **obj_state_ptr); > > + void (*destroy_state)(void *obj_state); > > + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj); > > +}; > > + > > +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) > > @@ -164,6 +176,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 { > > @@ -177,6 +191,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; > > > > @@ -269,6 +285,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_priv_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 > > @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); > > (__i)++) \ > > for_each_if (plane_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) > > You are not filtering for the function table here, which is important to > make sure that this can be used to only walk over objects with a given > vtable. With that we can then #define specific macros for e.g. mst: > > struct drm_private_state_funcs mst_state_funcs; > > #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \ > for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs) > > I'd place the vfunc right after the state, since those are both input > parameters to the macro, and specify what exactly we're looping over. To > make this work you need something like: > > #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) > > Note the check to compare __funcs == obj_funcs. > > With that other subsytem can the filter for their own objects only with > e.g. > > #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \ > for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs) > > Would be good to also then have kerneldoc for this iterator, to explain > how to use it. > -Daniel > I see your point but we can't use this iterator in the swap_state() helper if we do that. I have used it to swap states for all objects using this version without filtering. I guess, I can just code the iterator explicitly for swap_state() and re-write the iterator with the filtering. -DK > > /** > > * drm_atomic_crtc_needs_modeset - compute combined modeset need > > * @state: &drm_crtc_state for the CRTC > > -- > > 2.7.4 > > >
On 2017-01-25 12:59 AM, Daniel Vetter wrote: > On Tue, Jan 24, 2017 at 03:49:32PM -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. >> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > I think an overview DOC: section to explain how to subclass atomic state > and how to do private state objects would be great. But I can do that once > your patch has landed, since it'd be much more about the overall design of > atomic (and I promised to do that anyway). > > Looks pretty good, a few nits below still. I'll also ask amd folks to > check this out, and I think Ville could use it for his cdclk stuff. Thanks for pinging me again about this. This should help with attaching our context to atomic_state in a clean fashion. I hope to show some patches of it soon after I rebase on top of drm-next + these patches. > >> --- >> drivers/gpu/drm/drm_atomic.c | 55 +++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_atomic_helper.c | 6 ++++ >> include/drm/drm_atomic.h | 30 ++++++++++++++++++++ >> 3 files changed, 91 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 723392f..f3a71cc 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 *priv_obj = state->private_objs[i].obj; >> + void *obj_state = state->private_objs[i].obj_state; >> + >> + if (!priv_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); >> >> @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, >> plane->funcs->atomic_print_state(p, state); >> } >> >> + > > Needs kerneldoc here. > >> +void * >> +drm_atomic_get_priv_obj_state(struct drm_atomic_state *state, void *obj, > > ocd bikeshed: priv_obj vs private_obj inconsistency here, please stick to > one (I don't care which one). > >> + 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); > > I wondered whether we need to allow other error codes than ENOMEM, e.g. > if duplicate_state needs to acquire a ww_mutex. But we can always acquire > the necessary locks outside of drm_atomic_get_priv_obj_state in some > helper/driver wrapper. So no big deal, but worth explaining that > drm_atomic_get_priv_obj_state won't acquire necessarily locsk (since it > doesn't know about them) in the kernel-doc. > >> + >> + 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_priv_obj_state); >> + >> + >> /** >> * drm_atomic_get_connector_state - get connector state >> * @state: global atomic state object >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 1f0cd7e..dd34d21 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -1977,6 +1977,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) { >> @@ -2025,6 +2027,10 @@ 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 f1cb2b0..80d6e21 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -153,6 +153,18 @@ struct __drm_connnectors_state { >> struct drm_connector_state *state; >> }; >> > > This one needs kernel-doc too, since it's a struct that drivers/helpers > will need to implement. Please use the inline style and document expected > semantics in detail, like we do for other vfunc tables. > Kerneldocs would definitely help. >> +struct drm_private_state_funcs { > > I also wonder whether we shouldn't give this a drm_atomic_ prefix ... I think leaving the atomic prefix out is more consistent with the other atomic state objects (drm_crtc_state, etc). drm_xyz_state seems to apply atomic. I don't have a strong preference either way, though. Harry > >> + void (*swap_state)(void *obj, void **obj_state_ptr); >> + void (*destroy_state)(void *obj_state); >> + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj); >> +}; >> + >> +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) >> @@ -164,6 +176,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 { >> @@ -177,6 +191,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; >> >> @@ -269,6 +285,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_priv_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 >> @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); >> (__i)++) \ >> for_each_if (plane_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) > > You are not filtering for the function table here, which is important to > make sure that this can be used to only walk over objects with a given > vtable. With that we can then #define specific macros for e.g. mst: > > struct drm_private_state_funcs mst_state_funcs; > > #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \ > for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs) > > I'd place the vfunc right after the state, since those are both input > parameters to the macro, and specify what exactly we're looping over. To > make this work you need something like: > > #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) > > Note the check to compare __funcs == obj_funcs. > > With that other subsytem can the filter for their own objects only with > e.g. > > #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \ > for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs) > > Would be good to also then have kerneldoc for this iterator, to explain > how to use it. > -Daniel > >> /** >> * drm_atomic_crtc_needs_modeset - compute combined modeset need >> * @state: &drm_crtc_state for the CRTC >> -- >> 2.7.4 >> >
On Wed, Jan 25, 2017 at 08:47:09PM +0000, Pandiyan, Dhinakaran wrote: > On Wed, 2017-01-25 at 06:59 +0100, Daniel Vetter wrote: > > On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote: > > > +#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) > > > > You are not filtering for the function table here, which is important to > > make sure that this can be used to only walk over objects with a given > > vtable. With that we can then #define specific macros for e.g. mst: > > > > struct drm_private_state_funcs mst_state_funcs; > > > > #define for_each_mst_state(__state, obj, obj_state, __i, __funcs) \ > > for_each_private_obj(__state, &mst_state_funcs, obj, obj_state, __i, __funcs) > > > > I'd place the vfunc right after the state, since those are both input > > parameters to the macro, and specify what exactly we're looping over. To > > make this work you need something like: > > > > #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) > > > > Note the check to compare __funcs == obj_funcs. > > > > With that other subsytem can the filter for their own objects only with > > e.g. > > > > #define intel_for_each_cdclk_state(__state, obj, obj_state, __i, __funcs) \ > > for_each_private_obj(__state, &intel_cdclk_state_funcs, obj, obj_state, __i, __funcs) > > > > Would be good to also then have kerneldoc for this iterator, to explain > > how to use it. > > -Daniel > > > > I see your point but we can't use this iterator in the swap_state() > helper if we do that. I have used it to swap states for all objects > using this version without filtering. > > I guess, I can just code the iterator explicitly for swap_state() and > re-write the iterator with the filtering. For swap states I'd use a raw iterator with a __ prefix, which does not have the vtable check. So yes, you need two I think. -Daniel
On Wed, Jan 25, 2017 at 04:33:16PM -0500, Harry Wentland wrote: > On 2017-01-25 12:59 AM, Daniel Vetter wrote: > > On Tue, Jan 24, 2017 at 03:49:32PM -0800, Dhinakaran Pandiyan wrote: > >> +struct drm_private_state_funcs { > > > > I also wonder whether we shouldn't give this a drm_atomic_ prefix ... > > I think leaving the atomic prefix out is more consistent with the other > atomic state objects (drm_crtc_state, etc). drm_xyz_state seems to apply > atomic. > > I don't have a strong preference either way, though. Yeah, makes sense ... I suggested the atomic prefix since it's (at least right now) only used for atomic updates. But just calling them drm_private_{state,obj} makes sense too. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 723392f..f3a71cc 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 *priv_obj = state->private_objs[i].obj; + void *obj_state = state->private_objs[i].obj_state; + + if (!priv_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); @@ -976,6 +991,46 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, plane->funcs->atomic_print_state(p, state); } + +void * +drm_atomic_get_priv_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_priv_obj_state); + + /** * drm_atomic_get_connector_state - get connector state * @state: global atomic state object diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 1f0cd7e..dd34d21 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1977,6 +1977,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) { @@ -2025,6 +2027,10 @@ 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 f1cb2b0..80d6e21 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -153,6 +153,18 @@ struct __drm_connnectors_state { struct drm_connector_state *state; }; +struct drm_private_state_funcs { + void (*swap_state)(void *obj, void **obj_state_ptr); + void (*destroy_state)(void *obj_state); + void *(*duplicate_state)(struct drm_atomic_state *state, void *obj); +}; + +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) @@ -164,6 +176,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 { @@ -177,6 +191,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; @@ -269,6 +285,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_priv_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 @@ -413,6 +434,15 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++) \ for_each_if (plane_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) + /** * 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. Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/drm_atomic.c | 55 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 6 ++++ include/drm/drm_atomic.h | 30 ++++++++++++++++++++ 3 files changed, 91 insertions(+)