[PATCH 1/3] drm/dp/mst: Handle errors from drm_atomic_get_private_obj_state() correctly

[PATCH 1/3] drm/dp/mst: Handle errors from drm_atomic_get_private_obj_state() correctly

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On failure drm_atomic_get_private_obj_state() returns and error
pointer instead of NULL. Adjust the checks in the callers to match.

Cc: stable@vger.kernel.org
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index bfd237c15e76..18cecd94acb6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2515,8 +2515,8 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 	int req_slots;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
-	if (topology_state == NULL)
-		return -ENOMEM;
+	if (IS_ERR(topology_state))
+		return PTR_ERR(topology_state);
 
 	port = drm_dp_get_validated_port_ref(mgr, port);
 	if (port == NULL)
@@ -2555,8 +2555,8 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 	struct drm_dp_mst_topology_state *topology_state;
 
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
-	if (topology_state == NULL)
-		return -ENOMEM;
+	if (IS_ERR(topology_state))
+		return PTR_ERR(topology_state);
 
 	/* We cannot rely on port->vcpi.num_slots to update
 	 * topology_state->avail_slots as the port may not exist if the parent
-- 
2.13.0

[PATCH 2/3] drm/atomic: Remove pointless private object NULL state check

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We will never add private objects with a NULL state into the atomic
state, hence checking for that is pointless.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 09ca662fcd35..f0482247b31f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1013,8 +1013,7 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
 	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)
+		if (obj == state->private_objs[i].obj)
 			return state->private_objs[i].obj_state;
 
 	num_objs = state->num_private_objs + 1;
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/atomic: Make private objs proper objects

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make the atomic private object stuff less special by introducing proper
base classes for the object and its state. Drivers can embed these in
their own appropriate objects, after which these things will work
exactly like the plane/crtc/connector states during atomic operations.

v2: Reorder to not depend on drm_dynarray (Daniel)

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c          |  78 ++++++++++++++++------
 drivers/gpu/drm/drm_atomic_helper.c   |  30 +++++++--
 drivers/gpu/drm/drm_dp_mst_topology.c |  63 +++++++++---------
 include/drm/drm_atomic.h              | 120 +++++++++++++++++++++-------------
 include/drm/drm_atomic_helper.h       |   4 ++
 include/drm/drm_dp_mst_helper.h       |  10 +++
 6 files changed, 203 insertions(+), 102 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f0482247b31f..b59fd33c5786 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -187,12 +187,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 	}
 
 	for (i = 0; i < state->num_private_objs; i++) {
-		void *obj_state = state->private_objs[i].obj_state;
+		struct drm_private_obj *obj = state->private_objs[i].ptr;
 
-		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;
+		if (!obj)
+			continue;
+
+		obj->funcs->atomic_destroy_state(obj,
+						 state->private_objs[i].state);
+		state->private_objs[i].ptr = NULL;
+		state->private_objs[i].state = NULL;
 	}
 	state->num_private_objs = 0;
 
@@ -990,11 +993,44 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
 }
 
 /**
+ * drm_atomic_private_obj_init - initialize private object
+ * @obj: private object
+ * @state: initial private object state
+ * @funcs: pointer to the struct of function pointers that identify the object
+ * type
+ *
+ * Initialize the private object, which can be embedded into any
+ * driver private object that needs its own atomic state.
+ */
+void
+drm_atomic_private_obj_init(struct drm_private_obj *obj,
+			    struct drm_private_state *state,
+			    const struct drm_private_state_funcs *funcs)
+{
+	memset(obj, 0, sizeof(*obj));
+
+	obj->state = state;
+	obj->funcs = funcs;
+}
+EXPORT_SYMBOL(drm_atomic_private_obj_init);
+
+/**
+ * drm_atomic_private_obj_fini - finalize private object
+ * @obj: private object
+ *
+ * Finalize the private object.
+ */
+void
+drm_atomic_private_obj_fini(struct drm_private_obj *obj)
+{
+	obj->funcs->atomic_destroy_state(obj, obj->state);
+}
+EXPORT_SYMBOL(drm_atomic_private_obj_fini);
+
+/**
  * 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
@@ -1004,17 +1040,18 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
  *
  * 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)
+struct drm_private_state *
+drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
+				 struct drm_private_obj *obj)
 {
 	int index, num_objs, i;
 	size_t size;
 	struct __drm_private_objs_state *arr;
+	struct drm_private_state *obj_state;
 
 	for (i = 0; i < state->num_private_objs; i++)
-		if (obj == state->private_objs[i].obj)
-			return state->private_objs[i].obj_state;
+		if (obj == state->private_objs[i].ptr)
+			return state->private_objs[i].state;
 
 	num_objs = state->num_private_objs + 1;
 	size = sizeof(*state->private_objs) * num_objs;
@@ -1026,18 +1063,21 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, void *obj,
 	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)
+	obj_state = obj->funcs->atomic_duplicate_state(obj);
+	if (!obj_state)
 		return ERR_PTR(-ENOMEM);
 
-	state->private_objs[index].obj = obj;
-	state->private_objs[index].funcs = funcs;
+	state->private_objs[index].state = obj_state;
+	state->private_objs[index].old_state = obj->state;
+	state->private_objs[index].new_state = obj_state;
+	state->private_objs[index].ptr = obj;
+
 	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);
+	DRM_DEBUG_ATOMIC("Added new private object %p state %p to %p\n",
+			 obj, obj_state, state);
 
-	return state->private_objs[index].obj_state;
+	return obj_state;
 }
 EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 667ec97d4efb..9394b49c8cb8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2267,8 +2267,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;
+	struct drm_private_obj *obj;
+	struct drm_private_state *old_obj_state, *new_obj_state;
 
 	if (stall) {
 		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -2330,8 +2330,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *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);
+	for_each_oldnew_private_obj_in_state(state, obj, old_obj_state, new_obj_state, i) {
+		WARN_ON(obj->state != old_obj_state);
+
+		old_obj_state->state = state;
+		new_obj_state->state = NULL;
+
+		state->private_objs[i].state = old_obj_state;
+		obj->state = new_obj_state;
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_swap_state);
 
@@ -3828,3 +3835,18 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	return ret;
 }
 EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
+
+/**
+ * __drm_atomic_helper_private_duplicate_state - copy atomic private state
+ * @obj: CRTC object
+ * @state: new private object state
+ *
+ * Copies atomic state from a private objects's current state and resets inferred values.
+ * This is useful for drivers that subclass the private state.
+ */
+void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj,
+						     struct drm_private_state *state)
+{
+	memcpy(state, obj->state, sizeof(*state));
+}
+EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 18cecd94acb6..552e71d5aa5f 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -31,6 +31,8 @@
 #include <drm/drmP.h>
 
 #include <drm/drm_fixed.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
 
 /**
  * DOC: dp mst helper
@@ -2992,41 +2994,32 @@ 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)
+static struct drm_private_state *
+drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
 {
-	struct drm_dp_mst_topology_mgr *mgr = obj;
-	struct drm_dp_mst_topology_state *new_mst_state;
+	struct drm_dp_mst_topology_state *state;
 
-	if (WARN_ON(!mgr->state))
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!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;
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
-	mgr->state->state = (*topology_state_ptr)->state;
-	swap(*topology_state_ptr, mgr->state);
-	mgr->state->state = NULL;
+	return &state->base;
 }
 
-void drm_dp_mst_destroy_state(void *obj_state)
+static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
+				     struct drm_private_state *state)
 {
-	kfree(obj_state);
+	struct drm_dp_mst_topology_state *mst_state =
+		to_dp_mst_topology_state(state);
+
+	kfree(mst_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,
+	.atomic_duplicate_state = drm_dp_mst_duplicate_state,
+	.atomic_destroy_state = drm_dp_mst_destroy_state,
 };
 
 /**
@@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
 	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);
+	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
 }
 EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
 
@@ -3071,6 +3063,8 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 				 int max_dpcd_transaction_bytes,
 				 int max_payloads, int conn_base_id)
 {
+	struct drm_dp_mst_topology_state *mst_state;
+
 	mutex_init(&mgr->lock);
 	mutex_init(&mgr->qlock);
 	mutex_init(&mgr->payload_lock);
@@ -3099,14 +3093,18 @@ 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)
+	mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
+	if (mst_state == NULL)
 		return -ENOMEM;
-	mgr->state->mgr = mgr;
+
+	mst_state->mgr = mgr;
 
 	/* max. time slots - one slot for MTP header */
