[PATCH v7 0/4] Adding driver-private objects to atomic state

[PATCH v7 0/4] Adding driver-private objects to atomic state

[Dhinakaran Pandiyan]

Changes in this version:
Used connector->atomic_check() to release vcpi slots instead of the
atomic_release() callback.

This series introduces void * type driver-private objects in core and adds
helper functions that operate on these private objects. Drivers need to
implement object-specific functions to swap and clear object states. The
advantage of having void * for these objects in the core is objects of different
types can be managed in the same atomic state array. The series implements
DP-MST link bw tracking using the driver-private object infrastructure.

Pandiyan, Dhinakaran (4):
  drm: Add driver-private objects to atomic state
  drm/dp: Introduce MST topology state to track available link bandwidth
  drm/dp: Add DP MST helpers to atomically find and release vcpi slots
  drm/dp: Track MST link bandwidth

 drivers/gpu/drm/drm_atomic.c          |  65 +++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c   |   5 ++
 drivers/gpu/drm/drm_dp_mst_topology.c | 150 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  57 +++++++++++--
 include/drm/drm_atomic.h              | 101 +++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  26 ++++++
 6 files changed, 398 insertions(+), 6 deletions(-)

-- 
2.7.4

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

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

[Dhinakaran Pandiyan]

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)
+
+/**
  * 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

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

[Maarten Lankhorst]

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

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

[Dhinakaran Pandiyan]

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            | 95 +++++++++++++++++++++++++++++++++++++
 3 files changed, 165 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..8645dcd 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,45 @@ 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_each_private_obj(__state, obj, obj_state, __i, __funcs)		\
+		for_each_if (__funcs == obj_funcs)
+
+/**
  * 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

[PATCH v7 2/4] drm/dp: Introduce MST topology state to track available link bandwidth

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

Link bandwidth is shared between multiple display streams in DP MST
configurations. The DP MST topology manager structure maintains the
shared link bandwidth for a primary link directly connected to the GPU. For
atomic modesetting drivers, checking if there is sufficient link bandwidth
for a mode needs to be done during the atomic_check phase to avoid failed
modesets. Let's encapsulate the available link bw information in a
private state structure so that bw can be allocated and released atomically
for each of the ports sharing the primary link.

v3: WARN_ON() if connection_mutex is not held (Archit)
v2: Included kernel doc, moved state initialization and switched to
kmemdup() for allocation (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>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 75 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       | 20 ++++++++++
 2 files changed, 95 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d3fc7e4..0ad0baa 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2936,6 +2936,69 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 		(*mgr->cbs->hotplug)(mgr);
 }
 
+void *drm_dp_mst_duplicate_state(struct drm_atomic_state *state, void *obj)
+{
+	struct drm_dp_mst_topology_mgr *mgr = obj;
+	struct drm_dp_mst_topology_state *new_mst_state;
+
+	if (WARN_ON(!mgr->state))
+		return NULL;
+
+	new_mst_state = kmemdup(mgr->state, sizeof(*new_mst_state), GFP_KERNEL);
+	if (new_mst_state)
+		new_mst_state->state = state;
+	return new_mst_state;
+}
+
+void drm_dp_mst_swap_state(void *obj, void **obj_state_ptr)
+{
+	struct drm_dp_mst_topology_mgr *mgr = obj;
+	struct drm_dp_mst_topology_state **topology_state_ptr;
+
+	topology_state_ptr = (struct drm_dp_mst_topology_state **)obj_state_ptr;
+
+	mgr->state->state = (*topology_state_ptr)->state;
+	swap(*topology_state_ptr, mgr->state);
+	mgr->state->state = NULL;
+}
+
+void drm_dp_mst_destroy_state(void *obj_state)
+{
+	kfree(obj_state);
+}
+
+static const struct drm_private_state_funcs mst_state_funcs = {
+	.duplicate_state = drm_dp_mst_duplicate_state,
+	.swap_state = drm_dp_mst_swap_state,
+	.destroy_state = drm_dp_mst_destroy_state,
+};
+
+/**
+ * drm_atomic_get_mst_topology_state: get MST topology state
+ *
+ * @state: global atomic state
+ * @mgr: MST topology manager, also the private object in this case
+ *
+ * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
+ * state vtable so that the private object state returned is that of a MST
+ * topology object. Also, drm_atomic_get_private_obj_state() expects the caller
+ * to care of the locking, so warn if don't hold the connection_mutex.
+ *
+ * RETURNS:
+ *
+ * The MST topology state or error pointer.
+ */
+struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+								    struct drm_dp_mst_topology_mgr *mgr)
+{
+	struct drm_device *dev = mgr->dev;
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	return drm_atomic_get_private_obj_state(state, mgr,
+						&mst_state_funcs);
+}
+EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
+
 /**
  * drm_dp_mst_topology_mgr_init - initialise a topology manager
  * @mgr: manager struct to initialise
@@ -2980,6 +3043,15 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 	if (test_calc_pbn_mode() < 0)
 		DRM_ERROR("MST PBN self-test failed\n");
 
+	mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
+	if (mgr->state == NULL)
+		return -ENOMEM;
+	mgr->state->mgr = mgr;
+
+	/* max. time slots - one slot for MTP header */
+	mgr->state->avail_slots = 63;
+	mgr->funcs = &mst_state_funcs;
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
@@ -3000,6 +3072,9 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
 	mutex_unlock(&mgr->payload_lock);
 	mgr->dev = NULL;
 	mgr->aux = NULL;
+	kfree(mgr->state);
+	mgr->state = NULL;
+	mgr->funcs = NULL;
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
 
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 5b02476..0b371df 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -24,6 +24,7 @@
 
 #include <linux/types.h>
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_atomic.h>
 
 struct drm_dp_mst_branch;
 
@@ -403,6 +404,12 @@ struct drm_dp_payload {
 	int vcpi;
 };
 
+struct drm_dp_mst_topology_state {
+	int avail_slots;
+	struct drm_atomic_state *state;
+	struct drm_dp_mst_topology_mgr *mgr;
+};
+
 /**
  * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
  *
@@ -481,6 +488,16 @@ struct drm_dp_mst_topology_mgr {
 	int pbn_div;
 
 	/**
+	 * @state: State information for topology manager
+	 */
+	struct drm_dp_mst_topology_state *state;
+
+	/**
+	 * @funcs: Atomic helper callbacks
+	 */
+	const struct drm_private_state_funcs *funcs;
+
+	/**
 	 * @qlock: protects @tx_msg_downq, the &drm_dp_mst_branch.txslost and
 	 * &drm_dp_sideband_msg_tx.state once they are queued
 	 */
@@ -596,4 +613,7 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
+struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
+								    struct drm_dp_mst_topology_mgr *mgr);
+
 #endif
-- 
2.7.4

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

[PATCH v7 3/4] drm/dp: Add DP MST helpers to atomically find and release vcpi slots

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

drm_dp_atomic_find_vcpi_slots() should be called from ->atomic_check() to
check there are sufficient vcpi slots for a mode and to add that to the
state. This should be followed by a call to drm_dp_mst_allocate_vcpi()
in ->atomic_commit() to initialize a struct vcpi for the port.

drm_dp_atomic_release_vcpi_slots() should be called from
->atomic_check() to release a port's vcpi slot allocation from the
state.

Drivers that do not make use of this atomic helper are expected to call
drm_dp_find_vcpi_slots() instead before calling
drm_dp_mst_allocate_vcpi().

v3: drm_dp_atomic_release_vcpi_slots() now needs to know how many slots
    to release as we may not have a valid reference to port.
v2:
Added checks for verifying the port reference is valid
Moved get_mst_topology_state() into the helpers (Daniel)
Changed find_vcpi_slots() to not depend on current allocation

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>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 75 +++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  6 +++
 2 files changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 0ad0baa..d1cbb9c 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2498,6 +2498,81 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 }
 
 /**
+ * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * @state: global atomic state
+ * @mgr: MST topology manager for the port
+ * @port: port to find vcpi slots for
+ * @pbn: bandwidth required for the mode in PBN
+ *
+ * RETURNS:
+ * Total slots in the atomic state assigned for this port or error
+ */
+int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
+				  struct drm_dp_mst_topology_mgr *mgr,
+				  struct drm_dp_mst_port *port, int pbn)
+{
+	struct drm_dp_mst_topology_state *topology_state;
+	int req_slots;
+
+	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
+	if (topology_state == NULL)
+		return -ENOMEM;
+
+	port = drm_dp_get_validated_port_ref(mgr, port);
+	if (port == NULL)
+		return -EINVAL;
+	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+	DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
+			req_slots, topology_state->avail_slots);
+
+	if (req_slots > topology_state->avail_slots) {
+		drm_dp_put_port(port);
+		return -ENOSPC;
+	}
+
+	topology_state->avail_slots -= req_slots;
+	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+
+	drm_dp_put_port(port);
+	return req_slots;
+}
+EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
+
+/**
+ * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
+ * @state: global atomic state
+ * @mgr: MST topology manager for the port
+ * @slots: number of vcpi slots to release
+ *
+ * RETURNS:
+ * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
+ * negative error code
+ */
+int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     int slots)
+{
+	struct drm_dp_mst_topology_state *topology_state;
+
+	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
+	if (topology_state == NULL)
+		return -ENOMEM;
+
+	/* We cannot rely on port->vcpi.num_slots to update
+	 * topology_state->avail_slots as the port may not exist if the parent
+	 * branch device was unplugged. This should be fixed by tracking
+	 * per-port slot allocation in drm_dp_mst_topology_state instead of
+	 * depending on the caller to tell us how many slots to release.
+	 */
+	topology_state->avail_slots += slots;
+	DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
+			slots, topology_state->avail_slots);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
+
+/**
  * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
  * @mgr: manager for this port
  * @port: port to allocate a virtual channel for.
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 0b371df..177ab6f 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -615,5 +615,11 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct drm_dp_mst_topology_mgr *mgr);
+int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
+				  struct drm_dp_mst_topology_mgr *mgr,
+				  struct drm_dp_mst_port *port, int pbn);
+int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
+				     struct drm_dp_mst_topology_mgr *mgr,
+				     int slots);
 
 #endif
-- 
2.7.4

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

[PATCH v7 4/4] drm/dp: Track MST link bandwidth

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

Use the added helpers to track MST link bandwidth for atomic modesets.
Link bw is acquired in the ->atomic_check() phase when CRTCs are being
enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
Similarly, link bw is released during ->atomic_check() with the connector
helper callback ->atomic_release() when CRTCs are disabled.

v5: Implement connector->atomic_check() in place of atomic_release()
v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
v3:
 Handled the case where ->atomic_release() is called more than once
 during an atomic_check() for the same state.
v2:
 Squashed atomic_release() implementation and caller (Daniel)
 Fixed logic for connector-crtc switching case (Daniel)
 Fixed logic for suspend-resume case.

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>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 57 +++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 5af22a7..20c557c 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(conn_state->connector);
-	struct drm_atomic_state *state;
+	struct drm_atomic_state *state = pipe_config->base.state;
 	int bpp;
-	int lane_count, slots;
+	int lane_count, slots = 0;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
 
@@ -63,24 +63,68 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	pipe_config->pipe_bpp = bpp;
 	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
-	state = pipe_config->base.state;
-
 	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
 		pipe_config->has_audio = true;
-	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 
+	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 	pipe_config->pbn = mst_pbn;
-	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
 
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (pipe_config->base.active) {
+		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
+					      connector->port, mst_pbn);
+		if (slots < 0) {
+			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
+			return false;
+		}
+	}
 	pipe_config->dp_m_n.tu = slots;
 
 	return true;
+}
 
+static inline bool release_resources(struct drm_crtc_state *crtc_state)
+{
+	return (crtc_state->connectors_changed ||
+		crtc_state->mode_changed ||
+		(crtc_state->active_changed && !crtc_state->active));
+}
+
+static int intel_dp_mst_atomic_check(struct drm_connector *connector,
+		struct drm_connector_state *new_conn_state)
+{
+	struct drm_atomic_state *state = new_conn_state->state;
+	struct drm_connector_state *old_conn_state;
+	struct drm_crtc *old_crtc;
+	struct drm_crtc_state *crtc_state;
+	int slots, ret = 0;
+
+	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
+	old_crtc = old_conn_state->crtc;
+	if (!old_crtc)
+		return 0;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
+	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
+
+	if (release_resources(crtc_state) && slots > 0) {
+		struct drm_dp_mst_topology_mgr *mgr;
+		struct drm_encoder *old_encoder;
+
+		old_encoder = old_conn_state->best_encoder;
+		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+
+		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
+		if (ret)
+			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
+		else
+			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
+	}
+	return ret;
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,
@@ -378,6 +422,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
 	.mode_valid = intel_dp_mst_mode_valid,
 	.atomic_best_encoder = intel_mst_atomic_best_encoder,
 	.best_encoder = intel_mst_best_encoder,
+	.atomic_check = intel_dp_mst_atomic_check,
 };
 
 static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
-- 
2.7.4

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

Re: [PATCH v7 4/4] drm/dp: Track MST link bandwidth

[Maarten Lankhorst]

On 21-04-17 07:51, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> Use the added helpers to track MST link bandwidth for atomic modesets.
> Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> Similarly, link bw is released during ->atomic_check() with the connector
> helper callback ->atomic_release() when CRTCs are disabled.
>
> v5: Implement connector->atomic_check() in place of atomic_release()
> v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
> v3:
>  Handled the case where ->atomic_release() is called more than once
>  during an atomic_check() for the same state.
> v2:
>  Squashed atomic_release() implementation and caller (Daniel)
>  Fixed logic for connector-crtc switching case (Daniel)
>  Fixed logic for suspend-resume case.
>
> 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>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 57 +++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 5af22a7..20c557c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct intel_connector *connector =
>  		to_intel_connector(conn_state->connector);
> -	struct drm_atomic_state *state;
> +	struct drm_atomic_state *state = pipe_config->base.state;
>  	int bpp;
> -	int lane_count, slots;
> +	int lane_count, slots = 0;
>  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	int mst_pbn;
>  
> @@ -63,24 +63,68 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	pipe_config->pipe_bpp = bpp;
>  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
>  
> -	state = pipe_config->base.state;
> -
>  	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
>  		pipe_config->has_audio = true;
> -	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>  
> +	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>  	pipe_config->pbn = mst_pbn;
> -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
>  
>  	intel_link_compute_m_n(bpp, lane_count,
>  			       adjusted_mode->crtc_clock,
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n);
>  
> +	if (pipe_config->base.active) {
Is this check needed? If so it needs a comment as to why.

I know that the output won't actually turn on, but if the crtc is configured vcpi slots should be allocated if possible.

Else the following will fail in the wrong place:
1. Configure CRTC with a valid mode and DP-MST connector, but set active=false. connectors_changed = true, mode_changed = true. Not enough VCPI slots to enable all crtc's at the same time now, but this returns OK.
2. Commit with active property set to true. There are too few vcpi slots to make this work, this atomic commit returns -EINVAL. This might happen on a different crtc than the newly configured one in stap 1, if they were all previously set to !active.

> +		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> +					      connector->port, mst_pbn);
> +		if (slots < 0) {
> +			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> +			return false;
> +		}
> +	}
>  	pipe_config->dp_m_n.tu = slots;
>  
>  	return true;
> +}
>  
> +static inline bool release_resources(struct drm_crtc_state *crtc_state)
> +{
> +	return (crtc_state->connectors_changed ||
> +		crtc_state->mode_changed ||
> +		(crtc_state->active_changed && !crtc_state->active));
> +}
> +
> +static int intel_dp_mst_atomic_check(struct drm_connector *connector,
> +		struct drm_connector_state *new_conn_state)
> +{
> +	struct drm_atomic_state *state = new_conn_state->state;
> +	struct drm_connector_state *old_conn_state;
> +	struct drm_crtc *old_crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int slots, ret = 0;
> +
> +	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
> +	old_crtc = old_conn_state->crtc;
> +	if (!old_crtc)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
> +	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
> +
> +	if (release_resources(crtc_state) && slots > 0) {
> +		struct drm_dp_mst_topology_mgr *mgr;
> +		struct drm_encoder *old_encoder;
> +
> +		old_encoder = old_conn_state->best_encoder;
> +		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> +
> +		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
> +		if (ret)
> +			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
> +		else
> +			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
> +	}
> +	return ret;
>  }
>  
>  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> @@ -378,6 +422,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
>  	.mode_valid = intel_dp_mst_mode_valid,
>  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
>  	.best_encoder = intel_mst_best_encoder,
> +	.atomic_check = intel_dp_mst_atomic_check,
>  };
>  
>  static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)


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

Re: [PATCH v7 4/4] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Tue, 2017-04-25 at 09:51 +0200, Maarten Lankhorst wrote:
> On 21-04-17 07:51, Dhinakaran Pandiyan wrote:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> >
> > v5: Implement connector->atomic_check() in place of atomic_release()
> > v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
> > v3:
> >  Handled the case where ->atomic_release() is called more than once
> >  during an atomic_check() for the same state.
> > v2:
> >  Squashed atomic_release() implementation and caller (Daniel)
> >  Fixed logic for connector-crtc switching case (Daniel)
> >  Fixed logic for suspend-resume case.
> >
> > 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>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 57 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 51 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 5af22a7..20c557c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct intel_connector *connector =
> >  		to_intel_connector(conn_state->connector);
> > -	struct drm_atomic_state *state;
> > +	struct drm_atomic_state *state = pipe_config->base.state;
> >  	int bpp;
> > -	int lane_count, slots;
> > +	int lane_count, slots = 0;
> >  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >  	int mst_pbn;
> >  
> > @@ -63,24 +63,68 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	pipe_config->pipe_bpp = bpp;
> >  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
> >  
> > -	state = pipe_config->base.state;
> > -
> >  	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
> >  		pipe_config->has_audio = true;
> > -	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  
> > +	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  	pipe_config->pbn = mst_pbn;
> > -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> >  
> >  	intel_link_compute_m_n(bpp, lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n);
> >  
> > +	if (pipe_config->base.active) {
> Is this check needed? If so it needs a comment as to why.
> 
> I know that the output won't actually turn on, but if the crtc is configured vcpi slots should be allocated if possible.
> 
> Else the following will fail in the wrong place:
> 1. Configure CRTC with a valid mode and DP-MST connector, but set active=false. connectors_changed = true, mode_changed = true. Not enough VCPI slots to enable all crtc's at the same time now, but this returns OK.
> 2. Commit with active property set to true. There are too few vcpi slots to make this work, this atomic commit returns -EINVAL. This might happen on a different crtc than the newly configured one in stap 1, if they were all previously set to !active.
> 
Thanks for the review. 
 
I am not sure if it's supposed to fail earlier i.e, when active=false,
connectors_changed = true, mode_changed = true.

In your example, 
1. We have enough vcpi slots for crtc1 and crtc2- for both active=false
and enable=true.
2. Now, crtc3 is also configured with active=false and it does not have
enough bandwidth to be active along with crtc1 and crtc2. 
3. You are saying the problem is that if there is a commit with crtc3,
crtc2, crtc1, in that order, setting active=true, then the commit will
fail?


I don't know how likely this scenario is, but what if userspace is not
going to set crtc1 and crtc2 active=true at all and we return -EINVAL in
Step2 for crtc3?


iiuc we disable pipes, drop payload allocations, disable shared_dpll
when active goes from true->false, even if enable is still true. For eg.
this happens when I 'xset dpms force standby' without these patches. If
we are dropping resources when active goes true->false with enable=true,
shouldn't we also acquire only when active changes from false->true ?


-DK

> > +		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> > +					      connector->port, mst_pbn);
> > +		if (slots < 0) {
> > +			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> > +			return false;
> > +		}
> > +	}
> >  	pipe_config->dp_m_n.tu = slots;
> >  
> >  	return true;
> > +}
> >  
> > +static inline bool release_resources(struct drm_crtc_state *crtc_state)
> > +{
> > +	return (crtc_state->connectors_changed ||
> > +		crtc_state->mode_changed ||
> > +		(crtc_state->active_changed && !crtc_state->active));
> > +}
> > +
> > +static int intel_dp_mst_atomic_check(struct drm_connector *connector,
> > +		struct drm_connector_state *new_conn_state)
> > +{
> > +	struct drm_atomic_state *state = new_conn_state->state;
> > +	struct drm_connector_state *old_conn_state;
> > +	struct drm_crtc *old_crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	int slots, ret = 0;
> > +
> > +	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
> > +	old_crtc = old_conn_state->crtc;
> > +	if (!old_crtc)
> > +		return 0;
> > +
> > +	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
> > +	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
> > +
> > +	if (release_resources(crtc_state) && slots > 0) {
> > +		struct drm_dp_mst_topology_mgr *mgr;
> > +		struct drm_encoder *old_encoder;
> > +
> > +		old_encoder = old_conn_state->best_encoder;
> > +		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> > +
> > +		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
> > +		if (ret)
> > +			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
> > +		else
> > +			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
> > +	}
> > +	return ret;
> >  }
> >  
> >  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> > @@ -378,6 +422,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
> >  	.mode_valid = intel_dp_mst_mode_valid,
> >  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
> >  	.best_encoder = intel_mst_best_encoder,
> > +	.atomic_check = intel_dp_mst_atomic_check,
> >  };
> >  
> >  static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
> 
> 

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

Re: [PATCH v7 4/4] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Tue, 2017-04-25 at 09:51 +0200, Maarten Lankhorst wrote:
> On 21-04-17 07:51, Dhinakaran Pandiyan wrote:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> >
> > v5: Implement connector->atomic_check() in place of atomic_release()
> > v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
> > v3:
> >  Handled the case where ->atomic_release() is called more than once
> >  during an atomic_check() for the same state.
> > v2:
> >  Squashed atomic_release() implementation and caller (Daniel)
> >  Fixed logic for connector-crtc switching case (Daniel)
> >  Fixed logic for suspend-resume case.
> >
> > 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>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 57 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 51 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 5af22a7..20c557c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -39,9 +39,9 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct intel_connector *connector =
> >  		to_intel_connector(conn_state->connector);
> > -	struct drm_atomic_state *state;
> > +	struct drm_atomic_state *state = pipe_config->base.state;
> >  	int bpp;
> > -	int lane_count, slots;
> > +	int lane_count, slots = 0;
> >  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >  	int mst_pbn;
> >  
> > @@ -63,24 +63,68 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  	pipe_config->pipe_bpp = bpp;
> >  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
> >  
> > -	state = pipe_config->base.state;
> > -
> >  	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
> >  		pipe_config->has_audio = true;
> > -	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  
> > +	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >  	pipe_config->pbn = mst_pbn;
> > -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> >  
> >  	intel_link_compute_m_n(bpp, lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n);
> >  
> > +	if (pipe_config->base.active) {
> Is this check needed? If so it needs a comment as to why.
> 
> I know that the output won't actually turn on, but if the crtc is configured vcpi slots should be allocated if possible.
> 
> Else the following will fail in the wrong place:
> 1. Configure CRTC with a valid mode and DP-MST connector, but set active=false. connectors_changed = true, mode_changed = true. Not enough VCPI slots to enable all crtc's at the same time now, but this returns OK.
> 2. Commit with active property set to true. There are too few vcpi slots to make this work, this atomic commit returns -EINVAL. This might happen on a different crtc than the newly configured one in stap 1, if they were all previously set to !active.
> 

I gave this more thought, what you are recommending is not allocating
vcpi for active false->true but instead doing it only for enable
false->true. The problem I see is encoder->compute_config() is called to
configure the link for a modeset with active=false->true transition. If
we don't want to allocate vcpi slots in this case, we should not be
doing other config. computations as well i.e., not call
encoder->compute_config() at all for active_changed. This means we
should complete this FIXME in intel_atomic_check()

 /* FIXME: For only active_changed we shouldn't need to do any
                 * state recomputation at all. */ 

