Re: [PATCH v7 1/4] drm: Add driver-private objects to atomic state

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: daniel.vetter@ffwll.ch, architt@codeaurora.org,
	Harry.wentland@amd.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v7 1/4] drm: Add driver-private objects to atomic state
Date: Tue, 25 Apr 2017 09:37:51 +0200	[thread overview]
Message-ID: <a1241025-58e9-45bb-2259-41263080e7fa@linux.intel.com> (raw)
In-Reply-To: <1492753893-3748-2-git-send-email-dhinakaran.pandiyan@intel.com>

On 21-04-17 07:51, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> 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.
>
> v6: More kernel-doc to keep 0-day happy
> v5: Remove more NULL checks (Maarten)
> v4: Avoid redundant NULL checks when private_objs array is empty (Maarten)
> v3: Macro alignment (Chris)
> v2: Added docs and new iterator to filter private objects (Daniel)
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Harry Wentland <Harry.wentland@amd.com>
>
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  65 +++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |   5 ++
>  include/drm/drm_atomic.h            | 101 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 171 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 30229ab..8e5f3eb 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,17 @@ 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 *obj_state = state->private_objs[i].obj_state;
> +
> +		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;
> +	}
> +	state->num_private_objs = 0;
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> @@ -978,6 +990,59 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_get_private_obj_state - get private object state
> + * @state: global atomic state
> + * @obj: private object to get the state for
> + * @funcs: pointer to the struct of function pointers that identify the object
> + * type
> + *
> + * This function returns the private object state for the given private object,
> + * allocating the state if needed. It does not grab any locks as the caller is
> + * expected to care of any required locking.
> + *
> + * RETURNS:
> + *
> + * Either the allocated state or the error code encoded into a pointer.
> + */
> +void *
> +drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
> +			      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);
> +
> +	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_private_obj_state);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index cfeda5f..cce05fb 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2032,6 +2032,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct drm_crtc_commit *commit;
> +	void *obj, *obj_state;
> +	const struct drm_private_state_funcs *funcs;
>  
>  	if (stall) {
>  		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -2092,6 +2094,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		state->planes[i].state = old_plane_state;
>  		plane->state = new_plane_state;
>  	}
> +
> +	__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 788daf7..285b3a3 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -155,6 +155,53 @@ struct __drm_connnectors_state {
>  };
>  
>  /**
> + * struct drm_private_state_funcs - atomic state functions for private objects
> + *
> + * These hooks are used by atomic helpers to create, swap and destroy states of
> + * private objects. The structure itself is used as a vtable to identify the
> + * associated private object type. Each private object type that needs to be
> + * added to the atomic states is expected to have an implementation of these
> + * hooks and pass a pointer to it's drm_private_state_funcs struct to
> + * drm_atomic_get_private_obj_state().
> + */
> +struct drm_private_state_funcs {
> +	/**
> +	 * @duplicate_state:
> +	 *
> +	 * Duplicate the current state of the private object and return it. It
> +	 * is an error to call this before obj->state has been initialized.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * Duplicated atomic state or NULL when obj->state is not
> +	 * initialized or allocation failed.
> +	 */
> +	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
> +
> +	/**
> +	 * @swap_state:
> +	 *
> +	 * This function swaps the existing state of a private object @obj with
> +	 * it's newly created state, the pointer to which is passed as
> +	 * @obj_state_ptr.
> +	 */
> +	void (*swap_state)(void *obj, void **obj_state_ptr);
> +
> +	/**
> +	 * @destroy_state:
> +	 *
> +	 * Frees the private object state created with @duplicate_state.
> +	 */
> +	void (*destroy_state)(void *obj_state);
> +};
> +
> +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)
>   * @dev: parent DRM device
> @@ -164,6 +211,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 {
> @@ -176,6 +225,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;
>  
> @@ -268,6 +319,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_private_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
> @@ -753,6 +809,51 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		for_each_if (plane)
>  
>  /**
> + * __for_each_private_obj - iterate over all private objects
> + * @__state: &struct drm_atomic_state pointer
> + * @obj: private object iteration cursor
> + * @obj_state: private object state iteration cursor
> + * @__i: int iteration cursor, for macro-internal use
> + * @__funcs: &struct drm_private_state_funcs iteration cursor
> + *
> + * This macro iterates over the array containing private object data in atomic
> + * 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_private_obj - iterate over a specify type of private object
> + * @__state: &struct drm_atomic_state pointer
> + * @obj_funcs: &struct drm_private_state_funcs function table to filter
> + * 	private objects
> + * @obj: private object iteration cursor
> + * @obj_state: private object state iteration cursor
> + * @__i: int iteration cursor, for macro-internal use
> + * @__funcs: &struct drm_private_state_funcs iteration cursor
> + *
> + * This macro iterates over the private objects state array while filtering the
> + * objects based on the vfunc table that is passed as @obj_funcs. New macros
> + * can be created by passing in the vfunc table associated with a specific
> + * private object.
> + */
> +#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)
> +

Maybe use __for_each_private_obj in this macro for clarity?

#define for_each_private_obj(...) \
	__for_each_private_obj(...) \
		for_each_if (__funcs == obj_funcs) ?

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx