Re: [Intel-gfx] [PATCH v2 4/9] drm: Add driver private objects to atomic state

[Daniel Vetter]

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

[Pandiyan, Dhinakaran]

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",
> > +