Otoh if you want me to remove 'if (pipe_config->base.active)', we can do
this - 

a) allocate slots unconditionally in compute_config()
        intel_link_compute_m_n(bpp, lane_count,
                               adjusted_mode->crtc_clock,
                               pipe_config->port_clock,
                               &pipe_config->dp_m_n);

+       slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
+                                             connector->port, mst_pbn);
+       if (slots < 0) {
+               DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
+               return false;
+       }
        pipe_config->dp_m_n.tu = slots;

b) release slots on connectors_changed||mode_changed||active_changed. 
 
+       if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
+               struct drm_dp_mst_topology_mgr *mgr;
+               struct drm_encoder *old_encoder;
+
+               old_encoder = old_conn_state->best_encoder;
+               mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+
+               ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
slots);

Full patch - https://pastebin.ubuntu.com/24469988/
This assumes encoder->compute_config() will be called only once for a
state.

-DK



> > +		slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
> > +					      connector->port, mst_pbn);
> > +		if (slots < 0) {
> > +			DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
> > +			return false;
> > +		}
> > +	}
> >  	pipe_config->dp_m_n.tu = slots;
> >  
> >  	return true;
> > +}
> >  
> > +static inline bool release_resources(struct drm_crtc_state *crtc_state)
> > +{
> > +	return (crtc_state->connectors_changed ||
> > +		crtc_state->mode_changed ||
> > +		(crtc_state->active_changed && !crtc_state->active));
> > +}
> > +
> > +static int intel_dp_mst_atomic_check(struct drm_connector *connector,
> > +		struct drm_connector_state *new_conn_state)
> > +{
> > +	struct drm_atomic_state *state = new_conn_state->state;
> > +	struct drm_connector_state *old_conn_state;
> > +	struct drm_crtc *old_crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	int slots, ret = 0;
> > +
> > +	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
> > +	old_crtc = old_conn_state->crtc;
> > +	if (!old_crtc)
> > +		return 0;
> > +
> > +	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
> > +	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
> > +
> > +	if (release_resources(crtc_state) && slots > 0) {
> > +		struct drm_dp_mst_topology_mgr *mgr;
> > +		struct drm_encoder *old_encoder;
> > +
> > +		old_encoder = old_conn_state->best_encoder;
> > +		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> > +
> > +		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
> > +		if (ret)
> > +			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
> > +		else
> > +			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
> > +	}
> > +	return ret;
> >  }
> >  
> >  static void intel_mst_disable_dp(struct intel_encoder *encoder,
> > @@ -378,6 +422,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
> >  	.mode_valid = intel_dp_mst_mode_valid,
> >  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
> >  	.best_encoder = intel_mst_best_encoder,
> > +	.atomic_check = intel_dp_mst_atomic_check,
> >  };
> >  
> >  static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v8 4/4] drm/dp: Track MST link bandwidth

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

Use the added helpers to track MST link bandwidth for atomic modesets.
Link bw is acquired in the ->atomic_check() phase when CRTCs are being
enabled with drm_atomic_find_vcpi_slots(). Similarly, link bw is released
during ->atomic_check() with the connector helper callback ->atomic_check()
when CRTCs are disabled.

v6: active_changed does not alter vcpi allocations (Maarten)
v5: Implement connector->atomic_check() in place of atomic_release()
v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
v3:
 Handled the case where ->atomic_release() is called more than once
 during an atomic_check() for the same state.
v2:
 Squashed atomic_release() implementation and caller (Daniel)
 Fixed logic for connector-crtc switching case (Daniel)
 Fixed logic for suspend-resume case.

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>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 48 ++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 5af22a7..68c788e 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -39,7 +39,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(conn_state->connector);
-	struct drm_atomic_state *state;
+	struct drm_atomic_state *state = pipe_config->base.state;
 	int bpp;
 	int lane_count, slots;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
@@ -57,20 +57,24 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	 * seem to suggest we should do otherwise.
 	 */
 	lane_count = intel_dp_max_lane_count(intel_dp);
-
 	pipe_config->lane_count = lane_count;
 
 	pipe_config->pipe_bpp = bpp;
-	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
-	state = pipe_config->base.state;
+	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
 	if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
 		pipe_config->has_audio = true;