-	mgr->state->avail_slots = 63;
-	mgr->funcs = &mst_state_funcs;
+	mst_state->avail_slots = 63;
+
+	drm_atomic_private_obj_init(&mgr->base,
+				    &mst_state->base,
+				    &mst_state_funcs);
 
 	return 0;
 }
@@ -3128,8 +3126,7 @@ 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;
+	drm_atomic_private_obj_fini(&mgr->base);
 	mgr->funcs = NULL;
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index dcc8e0cdb7ff..7cd0f303f5a3 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -154,6 +154,9 @@ struct __drm_connnectors_state {
 	struct drm_connector_state *state, *old_state, *new_state;
 };
 
+struct drm_private_obj;
+struct drm_private_state;
+
 /**
  * struct drm_private_state_funcs - atomic state functions for private objects
  *
@@ -166,7 +169,7 @@ struct __drm_connnectors_state {
  */
 struct drm_private_state_funcs {
 	/**
-	 * @duplicate_state:
+	 * @atomic_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.
@@ -176,29 +179,30 @@ struct drm_private_state_funcs {
 	 * Duplicated atomic state or NULL when obj->state is not
 	 * initialized or allocation failed.
 	 */
-	void *(*duplicate_state)(struct drm_atomic_state *state, void *obj);
+	struct drm_private_state *(*atomic_duplicate_state)(struct drm_private_obj *obj);
 
 	/**
-	 * @swap_state:
+	 * @atomic_destroy_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.
+	 * Frees the private object state created with @atomic_duplicate_state.
 	 */
-	void (*swap_state)(void *obj, void **obj_state_ptr);
+	void (*atomic_destroy_state)(struct drm_private_obj *obj,
+				     struct drm_private_state *state);
+};
 
