[PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.

[PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.

[Maarten Lankhorst]

Fourth iteration. Instead of trying to convert all drivers straight away,
implement all macros that are required to get state working.

Old situation:
Use obj->state, which can refer to old or new state.
Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state.
Use for_each_obj_in_state, which refers to new or old state.

New situation:

During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state,
  or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state,
  without adding the object. This will return NULL if the object is
  not part of the state. For planes and connectors the relevant crtc_state
  is added, so this will work to get the crtc_state from obj_state->crtc
  too, this means not having to write some error handling. 

During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state
  any more, replace with drm_atomic_get_old/new_obj_state calls as required.

During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed.
  oldnew will be renamed to for_each_obj_in_state after all callers are converted
  to the new api.

When not doing atomic updates:
Look at obj->state for now. I have some patches to fix this but I was asked to
make it return a const state. This breaks a lot of users though so I skipped that
patch in this iteration.

This series will return the correct state regardless of swapping.

Maarten Lankhorst (7):
  drm/atomic: Add new iterators over all state, v3.
  drm/atomic: Make add_affected_connectors look at crtc_state.
  drm/atomic: Use new atomic iterator macros.
  drm/atomic: Fix atomic helpers to use the new iterator macros.
  drm/atomic: Add macros to access existing old/new state
  drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.
  drm/blend: Use new atomic iterator macros.

 drivers/gpu/drm/drm_atomic.c         |  39 ++--
 drivers/gpu/drm/drm_atomic_helper.c  | 377 ++++++++++++++++++++---------------
 drivers/gpu/drm/drm_blend.c          |  23 +--
 drivers/gpu/drm/i915/intel_display.c |  13 +-
 include/drm/drm_atomic.h             | 180 ++++++++++++++++-
 include/drm/drm_atomic_helper.h      |   2 +
 6 files changed, 438 insertions(+), 196 deletions(-)

-- 
2.7.4

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

[PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Maarten Lankhorst]

Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
replace the old for_each_xxx_in_state ones. This is useful for >1 flip
depth and getting rid of all xxx->state dereferences.

This requires extra fixups done when committing a state after
duplicating, which in general isn't valid but is used by suspend/resume.
To handle these, introduce drm_atomic_helper_commit_duplicated_state
which performs those fixups before checking & committing the state.

Changes since v1:
- Remove nonblock parameter for commit_duplicated_state.
Changes since v2:
- Use commit_duplicated_state for i915 load detection.
- Add WARN_ON(old_state != obj->state) before swapping.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c         |  6 +++
 drivers/gpu/drm/drm_atomic_helper.c  | 65 +++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c | 13 +++---
 include/drm/drm_atomic.h             | 81 ++++++++++++++++++++++++++++++++++--
 include/drm/drm_atomic_helper.h      |  2 +
 5 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6414bcf7f41b..1c1cbf436717 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -275,6 +275,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 		return ERR_PTR(-ENOMEM);
 
 	state->crtcs[index].state = crtc_state;
+	state->crtcs[index].old_state = crtc->state;
+	state->crtcs[index].new_state = crtc_state;
 	state->crtcs[index].ptr = crtc;
 	crtc_state->state = state;
 
@@ -691,6 +693,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 
 	state->planes[index].state = plane_state;
 	state->planes[index].ptr = plane;
+	state->planes[index].old_state = plane->state;
+	state->planes[index].new_state = plane_state;
 	plane_state->state = state;
 
 	DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
@@ -1031,6 +1035,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
 
 	drm_connector_reference(connector);
 	state->connectors[index].state = connector_state;
+	state->connectors[index].old_state = connector->state;
+	state->connectors[index].new_state = connector_state;
 	state->connectors[index].ptr = connector;
 	connector_state->state = state;
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b26e3419027e..d153e8a72921 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	int i;
 	long ret;
 	struct drm_connector *connector;
-	struct drm_connector_state *conn_state;
+	struct drm_connector_state *conn_state, *old_conn_state;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state, *old_crtc_state;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *plane_state, *old_plane_state;
 	struct drm_crtc_commit *commit;
 
 	if (stall) {
@@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		}
 	}
 
-	for_each_connector_in_state(state, connector, conn_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) {
+		WARN_ON(connector->state != old_conn_state);
+
 		connector->state->state = state;
 		swap(state->connectors[i].state, connector->state);
 		connector->state->state = NULL;
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
+		WARN_ON(crtc->state != old_crtc_state);
+
 		crtc->state->state = state;
 		swap(state->crtcs[i].state, crtc->state);
 		crtc->state->state = NULL;
@@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		}
 	}
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) {
+		WARN_ON(plane->state != old_plane_state);
+
 		plane->state->state = state;
 		swap(state->planes[i].state, plane->state);
 		plane->state->state = NULL;
@@ -2471,7 +2477,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
  *
  * See also:
  * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
- * drm_atomic_helper_resume()
+ * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
  */
 struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
 {
@@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
 EXPORT_SYMBOL(drm_atomic_helper_suspend);
 
 /**
+ * drm_atomic_helper_commit_duplicated_state - commit duplicated state
+ * @state: duplicated atomic state to commit
+ * @ctx: pointer to acquire_ctx to use for commit.
+ *
+ * The state returned by drm_atomic_helper_duplicate_state() and
+ * drm_atomic_helper_suspend() is partially invalid, and needs to
+ * be fixed up before commit.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ *
+ * See also:
+ * drm_atomic_helper_suspend()
+ */
+int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
+					      struct drm_modeset_acquire_ctx *ctx)
+{
+	int i;
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+
+	state->acquire_ctx = ctx;
+
+	for_each_new_plane_in_state(state, plane, plane_state, i)
+		state->planes[i].old_state = plane->state;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+		state->crtcs[i].old_state = crtc->state;
+
+	for_each_new_connector_in_state(state, connector, conn_state, i)
+		state->connectors[i].old_state = connector->state;
+
+	return drm_atomic_commit(state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
+
+/**
  * drm_atomic_helper_resume - subsystem-level resume helper
  * @dev: DRM device
  * @state: atomic state to resume to
@@ -2534,9 +2581,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	int err;
 
 	drm_mode_config_reset(dev);
+
 	drm_modeset_lock_all(dev);
-	state->acquire_ctx = config->acquire_ctx;
-	err = drm_atomic_commit(state);
+	err = drm_atomic_helper_commit_duplicated_state(state, config->acquire_ctx);
 	drm_modeset_unlock_all(dev);
 
 	return err;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f523256ef77c..74da1c380b32 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3488,7 +3488,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
 
 static int
 __intel_display_resume(struct drm_device *dev,
-		       struct drm_atomic_state *state)
+		       struct drm_atomic_state *state,
+		       struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
@@ -3512,7 +3513,7 @@ __intel_display_resume(struct drm_device *dev,
 	/* ignore any reset values/BIOS leftovers in the WM registers */
 	to_intel_atomic_state(state)->skip_intermediate_wm = true;
 
-	ret = drm_atomic_commit(state);
+	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
 
 	WARN_ON(ret == -EDEADLK);
 	return ret;
@@ -3606,7 +3607,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 			 */
 			intel_update_primary_planes(dev);
 		} else {
-			ret = __intel_display_resume(dev, state);
+			ret = __intel_display_resume(dev, state, ctx);
 			if (ret)
 				DRM_ERROR("Restoring old state failed with %i\n", ret);
 		}
@@ -3626,7 +3627,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 			dev_priv->display.hpd_irq_setup(dev_priv);
 		spin_unlock_irq(&dev_priv->irq_lock);
 
-		ret = __intel_display_resume(dev, state);
+		ret = __intel_display_resume(dev, state, ctx);
 		if (ret)
 			DRM_ERROR("Restoring old state failed with %i\n", ret);
 
@@ -11327,7 +11328,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 	if (!state)
 		return;
 
-	ret = drm_atomic_commit(state);
+	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
 	if (ret)
 		DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret);
 	drm_atomic_state_put(state);
@@ -17210,7 +17211,7 @@ void intel_display_resume(struct drm_device *dev)
 	}
 
 	if (!ret)