-	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 
+	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 	pipe_config->pbn = mst_pbn;
-	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
+
+	slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr,
+					      connector->port, mst_pbn);
+	if (slots < 0) {
+		DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", slots);
+		return false;
+	}
 
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
@@ -80,7 +84,38 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	pipe_config->dp_m_n.tu = slots;
 
 	return true;
+}
 
+static int intel_dp_mst_atomic_check(struct drm_connector *connector,
+		struct drm_connector_state *new_conn_state)
+{
+	struct drm_atomic_state *state = new_conn_state->state;
+	struct drm_connector_state *old_conn_state;
+	struct drm_crtc *old_crtc;
+	struct drm_crtc_state *crtc_state;
+	int slots, ret = 0;
+
+	old_conn_state = drm_atomic_get_old_connector_state(state, connector);
+	old_crtc = old_conn_state->crtc;
+	if (!old_crtc)
+		return ret;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
+	slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
+	if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
+		struct drm_dp_mst_topology_mgr *mgr;
+		struct drm_encoder *old_encoder;
+
+		old_encoder = old_conn_state->best_encoder;
+		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
+
+		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
+		if (ret)
+			DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
+		else
+			to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
+	}
+	return ret;
 }
 
 static void intel_mst_disable_dp(struct intel_encoder *encoder,
@@ -378,6 +413,7 @@ static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_fun
 	.mode_valid = intel_dp_mst_mode_valid,
 	.atomic_best_encoder = intel_mst_atomic_best_encoder,
 	.best_encoder = intel_mst_best_encoder,
+	.atomic_check = intel_dp_mst_atomic_check,
 };
 
 static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