-	/**
-	 * @destroy_state:
-	 *
-	 * Frees the private object state created with @duplicate_state.
-	 */
-	void (*destroy_state)(void *obj_state);
+struct drm_private_obj {
+	struct drm_private_state *state;
+
+	const struct drm_private_state_funcs *funcs;
+};
+
+struct drm_private_state {
+	struct drm_atomic_state *state;
 };
 
 struct __drm_private_objs_state {
-	void *obj;
-	void *obj_state;
-	const struct drm_private_state_funcs *funcs;
+	struct drm_private_obj *ptr;
+	struct drm_private_state *state, *old_state, *new_state;
 };
 
 /**
@@ -321,10 +325,14 @@ 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
+void drm_atomic_private_obj_init(struct drm_private_obj *obj,
+				 struct drm_private_state *state,
+				 const struct drm_private_state_funcs *funcs);
+void drm_atomic_private_obj_fini(struct drm_private_obj *obj);
+
+struct drm_private_state * __must_check
 drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
-			      void *obj,
-			      const struct drm_private_state_funcs *funcs);
+				 struct drm_private_obj *obj);
 
 /**
  * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
@@ -811,43 +819,63 @@ 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
+ * for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
  * @__state: &struct drm_atomic_state pointer
- * @obj: private object iteration cursor
- * @obj_state: private object state iteration cursor
+ * @obj: &struct drm_private_obj iteration cursor
+ * @old_obj_state: &struct drm_private_state iteration cursor for the old state
+ * @new_obj_state: &struct drm_private_state iteration cursor for the new state
  * @__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
+ * This iterates over all private objects in an atomic update, tracking both
+ * old and new state. This is useful in places where the state delta needs
+ * to be considered, for example in atomic check functions.
  */
-#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)++)							\
+#define for_each_oldnew_private_obj_in_state(__state, obj, old_obj_state, new_obj_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->num_private_objs && \
+		     ((obj) = (__state)->private_objs[__i].ptr, \
+		      (old_obj_state) = (__state)->private_objs[__i].old_state,	\
+		      (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
+	     (__i)++) \
+		for_each_if (obj)
+
+/**
+ * for_each_old_private_obj_in_state - iterate over all private objects in an atomic update
+ * @__state: &struct drm_atomic_state pointer
+ * @obj: &struct drm_private_obj iteration cursor
+ * @old_obj_state: &struct drm_private_state iteration cursor for the old state
+ * @__i: int iteration cursor, for macro-internal use
+ *
+ * This iterates over all private objects in an atomic update, tracking only
+ * the old state. This is useful in disable functions, where we need the old
+ * state the hardware is still in.
+ */
+#define for_each_old_private_obj_in_state(__state, obj, old_obj_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->num_private_objs && \
+		     ((obj) = (__state)->private_objs[__i].ptr, \
+		      (old_obj_state) = (__state)->private_objs[__i].old_state, 1); \
+	     (__i)++) \
+		for_each_if (obj)
 
 /**
- * for_each_private_obj - iterate over a specify type of private object
+ * for_each_new_private_obj_in_state - iterate over all private objects in an atomic update
  * @__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
+ * @obj: &struct drm_private_obj iteration cursor
+ * @new_obj_state: &struct drm_private_state iteration cursor for the new state
  * @__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.
+ * This iterates over all private objects in an atomic update, tracking only
+ * the new state. This is useful in enable functions, where we need the new state the
+ * hardware should be in when the atomic commit operation has completed.
  */