-		ret = __intel_display_resume(dev, state);
+		ret = __intel_display_resume(dev, state, &ctx);
 
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f96220ed4004..6062e7f27325 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -137,12 +137,12 @@ struct drm_crtc_commit {
 
 struct __drm_planes_state {
 	struct drm_plane *ptr;
-	struct drm_plane_state *state;
+	struct drm_plane_state *state, *old_state, *new_state;
 };
 
 struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
-	struct drm_crtc_state *state;
+	struct drm_crtc_state *state, *old_state, *new_state;
 	struct drm_crtc_commit *commit;
 	s64 __user *out_fence_ptr;
 	unsigned last_vblank_count;
@@ -150,7 +150,7 @@ struct __drm_crtcs_state {
 
 struct __drm_connnectors_state {
 	struct drm_connector *ptr;
-	struct drm_connector_state *state;
+	struct drm_connector_state *state, *old_state, *new_state;
 };
 
 /**
@@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 	     (__i)++)							\
 		for_each_if (connector)
 
+#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \
+	for ((__i) = 0;								\
+	     (__i) < (__state)->num_connector &&				\
+	     ((connector) = (__state)->connectors[__i].ptr,			\
+	     (old_connector_state) = (__state)->connectors[__i].old_state,	\
+	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 	\
+	     (__i)++)							\
+		for_each_if (connector)
+
+#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \
+	for ((__i) = 0;								\
+	     (__i) < (__state)->num_connector &&				\
+	     ((connector) = (__state)->connectors[__i].ptr,			\
+	     (old_connector_state) = (__state)->connectors[__i].old_state, 1); 	\
+	     (__i)++)							\
+		for_each_if (connector)
+
+#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \
+	for ((__i) = 0;								\
+	     (__i) < (__state)->num_connector &&				\
+	     ((connector) = (__state)->connectors[__i].ptr,			\
+	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 	\
+	     (__i)++)							\
+		for_each_if (connector)
+
 #define for_each_crtc_in_state(__state, crtc, crtc_state, __i)	\
 	for ((__i) = 0;						\
 	     (__i) < (__state)->dev->mode_config.num_crtc &&	\
@@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 	     (__i)++)						\
 		for_each_if (crtc_state)
 
+#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \
+	for ((__i) = 0;							\
+	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
+	     ((crtc) = (__state)->crtcs[__i].ptr,			\
+	     (old_crtc_state) = (__state)->crtcs[__i].old_state,	\
+	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
+	     (__i)++)							\
+		for_each_if (crtc)
+
+#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i)	\
+	for ((__i) = 0;							\
+	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
+	     ((crtc) = (__state)->crtcs[__i].ptr,			\
+	     (old_crtc_state) = (__state)->crtcs[__i].old_state, 1);	\
+	     (__i)++)							\
+		for_each_if (crtc)
+
+#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i)	\
+	for ((__i) = 0;							\
+	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
+	     ((crtc) = (__state)->crtcs[__i].ptr,			\
+	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
+	     (__i)++)							\
+		for_each_if (crtc)
+
 #define for_each_plane_in_state(__state, plane, plane_state, __i)		\
 	for ((__i) = 0;							\
 	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
@@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 	     (__i)++)							\
 		for_each_if (plane_state)
 
+#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \
+	for ((__i) = 0;							\
+	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
+	     ((plane) = (__state)->planes[__i].ptr,			\
+	     (old_plane_state) = (__state)->planes[__i].old_state,	\
+	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
+	     (__i)++)							\
+		for_each_if (plane)
+
+#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \
+	for ((__i) = 0;							\
+	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
+	     ((plane) = (__state)->planes[__i].ptr,			\
+	     (old_plane_state) = (__state)->planes[__i].old_state, 1);	\
+	     (__i)++)							\
+		for_each_if (plane)
+
+#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \
+	for ((__i) = 0;							\
+	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
+	     ((plane) = (__state)->planes[__i].ptr,			\
+	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
+	     (__i)++)							\
+		for_each_if (plane)
+
 /**
  * drm_atomic_crtc_needs_modeset - compute combined modeset need
  * @state: &drm_crtc_state for the CRTC
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9afcd3810785..2c4549e98c16 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -105,6 +105,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx);
 struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
+int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
+					      struct drm_modeset_acquire_ctx *ctx);
 int drm_atomic_helper_resume(struct drm_device *dev,
 			     struct drm_atomic_state *state);
 
-- 
2.7.4

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

Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> depth and getting rid of all xxx->state dereferences.
> 
> This requires extra fixups done when committing a state after
> duplicating, which in general isn't valid but is used by suspend/resume.
> To handle these, introduce drm_atomic_helper_commit_duplicated_state
> which performs those fixups before checking & committing the state.
> 
> Changes since v1:
> - Remove nonblock parameter for commit_duplicated_state.
> Changes since v2:
> - Use commit_duplicated_state for i915 load detection.
> - Add WARN_ON(old_state != obj->state) before swapping.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c         |  6 +++
>  drivers/gpu/drm/drm_atomic_helper.c  | 65 +++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_display.c | 13 +++---
>  include/drm/drm_atomic.h             | 81 +++++++++++++++++++++++++++++++--
>  include/drm/drm_atomic_helper.h      |  2 +
>  5 files changed, 149 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6414bcf7f41b..1c1cbf436717 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -275,6 +275,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state
> *state, return ERR_PTR(-ENOMEM);
> 
>  	state->crtcs[index].state = crtc_state;
> +	state->crtcs[index].old_state = crtc->state;
> +	state->crtcs[index].new_state = crtc_state;
>  	state->crtcs[index].ptr = crtc;
>  	crtc_state->state = state;
> 
> @@ -691,6 +693,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state
> *state,
> 
>  	state->planes[index].state = plane_state;
>  	state->planes[index].ptr = plane;
> +	state->planes[index].old_state = plane->state;
> +	state->planes[index].new_state = plane_state;
>  	plane_state->state = state;
> 
>  	DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
> @@ -1031,6 +1035,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state
> *state,
> 
>  	drm_connector_reference(connector);
>  	state->connectors[index].state = connector_state;
> +	state->connectors[index].old_state = connector->state;
> +	state->connectors[index].new_state = connector_state;
>  	state->connectors[index].ptr = connector;
>  	connector_state->state = state;
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, int i;
>  	long ret;
>  	struct drm_connector *connector;
> -	struct drm_connector_state *conn_state;
> +	struct drm_connector_state *conn_state, *old_conn_state;
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *crtc_state, *old_crtc_state;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *plane_state, *old_plane_state;
>  	struct drm_crtc_commit *commit;
> 
>  	if (stall) {
> @@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, }
>  	}
> 
> -	for_each_connector_in_state(state, connector, conn_state, i) {
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> conn_state, i) {
> +		WARN_ON(connector->state != old_conn_state);
> +
>  		connector->state->state = state;
>  		swap(state->connectors[i].state, connector->state);
>  		connector->state->state = NULL;
>  	}
> 
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state,
> i) {
> +		WARN_ON(crtc->state != old_crtc_state);
> +
>  		crtc->state->state = state;
>  		swap(state->crtcs[i].state, crtc->state);
>  		crtc->state->state = NULL;
> @@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, }
>  	}
> 
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> plane_state, i) {
> +		WARN_ON(plane->state != old_plane_state);
> +
>  		plane->state->state = state;
>  		swap(state->planes[i].state, plane->state);
>  		plane->state->state = NULL;
> @@ -2471,7 +2477,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>   *
>   * See also:
>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
> - * drm_atomic_helper_resume()
> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
>   */
>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
>  {
> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
> *drm_atomic_helper_suspend(struct drm_device *dev)
> EXPORT_SYMBOL(drm_atomic_helper_suspend);
> 
>  /**
> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> + * @state: duplicated atomic state to commit
> + * @ctx: pointer to acquire_ctx to use for commit.
> + *
> + * The state returned by drm_atomic_helper_duplicate_state() and
> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> + * be fixed up before commit.

Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for 
this function ?

Apart from this the patch looks good to me. I don't like the fact that we now 
have two sets of states though (state and old_state/new_state). I understand 
that you'd like to address this as part of a separate patch series, which I'm 
fine with to avoid delaying this one, but I think we should address this 
sooner rather than latter, or the API will become very confusing. Yes, that 
means mass-patching drivers I'm afraid. Could you confirm that this is your 
plan ?

> + * Returns:
> + * 0 on success or a negative error code on failure.
> + *
> + * See also:
> + * drm_atomic_helper_suspend()
> + */
> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
> *state,
> +					      struct drm_modeset_acquire_ctx
> *ctx)
> +{
> +	int i;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +
> +	state->acquire_ctx = ctx;
> +
> +	for_each_new_plane_in_state(state, plane, plane_state, i)
> +		state->planes[i].old_state = plane->state;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> +		state->crtcs[i].old_state = crtc->state;
> +
> +	for_each_new_connector_in_state(state, connector, conn_state, i)
> +		state->connectors[i].old_state = connector->state;
> +
> +	return drm_atomic_commit(state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
> +
> +/**
>   * drm_atomic_helper_resume - subsystem-level resume helper
>   * @dev: DRM device
>   * @state: atomic state to resume to
> @@ -2534,9 +2581,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>  	int err;
> 
>  	drm_mode_config_reset(dev);
> +
>  	drm_modeset_lock_all(dev);
> -	state->acquire_ctx = config->acquire_ctx;
> -	err = drm_atomic_commit(state);
> +	err = drm_atomic_helper_commit_duplicated_state(state,
> config->acquire_ctx);
> 	drm_modeset_unlock_all(dev);
> 
>  	return err;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index f523256ef77c..74da1c380b32
> 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3488,7 +3488,8 @@ static void intel_update_primary_planes(struct
> drm_device *dev)
> 
>  static int
>  __intel_display_resume(struct drm_device *dev,
> -		       struct drm_atomic_state *state)
> +		       struct drm_atomic_state *state,
> +		       struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> @@ -3512,7 +3513,7 @@ __intel_display_resume(struct drm_device *dev,
>  	/* ignore any reset values/BIOS leftovers in the WM registers */
>  	to_intel_atomic_state(state)->skip_intermediate_wm = true;
> 
> -	ret = drm_atomic_commit(state);
> +	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
> 
>  	WARN_ON(ret == -EDEADLK);
>  	return ret;
> @@ -3606,7 +3607,7 @@ void intel_finish_reset(struct drm_i915_private
> *dev_priv) */
>  			intel_update_primary_planes(dev);
>  		} else {
> -			ret = __intel_display_resume(dev, state);
> +			ret = __intel_display_resume(dev, state, ctx);
>  			if (ret)
>  				DRM_ERROR("Restoring old state failed with 
%i\n", ret);
>  		}
> @@ -3626,7 +3627,7 @@ void intel_finish_reset(struct drm_i915_private
> *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv);
>  		spin_unlock_irq(&dev_priv->irq_lock);
> 
> -		ret = __intel_display_resume(dev, state);
> +		ret = __intel_display_resume(dev, state, ctx);
>  		if (ret)
>  			DRM_ERROR("Restoring old state failed with %i\n", 
ret);
> 
> @@ -11327,7 +11328,7 @@ void intel_release_load_detect_pipe(struct
> drm_connector *connector, if (!state)
>  		return;
> 
> -	ret = drm_atomic_commit(state);
> +	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
>  	if (ret)
>  		DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret);
>  	drm_atomic_state_put(state);
> @@ -17210,7 +17211,7 @@ void intel_display_resume(struct drm_device *dev)
>  	}
> 
>  	if (!ret)
> -		ret = __intel_display_resume(dev, state);
> +		ret = __intel_display_resume(dev, state, &ctx);
> 
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index f96220ed4004..6062e7f27325 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -137,12 +137,12 @@ struct drm_crtc_commit {
> 
>  struct __drm_planes_state {
>  	struct drm_plane *ptr;
> -	struct drm_plane_state *state;
> +	struct drm_plane_state *state, *old_state, *new_state;
>  };
> 
>  struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
> -	struct drm_crtc_state *state;
> +	struct drm_crtc_state *state, *old_state, *new_state;
>  	struct drm_crtc_commit *commit;
>  	s64 __user *out_fence_ptr;
>  	unsigned last_vblank_count;
> @@ -150,7 +150,7 @@ struct __drm_crtcs_state {
> 
>  struct __drm_connnectors_state {
>  	struct drm_connector *ptr;
> -	struct drm_connector_state *state;
> +	struct drm_connector_state *state, *old_state, *new_state;
>  };
> 
>  /**
> @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct
> drm_printer *p); (__i)++)							
\
>  		for_each_if (connector)
> 
> +#define for_each_oldnew_connector_in_state(__state, connector,
> old_connector_state, new_connector_state, __i) \
> +	for ((__i) = 0;								
\
> +	     (__i) < (__state)->num_connector &&				
\
> +	     ((connector) = (__state)->connectors[__i].ptr,			
\
> +	     (old_connector_state) = (__state)->connectors[__i].old_state,	
\
> +	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 
	\
> +	     (__i)++)							\
> +		for_each_if (connector)
> +
> +#define for_each_old_connector_in_state(__state, connector,
> old_connector_state, __i) \ +	for ((__i) = 0;					
			\
> +	     (__i) < (__state)->num_connector &&				
\
> +	     ((connector) = (__state)->connectors[__i].ptr,			
\
> +	     (old_connector_state) = (__state)->connectors[__i].old_state, 1); 
	\
> +	     (__i)++)							\
> +		for_each_if (connector)
> +
> +#define for_each_new_connector_in_state(__state, connector,
> new_connector_state, __i) \ +	for ((__i) = 0;					
			\
> +	     (__i) < (__state)->num_connector &&				
\
> +	     ((connector) = (__state)->connectors[__i].ptr,			
\
> +	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 
	\
> +	     (__i)++)							\
> +		for_each_if (connector)
> +
>  #define for_each_crtc_in_state(__state, crtc, crtc_state, __i)	\
>  	for ((__i) = 0;						\
>  	     (__i) < (__state)->dev->mode_config.num_crtc &&	\
> @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct
> drm_printer *p); (__i)++)						\
>  		for_each_if (crtc_state)
> 
> +#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state,
> new_crtc_state, __i) \ +	for ((__i) = 0;					
		\
> +	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
> +	     ((crtc) = (__state)->crtcs[__i].ptr,			\
> +	     (old_crtc_state) = (__state)->crtcs[__i].old_state,	\
> +	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
> +	     (__i)++)							\
> +		for_each_if (crtc)
> +
> +#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i)	
\
> +	for ((__i) = 0;							\
> +	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
> +	     ((crtc) = (__state)->crtcs[__i].ptr,			\
> +	     (old_crtc_state) = (__state)->crtcs[__i].old_state, 1);	\
> +	     (__i)++)							\
> +		for_each_if (crtc)
> +
> +#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i)	
\
> +	for ((__i) = 0;							\
> +	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
> +	     ((crtc) = (__state)->crtcs[__i].ptr,			\
> +	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
> +	     (__i)++)							\
> +		for_each_if (crtc)
> +
>  #define for_each_plane_in_state(__state, plane, plane_state, __i)		
\
>  	for ((__i) = 0;							\
>  	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
> @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct
> drm_printer *p); (__i)++)							
\
>  		for_each_if (plane_state)
> 
> +#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state,
> new_plane_state, __i) \ +	for ((__i) = 0;					
		\
> +	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
> +	     ((plane) = (__state)->planes[__i].ptr,			\
> +	     (old_plane_state) = (__state)->planes[__i].old_state,	\
> +	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
> +	     (__i)++)							\
> +		for_each_if (plane)
> +
> +#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \
> +	for ((__i) = 0;							\
> +	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
> +	     ((plane) = (__state)->planes[__i].ptr,			\
> +	     (old_plane_state) = (__state)->planes[__i].old_state, 1);	\
> +	     (__i)++)							\
> +		for_each_if (plane)
> +
> +#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \
> +	for ((__i) = 0;							\
> +	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
> +	     ((plane) = (__state)->planes[__i].ptr,			\
> +	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
> +	     (__i)++)							\
> +		for_each_if (plane)
> +
>  /**
>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>   * @state: &drm_crtc_state for the CRTC
> diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h index 9afcd3810785..2c4549e98c16 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -105,6 +105,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set
> *set, int drm_atomic_helper_disable_all(struct drm_device *dev,
>  				  struct drm_modeset_acquire_ctx *ctx);
>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
> *state,
> +					      struct drm_modeset_acquire_ctx
> *ctx);
>  int drm_atomic_helper_resume(struct drm_device *dev,
>  			     struct drm_atomic_state *state);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Maarten Lankhorst]

Op 17-01-17 om 00:11 schreef Laurent Pinchart:
> Hi Maarten,
>
> Thank you for the patch.
>
> On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
>> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
>> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
>> depth and getting rid of all xxx->state dereferences.
>>
>> This requires extra fixups done when committing a state after
>> duplicating, which in general isn't valid but is used by suspend/resume.
>> To handle these, introduce drm_atomic_helper_commit_duplicated_state
>> which performs those fixups before checking & committing the state.
>>
>> Changes since v1:
>> - Remove nonblock parameter for commit_duplicated_state.
>> Changes since v2:
>> - Use commit_duplicated_state for i915 load detection.
>> - Add WARN_ON(old_state != obj->state) before swapping.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c         |  6 +++
>>  drivers/gpu/drm/drm_atomic_helper.c  | 65 +++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_display.c | 13 +++---
>>  include/drm/drm_atomic.h             | 81 +++++++++++++++++++++++++++++++--
>>  include/drm/drm_atomic_helper.h      |  2 +
>>  5 files changed, 149 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 6414bcf7f41b..1c1cbf436717 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -275,6 +275,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state
>> *state, return ERR_PTR(-ENOMEM);
>>
>>  	state->crtcs[index].state = crtc_state;
>> +	state->crtcs[index].old_state = crtc->state;
>> +	state->crtcs[index].new_state = crtc_state;
>>  	state->crtcs[index].ptr = crtc;
>>  	crtc_state->state = state;
>>
>> @@ -691,6 +693,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state
>> *state,
>>
>>  	state->planes[index].state = plane_state;
>>  	state->planes[index].ptr = plane;
>> +	state->planes[index].old_state = plane->state;
>> +	state->planes[index].new_state = plane_state;
>>  	plane_state->state = state;
>>
>>  	DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
>> @@ -1031,6 +1035,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state
>> *state,
>>
>>  	drm_connector_reference(connector);
>>  	state->connectors[index].state = connector_state;
>> +	state->connectors[index].old_state = connector->state;
>> +	state->connectors[index].new_state = connector_state;
>>  	state->connectors[index].ptr = connector;
>>  	connector_state->state = state;
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct
>> drm_atomic_state *state, int i;
>>  	long ret;
>>  	struct drm_connector *connector;
>> -	struct drm_connector_state *conn_state;
>> +	struct drm_connector_state *conn_state, *old_conn_state;
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *crtc_state, *old_crtc_state;
>>  	struct drm_plane *plane;
>> -	struct drm_plane_state *plane_state;
>> +	struct drm_plane_state *plane_state, *old_plane_state;
>>  	struct drm_crtc_commit *commit;
>>
>>  	if (stall) {
>> @@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct
>> drm_atomic_state *state, }
>>  	}
>>
>> -	for_each_connector_in_state(state, connector, conn_state, i) {
>> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state,
>> conn_state, i) {
>> +		WARN_ON(connector->state != old_conn_state);
>> +
>>  		connector->state->state = state;
>>  		swap(state->connectors[i].state, connector->state);
>>  		connector->state->state = NULL;
>>  	}
>>
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state,
>> i) {
>> +		WARN_ON(crtc->state != old_crtc_state);
>> +
>>  		crtc->state->state = state;
>>  		swap(state->crtcs[i].state, crtc->state);
>>  		crtc->state->state = NULL;
>> @@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct
>> drm_atomic_state *state, }
>>  	}
>>
>> -	for_each_plane_in_state(state, plane, plane_state, i) {
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
>> plane_state, i) {
>> +		WARN_ON(plane->state != old_plane_state);
>> +
>>  		plane->state->state = state;
>>  		swap(state->planes[i].state, plane->state);
>>  		plane->state->state = NULL;
>> @@ -2471,7 +2477,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>>   *
>>   * See also:
>>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
>> - * drm_atomic_helper_resume()
>> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
>>   */
>>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
>>  {
>> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
>> *drm_atomic_helper_suspend(struct drm_device *dev)
>> EXPORT_SYMBOL(drm_atomic_helper_suspend);
>>
>>  /**
>> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
>> + * @state: duplicated atomic state to commit
>> + * @ctx: pointer to acquire_ctx to use for commit.
>> + *
>> + * The state returned by drm_atomic_helper_duplicate_state() and
>> + * drm_atomic_helper_suspend() is partially invalid, and needs to
>> + * be fixed up before commit.
> Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for 
> this function ?
We would have to fix up other areas that duplicate state too, like i915 suspend and load detect. This means moving it to a helper.

It was my original approach, but Daniel preferred this approach.
> Apart from this the patch looks good to me. I don't like the fact that we now 
> have two sets of states though (state and old_state/new_state). I understand 
> that you'd like to address this as part of a separate patch series, which I'm 
> fine with to avoid delaying this one, but I think we should address this 
> sooner rather than latter, or the API will become very confusing. Yes, that 
> means mass-patching drivers I'm afraid. Could you confirm that this is your 
> plan ?
Yes, working on it.
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + *
>> + * See also:
>> + * drm_atomic_helper_suspend()
>> + */
>> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
>> *state,
>> +					      struct drm_modeset_acquire_ctx
>> *ctx)
>> +{
>> +	int i;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *plane_state;
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +
>> +	state->acquire_ctx = ctx;
>> +
>> +	for_each_new_plane_in_state(state, plane, plane_state, i)
>> +		state->planes[i].old_state = plane->state;
>> +
>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
>> +		state->crtcs[i].old_state = crtc->state;
>> +
>> +	for_each_new_connector_in_state(state, connector, conn_state, i)
>> +		state->connectors[i].old_state = connector->state;
>> +
>> +	return drm_atomic_commit(state);
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
>> +
>> +/**
>>   * drm_atomic_helper_resume - subsystem-level resume helper
>>   * @dev: DRM device
>>   * @state: atomic state to resume to
>> @@ -2534,9 +2581,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>>  	int err;
>>
>>  	drm_mode_config_reset(dev);
>> +
>>  	drm_modeset_lock_all(dev);
>> -	state->acquire_ctx = config->acquire_ctx;
>> -	err = drm_atomic_commit(state);
>> +	err = drm_atomic_helper_commit_duplicated_state(state,
>> config->acquire_ctx);
>> 	drm_modeset_unlock_all(dev);
>>
>>  	return err;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c index f523256ef77c..74da1c380b32
>> 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3488,7 +3488,8 @@ static void intel_update_primary_planes(struct
>> drm_device *dev)
>>
>>  static int
>>  __intel_display_resume(struct drm_device *dev,
>> -		       struct drm_atomic_state *state)
>> +		       struct drm_atomic_state *state,
>> +		       struct drm_modeset_acquire_ctx *ctx)
>>  {
>>  	struct drm_crtc_state *crtc_state;
>>  	struct drm_crtc *crtc;
>> @@ -3512,7 +3513,7 @@ __intel_display_resume(struct drm_device *dev,
>>  	/* ignore any reset values/BIOS leftovers in the WM registers */
>>  	to_intel_atomic_state(state)->skip_intermediate_wm = true;
>>
>> -	ret = drm_atomic_commit(state);
>> +	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
>>
>>  	WARN_ON(ret == -EDEADLK);
>>  	return ret;
>> @@ -3606,7 +3607,7 @@ void intel_finish_reset(struct drm_i915_private
>> *dev_priv) */
>>  			intel_update_primary_planes(dev);
>>  		} else {
>> -			ret = __intel_display_resume(dev, state);
>> +			ret = __intel_display_resume(dev, state, ctx);
>>  			if (ret)
>>  				DRM_ERROR("Restoring old state failed with 
> %i\n", ret);
>>  		}
>> @@ -3626,7 +3627,7 @@ void intel_finish_reset(struct drm_i915_private
>> *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv);
>>  		spin_unlock_irq(&dev_priv->irq_lock);
>>
>> -		ret = __intel_display_resume(dev, state);
>> +		ret = __intel_display_resume(dev, state, ctx);
>>  		if (ret)
>>  			DRM_ERROR("Restoring old state failed with %i\n", 
> ret);
>> @@ -11327,7 +11328,7 @@ void intel_release_load_detect_pipe(struct
>> drm_connector *connector, if (!state)
>>  		return;
>>
>> -	ret = drm_atomic_commit(state);
>> +	ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
>>  	if (ret)
>>  		DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret);
>>  	drm_atomic_state_put(state);
>> @@ -17210,7 +17211,7 @@ void intel_display_resume(struct drm_device *dev)
>>  	}
>>
>>  	if (!ret)
>> -		ret = __intel_display_resume(dev, state);
>> +		ret = __intel_display_resume(dev, state, &ctx);
>>
>>  	drm_modeset_drop_locks(&ctx);
>>  	drm_modeset_acquire_fini(&ctx);
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index f96220ed4004..6062e7f27325 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -137,12 +137,12 @@ struct drm_crtc_commit {
>>
>>  struct __drm_planes_state {
>>  	struct drm_plane *ptr;
>> -	struct drm_plane_state *state;
>> +	struct drm_plane_state *state, *old_state, *new_state;
>>  };
>>
>>  struct __drm_crtcs_state {
>>  	struct drm_crtc *ptr;
>> -	struct drm_crtc_state *state;
>> +	struct drm_crtc_state *state, *old_state, *new_state;
>>  	struct drm_crtc_commit *commit;
>>  	s64 __user *out_fence_ptr;
>>  	unsigned last_vblank_count;
>> @@ -150,7 +150,7 @@ struct __drm_crtcs_state {
>>
>>  struct __drm_connnectors_state {
>>  	struct drm_connector *ptr;
>> -	struct drm_connector_state *state;
>> +	struct drm_connector_state *state, *old_state, *new_state;
>>  };
>>
>>  /**
>> @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct
>> drm_printer *p); (__i)++)							
> \
>>  		for_each_if (connector)
>>
>> +#define for_each_oldnew_connector_in_state(__state, connector,
>> old_connector_state, new_connector_state, __i) \
>> +	for ((__i) = 0;								
> \
>> +	     (__i) < (__state)->num_connector &&				
> \
>> +	     ((connector) = (__state)->connectors[__i].ptr,			
> \
>> +	     (old_connector_state) = (__state)->connectors[__i].old_state,	
> \
>> +	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 
> 	\
>> +	     (__i)++)							\
>> +		for_each_if (connector)
>> +
>> +#define for_each_old_connector_in_state(__state, connector,
>> old_connector_state, __i) \ +	for ((__i) = 0;					
> 			\
>> +	     (__i) < (__state)->num_connector &&				
> \
>> +	     ((connector) = (__state)->connectors[__i].ptr,			
> \
>> +	     (old_connector_state) = (__state)->connectors[__i].old_state, 1); 
> 	\
>> +	     (__i)++)							\
>> +		for_each_if (connector)
>> +
>> +#define for_each_new_connector_in_state(__state, connector,
>> new_connector_state, __i) \ +	for ((__i) = 0;					
> 			\
>> +	     (__i) < (__state)->num_connector &&				
> \
>> +	     ((connector) = (__state)->connectors[__i].ptr,			
> \
>> +	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 
> 	\
>> +	     (__i)++)							\
>> +		for_each_if (connector)
>> +
>>  #define for_each_crtc_in_state(__state, crtc, crtc_state, __i)	\
>>  	for ((__i) = 0;						\
>>  	     (__i) < (__state)->dev->mode_config.num_crtc &&	\
>> @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct
>> drm_printer *p); (__i)++)						\
>>  		for_each_if (crtc_state)
>>
>> +#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state,
>> new_crtc_state, __i) \ +	for ((__i) = 0;					
> 		\
>> +	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
>> +	     ((crtc) = (__state)->crtcs[__i].ptr,			\
>> +	     (old_crtc_state) = (__state)->crtcs[__i].old_state,	\
>> +	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
>> +	     (__i)++)							\
>> +		for_each_if (crtc)
>> +
>> +#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i)	
> \
>> +	for ((__i) = 0;							\
>> +	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
>> +	     ((crtc) = (__state)->crtcs[__i].ptr,			\
>> +	     (old_crtc_state) = (__state)->crtcs[__i].old_state, 1);	\
>> +	     (__i)++)							\
>> +		for_each_if (crtc)
>> +
>> +#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i)	
> \
>> +	for ((__i) = 0;							\
>> +	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
>> +	     ((crtc) = (__state)->crtcs[__i].ptr,			\
>> +	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
>> +	     (__i)++)							\
>> +		for_each_if (crtc)
>> +
>>  #define for_each_plane_in_state(__state, plane, plane_state, __i)		
> \
>>  	for ((__i) = 0;							\
>>  	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct
>> drm_printer *p); (__i)++)							
> \
>>  		for_each_if (plane_state)
>>
>> +#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state,
>> new_plane_state, __i) \ +	for ((__i) = 0;					
> 		\
>> +	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> +	     ((plane) = (__state)->planes[__i].ptr,			\
>> +	     (old_plane_state) = (__state)->planes[__i].old_state,	\
>> +	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
>> +	     (__i)++)							\
>> +		for_each_if (plane)
>> +
>> +#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \
>> +	for ((__i) = 0;							\
>> +	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> +	     ((plane) = (__state)->planes[__i].ptr,			\
>> +	     (old_plane_state) = (__state)->planes[__i].old_state, 1);	\
>> +	     (__i)++)							\
>> +		for_each_if (plane)
>> +
>> +#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \
>> +	for ((__i) = 0;							\
>> +	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> +	     ((plane) = (__state)->planes[__i].ptr,			\
>> +	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
>> +	     (__i)++)							\
>> +		for_each_if (plane)
>> +
>>  /**
>>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>>   * @state: &drm_crtc_state for the CRTC
>> diff --git a/include/drm/drm_atomic_helper.h
>> b/include/drm/drm_atomic_helper.h index 9afcd3810785..2c4549e98c16 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -105,6 +105,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set
>> *set, int drm_atomic_helper_disable_all(struct drm_device *dev,
>>  				  struct drm_modeset_acquire_ctx *ctx);
>>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
>> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
>> *state,
>> +					      struct drm_modeset_acquire_ctx
>> *ctx);
>>  int drm_atomic_helper_resume(struct drm_device *dev,
>>  			     struct drm_atomic_state *state);


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

Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Laurent Pinchart]

Hi Maarten,

On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
> Op 17-01-17 om 00:11 schreef Laurent Pinchart:
> > On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
> >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> >> depth and getting rid of all xxx->state dereferences.
> >> 
> >> This requires extra fixups done when committing a state after
> >> duplicating, which in general isn't valid but is used by suspend/resume.
> >> To handle these, introduce drm_atomic_helper_commit_duplicated_state
> >> which performs those fixups before checking & committing the state.
> >> 
> >> Changes since v1:
> >> - Remove nonblock parameter for commit_duplicated_state.
> >> Changes since v2:
> >> - Use commit_duplicated_state for i915 load detection.
> >> - Add WARN_ON(old_state != obj->state) before swapping.
> >> 
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_atomic.c         |  6 +++
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 65 +++++++++++++++++++++++++----
> >>  drivers/gpu/drm/i915/intel_display.c | 13 +++---
> >>  include/drm/drm_atomic.h             | 81 ++++++++++++++++++++++++++++--
> >>  include/drm/drm_atomic_helper.h      |  2 +
> >>  5 files changed, 149 insertions(+), 18 deletions(-)

[snip]

> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921
> >> 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c

[snip]

> >> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
> >> *drm_atomic_helper_suspend(struct drm_device *dev)
> >> EXPORT_SYMBOL(drm_atomic_helper_suspend);
> >> 
> >>  /**
> >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> >> + * @state: duplicated atomic state to commit
> >> + * @ctx: pointer to acquire_ctx to use for commit.
> >> + *
> >> + * The state returned by drm_atomic_helper_duplicate_state() and
> >> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> >> + * be fixed up before commit.
> > 
> > Can't you fix the state in drm_atomic_helper_suspend() to avoid the need
> > for this function ?
> 
> We would have to fix up other areas that duplicate state too, like i915
> suspend and load detect. This means moving it to a helper.
> 
> It was my original approach, but Daniel preferred this approach.

We have to fix it somewhere, that's for sure. I think it's better to fix it as 
close as possible to state duplication, instead of carrying a state that we 
know is invalid and fix it at the last minute. The drawback of this approach 
is that the window during which the state will be invalid is much larger, 
which could cause bugs later when new code will try to touch that state.

Daniel, is there a specific reason why you prefer this approach ?

> > Apart from this the patch looks good to me. I don't like the fact that we
> > now have two sets of states though (state and old_state/new_state). I
> > understand that you'd like to address this as part of a separate patch
> > series, which I'm fine with to avoid delaying this one, but I think we
> > should address this sooner rather than latter, or the API will become
> > very confusing. Yes, that means mass-patching drivers I'm afraid. Could
> > you confirm that this is your plan ?
> 
> Yes, working on it.
> 
> >> + * Returns:
> >> + * 0 on success or a negative error code on failure.
> >> + *
> >> + * See also:
> >> + * drm_atomic_helper_suspend()
> >> + */
> >> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
> >> *state,
> >> +					      struct drm_modeset_acquire_ctx
> >> *ctx)
> >> +{
> >> +	int i;
> >> +	struct drm_plane *plane;
> >> +	struct drm_plane_state *plane_state;
> >> +	struct drm_connector *connector;
> >> +	struct drm_connector_state *conn_state;
> >> +	struct drm_crtc *crtc;
> >> +	struct drm_crtc_state *crtc_state;
> >> +
> >> +	state->acquire_ctx = ctx;
> >> +
> >> +	for_each_new_plane_in_state(state, plane, plane_state, i)
> >> +		state->planes[i].old_state = plane->state;
> >> +
> >> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> >> +		state->crtcs[i].old_state = crtc->state;
> >> +
> >> +	for_each_new_connector_in_state(state, connector, conn_state, i)
> >> +		state->connectors[i].old_state = connector->state;
> >> +
> >> +	return drm_atomic_commit(state);
> >> +}
> >> +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);

[snip]

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Daniel Vetter]

On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote:
> On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
> > Op 17-01-17 om 00:11 schreef Laurent Pinchart:
> > > On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
> > >> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
> > >> *drm_atomic_helper_suspend(struct drm_device *dev)
> > >> EXPORT_SYMBOL(drm_atomic_helper_suspend);
> > >> 
> > >>  /**
> > >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> > >> + * @state: duplicated atomic state to commit
> > >> + * @ctx: pointer to acquire_ctx to use for commit.
> > >> + *
> > >> + * The state returned by drm_atomic_helper_duplicate_state() and
> > >> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> > >> + * be fixed up before commit.
> > > 
> > > Can't you fix the state in drm_atomic_helper_suspend() to avoid the need
> > > for this function ?
> > 
> > We would have to fix up other areas that duplicate state too, like i915
> > suspend and load detect. This means moving it to a helper.
> > 
> > It was my original approach, but Daniel preferred this approach.
> 
> We have to fix it somewhere, that's for sure. I think it's better to fix it as 
> close as possible to state duplication, instead of carrying a state that we 
> know is invalid and fix it at the last minute. The drawback of this approach 
> is that the window during which the state will be invalid is much larger, 
> which could cause bugs later when new code will try to touch that state.
> 
> Daniel, is there a specific reason why you prefer this approach ?

You can't fix it in suspend? The issue is that on resume, we have reset
states (to reflect the hw state of everything being off), so we can only
do this on commit. That holds in general for the duplicate, do something
nasty, restore state pattern.

And then I think a dedicated helper to commit duplicated state makes
sense. The kernel-doc might need a bit of work to explain this better
though. I think it should explain what exactly is partially invalid with
duplicated states.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Laurent Pinchart]

Hi Daniel,

On Monday 23 Jan 2017 09:48:54 Daniel Vetter wrote:
> On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote:
> > On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
> >> Op 17-01-17 om 00:11 schreef Laurent Pinchart:
> >>> On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
> >>>> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
> >>>> *drm_atomic_helper_suspend(struct drm_device *dev)
> >>>> EXPORT_SYMBOL(drm_atomic_helper_suspend);
> >>>> 
> >>>>  /**
> >>>> 
> >>>> + * drm_atomic_helper_commit_duplicated_state - commit duplicated
> >>>> state
> >>>> + * @state: duplicated atomic state to commit
> >>>> + * @ctx: pointer to acquire_ctx to use for commit.
> >>>> + *
> >>>> + * The state returned by drm_atomic_helper_duplicate_state() and
> >>>> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> >>>> + * be fixed up before commit.
> >>> 
> >>> Can't you fix the state in drm_atomic_helper_suspend() to avoid the
> >>> need for this function ?
> >> 
> >> We would have to fix up other areas that duplicate state too, like i915
> >> suspend and load detect. This means moving it to a helper.
> >> 
> >> It was my original approach, but Daniel preferred this approach.
> > 
> > We have to fix it somewhere, that's for sure. I think it's better to fix
> > it as close as possible to state duplication, instead of carrying a state
> > that we know is invalid and fix it at the last minute. The drawback of
> > this approach is that the window during which the state will be invalid
> > is much larger, which could cause bugs later when new code will try to
> > touch that state.
> > 
> > Daniel, is there a specific reason why you prefer this approach ?
> 
> You can't fix it in suspend? The issue is that on resume, we have reset
> states (to reflect the hw state of everything being off), so we can only
> do this on commit. That holds in general for the duplicate, do something
> nasty, restore state pattern.

Ok, I got it now. You can't fix the state up at suspend time because you need 
to set the old_state pointers to the state allocated by the .reset() handlers, 
which are called at resume time.

> And then I think a dedicated helper to commit duplicated state makes
> sense. The kernel-doc might need a bit of work to explain this better
> though. I think it should explain what exactly is partially invalid with
> duplicated states.

Yes, the documentation should be clarified, and include the above explanation 
in some form.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> depth and getting rid of all xxx->state dereferences.
> 
> This requires extra fixups done when committing a state after
> duplicating, which in general isn't valid but is used by suspend/resume.
> To handle these, introduce drm_atomic_helper_commit_duplicated_state
> which performs those fixups before checking & committing the state.
> 
> Changes since v1:
> - Remove nonblock parameter for commit_duplicated_state.
> Changes since v2:
> - Use commit_duplicated_state for i915 load detection.
> - Add WARN_ON(old_state != obj->state) before swapping.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_atomic.c         |  6 +++
>  drivers/gpu/drm/drm_atomic_helper.c  | 65 +++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_display.c | 13 +++---
>  include/drm/drm_atomic.h             | 81 +++++++++++++++++++++++++++++++-- 
> include/drm/drm_atomic_helper.h      |  2 +
>  5 files changed, 149 insertions(+), 18 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH v3 2/7] drm/atomic: Make add_affected_connectors look at crtc_state.

[Maarten Lankhorst]

This kills another dereference of connector->state. connector_mask
holds all unchanged connectors at least and any changed connectors
are already in state anyway.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 1c1cbf436717..18cdf2c956c6 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1419,8 +1419,13 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 	struct drm_connector *connector;
 	struct drm_connector_state *conn_state;
 	struct drm_connector_list_iter conn_iter;
+	struct drm_crtc_state *crtc_state;
 	int ret;
 
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
 	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
 	if (ret)
 		return ret;
@@ -1429,12 +1434,12 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 			 crtc->base.id, crtc->name, state);
 
 	/*
-	 * Changed connectors are already in @state, so only need to look at the
-	 * current configuration.
+	 * Changed connectors are already in @state, so only need to look
+	 * at the connector_mask in crtc_state.
 	 */
 	drm_connector_list_iter_get(state->dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
-		if (connector->state->crtc != crtc)
+		if (!(crtc_state->connector_mask & (1 << drm_connector_index(connector))))
 			continue;
 
 		conn_state = drm_atomic_get_connector_state(state, connector);
-- 
2.7.4

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

Re: [PATCH v3 2/7] drm/atomic: Make add_affected_connectors look at crtc_state.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 16 Jan 2017 10:37:39 Maarten Lankhorst wrote:
> This kills another dereference of connector->state. connector_mask
> holds all unchanged connectors at least and any changed connectors
> are already in state anyway.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_atomic.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1c1cbf436717..18cdf2c956c6 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1419,8 +1419,13 @@ drm_atomic_add_affected_connectors(struct
> drm_atomic_state *state, struct drm_connector *connector;
>  	struct drm_connector_state *conn_state;
>  	struct drm_connector_list_iter conn_iter;
> +	struct drm_crtc_state *crtc_state;
>  	int ret;
> 
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
>  	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
>  	if (ret)
>  		return ret;
> @@ -1429,12 +1434,12 @@ drm_atomic_add_affected_connectors(struct
> drm_atomic_state *state, crtc->base.id, crtc->name, state);
> 
>  	/*
> -	 * Changed connectors are already in @state, so only need to look at 
the
> -	 * current configuration.
> +	 * Changed connectors are already in @state, so only need to look
> +	 * at the connector_mask in crtc_state.
>  	 */
>  	drm_connector_list_iter_get(state->dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> -		if (connector->state->crtc != crtc)
> +		if (!(crtc_state->connector_mask & (1 <<
> drm_connector_index(connector)))) continue;
> 
>  		conn_state = drm_atomic_get_connector_state(state, connector);

-- 
Regards,

Laurent Pinchart

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

[PATCH v3 3/7] drm/atomic: Use new atomic iterator macros.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 18cdf2c956c6..7b61e0da9ace 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 
 	DRM_DEBUG_ATOMIC("checking %p\n", state);
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
+	for_each_new_plane_in_state(state, plane, plane_state, i) {
 		ret = drm_atomic_plane_check(plane, plane_state);
 		if (ret) {
 			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n",
@@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		ret = drm_atomic_crtc_check(crtc, crtc_state);
 		if (ret) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check failed\n",
@@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		ret = config->funcs->atomic_check(state->dev, state);
 
 	if (!state->allow_modeset) {
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full modeset\n",
 						 crtc->base.id, crtc->name);
@@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
 
 	DRM_DEBUG_ATOMIC("checking %p\n", state);
 
-	for_each_plane_in_state(state, plane, plane_state, i)
+	for_each_new_plane_in_state(state, plane, plane_state, i)
 		drm_atomic_plane_print_state(&p, plane_state);
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i)
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
 		drm_atomic_crtc_print_state(&p, crtc_state);
 
-	for_each_connector_in_state(state, connector, connector_state, i)
+	for_each_new_connector_in_state(state, connector, connector_state, i)
 		drm_atomic_connector_print_state(&p, connector_state);
 }
 
@@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
 		return 0;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		u64 __user *fence_ptr;
 
 		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
@@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device *dev,
 		return;
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		/*
 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
 		 * exclusive, if they weren't, this code should be
-- 
2.7.4

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

Re: [PATCH v3 3/7] drm/atomic: Use new atomic iterator macros.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 16 Jan 2017 10:37:40 Maarten Lankhorst wrote:

Could we please get a description ? Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 18cdf2c956c6..7b61e0da9ace 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> *state)
> 
>  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> 
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> +	for_each_new_plane_in_state(state, plane, plane_state, i) {
>  		ret = drm_atomic_plane_check(plane, plane_state);
>  		if (ret) {
>  			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check 
failed\n",
> @@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> *state) }
>  	}
> 
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		ret = drm_atomic_crtc_check(crtc, crtc_state);
>  		if (ret) {
>  			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check 
failed\n",
> @@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> *state) ret = config->funcs->atomic_check(state->dev, state);
> 
>  	if (!state->allow_modeset) {
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full 
modeset\n",
>  						 crtc->base.id, crtc->name);
> @@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct
> drm_atomic_state *state)
> 
>  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> 
> -	for_each_plane_in_state(state, plane, plane_state, i)
> +	for_each_new_plane_in_state(state, plane, plane_state, i)
>  		drm_atomic_plane_print_state(&p, plane_state);
> 
> -	for_each_crtc_in_state(state, crtc, crtc_state, i)
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
>  		drm_atomic_crtc_print_state(&p, crtc_state);
> 
> -	for_each_connector_in_state(state, connector, connector_state, i)
> +	for_each_new_connector_in_state(state, connector, connector_state, i)
>  		drm_atomic_connector_print_state(&p, connector_state);
>  }
> 
> @@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device
> *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>  		return 0;
> 
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		u64 __user *fence_ptr;
> 
>  		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
> @@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device
> *dev, return;
>  	}
> 
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		/*
>  		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
>  		 * exclusive, if they weren't, this code should be

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 3/7] drm/atomic: Use new atomic iterator macros.

[Daniel Vetter]

On Tue, Jan 17, 2017 at 01:55:50AM +0200, Laurent Pinchart wrote:
> Hi Maarten,
> 
> Thank you for the patch.
> 
> On Monday 16 Jan 2017 10:37:40 Maarten Lankhorst wrote:
> 
> Could we please get a description ? Apart from that,

Typed something small and merged the first 3 patches from this series.

Thanks, Daniel

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 18cdf2c956c6..7b61e0da9ace 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> > *state)
> > 
> >  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> > 
> > -	for_each_plane_in_state(state, plane, plane_state, i) {
> > +	for_each_new_plane_in_state(state, plane, plane_state, i) {
> >  		ret = drm_atomic_plane_check(plane, plane_state);
> >  		if (ret) {
> >  			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check 
> failed\n",
> > @@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> > *state) }
> >  	}
> > 
> > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >  		ret = drm_atomic_crtc_check(crtc, crtc_state);
> >  		if (ret) {
> >  			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check 
> failed\n",
> > @@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> > *state) ret = config->funcs->atomic_check(state->dev, state);
> > 
> >  	if (!state->allow_modeset) {
> > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> >  				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full 
> modeset\n",
> >  						 crtc->base.id, crtc->name);
> > @@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct
> > drm_atomic_state *state)
> > 
> >  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> > 
> > -	for_each_plane_in_state(state, plane, plane_state, i)
> > +	for_each_new_plane_in_state(state, plane, plane_state, i)
> >  		drm_atomic_plane_print_state(&p, plane_state);
> > 
> > -	for_each_crtc_in_state(state, crtc, crtc_state, i)
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> >  		drm_atomic_crtc_print_state(&p, crtc_state);
> > 
> > -	for_each_connector_in_state(state, connector, connector_state, i)
> > +	for_each_new_connector_in_state(state, connector, connector_state, i)
> >  		drm_atomic_connector_print_state(&p, connector_state);
> >  }
> > 
> > @@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device
> > *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> >  		return 0;
> > 
> > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >  		u64 __user *fence_ptr;
> > 
> >  		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
> > @@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device
> > *dev, return;
> >  	}
> > 
> > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >  		/*
> >  		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
> >  		 * exclusive, if they weren't, this code should be
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/7] drm/atomic: Use new atomic iterator macros.

[Laurent Pinchart]

Hi Daniel,

On Tuesday 14 Feb 2017 21:03:07 Daniel Vetter wrote:
> On Tue, Jan 17, 2017 at 01:55:50AM +0200, Laurent Pinchart wrote:
> > On Monday 16 Jan 2017 10:37:40 Maarten Lankhorst wrote:
> > 
> > Could we please get a description ? Apart from that,
> 
> Typed something small and merged the first 3 patches from this series.

Thanks. 4 more patches to go. Maarten ? :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_atomic.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 18cdf2c956c6..7b61e0da9ace 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> > > *state)
> > > 
> > >  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> > > 
> > > -	for_each_plane_in_state(state, plane, plane_state, i) {
> > > +	for_each_new_plane_in_state(state, plane, plane_state, i) {
> > > 
> > >  		ret = drm_atomic_plane_check(plane, plane_state);
> > >  		if (ret) {
> > >  		
> > >  			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check
> > 
> > failed\n",
> > 
> > > @@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> > > *state) }
> > > 
> > >  	}
> > > 
> > > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > 
> > >  		ret = drm_atomic_crtc_check(crtc, crtc_state);
> > >  		if (ret) {
> > >  		
> > >  			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check
> > 
> > failed\n",
> > 
> > > @@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> > > *state) ret = config->funcs->atomic_check(state->dev, state);
> > > 
> > >  	if (!state->allow_modeset) {
> > > 
> > > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > 
> > >  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> > >  			
> > >  				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full
> > 
> > modeset\n",
> > 
> > >  						 crtc->base.id, crtc->name);
> > > 
> > > @@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct
> > > drm_atomic_state *state)
> > > 
> > >  	DRM_DEBUG_ATOMIC("checking %p\n", state);
> > > 
> > > -	for_each_plane_in_state(state, plane, plane_state, i)
> > > +	for_each_new_plane_in_state(state, plane, plane_state, i)
> > > 
> > >  		drm_atomic_plane_print_state(&p, plane_state);
> > > 
> > > -	for_each_crtc_in_state(state, crtc, crtc_state, i)
> > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > 
> > >  		drm_atomic_crtc_print_state(&p, crtc_state);
> > > 
> > > -	for_each_connector_in_state(state, connector, connector_state, i)
> > > +	for_each_new_connector_in_state(state, connector, connector_state, i)
> > > 
> > >  		drm_atomic_connector_print_state(&p, connector_state);
> > >  
> > >  }
> > > 
> > > @@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct
> > > drm_device
> > > *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> > > 
> > >  		return 0;
> > > 
> > > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > 
> > >  		u64 __user *fence_ptr;
> > >  		
> > >  		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
> > > 
> > > @@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct
> > > drm_device
> > > *dev, return;
> > > 
> > >  	}
> > > 
> > > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > 
> > >  		/*
> > >  		
> > >  		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
> > >  		 * exclusive, if they weren't, this code should be

-- 
Regards,

Laurent Pinchart

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

[PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++++-----------------
 1 file changed, 152 insertions(+), 141 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d153e8a72921..b26cf786ce12 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -63,14 +63,15 @@
  */
 static void
 drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
+				struct drm_plane_state *old_plane_state,
 				struct drm_plane_state *plane_state,
 				struct drm_plane *plane)
 {
 	struct drm_crtc_state *crtc_state;
 
-	if (plane->state->crtc) {
+	if (old_plane_state->crtc) {
 		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								plane->state->crtc);
+								old_plane_state->crtc);
 
 		if (WARN_ON(!crtc_state))
 			return;
@@ -104,7 +105,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	 * part of the state. If the same encoder is assigned to multiple
 	 * connectors bail out.
 	 */
-	for_each_connector_in_state(state, connector, conn_state, i) {
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
 		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 		struct drm_encoder *new_encoder;
 
@@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
-	struct drm_connector_state *connector_state;
+	struct drm_connector_state *old_connector_state, *new_connector_state;
 	int i;
 
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
 		struct drm_crtc *encoder_crtc;
 
-		if (connector_state->best_encoder != encoder)
+		if (new_connector_state->best_encoder != encoder)
 			continue;
 
-		encoder_crtc = connector->state->crtc;
+		encoder_crtc = old_connector_state->crtc;
 
 		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n",
 				 encoder->base.id, encoder->name,
 				 encoder_crtc->base.id, encoder_crtc->name);
 
-		set_best_encoder(state, connector_state, NULL);
+		set_best_encoder(state, new_connector_state, NULL);
 
 		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
 		crtc_state->connectors_changed = true;
@@ -272,7 +273,8 @@ steal_encoder(struct drm_atomic_state *state,
 static int
 update_connector_routing(struct drm_atomic_state *state,
 			 struct drm_connector *connector,
-			 struct drm_connector_state *connector_state)
+			 struct drm_connector_state *old_connector_state,
+			 struct drm_connector_state *new_connector_state)
 {
 	const struct drm_connector_helper_funcs *funcs;
 	struct drm_encoder *new_encoder;
@@ -282,24 +284,24 @@ update_connector_routing(struct drm_atomic_state *state,
 			 connector->base.id,
 			 connector->name);
 
-	if (connector->state->crtc != connector_state->crtc) {
-		if (connector->state->crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
+	if (old_connector_state->crtc != new_connector_state->crtc) {
+		if (old_connector_state->crtc) {
+			crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 
-		if (connector_state->crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
+		if (new_connector_state->crtc) {
+			crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 	}
 
-	if (!connector_state->crtc) {
+	if (!new_connector_state->crtc) {
 		DRM_DEBUG_ATOMIC("Disabling [CONNECTOR:%d:%s]\n",
 				connector->base.id,
 				connector->name);
 
-		set_best_encoder(state, connector_state, NULL);
+		set_best_encoder(state, new_connector_state, NULL);
 
 		return 0;
 	}
@@ -308,7 +310,7 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	if (funcs->atomic_best_encoder)
 		new_encoder = funcs->atomic_best_encoder(connector,
-							 connector_state);
+							 new_connector_state);
 	else if (funcs->best_encoder)
 		new_encoder = funcs->best_encoder(connector);
 	else
@@ -321,33 +323,33 @@ update_connector_routing(struct drm_atomic_state *state,
 		return -EINVAL;
 	}
 
-	if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) {
+	if (!drm_encoder_crtc_ok(new_encoder, new_connector_state->crtc)) {
 		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n",
 				 new_encoder->base.id,
 				 new_encoder->name,
-				 connector_state->crtc->base.id);
+				 new_connector_state->crtc->base.id);
 		return -EINVAL;
 	}
 
-	if (new_encoder == connector_state->best_encoder) {
-		set_best_encoder(state, connector_state, new_encoder);
+	if (new_encoder == new_connector_state->best_encoder) {
+		set_best_encoder(state, new_connector_state, new_encoder);
 
 		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d:%s]\n",
 				 connector->base.id,
 				 connector->name,
 				 new_encoder->base.id,
 				 new_encoder->name,
-				 connector_state->crtc->base.id,
-				 connector_state->crtc->name);
+				 new_connector_state->crtc->base.id,
+				 new_connector_state->crtc->name);
 
 		return 0;
 	}
 
 	steal_encoder(state, new_encoder);
 
-	set_best_encoder(state, connector_state, new_encoder);
+	set_best_encoder(state, new_connector_state, new_encoder);
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
+	crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc);
 	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
@@ -355,8 +357,8 @@ update_connector_routing(struct drm_atomic_state *state,
 			 connector->name,
 			 new_encoder->base.id,
 			 new_encoder->name,
-			 connector_state->crtc->base.id,
-			 connector_state->crtc->name);
+			 new_connector_state->crtc->base.id,
+			 new_connector_state->crtc->name);
 
 	return 0;
 }
@@ -371,7 +373,7 @@ mode_fixup(struct drm_atomic_state *state)
 	int i;
 	bool ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!crtc_state->mode_changed &&
 		    !crtc_state->connectors_changed)
 			continue;
@@ -379,7 +381,7 @@ mode_fixup(struct drm_atomic_state *state)
 		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
 	}
 
-	for_each_connector_in_state(state, connector, conn_state, i) {
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 
@@ -424,7 +426,7 @@ mode_fixup(struct drm_atomic_state *state)
 		}
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		if (!crtc_state->enable)
@@ -483,19 +485,19 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_connector *connector;
-	struct drm_connector_state *connector_state;
+	struct drm_connector_state *old_connector_state, *new_connector_state;
 	int i, ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		if (!drm_mode_equal(&old_crtc_state->mode, &new_crtc_state->mode)) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode changed\n",
 					 crtc->base.id, crtc->name);
-			crtc_state->mode_changed = true;
+			new_crtc_state->mode_changed = true;
 		}
 
-		if (crtc->state->enable != crtc_state->enable) {
+		if (old_crtc_state->enable != new_crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enable changed\n",
 					 crtc->base.id, crtc->name);
 
@@ -507,8 +509,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			 * The other way around is true as well. enable != 0
 			 * iff connectors are attached and a mode is set.
 			 */
-			crtc_state->mode_changed = true;
-			crtc_state->connectors_changed = true;
+			new_crtc_state->mode_changed = true;
+			new_crtc_state->connectors_changed = true;
 		}
 	}
 
@@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
 		/*
 		 * This only sets crtc->connectors_changed for routing changes,
 		 * drivers must set crtc->connectors_changed themselves when
 		 * connector properties need to be updated.
 		 */
 		ret = update_connector_routing(state, connector,
-					       connector_state);
+					       old_connector_state,
+					       new_connector_state);
 		if (ret)
 			return ret;
 	}
@@ -534,28 +537,28 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	 * configuration. This must be done before calling mode_fixup in case a
 	 * crtc only changed its mode but has the same set of connectors.
 	 */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		bool has_connectors =
-			!!crtc_state->connector_mask;
+			!!new_crtc_state->connector_mask;
 
 		/*
 		 * We must set ->active_changed after walking connectors for
 		 * otherwise an update that only changes active would result in
 		 * a full modeset because update_connector_routing force that.
 		 */
-		if (crtc->state->active != crtc_state->active) {
+		if (old_crtc_state->active != new_crtc_state->active) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active changed\n",
 					 crtc->base.id, crtc->name);
-			crtc_state->active_changed = true;
+			new_crtc_state->active_changed = true;
 		}
 
-		if (!drm_atomic_crtc_needs_modeset(crtc_state))
+		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
 
 		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] needs all connectors, enable: %c, active: %c\n",
 				 crtc->base.id, crtc->name,
-				 crtc_state->enable ? 'y' : 'n',
-				 crtc_state->active ? 'y' : 'n');
+				 new_crtc_state->enable ? 'y' : 'n',
+				 new_crtc_state->active ? 'y' : 'n');
 
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret != 0)
@@ -565,7 +568,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (ret != 0)
 			return ret;
 
-		if (crtc_state->enable != has_connectors) {
+		if (new_crtc_state->enable != has_connectors) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
 					 crtc->base.id, crtc->name);
 
@@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *plane_state, *old_plane_state;
 	int i, ret = 0;
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
 		funcs = plane->helper_private;
 
-		drm_atomic_helper_plane_changed(state, plane_state, plane);
+		drm_atomic_helper_plane_changed(state, old_plane_state, plane_state, plane);
 
 		if (!funcs || !funcs->atomic_check)
 			continue;
@@ -620,7 +623,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 		}
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		funcs = crtc->helper_private;
@@ -681,12 +684,12 @@ static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 
-	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 
@@ -723,7 +726,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		/* Right function depends upon target state. */
 		if (funcs) {
-			if (connector->state->crtc && funcs->prepare)
+			if (new_conn_state->crtc && funcs->prepare)
 				funcs->prepare(encoder);
 			else if (funcs->disable)
 				funcs->disable(encoder);
@@ -734,11 +737,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		drm_bridge_post_disable(encoder->bridge);
 	}
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		/* Shut down everything that needs a full modeset. */
-		if (!drm_atomic_crtc_needs_modeset(crtc->state))
+		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
 
 		if (!old_crtc_state->active)
@@ -751,7 +754,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 
 		/* Right function depends upon target state. */
-		if (crtc->state->enable && funcs->prepare)
+		if (new_crtc_state->enable && funcs->prepare)
 			funcs->prepare(crtc);
 		else if (funcs->atomic_disable)
 			funcs->atomic_disable(crtc, old_crtc_state);
