[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 +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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch