Re: [Intel-gfx] [PATCH v2 4/9] drm: Add driver private objects to atomic state
On Wed, Jan 25, 2017 at 08:47:09PM +, 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, _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, _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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 4/9] drm: Add driver private objects to atomic state
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> > Signed-off-by: Dhinakaran Pandiyan > > 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(>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", > > +