@@ -780,13 +783,13 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 					      struct drm_atomic_state *old_state)
 {
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *crtc_state;
 	int i;
 
 	/* clear out existing links and update dpms */
-	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
 		if (connector->encoder) {
 			WARN_ON(!connector->encoder->crtc);
 
@@ -794,7 +797,7 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 			connector->encoder = NULL;
 		}
 
-		crtc = connector->state->crtc;
+		crtc = new_conn_state->crtc;
 		if ((!crtc && old_conn_state->crtc) ||
 		    (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) {
 			struct drm_property *dpms_prop =
@@ -811,23 +814,23 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 	}
 
 	/* set new links */
-	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
-		if (!connector->state->crtc)
+	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+		if (!new_conn_state->crtc)
 			continue;
 
-		if (WARN_ON(!connector->state->best_encoder))
+		if (WARN_ON(!new_conn_state->best_encoder))
 			continue;
 
-		connector->encoder = connector->state->best_encoder;
-		connector->encoder->crtc = connector->state->crtc;
+		connector->encoder = new_conn_state->best_encoder;
+		connector->encoder->crtc = new_conn_state->crtc;
 	}
 
 	/* set legacy state in the crtc structure */
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		struct drm_plane *primary = crtc->primary;
 
-		crtc->mode = crtc->state->mode;
-		crtc->enabled = crtc->state->enable;
+		crtc->mode = crtc_state->mode;
+		crtc->enabled = crtc_state->enable;
 
 		if (drm_atomic_get_existing_plane_state(old_state, primary) &&
 		    primary->state->crtc == crtc) {
@@ -835,9 +838,9 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 			crtc->y = primary->state->src_y >> 16;
 		}
 
-		if (crtc->state->enable)
+		if (crtc_state->enable)
 			drm_calc_timestamping_constants(crtc,
-							&crtc->state->adjusted_mode);
+							&crtc_state->adjusted_mode);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_update_legacy_modeset_state);
@@ -846,20 +849,20 @@ static void
 crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *conn_state;
 	int i;
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
-		if (!crtc->state->mode_changed)
+		if (!crtc_state->mode_changed)
 			continue;
 
 		funcs = crtc->helper_private;
 
-		if (crtc->state->enable && funcs->mode_set_nofb) {
+		if (crtc_state->enable && funcs->mode_set_nofb) {
 			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 
@@ -867,18 +870,18 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		}
 	}
 
-	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+	for_each_new_connector_in_state(old_state, connector, conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_crtc_state *new_crtc_state;
 		struct drm_encoder *encoder;
 		struct drm_display_mode *mode, *adjusted_mode;
 
-		if (!connector->state->best_encoder)
+		if (!conn_state->best_encoder)
 			continue;
 
-		encoder = connector->state->best_encoder;
+		encoder = conn_state->best_encoder;
 		funcs = encoder->helper_private;
-		new_crtc_state = connector->state->crtc->state;
+		new_crtc_state = conn_state->crtc->state;
 		mode = &new_crtc_state->mode;
 		adjusted_mode = &new_crtc_state->adjusted_mode;
 
@@ -894,7 +897,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		 */
 		if (funcs && funcs->atomic_mode_set) {
 			funcs->atomic_mode_set(encoder, new_crtc_state,
-					       connector->state);
+					       conn_state);
 		} else if (funcs && funcs->mode_set) {
 			funcs->mode_set(encoder, mode, adjusted_mode);
 		}
@@ -946,24 +949,24 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 					      struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *conn_state;
 	int i;
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		/* Need to filter out CRTCs where only planes change. */
-		if (!drm_atomic_crtc_needs_modeset(crtc->state))
+		if (!drm_atomic_crtc_needs_modeset(crtc_state))
 			continue;
 
-		if (!crtc->state->active)
+		if (!crtc_state->active)
 			continue;
 
 		funcs = crtc->helper_private;
 
-		if (crtc->state->enable) {
+		if (crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
 					 crtc->base.id, crtc->name);
 
@@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		}
 	}
 
-	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+	for_each_new_connector_in_state(old_state, connector, conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 
-		if (!connector->state->best_encoder)
+		if (!conn_state->best_encoder)
 			continue;
 
-		if (!connector->state->crtc->state->active ||
-		    !drm_atomic_crtc_needs_modeset(connector->state->crtc->state))
+		if (!conn_state->crtc->state->active ||
+		    !drm_atomic_crtc_needs_modeset(conn_state->crtc->state))
 			continue;
 
-		encoder = connector->state->best_encoder;
+		encoder = conn_state->best_encoder;
 		funcs = encoder->helper_private;
 
 		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
@@ -1038,10 +1041,7 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 	struct drm_plane_state *plane_state;
 	int i, ret;
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		if (!pre_swap)
-			plane_state = plane->state;
-
+	for_each_new_plane_in_state(state, plane, plane_state, i) {
 		if (!plane_state->fence)
 			continue;
 
@@ -1080,7 +1080,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i, ret;
 	unsigned crtc_mask = 0;
 
@@ -1091,9 +1091,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 	if (old_state->legacy_cursor_update)
 		return;
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		struct drm_crtc_state *new_crtc_state = crtc->state;
-
+	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		if (!new_crtc_state->active || !new_crtc_state->planes_changed)
 			continue;
 
@@ -1105,7 +1103,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		old_state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc);
 	}
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
 		if (!(crtc_mask & drm_crtc_mask(crtc)))
 			continue;
 
@@ -1412,11 +1410,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 				   bool nonblock)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *crtc_state;
 	struct drm_crtc_commit *commit;
 	int i, ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
 		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
 		if (!commit)
 			return -ENOMEM;
@@ -1437,7 +1435,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		/* Drivers only send out events when at least either current or
 		 * new CRTC state is active. Complete right away if everything
 		 * stays off. */
-		if (!crtc->state->active && !crtc_state->active) {
+		if (!old_crtc_state->active && !crtc_state->active) {
 			complete_all(&commit->flip_done);
 			continue;
 		}
@@ -1503,7 +1501,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		spin_lock(&crtc->commit_lock);
 		commit = preceeding_commit(crtc);
 		if (commit)
@@ -1554,13 +1552,13 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 	struct drm_crtc_commit *commit;
 	int i;
 
-	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		commit = old_state->crtcs[i].commit;
 		if (!commit)
 			continue;
 
 		/* backend must have consumed any event by now */
-		WARN_ON(crtc->state->event);
+		WARN_ON(crtc_state->event);
 		spin_lock(&crtc->commit_lock);
 		complete_all(&commit->hw_done);
 		spin_unlock(&crtc->commit_lock);
@@ -1587,7 +1585,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		commit = old_state->crtcs[i].commit;
 		if (WARN_ON(!commit))
 			continue;
@@ -1640,7 +1638,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 	struct drm_plane_state *plane_state;
 	int ret, i, j;
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
+	for_each_new_plane_in_state(state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
 		funcs = plane->helper_private;
@@ -1655,7 +1653,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 	return 0;
 
 fail:
-	for_each_plane_in_state(state, plane, plane_state, j) {
+	for_each_new_plane_in_state(state, plane, plane_state, j) {
 		const struct drm_plane_helper_funcs *funcs;
 
 		if (j >= i)
@@ -1722,14 +1720,14 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 				     uint32_t flags)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
-	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	int i;
 	bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
 	bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		funcs = crtc->helper_private;
@@ -1737,13 +1735,13 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs || !funcs->atomic_begin)
 			continue;
 
-		if (active_only && !crtc->state->active)
+		if (active_only && !new_crtc_state->active)
 			continue;
 
 		funcs->atomic_begin(crtc, old_crtc_state);
 	}
 
-	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
+	for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 		bool disabling;
 
@@ -1762,7 +1760,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 			 * CRTC to avoid skipping planes being disabled on an
 			 * active CRTC.
 			 */
-			if (!disabling && !plane_crtc_active(plane->state))
+			if (!disabling && !plane_crtc_active(new_plane_state))
 				continue;
 			if (disabling && !plane_crtc_active(old_plane_state))
 				continue;
@@ -1781,12 +1779,12 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 				continue;
 
 			funcs->atomic_disable(plane, old_plane_state);
-		} else if (plane->state->crtc || disabling) {
+		} else if (new_plane_state->crtc || disabling) {
 			funcs->atomic_update(plane, old_plane_state);
 		}
 	}
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		funcs = crtc->helper_private;
@@ -1794,7 +1792,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs || !funcs->atomic_flush)
 			continue;
 
-		if (active_only && !crtc->state->active)
+		if (active_only && !new_crtc_state->active)
 			continue;
 
 		funcs->atomic_flush(crtc, old_crtc_state);
@@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 				      struct drm_atomic_state *old_state)
 {
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *plane_state, *new_plane_state;
 	int i;
 
-	for_each_plane_in_state(old_state, plane, plane_state, i) {
+	for_each_oldnew_plane_in_state(old_state, plane, plane_state, new_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
+		/*
+		 * This might be called before swapping when commit is aborted,
+		 * in which case we have to free the new state.
+		 */
+		if (plane_state == plane->state)
+			plane_state = new_plane_state;
+
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
@@ -1971,15 +1976,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	int i;
 	long ret;
 	struct drm_connector *connector;
-	struct drm_connector_state *conn_state, *old_conn_state;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state, *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state, *old_plane_state;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
 
 	if (stall) {
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 			spin_lock(&crtc->commit_lock);
 			commit = list_first_entry_or_null(&crtc->commit_list,
 					struct drm_crtc_commit, commit_entry);
@@ -1999,20 +2004,24 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		}
 	}
 
-	for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
 		WARN_ON(connector->state != old_conn_state);
 
-		connector->state->state = state;
-		swap(state->connectors[i].state, connector->state);
-		connector->state->state = NULL;
+		old_conn_state->state = state;
+		new_conn_state->state = NULL;
+
+		state->connectors[i].state = old_conn_state;
+		connector->state = new_conn_state;
 	}
 
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		WARN_ON(crtc->state != old_crtc_state);
 
-		crtc->state->state = state;
-		swap(state->crtcs[i].state, crtc->state);
-		crtc->state->state = NULL;
+		old_crtc_state->state = state;
+		new_crtc_state->state = NULL;
+
+		state->crtcs[i].state = old_crtc_state;
+		crtc->state = new_crtc_state;
 
 		if (state->crtcs[i].commit) {
 			spin_lock(&crtc->commit_lock);
@@ -2024,12 +2033,14 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		}
 	}
 
-	for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) {
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		WARN_ON(plane->state != old_plane_state);
 
-		plane->state->state = state;
-		swap(state->planes[i].state, plane->state);
-		plane->state->state = NULL;
+		old_plane_state->state = state;
+		new_plane_state->state = NULL;
+
+		state->planes[i].state = old_plane_state;
+		plane->state = new_plane_state;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_swap_state);
@@ -2227,7 +2238,7 @@ static int update_output_state(struct drm_atomic_state *state,
 	if (ret)
 		return ret;
 
-	for_each_connector_in_state(state, connector, conn_state, i) {
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
 		if (conn_state->crtc == set->crtc) {
 			ret = drm_atomic_set_crtc_for_connector(conn_state,
 								NULL);
@@ -2249,7 +2260,7 @@ static int update_output_state(struct drm_atomic_state *state,
 			return ret;
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		/* Don't update ->enable for the CRTC in the set_config request,
 		 * since a mismatch would indicate a bug in the upper layers.
 		 * The actual modeset code later on will catch any
-- 
2.7.4

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

Re: [PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Laurent Pinchart]

Hi Maarten,

(CC'ing Daniel)

Thank you for the patch.

On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++---------------
>  1 file changed, 152 insertions(+), 141 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c

[snip]

> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
>  {
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *connector;
> -	struct drm_connector_state *connector_state;
> +	struct drm_connector_state *old_connector_state, *new_connector_state;

The kernel favours one variable declaration per line, and I think this file 
does as well, at least mostly (this comment holds for the rest of this patch 
and the other patches in the series).

>  	int i;
> 
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_oldnew_connector_in_state(state, connector,
> old_connector_state, new_connector_state, i) {

This is getting quite long, you could wrap the line.

> 		struct drm_crtc *encoder_crtc;
> 
> -		if (connector_state->best_encoder != encoder)
> +		if (new_connector_state->best_encoder != encoder)
>  			continue;
> 
> -		encoder_crtc = connector->state->crtc;
> +		encoder_crtc = old_connector_state->crtc;
> 
>  		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], 
stealing it\n",
> encoder->base.id, encoder->name,
>  				 encoder_crtc->base.id, encoder_crtc->name);
> 
> -		set_best_encoder(state, connector_state, NULL);
> +		set_best_encoder(state, new_connector_state, NULL);
> 
>  		crtc_state = drm_atomic_get_existing_crtc_state(state, 
encoder_crtc);
>  		crtc_state->connectors_changed = true;

[snip]

> @@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device
> *dev, if (ret)
>  		return ret;
> 
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_oldnew_connector_in_state(state, connector,
> old_connector_state, new_connector_state, i) {

Line wrap here too ?

> 		/*
>  		 * This only sets crtc->connectors_changed for routing 
changes,
>  		 * drivers must set crtc->connectors_changed themselves when
>  		 * connector properties need to be updated.
>  		 */
>  		ret = update_connector_routing(state, connector,
> -					       connector_state);
> +					       old_connector_state,
> +					       new_connector_state);
>  		if (ret)
>  			return ret;
>  	}

[snip]

> @@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *plane_state, *old_plane_state;

In some location you use new_*_state and in others *_state. I believe this 
should this be standardized to avoid confusion (the code is hard enough to 
read as it is :-)).

Similarly, you sometimes use *_conn_state and sometimes *_connector_state. 
That should be standardized as well.

>  	int i, ret = 0;
> 
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> plane_state, i) {
> 		const struct drm_plane_helper_funcs *funcs;
> 
>  		funcs = plane->helper_private;
> 
> -		drm_atomic_helper_plane_changed(state, plane_state, plane);
> +		drm_atomic_helper_plane_changed(state, old_plane_state,
> plane_state, plane);
> 
>  		if (!funcs || !funcs->atomic_check)
>  			continue;

[snip]

> @@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct
> drm_device *dev, }
>  	}
> 
> -	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> +	for_each_new_connector_in_state(old_state, connector, conn_state, i) {

Not strictly related to this patch, but I've been wondering how this works, 
given that we need to loop over connectors in the new state, not the old 
state. After some investigation I've realized that both the old and new states 
contain the same list of objects, as we don't keep a copy of the old global 
atomic state. old_state was the new state before the state swap, and the swap 
operation sets state->/objects/[*].state to the old state for all objects, but 
doesn't modify the object pointers. This is possibly more of a question for 
Daniel, should this be captured in the documentation somewhere (and is my 
understanding correct) ?

>  		const struct drm_encoder_helper_funcs *funcs;
>  		struct drm_encoder *encoder;
> 
> -		if (!connector->state->best_encoder)
> +		if (!conn_state->best_encoder)
>  			continue;
> 
> -		if (!connector->state->crtc->state->active ||
> -		    !drm_atomic_crtc_needs_modeset(connector->state->crtc-
>state))
> +		if (!conn_state->crtc->state->active ||
> +		    !drm_atomic_crtc_needs_modeset(conn_state->crtc->state))
>  			continue;
> 
> -		encoder = connector->state->best_encoder;
> +		encoder = conn_state->best_encoder;
>  		funcs = encoder->helper_private;
> 
>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",

[snip]

> @@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct
> drm_device *dev, struct drm_atomic_state *old_state)
>  {
>  	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *plane_state, *new_plane_state;
>  	int i;
> 
> -	for_each_plane_in_state(old_state, plane, plane_state, i) {
> +	for_each_oldnew_plane_in_state(old_state, plane, plane_state,

> new_plane_state, i) {
> 		const struct drm_plane_helper_funcs *funcs;
> 
> +		/*
> +		 * This might be called before swapping when commit is 
aborted,
> +		 * in which case we have to free the new state.

Should this be "cleanup the new state" instead of "free the new state" ?

> +		 */
> +		if (plane_state == plane->state)
> +			plane_state = new_plane_state;
> +
>  		funcs = plane->helper_private;
> 
>  		if (funcs->cleanup_fb)

[snip]

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Maarten Lankhorst]

Op 17-01-17 om 02:01 schreef Laurent Pinchart:
> Hi Maarten,
>
> (CC'ing Daniel)
>
> Thank you for the patch.
>
> On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++---------------
>>  1 file changed, 152 insertions(+), 141 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> [snip]
>
>> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
>>  {
>>  	struct drm_crtc_state *crtc_state;
>>  	struct drm_connector *connector;
>> -	struct drm_connector_state *connector_state;
>> +	struct drm_connector_state *old_connector_state, *new_connector_state;
> The kernel favours one variable declaration per line, and I think this file 
> does as well, at least mostly (this comment holds for the rest of this patch 
> and the other patches in the series).
This is the first I've heard of it..

~/nfs/linux$ git grep 'int i,'  | wc -l
8490
~/nfs/linux$ git grep 'int i;'  | wc -l
23636

Based on this, I don't think it's uncommon to have multiple declarations per line.

>>  	int i;
>>
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_oldnew_connector_in_state(state, connector,
>> old_connector_state, new_connector_state, i) {
> This is getting quite long, you could wrap the line.
Sounds good.
>> 		struct drm_crtc *encoder_crtc;
>>
>> -		if (connector_state->best_encoder != encoder)
>> +		if (new_connector_state->best_encoder != encoder)
>>  			continue;
>>
>> -		encoder_crtc = connector->state->crtc;
>> +		encoder_crtc = old_connector_state->crtc;
>>
>>  		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], 
> stealing it\n",
>> encoder->base.id, encoder->name,
>>  				 encoder_crtc->base.id, encoder_crtc->name);
>>
>> -		set_best_encoder(state, connector_state, NULL);
>> +		set_best_encoder(state, new_connector_state, NULL);
>>
>>  		crtc_state = drm_atomic_get_existing_crtc_state(state, 
> encoder_crtc);
>>  		crtc_state->connectors_changed = true;
> [snip]
>
>> @@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device
>> *dev, if (ret)
>>  		return ret;
>>
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_oldnew_connector_in_state(state, connector,
>> old_connector_state, new_connector_state, i) {
> Line wrap here too ?
>
>> 		/*
>>  		 * This only sets crtc->connectors_changed for routing 
> changes,
>>  		 * drivers must set crtc->connectors_changed themselves when
>>  		 * connector properties need to be updated.
>>  		 */
>>  		ret = update_connector_routing(state, connector,
>> -					       connector_state);
>> +					       old_connector_state,
>> +					       new_connector_state);
>>  		if (ret)
>>  			return ret;
>>  	}
> [snip]
>
>> @@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>> struct drm_crtc *crtc;
>>  	struct drm_crtc_state *crtc_state;
>>  	struct drm_plane *plane;
>> -	struct drm_plane_state *plane_state;
>> +	struct drm_plane_state *plane_state, *old_plane_state;
> In some location you use new_*_state and in others *_state. I believe this 
> should this be standardized to avoid confusion (the code is hard enough to 
> read as it is :-)).
>
> Similarly, you sometimes use *_conn_state and sometimes *_connector_state. 
> That should be standardized as well.
Indeed, at least for those that use both.
>>  	int i, ret = 0;
>>
>> -	for_each_plane_in_state(state, plane, plane_state, i) {
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
>> plane_state, i) {
>> 		const struct drm_plane_helper_funcs *funcs;
>>
>>  		funcs = plane->helper_private;
>>
>> -		drm_atomic_helper_plane_changed(state, plane_state, plane);
>> +		drm_atomic_helper_plane_changed(state, old_plane_state,
>> plane_state, plane);
>>
>>  		if (!funcs || !funcs->atomic_check)
>>  			continue;
> [snip]
>
>> @@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct
>> drm_device *dev, }
>>  	}
>>
>> -	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
>> +	for_each_new_connector_in_state(old_state, connector, conn_state, i) {
> Not strictly related to this patch, but I've been wondering how this works, 
> given that we need to loop over connectors in the new state, not the old 
> state. After some investigation I've realized that both the old and new states 
> contain the same list of objects, as we don't keep a copy of the old global 
> atomic state. old_state was the new state before the state swap, and the swap 
> operation sets state->/objects/[*].state to the old state for all objects, but 
> doesn't modify the object pointers. This is possibly more of a question for 
> Daniel, should this be captured in the documentation somewhere (and is my 
> understanding correct) ?
Yes. But that will go away soon after this patch + all drivers converted.

This is why get_state should not be called during atomic commit, and get_existing_state will be removed.
>>  		const struct drm_encoder_helper_funcs *funcs;
>>  		struct drm_encoder *encoder;
>>
>> -		if (!connector->state->best_encoder)
>> +		if (!conn_state->best_encoder)
>>  			continue;
>>
>> -		if (!connector->state->crtc->state->active ||
>> -		    !drm_atomic_crtc_needs_modeset(connector->state->crtc-
>> state))
>> +		if (!conn_state->crtc->state->active ||
>> +		    !drm_atomic_crtc_needs_modeset(conn_state->crtc->state))
>>  			continue;
>>
>> -		encoder = connector->state->best_encoder;
>> +		encoder = conn_state->best_encoder;
>>  		funcs = encoder->helper_private;
>>
>>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> [snip]
>
>> @@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct
>> drm_device *dev, struct drm_atomic_state *old_state)
>>  {
>>  	struct drm_plane *plane;
>> -	struct drm_plane_state *plane_state;
>> +	struct drm_plane_state *plane_state, *new_plane_state;
>>  	int i;
>>
>> -	for_each_plane_in_state(old_state, plane, plane_state, i) {
>> +	for_each_oldnew_plane_in_state(old_state, plane, plane_state,
>> new_plane_state, i) {
>> 		const struct drm_plane_helper_funcs *funcs;
>>
>> +		/*
>> +		 * This might be called before swapping when commit is 
> aborted,
>> +		 * in which case we have to free the new state.
> Should this be "cleanup the new state" instead of "free the new state" ?
Indeed.
>> +		 */
>> +		if (plane_state == plane->state)
>> +			plane_state = new_plane_state;
>> +
>>  		funcs = plane->helper_private;
>>
>>  		if (funcs->cleanup_fb)
> [snip]
>

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

Re: [PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Laurent Pinchart]

Hi Maarten,

On Wednesday 18 Jan 2017 15:49:56 Maarten Lankhorst wrote:
> Op 17-01-17 om 02:01 schreef Laurent Pinchart:
> > On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++---------------
> >>  1 file changed, 152 insertions(+), 141 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
> >> 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > 
> > [snip]
> > 
> >> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
> >>  {
> >>  	struct drm_crtc_state *crtc_state;
> >>  	struct drm_connector *connector;
> >> -	struct drm_connector_state *connector_state;
> >> +	struct drm_connector_state *old_connector_state, *new_connector_state;
> > 
> > The kernel favours one variable declaration per line, and I think this
> > file does as well, at least mostly (this comment holds for the rest of
> > this patch and the other patches in the series).
> 
> This is the first I've heard of it..
> 
> ~/nfs/linux$ git grep 'int i,'  | wc -l
> 8490
> ~/nfs/linux$ git grep 'int i;'  | wc -l
> 23636
> 
> Based on this, I don't think it's uncommon to have multiple declarations per
> line.

It's not uncommon, no. I still think it hinders readability though, especially 
for long statements like here.

> >>  	int i;

[snip]

> >> @@ -974,18 +977,18 @@ void
> >> drm_atomic_helper_commit_modeset_enables(struct
> >> drm_device *dev,
> >> 		}
> >>  	}
> >> 
> >> -	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> >> +	for_each_new_connector_in_state(old_state, connector, conn_state, i) {
> > 
> > Not strictly related to this patch, but I've been wondering how this
> > works, given that we need to loop over connectors in the new state, not
> > the old state. After some investigation I've realized that both the old
> > and new states contain the same list of objects, as we don't keep a copy
> > of the old global atomic state. old_state was the new state before the
> > state swap, and the swap operation sets state->/objects/[*].state to the
> > old state for all objects, but doesn't modify the object pointers. This is
> > possibly more of a question for Daniel, should this be captured in the
> > documentation somewhere (and is my understanding correct) ?
> 
> Yes. But that will go away soon after this patch + all drivers converted.

Will it ? Even with the new helpers we will have code looping over objects 
from the old state, like here. As the code intends on looping over objects in 
the new state this is confusing until you realize that the old state always 
contains the same objects as the new state. As Ville mentioned, 
drm_atomic_state would be better named drm_atomic_transaction, this would 
remove the ambiguity.

> This is why get_state should not be called during atomic commit, and
> get_existing_state will be removed.

[snip]

-- 
Regards,

Laurent Pinchart

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

[PATCH v3 5/7] drm/atomic: Add macros to access existing old/new state

[Maarten Lankhorst]

After atomic commit, these macros should be used in place of
get_existing_state. Also after commit get_xx_state should no longer
be used because it may not have the required locks.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 include/drm/drm_atomic.h | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 6062e7f27325..2e6bb7acc837 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -285,6 +285,35 @@ drm_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
 }
 
 /**
+ * drm_atomic_get_old_crtc_state - get old crtc state, if it exists
+ * @state: global atomic state object
+ * @crtc: crtc to grab
+ *
+ * This function returns the old crtc state for the given crtc, or
+ * NULL if the crtc is not part of the global atomic state.
+ */
+static inline struct drm_crtc_state *
+drm_atomic_get_old_crtc_state(struct drm_atomic_state *state,
+			      struct drm_crtc *crtc)
+{
+	return state->crtcs[drm_crtc_index(crtc)].old_state;
+}
+/**
+ * drm_atomic_get_new_crtc_state - get new crtc state, if it exists
+ * @state: global atomic state object
+ * @crtc: crtc to grab
+ *
+ * This function returns the new crtc state for the given crtc, or
+ * NULL if the crtc is not part of the global atomic state.
+ */
+static inline struct drm_crtc_state *
+drm_atomic_get_new_crtc_state(struct drm_atomic_state *state,
+			      struct drm_crtc *crtc)
+{
+	return state->crtcs[drm_crtc_index(crtc)].new_state;
+}
+
+/**
  * drm_atomic_get_existing_plane_state - get plane state, if it exists
  * @state: global atomic state object
  * @plane: plane to grab
@@ -300,6 +329,36 @@ drm_atomic_get_existing_plane_state(struct drm_atomic_state *state,
 }
 
 /**
+ * drm_atomic_get_old_plane_state - get plane state, if it exists
+ * @state: global atomic state object
+ * @plane: plane to grab
+ *
+ * This function returns the old plane state for the given plane, or
+ * NULL if the plane is not part of the global atomic state.
+ */
+static inline struct drm_plane_state *
+drm_atomic_get_old_plane_state(struct drm_atomic_state *state,
+			       struct drm_plane *plane)
+{
+	return state->planes[drm_plane_index(plane)].old_state;
+}
+
+/**
+ * drm_atomic_get_new_plane_state - get plane state, if it exists
+ * @state: global atomic state object
+ * @plane: plane to grab
+ *
+ * This function returns the new plane state for the given plane, or
+ * NULL if the plane is not part of the global atomic state.
+ */
+static inline struct drm_plane_state *
+drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
+			       struct drm_plane *plane)
+{
+	return state->planes[drm_plane_index(plane)].new_state;
+}
+
+/**
  * drm_atomic_get_existing_connector_state - get connector state, if it exists
  * @state: global atomic state object
  * @connector: connector to grab
@@ -320,6 +379,46 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state,
 }
 
 /**
+ * drm_atomic_get_old_connector_state - get connector state, if it exists
+ * @state: global atomic state object
+ * @connector: connector to grab
+ *
+ * This function returns the old connector state for the given connector,
+ * or NULL if the connector is not part of the global atomic state.
+ */
+static inline struct drm_connector_state *
+drm_atomic_get_old_connector_state(struct drm_atomic_state *state,
+				   struct drm_connector *connector)
+{
+	int index = drm_connector_index(connector);
+
+	if (index >= state->num_connector)
+		return NULL;
+
+	return state->connectors[index].old_state;
+}
+
+/**
+ * drm_atomic_get_new_connector_state - get connector state, if it exists
+ * @state: global atomic state object
+ * @connector: connector to grab
+ *
+ * This function returns the new connector state for the given connector,
+ * or NULL if the connector is not part of the global atomic state.
+ */
+static inline struct drm_connector_state *
+drm_atomic_get_new_connector_state(struct drm_atomic_state *state,
+				   struct drm_connector *connector)
+{
+	int index = drm_connector_index(connector);
+
+	if (index >= state->num_connector)
+		return NULL;
+
+	return state->connectors[index].new_state;
+}
+
+/**
  * __drm_atomic_get_current_plane_state - get current plane state
  * @state: global atomic state object
  * @plane: plane to grab
-- 
2.7.4

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

Re: [PATCH v3 5/7] drm/atomic: Add macros to access existing old/new state

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 16 Jan 2017 10:37:42 Maarten Lankhorst wrote:
> After atomic commit, these macros should be used in place of
> get_existing_state. Also after commit get_xx_state should no longer
> be used because it may not have the required locks.

Should this be captured in the functions' documentation ? In particular, 
should the drm_atomic_get_existing_*_state() functions be marked as deprecated 
?

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  include/drm/drm_atomic.h | 99 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 6062e7f27325..2e6bb7acc837 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -285,6 +285,35 @@ drm_atomic_get_existing_crtc_state(struct
> drm_atomic_state *state, }
> 
>  /**
> + * drm_atomic_get_old_crtc_state - get old crtc state, if it exists
> + * @state: global atomic state object
> + * @crtc: crtc to grab
> + *
> + * This function returns the old crtc state for the given crtc, or
> + * NULL if the crtc is not part of the global atomic state.
> + */
> +static inline struct drm_crtc_state *
> +drm_atomic_get_old_crtc_state(struct drm_atomic_state *state,
> +			      struct drm_crtc *crtc)
> +{
> +	return state->crtcs[drm_crtc_index(crtc)].old_state;
> +}
> +/**
> + * drm_atomic_get_new_crtc_state - get new crtc state, if it exists
> + * @state: global atomic state object
> + * @crtc: crtc to grab
> + *
> + * This function returns the new crtc state for the given crtc, or
> + * NULL if the crtc is not part of the global atomic state.
> + */
> +static inline struct drm_crtc_state *
> +drm_atomic_get_new_crtc_state(struct drm_atomic_state *state,
> +			      struct drm_crtc *crtc)
> +{
> +	return state->crtcs[drm_crtc_index(crtc)].new_state;
> +}
> +
> +/**
>   * drm_atomic_get_existing_plane_state - get plane state, if it exists
>   * @state: global atomic state object
>   * @plane: plane to grab
> @@ -300,6 +329,36 @@ drm_atomic_get_existing_plane_state(struct
> drm_atomic_state *state, }
> 
>  /**
> + * drm_atomic_get_old_plane_state - get plane state, if it exists
> + * @state: global atomic state object
> + * @plane: plane to grab
> + *
> + * This function returns the old plane state for the given plane, or
> + * NULL if the plane is not part of the global atomic state.
> + */
> +static inline struct drm_plane_state *
> +drm_atomic_get_old_plane_state(struct drm_atomic_state *state,
> +			       struct drm_plane *plane)
> +{
> +	return state->planes[drm_plane_index(plane)].old_state;
> +}
> +
> +/**
> + * drm_atomic_get_new_plane_state - get plane state, if it exists
> + * @state: global atomic state object
> + * @plane: plane to grab
> + *
> + * This function returns the new plane state for the given plane, or
> + * NULL if the plane is not part of the global atomic state.
> + */
> +static inline struct drm_plane_state *
> +drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
> +			       struct drm_plane *plane)
> +{
> +	return state->planes[drm_plane_index(plane)].new_state;
> +}
> +
> +/**
>   * drm_atomic_get_existing_connector_state - get connector state, if it
> exists * @state: global atomic state object
>   * @connector: connector to grab
> @@ -320,6 +379,46 @@ drm_atomic_get_existing_connector_state(struct
> drm_atomic_state *state, }
> 
>  /**
> + * drm_atomic_get_old_connector_state - get connector state, if it exists
> + * @state: global atomic state object
> + * @connector: connector to grab
> + *
> + * This function returns the old connector state for the given connector,
> + * or NULL if the connector is not part of the global atomic state.
> + */
> +static inline struct drm_connector_state *
> +drm_atomic_get_old_connector_state(struct drm_atomic_state *state,
> +				   struct drm_connector *connector)
> +{
> +	int index = drm_connector_index(connector);
> +
> +	if (index >= state->num_connector)
> +		return NULL;
> +
> +	return state->connectors[index].old_state;
> +}
> +
> +/**
> + * drm_atomic_get_new_connector_state - get connector state, if it exists
> + * @state: global atomic state object
> + * @connector: connector to grab
> + *
> + * This function returns the new connector state for the given connector,
> + * or NULL if the connector is not part of the global atomic state.
> + */
> +static inline struct drm_connector_state *
> +drm_atomic_get_new_connector_state(struct drm_atomic_state *state,
> +				   struct drm_connector *connector)
> +{
> +	int index = drm_connector_index(connector);
> +
> +	if (index >= state->num_connector)
> +		return NULL;
> +
> +	return state->connectors[index].new_state;
> +}
> +
> +/**
>   * __drm_atomic_get_current_plane_state - get current plane state
>   * @state: global atomic state object
>   * @plane: plane to grab

-- 
Regards,

Laurent Pinchart

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

[PATCH v3 6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

[Maarten Lankhorst]

This is a straightforward conversion that converts all the users of
get_existing_state in atomic core to use get_old_state or get_new_state

Changes since v1:
- Fix using the wrong state in drm_atomic_helper_update_legacy_modeset_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        |  6 +++---
 drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++++--------------------
 drivers/gpu/drm/drm_blend.c         |  3 +--
 3 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7b61e0da9ace..6428e9469607 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1362,8 +1362,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 		return 0;
 
 	if (conn_state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
-								conn_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(conn_state->state,
+							   conn_state->crtc);
 
 		crtc_state->connector_mask &=
 			~(1 << drm_connector_index(conn_state->connector));
@@ -1480,7 +1480,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 {
 	struct drm_plane *plane;
 
-	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
+	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
 
 	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
 		struct drm_plane_state *plane_state =
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b26cf786ce12..1de8d5fbc8d3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -70,8 +70,7 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	struct drm_crtc_state *crtc_state;
 
 	if (old_plane_state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								old_plane_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, old_plane_state->crtc);
 
 		if (WARN_ON(!crtc_state))
 			return;
@@ -80,8 +79,7 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	}
 
 	if (plane_state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								plane_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
 
 		if (WARN_ON(!crtc_state))
 			return;
@@ -150,7 +148,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	drm_for_each_connector_iter(connector, &conn_iter) {
 		struct drm_crtc_state *crtc_state;
 
-		if (drm_atomic_get_existing_connector_state(state, connector))
+		if (drm_atomic_get_new_connector_state(state, connector))
 			continue;
 
 		encoder = connector->state->best_encoder;
@@ -178,7 +176,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 				 conn_state->crtc->base.id, conn_state->crtc->name,
 				 connector->base.id, connector->name);
 
-		crtc_state = drm_atomic_get_existing_crtc_state(state, conn_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
 
 		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
 		if (ret)
@@ -219,7 +217,7 @@ set_best_encoder(struct drm_atomic_state *state,
 		 */
 		WARN_ON(!crtc && encoder != conn_state->best_encoder);
 		if (crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
 			crtc_state->encoder_mask &=
 				~(1 << drm_encoder_index(conn_state->best_encoder));
@@ -230,7 +228,7 @@ set_best_encoder(struct drm_atomic_state *state,
 		crtc = conn_state->crtc;
 		WARN_ON(!crtc);
 		if (crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
 			crtc_state->encoder_mask |=
 				1 << drm_encoder_index(encoder);
@@ -263,7 +261,7 @@ steal_encoder(struct drm_atomic_state *state,
 
 		set_best_encoder(state, new_connector_state, NULL);
 
-		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, encoder_crtc);
 		crtc_state->connectors_changed = true;
 
 		return;
@@ -286,12 +284,12 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	if (old_connector_state->crtc != new_connector_state->crtc) {
 		if (old_connector_state->crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, old_connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 
 		if (new_connector_state->crtc) {
-			crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 	}
@@ -349,7 +347,7 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	set_best_encoder(state, new_connector_state, new_encoder);
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc);
+	crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
 	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
@@ -390,8 +388,7 @@ mode_fixup(struct drm_atomic_state *state)
 		if (!conn_state->crtc || !conn_state->best_encoder)
 			continue;
 
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								conn_state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
 
 		/*
 		 * Each encoder has at most one connector (since we always steal
@@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (!old_conn_state->crtc)
 			continue;
 
-		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
-								    old_conn_state->crtc);
+		old_crtc_state = drm_atomic_get_new_crtc_state(old_state, old_conn_state->crtc);
 
 		if (!old_crtc_state->active ||
 		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
@@ -828,14 +824,15 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 	/* set legacy state in the crtc structure */
 	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		struct drm_plane *primary = crtc->primary;
+		struct drm_plane_state *plane_state;
 
 		crtc->mode = crtc_state->mode;
 		crtc->enabled = crtc_state->enable;
 
-		if (drm_atomic_get_existing_plane_state(old_state, primary) &&
-		    primary->state->crtc == crtc) {
-			crtc->x = primary->state->src_x >> 16;
-			crtc->y = primary->state->src_y >> 16;
+		plane_state = drm_atomic_get_new_plane_state(old_state, primary);
+		if (plane_state && plane_state->crtc == crtc) {
+			crtc->x = plane_state->src_x >> 16;
+			crtc->y = plane_state->src_y >> 16;
 		}
 
 		if (crtc_state->enable)
@@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 
 	drm_for_each_plane_mask(plane, crtc->dev, plane_mask) {
 		struct drm_plane_state *old_plane_state =
-			drm_atomic_get_existing_plane_state(old_state, plane);
+			drm_atomic_get_old_plane_state(old_state, plane);
 		const struct drm_plane_helper_funcs *plane_funcs;
 
 		plane_funcs = plane->helper_private;
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 1f2412c7ccfd..5c45d0852608 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -388,8 +388,7 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
 		if (!crtc)
 			continue;
 		if (plane->state->zpos != plane_state->zpos) {
-			crtc_state =
-				drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 			crtc_state->zpos_changed = true;
 		}
 	}
-- 
2.7.4

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

Re: [PATCH v3 6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

I don't think you need the "v2" at the end of the subject line.

On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
> This is a straightforward conversion that converts all the users of
> get_existing_state in atomic core to use get_old_state or get_new_state
> 
> Changes since v1:
> - Fix using the wrong state in
> drm_atomic_helper_update_legacy_modeset_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>  drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++------------------
>  drivers/gpu/drm/drm_blend.c         |  3 +--
>  3 files changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7b61e0da9ace..6428e9469607 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[snip]

> @@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct
> drm_crtc_state *old_crtc_state)
> 
>  	drm_for_each_plane_mask(plane, crtc->dev, plane_mask) {
>  		struct drm_plane_state *old_plane_state =
> -			drm_atomic_get_existing_plane_state(old_state, plane);
> +			drm_atomic_get_old_plane_state(old_state, plane);

I believe this change to be correct, but given that the function is called 
after state swap, didn't it use the new state before this patch ?

>  		const struct drm_plane_helper_funcs *plane_funcs;
> 
>  		plane_funcs = plane->helper_private;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

[Maarten Lankhorst]

Op 17-01-17 om 02:12 schreef Laurent Pinchart:
> Hi Maarten,
>
> Thank you for the patch.
>
> I don't think you need the "v2" at the end of the subject line.
>
> On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
>> This is a straightforward conversion that converts all the users of
>> get_existing_state in atomic core to use get_old_state or get_new_state
>>
>> Changes since v1:
>> - Fix using the wrong state in
>> drm_atomic_helper_update_legacy_modeset_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>>  drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++------------------
>>  drivers/gpu/drm/drm_blend.c         |  3 +--
>>  3 files changed, 22 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 7b61e0da9ace..6428e9469607 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
> [snip]
>
>> @@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct
>> drm_crtc_state *old_crtc_state)
>>
>>  	drm_for_each_plane_mask(plane, crtc->dev, plane_mask) {
>>  		struct drm_plane_state *old_plane_state =
>> -			drm_atomic_get_existing_plane_state(old_state, plane);
>> +			drm_atomic_get_old_plane_state(old_state, plane);
> I believe this change to be correct, but given that the function is called 
> after state swap, didn't it use the new state before this patch ?
get_existing_state returns the old state after swap.

But the call to drm_atomic_plane_disabling is confusing to me. I'm changing
the function to pass old and new plane state, which should make it easier to understand.

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

Re: [PATCH v3 6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

[Laurent Pinchart]

Hi Maarten,

One more thing.

On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
> This is a straightforward conversion that converts all the users of
> get_existing_state in atomic core to use get_old_state or get_new_state
> 
> Changes since v1:
> - Fix using the wrong state in
> drm_atomic_helper_update_legacy_modeset_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++--------------------
>  drivers/gpu/drm/drm_blend.c         |  3 +--
>  3 files changed, 22 insertions(+), 26 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c

[snip]

> @@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct
> drm_atomic_state *old_state) if (!old_conn_state->crtc)
>  			continue;
> 
> -		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
> -								    
old_conn_state->crtc);
> +		old_crtc_state = drm_atomic_get_new_crtc_state(old_state,
> old_conn_state->crtc);

This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() 
here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right 
thing as old_state->crtcs[*].state was set to the old state by the state swap 
operation.

On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses 
drm_atomic_get_new_plane_state() the same way you do here, even though it 
operates after state swap, and your changelog specifically mentions that 
you've fixed that in v2. It's getting too late to properly wrap my head around 
this, I'll let you check which option is correct (but I reserve the right to 
challenge it if your explanation isn't convincing enough ;-)).

>  		if (!old_crtc_state->active ||
>  		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-
>state))

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

[Maarten Lankhorst]

Op 17-01-17 om 02:27 schreef Laurent Pinchart:
> Hi Maarten,
>
> One more thing.
>
> On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
>> This is a straightforward conversion that converts all the users of
>> get_existing_state in atomic core to use get_old_state or get_new_state
>>
>> Changes since v1:
>> - Fix using the wrong state in
>> drm_atomic_helper_update_legacy_modeset_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++--------------------
>>  drivers/gpu/drm/drm_blend.c         |  3 +--
>>  3 files changed, 22 insertions(+), 26 deletions(-)
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> [snip]
>
>> @@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct
>> drm_atomic_state *old_state) if (!old_conn_state->crtc)
>>  			continue;
>>
>> -		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
>> -								    
> old_conn_state->crtc);
>> +		old_crtc_state = drm_atomic_get_new_crtc_state(old_state,
>> old_conn_state->crtc);
> This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() 
> here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right 
> thing as old_state->crtcs[*].state was set to the old state by the state swap 
> operation.
This is wrong, even. Fixed in next version. At least looking at the code made it obvious. :)
> On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses 
> drm_atomic_get_new_plane_state() the same way you do here, even though it 
> operates after state swap, and your changelog specifically mentions that 
> you've fixed that in v2. It's getting too late to properly wrap my head around 
> this, I'll let you check which option is correct (but I reserve the right to 
> challenge it if your explanation isn't convincing enough ;-)).
update_legacy_modeset_state was looking at primary->state (new state) before,
but was using get_existing_state to check whether it's part of the commit, so it's
valid to look at plane->state.

This was fixed to get the new state, so it makes sense there.
>>  		if (!old_crtc_state->active ||
>>  		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-
>> state))


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

[PATCH v3 7/7] drm/blend: Use new atomic iterator macros.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_blend.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 5c45d0852608..78cf9f6cae08 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -378,26 +378,26 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
 			      struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	int i, ret = 0;
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		crtc = plane_state->crtc;
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		crtc = new_plane_state->crtc;
 		if (!crtc)
 			continue;
-		if (plane->state->zpos != plane_state->zpos) {
-			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
-			crtc_state->zpos_changed = true;
+		if (old_plane_state->zpos != new_plane_state->zpos) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			new_crtc_state->zpos_changed = true;
 		}
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (crtc_state->plane_mask != crtc->state->plane_mask ||
-		    crtc_state->zpos_changed) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		if (old_crtc_state->plane_mask != new_crtc_state->plane_mask ||
+		    new_crtc_state->zpos_changed) {
 			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
-								    crtc_state);
+								    new_crtc_state);
 			if (ret)
 				return ret;
 		}