-- 
2.7.4

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

Re: [PATCH v8 4/4] drm/dp: Track MST link bandwidth

[Maarten Lankhorst]

Op 29-04-17 om 01:14 schreef Dhinakaran Pandiyan:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> Use the added helpers to track MST link bandwidth for atomic modesets.
> Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> enabled with drm_atomic_find_vcpi_slots(). Similarly, link bw is released
> during ->atomic_check() with the connector helper callback ->atomic_check()
> when CRTCs are disabled.
>
> v6: active_changed does not alter vcpi allocations (Maarten)
> v5: Implement connector->atomic_check() in place of atomic_release()
> v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
> v3:
>  Handled the case where ->atomic_release() is called more than once
>  during an atomic_check() for the same state.
> v2:
>  Squashed atomic_release() implementation and caller (Daniel)
>  Fixed logic for connector-crtc switching case (Daniel)
>  Fixed logic for suspend-resume case.
>
> 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>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Missing changes since v7. ;)
Otherwise this looks good, if testbot is happy too then for the whole series:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 4/4] drm/dp: Track MST link bandwidth

[Pandiyan, Dhinakaran]

On Mon, 2017-05-01 at 10:24 +0200, Maarten Lankhorst wrote:
> Op 29-04-17 om 01:14 schreef Dhinakaran Pandiyan:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots(). Similarly, link bw is released
> > during ->atomic_check() with the connector helper callback ->atomic_check()
> > when CRTCs are disabled.
> >
> > v6: active_changed does not alter vcpi allocations (Maarten)
> > v5: Implement connector->atomic_check() in place of atomic_release()
> > v4: Return an int from intel_dp_mst_atomic_release() (Maarten)
> > v3:
> >  Handled the case where ->atomic_release() is called more than once
> >  during an atomic_check() for the same state.
> > v2:
> >  Squashed atomic_release() implementation and caller (Daniel)
> >  Fixed logic for connector-crtc switching case (Daniel)
> >  Fixed logic for suspend-resume case.
> >
> > 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>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Missing changes since v7. ;)
> Otherwise this looks good, if testbot is happy too then for the whole series:
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>


I have updated the version number for the changes only when the specific
patch has changed. But I'll check that again and submit the series for
CI. Thanks a lot for the review!

-DK 


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

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

Re: [PATCH v7 0/4] Adding driver-private objects to atomic state

[Harry Wentland]

Patches 1-3: Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Patch 4: Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry

On 2017-04-21 01:51 AM, Dhinakaran Pandiyan wrote:
> Changes in this version:
> Used connector->atomic_check() to release vcpi slots instead of the
> atomic_release() callback.
>
> This series introduces void * type driver-private objects in core and adds
> helper functions that operate on these private objects. Drivers need to
> implement object-specific functions to swap and clear object states. The
> advantage of having void * for these objects in the core is objects of different
> types can be managed in the same atomic state array. The series implements
> DP-MST link bw tracking using the driver-private object infrastructure.
>
> Pandiyan, Dhinakaran (4):
>   drm: Add driver-private objects to atomic state
>   drm/dp: Introduce MST topology state to track available link bandwidth
>   drm/dp: Add DP MST helpers to atomically find and release vcpi slots
>   drm/dp: Track MST link bandwidth
>
>  drivers/gpu/drm/drm_atomic.c          |  65 +++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c   |   5 ++
>  drivers/gpu/drm/drm_dp_mst_topology.c | 150 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c   |  57 +++++++++++--
>  include/drm/drm_atomic.h              | 101 +++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  26 ++++++
>  6 files changed, 398 insertions(+), 6 deletions(-)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 0/4] Adding driver-private objects to atomic state

[Pandiyan, Dhinakaran]

On Mon, 2017-04-24 at 11:53 -0400, Harry Wentland wrote:
> Patches 1-3: Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> Patch 4: Acked-by: Harry Wentland <harry.wentland@amd.com>
> 
> Harry
> 

Thanks for the review.

-DK



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