-#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)
+#define for_each_new_private_obj_in_state(__state, obj, new_obj_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->num_private_objs && \
+		     ((obj) = (__state)->private_objs[__i].ptr, \
+		      (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
+	     (__i)++) \
+		for_each_if (obj)
 
 /**
  * drm_atomic_crtc_needs_modeset - compute combined modeset need
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index dd196cc0afd7..7db3438ff735 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -33,6 +33,8 @@
 #include <drm/drm_modeset_helper.h>
 
 struct drm_atomic_state;
+struct drm_private_obj;
+struct drm_private_state;
 
 int drm_atomic_helper_check_modeset(struct drm_device *dev,
 				struct drm_atomic_state *state);
@@ -185,6 +187,8 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 				       u16 *red, u16 *green, u16 *blue,
 				       uint32_t size,
 				       struct drm_modeset_acquire_ctx *ctx);
+void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj,
+						     struct drm_private_state *state);
 
 /**
  * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 177ab6f86855..d55abb75f29a 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -404,12 +404,17 @@ struct drm_dp_payload {
 	int vcpi;
 };
 
+#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
+
 struct drm_dp_mst_topology_state {
+	struct drm_private_state base;
 	int avail_slots;
 	struct drm_atomic_state *state;
 	struct drm_dp_mst_topology_mgr *mgr;
 };
 
+#define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base)
+
 /**
  * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
  *
@@ -419,6 +424,11 @@ struct drm_dp_mst_topology_state {
  */
 struct drm_dp_mst_topology_mgr {
 	/**
+	 * @base: Base private object for atomic
+	 */
+	struct drm_private_obj base;
+
+	/**
 	 * @dev: device pointer for adding i2c devices etc.
 	 */
 	struct drm_device *dev;
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/dp/mst: Handle errors from drm_atomic_get_private_obj_state() correctly

[Pandiyan, Dhinakaran]

On Wed, 2017-07-12 at 18:51 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On failure drm_atomic_get_private_obj_state() returns and error
> pointer instead of NULL. Adjust the checks in the callers to match.
> 
> Cc: stable@vger.kernel.org
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks for fixing this, 
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Fixes: edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index bfd237c15e76..18cecd94acb6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2515,8 +2515,8 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
>  	int req_slots;
>  
>  	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
> -	if (topology_state == NULL)
> -		return -ENOMEM;
> +	if (IS_ERR(topology_state))
> +		return PTR_ERR(topology_state);
>  
>  	port = drm_dp_get_validated_port_ref(mgr, port);
>  	if (port == NULL)
> @@ -2555,8 +2555,8 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>  	struct drm_dp_mst_topology_state *topology_state;
>  
>  	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
> -	if (topology_state == NULL)
> -		return -ENOMEM;
> +	if (IS_ERR(topology_state))
> +		return PTR_ERR(topology_state);
>  
>  	/* We cannot rely on port->vcpi.num_slots to update
>  	 * topology_state->avail_slots as the port may not exist if the parent
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic: Make private objs proper objects

[Jani Nikula]

On Wed, 12 Jul 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the atomic private object stuff less special by introducing proper
> base classes for the object and its state. Drivers can embed these in
> their own appropriate objects, after which these things will work
> exactly like the plane/crtc/connector states during atomic operations.
>
> v2: Reorder to not depend on drm_dynarray (Daniel)
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Stumbled upon an old commit

commit a4370c777406c2810e37fafd166ccddecdb2a60c
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Wed Jul 12 18:51:02 2017 +0300

    drm/atomic: Make private objs proper objects

which is this patch.

> @@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
>  	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);
> +	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
>  }
>  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);

I don't think this combines well with...

> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 177ab6f86855..d55abb75f29a 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -404,12 +404,17 @@ struct drm_dp_payload {
>  	int vcpi;
>  };
>  
> +#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)

...this in case of error pointers that
drm_atomic_get_private_obj_state() may return.

Is that right, or am I just ready to 'shutdown -h now' for 2021...?


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 3/3] drm/atomic: Make private objs proper objects

[Ville Syrjälä]

On Fri, Dec 31, 2021 at 03:23:31PM +0200, Jani Nikula wrote:
> On Wed, 12 Jul 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make the atomic private object stuff less special by introducing proper
> > base classes for the object and its state. Drivers can embed these in
> > their own appropriate objects, after which these things will work
> > exactly like the plane/crtc/connector states during atomic operations.
> >
> > v2: Reorder to not depend on drm_dynarray (Daniel)
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Stumbled upon an old commit
> 
> commit a4370c777406c2810e37fafd166ccddecdb2a60c
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Jul 12 18:51:02 2017 +0300
> 
>     drm/atomic: Make private objs proper objects
> 
> which is this patch.
> 
> > @@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
> >  	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);
> > +	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
> >  }
> >  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> 
> I don't think this combines well with...
> 
> > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> > index 177ab6f86855..d55abb75f29a 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -404,12 +404,17 @@ struct drm_dp_payload {
> >  	int vcpi;
> >  };
> >  
> > +#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
> 
> ...this in case of error pointers that
> drm_atomic_get_private_obj_state() may return.

offsetof(base)==0 so should work in practice.

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/3] drm/atomic: Make private objs proper objects

[Jani Nikula]

On Mon, 10 Jan 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Dec 31, 2021 at 03:23:31PM +0200, Jani Nikula wrote:
>> On Wed, 12 Jul 2017, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Make the atomic private object stuff less special by introducing proper
>> > base classes for the object and its state. Drivers can embed these in
>> > their own appropriate objects, after which these things will work
>> > exactly like the plane/crtc/connector states during atomic operations.
>> >
>> > v2: Reorder to not depend on drm_dynarray (Daniel)
>> >
>> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> Stumbled upon an old commit
>> 
>> commit a4370c777406c2810e37fafd166ccddecdb2a60c
>> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Date:   Wed Jul 12 18:51:02 2017 +0300
>> 
>>     drm/atomic: Make private objs proper objects
>> 
>> which is this patch.
>> 
>> > @@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
>> >  	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);
>> > +	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
>> >  }
>> >  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
>> 
>> I don't think this combines well with...
>> 
>> > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
>> > index 177ab6f86855..d55abb75f29a 100644
>> > --- a/include/drm/drm_dp_mst_helper.h
>> > +++ b/include/drm/drm_dp_mst_helper.h
>> > @@ -404,12 +404,17 @@ struct drm_dp_payload {
>> >  	int vcpi;
>> >  };
>> >  
>> > +#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
>> 
>> ...this in case of error pointers that
>> drm_atomic_get_private_obj_state() may return.
>
> offsetof(base)==0 so should work in practice.

Returning zeros is fine, but error pointers are another matter.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 3/3] drm/atomic: Make private objs proper objects

[Ville Syrjälä]

On Tue, Jan 11, 2022 at 10:34:34AM +0200, Jani Nikula wrote:
> On Mon, 10 Jan 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Dec 31, 2021 at 03:23:31PM +0200, Jani Nikula wrote:
> >> On Wed, 12 Jul 2017, ville.syrjala@linux.intel.com wrote:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > Make the atomic private object stuff less special by introducing proper
> >> > base classes for the object and its state. Drivers can embed these in
> >> > their own appropriate objects, after which these things will work
> >> > exactly like the plane/crtc/connector states during atomic operations.
> >> >
> >> > v2: Reorder to not depend on drm_dynarray (Daniel)
> >> >
> >> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> 
> >> Stumbled upon an old commit
> >> 
> >> commit a4370c777406c2810e37fafd166ccddecdb2a60c
> >> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Date:   Wed Jul 12 18:51:02 2017 +0300
> >> 
> >>     drm/atomic: Make private objs proper objects
> >> 
> >> which is this patch.
> >> 
> >> > @@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_a
> >> >  	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);
> >> > +	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
> >> >  }
> >> >  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> >> 
> >> I don't think this combines well with...
> >> 
> >> > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> >> > index 177ab6f86855..d55abb75f29a 100644
> >> > --- a/include/drm/drm_dp_mst_helper.h
> >> > +++ b/include/drm/drm_dp_mst_helper.h
> >> > @@ -404,12 +404,17 @@ struct drm_dp_payload {
> >> >  	int vcpi;
> >> >  };
> >> >  
> >> > +#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
> >> 
> >> ...this in case of error pointers that
> >> drm_atomic_get_private_obj_state() may return.
> >
> > offsetof(base)==0 so should work in practice.
> 
> Returning zeros is fine, but error pointers are another matter.

Why? This is just 'return ptr-0' so shouldn't matter what you
have in that pointer AFAICS.

-- 
Ville Syrjälä
Intel