-- 
2.7.4

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

Re: [PATCH v3 7/7] drm/blend: Use new atomic iterator macros.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Monday 16 Jan 2017 10:37:44 Maarten Lankhorst wrote:

Could we please get a commit message ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_blend.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 5c45d0852608..78cf9f6cae08 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -378,26 +378,26 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>  			      struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	int i, ret = 0;
> 
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		crtc = plane_state->crtc;
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> new_plane_state, i) {
> +		crtc = new_plane_state->crtc;
>  		if (!crtc)
>  			continue;
> -		if (plane->state->zpos != plane_state->zpos) {
> -			crtc_state = drm_atomic_get_new_crtc_state(state, 
crtc);
> -			crtc_state->zpos_changed = true;
> +		if (old_plane_state->zpos != new_plane_state->zpos) {
> +			new_crtc_state = drm_atomic_get_new_crtc_state(state, 
crtc);
> +			new_crtc_state->zpos_changed = true;
>  		}
>  	}
> 
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (crtc_state->plane_mask != crtc->state->plane_mask ||
> -		    crtc_state->zpos_changed) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> +		if (old_crtc_state->plane_mask != new_crtc_state->plane_mask
> ||
> +		    new_crtc_state->zpos_changed) {
>  			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
> -								    
crtc_state);
> +								    
new_crtc_state);
>  			if (ret)
>  				return ret;
>  		}

