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@xxxxxxxx>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
