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

[Harry Wentland <harry.wentland@amd.com>]

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
>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx