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

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