-- 
Regards,

Laurent Pinchart

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

✗ Fi.CI.BAT: warning for drm/atomic: Add accessor macros for all atomic state. (rev4)

[Patchwork]

== Series Details ==

Series: drm/atomic: Add accessor macros for all atomic state. (rev4)
URL   : https://patchwork.freedesktop.org/series/17745/
State : warning

== Summary ==

Series 17745v4 drm/atomic: Add accessor macros for all atomic state.
https://patchwork.freedesktop.org/api/1.0/series/17745/revisions/4/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-edid:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-load-detect:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-ivb-3520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

8f5a13bb4605ce9d60e1f2cd2722c9e2854e6749 drm-tip: 2017y-01m-16d-09h-31m-14s UTC integration manifest
355d0db drm/blend: Use new atomic iterator macros.
263f620 drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.
6499312 drm/atomic: Add macros to access existing old/new state
3b0dbcb drm/atomic: Fix atomic helpers to use the new iterator macros.
b0d01a7 drm/atomic: Use new atomic iterator macros.
320dd6f drm/atomic: Make add_affected_connectors look at crtc_state.
215d8ce drm/atomic: Add new iterators over all state, v3.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3524/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patches.

On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
> Fourth iteration. Instead of trying to convert all drivers straight away,
> implement all macros that are required to get state working.
> 
> Old situation:
> Use obj->state, which can refer to old or new state.
> Use drm_atomic_get_(existing_)obj_state, which can refer to new or old
> state. Use for_each_obj_in_state, which refers to new or old state.
> 
> New situation:
> 
> During atomic check:
> - Use drm_atomic_get_obj_state to add a object to the atomic state,
>   or get the new state.
> - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state,
>   without adding the object. This will return NULL if the object is
>   not part of the state. For planes and connectors the relevant crtc_state
>   is added, so this will work to get the crtc_state from obj_state->crtc
>   too, this means not having to write some error handling.
> 
> During atomic commit:
> - Do not use drm_atomic_get_obj_state, obj->state or
> drm_atomic_get_(existing_)obj_state any more, replace with
> drm_atomic_get_old/new_obj_state calls as required.

That will make driver code quite complicated :-/ I wonder if that's really a 
good solution. Maybe we should maintain obj->state only for the duration of 
the commit as a way to simplify drivers, and set it to NULL at all other times 
to avoid misusing it ? I know this contradicts the comment I've made in my 
review of patch 1/7, you can now choose which one to ignore :-)

> During both:
> - Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as
> needed. oldnew will be renamed to for_each_obj_in_state after all callers
> are converted to the new api.
> 
> When not doing atomic updates:
> Look at obj->state for now. I have some patches to fix this but I was asked
> to make it return a const state. This breaks a lot of users though so I
> skipped that patch in this iteration.
> 
> This series will return the correct state regardless of swapping.
> 
> Maarten Lankhorst (7):
>   drm/atomic: Add new iterators over all state, v3.
>   drm/atomic: Make add_affected_connectors look at crtc_state.
>   drm/atomic: Use new atomic iterator macros.
>   drm/atomic: Fix atomic helpers to use the new iterator macros.
>   drm/atomic: Add macros to access existing old/new state
>   drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.
>   drm/blend: Use new atomic iterator macros.
> 
>  drivers/gpu/drm/drm_atomic.c         |  39 ++--
>  drivers/gpu/drm/drm_atomic_helper.c  | 377 ++++++++++++++++++--------------
>  drivers/gpu/drm/drm_blend.c          |  23 +--
>  drivers/gpu/drm/i915/intel_display.c |  13 +-
>  include/drm/drm_atomic.h             | 180 ++++++++++++++++-
>  include/drm/drm_atomic_helper.h      |   2 +
>  6 files changed, 438 insertions(+), 196 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [Intel-gfx] [PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.

[Daniel Vetter]

On Tue, Jan 17, 2017 at 02:45:32AM +0200, Laurent Pinchart wrote:
> Hi Maarten,
> 
> Thank you for the patches.
> 
> On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
> > Fourth iteration. Instead of trying to convert all drivers straight away,
> > implement all macros that are required to get state working.
> > 
> > Old situation:
> > Use obj->state, which can refer to old or new state.
> > Use drm_atomic_get_(existing_)obj_state, which can refer to new or old
> > state. Use for_each_obj_in_state, which refers to new or old state.
> > 
> > New situation:
> > 
> > During atomic check:
> > - Use drm_atomic_get_obj_state to add a object to the atomic state,
> >   or get the new state.
> > - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state,
> >   without adding the object. This will return NULL if the object is
> >   not part of the state. For planes and connectors the relevant crtc_state
> >   is added, so this will work to get the crtc_state from obj_state->crtc
> >   too, this means not having to write some error handling.
> > 
> > During atomic commit:
> > - Do not use drm_atomic_get_obj_state, obj->state or
> > drm_atomic_get_(existing_)obj_state any more, replace with
> > drm_atomic_get_old/new_obj_state calls as required.
> 
> That will make driver code quite complicated :-/ I wonder if that's really a 
> good solution. Maybe we should maintain obj->state only for the duration of 
> the commit as a way to simplify drivers, and set it to NULL at all other times 
> to avoid misusing it ? I know this contradicts the comment I've made in my 
> review of patch 1/7, you can now choose which one to ignore :-)

We still need a state pointer to track the current logical state all the
time (ignoring nonblocking commits or other async magic). We might do that
in obj->_state or something to discourage use, but it needs to be there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.

[Laurent Pinchart]

Hi Maarten,

One more thing.

On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
> Fourth iteration. Instead of trying to convert all drivers straight away,
> implement all macros that are required to get state working.
> 
> Old situation:
> Use obj->state, which can refer to old or new state.
> Use drm_atomic_get_(existing_)obj_state, which can refer to new or old
> state. Use for_each_obj_in_state, which refers to new or old state.
> 
> New situation:
> 
> During atomic check:
> - Use drm_atomic_get_obj_state to add a object to the atomic state,
>   or get the new state.
> - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state,
>   without adding the object. This will return NULL if the object is
>   not part of the state. For planes and connectors the relevant crtc_state
>   is added, so this will work to get the crtc_state from obj_state->crtc
>   too, this means not having to write some error handling.
> 
> During atomic commit:
> - Do not use drm_atomic_get_obj_state, obj->state or
> drm_atomic_get_(existing_)obj_state any more, replace with
> drm_atomic_get_old/new_obj_state calls as required.

There are four calls to the drm_atomic_get_existing_*_state() functions left 
in the DRM core after this series. There's also one call to 
drm_atomic_get_plane_state() in drm_blend.c. Could you please fix that ?

> During both:
> - Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as
> needed. oldnew will be renamed to for_each_obj_in_state after all callers
> are converted to the new api.
> 
> When not doing atomic updates:
> Look at obj->state for now. I have some patches to fix this but I was asked
> to make it return a const state. This breaks a lot of users though so I
> skipped that patch in this iteration.
> 
> This series will return the correct state regardless of swapping.
> 
> Maarten Lankhorst (7):
>   drm/atomic: Add new iterators over all state, v3.
>   drm/atomic: Make add_affected_connectors look at crtc_state.
>   drm/atomic: Use new atomic iterator macros.
>   drm/atomic: Fix atomic helpers to use the new iterator macros.
>   drm/atomic: Add macros to access existing old/new state
>   drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.
>   drm/blend: Use new atomic iterator macros.
> 
>  drivers/gpu/drm/drm_atomic.c         |  39 ++--
>  drivers/gpu/drm/drm_atomic_helper.c  | 377 +++++++++++++++++-------------
>  drivers/gpu/drm/drm_blend.c          |  23 +--
>  drivers/gpu/drm/i915/intel_display.c |  13 +-
>  include/drm/drm_atomic.h             | 180 ++++++++++++++++-
>  include/drm/drm_atomic_helper.h      |   2 +
>  6 files changed, 438 insertions(+), 196 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.

[Laurent Pinchart]

Hi Maarten,

Do you plan to post a v4 ? I have two drivers that depends on this series for 
fence support, and I'd like to get that merged in v4.12.

On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
> Fourth iteration. Instead of trying to convert all drivers straight away,
> implement all macros that are required to get state working.
> 
> Old situation:
> Use obj->state, which can refer to old or new state.
> Use drm_atomic_get_(existing_)obj_state, which can refer to new or old
> state. Use for_each_obj_in_state, which refers to new or old state.
> 
> New situation:
> 
> During atomic check:
> - Use drm_atomic_get_obj_state to add a object to the atomic state,
>   or get the new state.
> - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state,
>   without adding the object. This will return NULL if the object is
>   not part of the state. For planes and connectors the relevant crtc_state
>   is added, so this will work to get the crtc_state from obj_state->crtc
>   too, this means not having to write some error handling.
> 
> During atomic commit:
> - Do not use drm_atomic_get_obj_state, obj->state or
> drm_atomic_get_(existing_)obj_state any more, replace with
> drm_atomic_get_old/new_obj_state calls as required.
> 
> During both:
> - Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as
> needed. oldnew will be renamed to for_each_obj_in_state after all callers
> are converted to the new api.
> 
> When not doing atomic updates:
> Look at obj->state for now. I have some patches to fix this but I was asked
> to make it return a const state. This breaks a lot of users though so I
> skipped that patch in this iteration.
> 
> This series will return the correct state regardless of swapping.
> 
> Maarten Lankhorst (7):
>   drm/atomic: Add new iterators over all state, v3.
>   drm/atomic: Make add_affected_connectors look at crtc_state.
>   drm/atomic: Use new atomic iterator macros.
>   drm/atomic: Fix atomic helpers to use the new iterator macros.
>   drm/atomic: Add macros to access existing old/new state
>   drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.
>   drm/blend: Use new atomic iterator macros.
> 
>  drivers/gpu/drm/drm_atomic.c         |  39 ++--
>  drivers/gpu/drm/drm_atomic_helper.c  | 377 ++++++++++++++++++--------------
>  drivers/gpu/drm/drm_blend.c          |  23 +--
>  drivers/gpu/drm/i915/intel_display.c |  13 +-
>  include/drm/drm_atomic.h             | 180 ++++++++++++++++-
>  include/drm/drm_atomic_helper.h      |   2 +
>  6 files changed, 438 insertions(+), 196 deletions(-)

-- 
Regards,

Laurent Pinchart

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