[PATCH 0/5] drm/i915: Use new atomic iterator macros.

[PATCH 0/5] drm/i915: Use new atomic iterator macros.

[Maarten Lankhorst]

Some small patchset to deal with the atomic iterator changes.

Maarten Lankhorst (5):
  drm/i915: Use new atomic iterator macros in ddi
  drm/i915: Use new atomic iterator macros in fbc
  drm/i915: Use new atomic iterator macros in wm code
  drm/i915: Use new atomic iterator macros in display code
  drm/i915: Use new atomic iterator macros in cdclk

 drivers/gpu/drm/i915/intel_cdclk.c   |   2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |   4 +-
 drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 drivers/gpu/drm/i915/intel_fbc.c     |   2 +-
 drivers/gpu/drm/i915/intel_pm.c      |   8 +-
 6 files changed, 90 insertions(+), 89 deletions(-)

-- 
2.7.4

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

[PATCH 1/5] drm/i915: Use new atomic iterator macros in ddi

[Maarten Lankhorst]

Use for_each_new_connector_in_state instead of for_each_connector_in_state.
Also make the function static, it's only used inside intel_ddi.c

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
 drivers/gpu/drm/i915/intel_drv.h | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 04676760e6fd..4da81d4ae166 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -837,7 +837,7 @@ intel_ddi_get_crtc_encoder(struct intel_crtc *crtc)
 	return ret;
 }
 
-struct intel_encoder *
+static struct intel_encoder *
 intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
@@ -850,7 +850,7 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
 
 	state = crtc_state->base.state;
 
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578f7d20501f..937623ff6d7c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1239,8 +1239,6 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
 				 struct intel_crtc *intel_crtc);
 void intel_ddi_get_config(struct intel_encoder *encoder,
 			  struct intel_crtc_state *pipe_config);
-struct intel_encoder *
-intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state);
 
 void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
 void intel_ddi_clock_get(struct intel_encoder *encoder,
-- 
2.7.4

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

Re: [PATCH 1/5] drm/i915: Use new atomic iterator macros in ddi

[Daniel Vetter]

On Thu, Mar 09, 2017 at 03:52:01PM +0100, Maarten Lankhorst wrote:
> Use for_each_new_connector_in_state instead of for_each_connector_in_state.
> Also make the function static, it's only used inside intel_ddi.c
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

On the topic of static, intel_ddi.h exports _waayaaay_ too much stuff to
intel_display.c. With a slight bit of refactoring we should be able to
condense this down to a few hooks, maybe even just the hooks we use for
encoders (since ddi is an encoder, as signified by the various !dsi checks
we now have).

Anyway, different rant, this looks reasonable.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
>  drivers/gpu/drm/i915/intel_drv.h | 2 --
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 04676760e6fd..4da81d4ae166 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -837,7 +837,7 @@ intel_ddi_get_crtc_encoder(struct intel_crtc *crtc)
>  	return ret;
>  }
>  
> -struct intel_encoder *
> +static struct intel_encoder *
>  intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -850,7 +850,7 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
>  
>  	state = crtc_state->base.state;
>  
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 578f7d20501f..937623ff6d7c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1239,8 +1239,6 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
>  				 struct intel_crtc *intel_crtc);
>  void intel_ddi_get_config(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *pipe_config);
> -struct intel_encoder *
> -intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state);
>  
>  void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
>  void intel_ddi_clock_get(struct intel_encoder *encoder,
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

[PATCH 2/5] drm/i915: Use new atomic iterator macros in fbc

[Maarten Lankhorst]

Use for_each_new_plane_in_state, only the new state is needed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 17d418b23d77..ded2add18b26 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1061,7 +1061,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 	 * plane. We could go for fancier schemes such as checking the plane
 	 * size, but this would just affect the few platforms that don't tie FBC
 	 * to pipe or plane A. */
-	for_each_plane_in_state(state, plane, plane_state, i) {
+	for_each_new_plane_in_state(state, plane, plane_state, i) {
 		struct intel_plane_state *intel_plane_state =
 			to_intel_plane_state(plane_state);
 		struct intel_crtc_state *intel_crtc_state;
-- 
2.7.4

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

Re: [PATCH 2/5] drm/i915: Use new atomic iterator macros in fbc

[Daniel Vetter]

On Thu, Mar 09, 2017 at 03:52:02PM +0100, Maarten Lankhorst wrote:
> Use for_each_new_plane_in_state, only the new state is needed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 17d418b23d77..ded2add18b26 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1061,7 +1061,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>  	 * plane. We could go for fancier schemes such as checking the plane
>  	 * size, but this would just affect the few platforms that don't tie FBC
>  	 * to pipe or plane A. */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> +	for_each_new_plane_in_state(state, plane, plane_state, i) {
>  		struct intel_plane_state *intel_plane_state =
>  			to_intel_plane_state(plane_state);
>  		struct intel_crtc_state *intel_crtc_state;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

[PATCH 3/5] drm/i915: Use new atomic iterator macros in wm code

[Maarten Lankhorst]

The watermark code needs to look at the new allocations, so use
for_each_new_crtc_in_state everywhere.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99e09f63d4b3..8670ef7707e7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4120,7 +4120,7 @@ pipes_modified(struct drm_atomic_state *state)
 	struct drm_crtc_state *cstate;
 	uint32_t i, ret = 0;
 
-	for_each_crtc_in_state(state, crtc, cstate, i)
+	for_each_new_crtc_in_state(state, crtc, cstate, i)
 		ret |= drm_crtc_mask(crtc);
 
 	return ret;
@@ -4263,7 +4263,7 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
 	const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
 	int i;
 
-	for_each_crtc_in_state(state, crtc, cstate, i) {
+	for_each_new_crtc_in_state(state, crtc, cstate, i) {
 		const struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		enum pipe pipe = intel_crtc->pipe;
 
@@ -4305,7 +4305,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 * since any racing commits that want to update them would need to
 	 * hold _all_ CRTC state mutexes.
 	 */
-	for_each_crtc_in_state(state, crtc, cstate, i)
+	for_each_new_crtc_in_state(state, crtc, cstate, i)
 		changed = true;
 	if (!changed)
 		return 0;
@@ -4327,7 +4327,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 * should allow skl_update_pipe_wm() to return failure in cases where
 	 * no suitable watermark values can be found.
 	 */
-	for_each_crtc_in_state(state, crtc, cstate, i) {
+	for_each_new_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *intel_cstate =
 			to_intel_crtc_state(cstate);
 		const struct skl_pipe_wm *old_pipe_wm =
-- 
2.7.4

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

Re: [PATCH 3/5] drm/i915: Use new atomic iterator macros in wm code

[Daniel Vetter]

On Thu, Mar 09, 2017 at 03:52:03PM +0100, Maarten Lankhorst wrote:
> The watermark code needs to look at the new allocations, so use
> for_each_new_crtc_in_state everywhere.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99e09f63d4b3..8670ef7707e7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4120,7 +4120,7 @@ pipes_modified(struct drm_atomic_state *state)
>  	struct drm_crtc_state *cstate;
>  	uint32_t i, ret = 0;
>  
> -	for_each_crtc_in_state(state, crtc, cstate, i)
> +	for_each_new_crtc_in_state(state, crtc, cstate, i)
>  		ret |= drm_crtc_mask(crtc);
>  
>  	return ret;
> @@ -4263,7 +4263,7 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
>  	const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
>  	int i;
>  
> -	for_each_crtc_in_state(state, crtc, cstate, i) {
> +	for_each_new_crtc_in_state(state, crtc, cstate, i) {
>  		const struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		enum pipe pipe = intel_crtc->pipe;
>  
> @@ -4305,7 +4305,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	 * since any racing commits that want to update them would need to
>  	 * hold _all_ CRTC state mutexes.
>  	 */
> -	for_each_crtc_in_state(state, crtc, cstate, i)
> +	for_each_new_crtc_in_state(state, crtc, cstate, i)
>  		changed = true;
>  	if (!changed)
>  		return 0;
> @@ -4327,7 +4327,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	 * should allow skl_update_pipe_wm() to return failure in cases where
>  	 * no suitable watermark values can be found.
>  	 */
> -	for_each_crtc_in_state(state, crtc, cstate, i) {
> +	for_each_new_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *intel_cstate =
>  			to_intel_crtc_state(cstate);
>  		const struct skl_pipe_wm *old_pipe_wm =
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

[PATCH 4/5] drm/i915: Use new atomic iterator macros in display code

[Maarten Lankhorst]

Add a big fat warning in __intel_display_resume that the old state is
invalid, and use the correct state everywhere.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
 1 file changed, 82 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5526a196e8a2..83f86cf44f66 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
 	if (!state)
 		return 0;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	/*
+	 * We've duplicated the state, pointers to the old state are invalid.
+	 *
+	 * Don't attempt to use the old state until we commit the duplicated state.
+	 */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		/*
 		 * Force recalculation even if we restore
 		 * current state. With fast modeset this may not result
@@ -5002,13 +5007,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	}
 }
 
-static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
+static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
+				   struct intel_crtc_state *pipe_config)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc_state *pipe_config =
-		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
@@ -5105,12 +5109,11 @@ static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
 					  struct intel_crtc_state *crtc_state,
 					  struct drm_atomic_state *old_state)
 {
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *conn_state;
 	struct drm_connector *conn;
 	int i;
 
-	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
-		struct drm_connector_state *conn_state = conn->state;
+	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
 		struct intel_encoder *encoder =
 			to_intel_encoder(conn_state->best_encoder);
 
@@ -5126,12 +5129,11 @@ static void intel_encoders_pre_enable(struct drm_crtc *crtc,
 				      struct intel_crtc_state *crtc_state,
 				      struct drm_atomic_state *old_state)
 {
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *conn_state;
 	struct drm_connector *conn;
 	int i;
 
-	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
-		struct drm_connector_state *conn_state = conn->state;
+	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
 		struct intel_encoder *encoder =
 			to_intel_encoder(conn_state->best_encoder);
 
@@ -5147,12 +5149,11 @@ static void intel_encoders_enable(struct drm_crtc *crtc,
 				  struct intel_crtc_state *crtc_state,
 				  struct drm_atomic_state *old_state)
 {
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *conn_state;
 	struct drm_connector *conn;
 	int i;
 
-	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
-		struct drm_connector_state *conn_state = conn->state;
+	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
 		struct intel_encoder *encoder =
 			to_intel_encoder(conn_state->best_encoder);
 
@@ -5172,7 +5173,7 @@ static void intel_encoders_disable(struct drm_crtc *crtc,
 	struct drm_connector *conn;
 	int i;
 
-	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
+	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
 		struct intel_encoder *encoder =
 			to_intel_encoder(old_conn_state->best_encoder);
 
@@ -5192,7 +5193,7 @@ static void intel_encoders_post_disable(struct drm_crtc *crtc,
 	struct drm_connector *conn;
 	int i;
 
-	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
+	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
 		struct intel_encoder *encoder =
 			to_intel_encoder(old_conn_state->best_encoder);
 
@@ -5212,7 +5213,7 @@ static void intel_encoders_post_pll_disable(struct drm_crtc *crtc,
 	struct drm_connector *conn;
 	int i;
 
-	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
+	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
 		struct intel_encoder *encoder =
 			to_intel_encoder(old_conn_state->best_encoder);
 
@@ -10877,7 +10878,7 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	struct drm_connector_state *connector_state;
 	int i;
 
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
@@ -11051,7 +11052,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 	state = pipe_config->base.state;
 
 	/* Clamp display bpp to EDID value */
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
@@ -11324,7 +11325,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 			       &pipe_config->pipe_src_w,
 			       &pipe_config->pipe_src_h);
 
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc)
 			continue;
 
@@ -11355,7 +11356,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
 	 */
-	for_each_connector_in_state(state, connector, connector_state, i) {
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc)
 			continue;
 
@@ -11407,16 +11408,16 @@ static void
 intel_modeset_update_crtc_state(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	int i;
 
 	/* Double check state. */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
 
 		/* Update hwmode for vblank functions */
-		if (crtc->state->active)
-			crtc->hwmode = crtc->state->adjusted_mode;
+		if (new_crtc_state->active)
+			crtc->hwmode = new_crtc_state->adjusted_mode;
 		else
 			crtc->hwmode.crtc_clock = 0;
 
@@ -11889,19 +11890,18 @@ verify_connector_state(struct drm_device *dev,
 		       struct drm_crtc *crtc)
 {
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *old_conn_state, *conn_state;
 	int i;
 
-	for_each_connector_in_state(state, connector, old_conn_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) {
 		struct drm_encoder *encoder = connector->encoder;
-		struct drm_connector_state *state = connector->state;
 
-		if (state->crtc != crtc)
+		if (conn_state->crtc != crtc)
 			continue;
 
 		intel_connector_verify_state(to_intel_connector(connector));
 
-		I915_STATE_WARN(state->best_encoder != encoder,
+		I915_STATE_WARN(conn_state->best_encoder != encoder,
 		     "connector's atomic encoder doesn't match legacy encoder\n");
 	}
 }
@@ -12187,21 +12187,21 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 
 	if (!dev_priv->display.crtc_compute_clock)
 		return;
 
-	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) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		struct intel_shared_dpll *old_dpll =
-			to_intel_crtc_state(crtc->state)->shared_dpll;
+			to_intel_crtc_state(old_crtc_state)->shared_dpll;
 
-		if (!needs_modeset(crtc_state))
+		if (!needs_modeset(new_crtc_state))
 			continue;
 
-		to_intel_crtc_state(crtc_state)->shared_dpll = NULL;
+		to_intel_crtc_state(new_crtc_state)->shared_dpll = NULL;
 
 		if (!old_dpll)
 			continue;
@@ -12227,7 +12227,7 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
 	int i;
 
 	/* look at all crtc's that are going to be enabled in during modeset */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		intel_crtc = to_intel_crtc(crtc);
 
 		if (!crtc_state->active || !needs_modeset(crtc_state))
@@ -12329,7 +12329,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int ret = 0, i;
 
 	if (!check_digital_port_conflicts(state)) {
@@ -12342,13 +12342,13 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 	intel_state->cdclk.logical = dev_priv->cdclk.logical;
 	intel_state->cdclk.actual = dev_priv->cdclk.actual;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (crtc_state->active)
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		if (new_crtc_state->active)
 			intel_state->active_crtcs |= 1 << i;
 		else
 			intel_state->active_crtcs &= ~(1 << i);
 
-		if (crtc_state->active != crtc->state->active)
+		if (old_crtc_state->active != new_crtc_state->active)
 			intel_state->active_pipe_changes |= drm_crtc_mask(crtc);
 	}
 
@@ -12427,7 +12427,7 @@ static int intel_atomic_check(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *crtc_state;
 	int ret, i;
 	bool any_ms = false;
 
@@ -12435,12 +12435,12 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return 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) {
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
 		/* Catch I915_MODE_FLAG_INHERITED */
-		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
+		if (crtc_state->mode.private_flags != old_crtc_state->mode.private_flags)
 			crtc_state->mode_changed = true;
 
 		if (!needs_modeset(crtc_state))
@@ -12467,10 +12467,10 @@ static int intel_atomic_check(struct drm_device *dev,
 
 		if (i915.fastboot &&
 		    intel_pipe_config_compare(dev_priv,
-					to_intel_crtc_state(crtc->state),
+					to_intel_crtc_state(old_crtc_state),
 					pipe_config, true)) {
 			crtc_state->mode_changed = false;
-			to_intel_crtc_state(crtc_state)->update_pipe = true;
+			pipe_config->update_pipe = true;
 		}
 
 		if (needs_modeset(crtc_state))
@@ -12510,7 +12510,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int i, ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		if (state->legacy_cursor_update)
 			continue;
 
@@ -12607,19 +12607,21 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
 static void intel_update_crtc(struct drm_crtc *crtc,
 			      struct drm_atomic_state *state,
 			      struct drm_crtc_state *old_crtc_state,
+			      struct drm_crtc_state *new_crtc_state,
 			      unsigned int *crtc_vblank_mask)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->state);
-	bool modeset = needs_modeset(crtc->state);
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
+	bool modeset = needs_modeset(new_crtc_state);
 
 	if (modeset) {
 		update_scanline_offset(intel_crtc);
 		dev_priv->display.crtc_enable(pipe_config, state);
 	} else {
-		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
+		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
+				       pipe_config);
 	}
 
 	if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
@@ -12638,15 +12640,15 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
 			       unsigned int *crtc_vblank_mask)
 {
 	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_crtc_in_state(state, crtc, old_crtc_state, i) {
-		if (!crtc->state->active)
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		if (!new_crtc_state->active)
 			continue;
 
 		intel_update_crtc(crtc, state, old_crtc_state,
-				  crtc_vblank_mask);
+				  new_crtc_state, crtc_vblank_mask);
 	}
 }
 
@@ -12657,7 +12659,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct intel_crtc_state *cstate;
 	unsigned int updated = 0;
 	bool progress;
@@ -12666,9 +12668,9 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 
 	const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
 
-	for_each_crtc_in_state(state, crtc, old_crtc_state, i)
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
 		/* ignore allocations for crtc's that have been turned off. */
-		if (crtc->state->active)
+		if (new_crtc_state->active)
 			entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
 
 	/*
@@ -12680,7 +12682,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 	do {
 		progress = false;
 
-		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 			bool vbl_wait = false;
 			unsigned int cmask = drm_crtc_mask(crtc);
 
@@ -12705,12 +12707,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 			 */
 			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
 						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
-			    !crtc->state->active_changed &&
+			    !new_crtc_state->active_changed &&
 			    intel_state->wm_results.dirty_pipes != updated)
 				vbl_wait = true;
 
 			intel_update_crtc(crtc, state, old_crtc_state,
-					  crtc_vblank_mask);
+					  new_crtc_state, crtc_vblank_mask);
 
 			if (vbl_wait)
 				intel_wait_for_vblank(dev_priv, pipe);
@@ -12743,7 +12745,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	struct intel_crtc_state *intel_cstate;
 	bool hw_check = intel_state->modeset;
@@ -12756,22 +12758,23 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	if (intel_state->modeset)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 
-	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-		if (needs_modeset(crtc->state) ||
-		    to_intel_crtc_state(crtc->state)->update_pipe) {
+		if (needs_modeset(new_crtc_state) ||
+		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
 			hw_check = true;
 
 			put_domains[to_intel_crtc(crtc)->pipe] =
 				modeset_get_crtc_power_domains(crtc,
-					to_intel_crtc_state(crtc->state));
+					to_intel_crtc_state(new_crtc_state));
 		}
 
-		if (!needs_modeset(crtc->state))
+		if (!needs_modeset(new_crtc_state))
 			continue;
 
-		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
+		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
+				       to_intel_crtc_state(new_crtc_state));
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
@@ -12821,16 +12824,16 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	/* Complete the events for pipes that have now been disabled */
-	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
-		bool modeset = needs_modeset(crtc->state);
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
 
 		/* Complete events for now disable pipes here. */
-		if (modeset && !crtc->state->active && crtc->state->event) {
+		if (modeset && !new_crtc_state->active && new_crtc_state->event) {
 			spin_lock_irq(&dev->event_lock);
-			drm_crtc_send_vblank_event(crtc, crtc->state->event);
+			drm_crtc_send_vblank_event(crtc, new_crtc_state->event);
 			spin_unlock_irq(&dev->event_lock);
 
-			crtc->state->event = NULL;
+			new_crtc_state->event = NULL;
 		}
 	}
 
@@ -12856,21 +12859,21 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	 *
 	 * TODO: Move this (and other cleanup) to an async worker eventually.
 	 */
-	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
-		intel_cstate = to_intel_crtc_state(crtc->state);
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		intel_cstate = to_intel_crtc_state(new_crtc_state);
 
 		if (dev_priv->display.optimize_watermarks)
 			dev_priv->display.optimize_watermarks(intel_state,
 							      intel_cstate);
 	}
 
-	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		intel_post_plane_update(to_intel_crtc_state(old_crtc_state));
 
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 
-		intel_modeset_verify_crtc(crtc, state, old_crtc_state, crtc->state);
+		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
 	}
 
 	if (intel_state->modeset && intel_can_enable_sagv(state))
@@ -12942,13 +12945,13 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,
 
 static void intel_atomic_track_fbs(struct drm_atomic_state *state)
 {
-	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_plane *plane;
 	int i;
 
-	for_each_plane_in_state(state, plane, old_plane_state, i)
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
 		i915_gem_track_fb(intel_fb_obj(old_plane_state->fb),
-				  intel_fb_obj(plane->state->fb),
+				  intel_fb_obj(new_plane_state->fb),
 				  to_intel_plane(plane)->frontbuffer_bit);
 }
 
@@ -14890,7 +14893,7 @@ static void sanitize_watermarks(struct drm_device *dev)
 	}
 
 	/* Write calculated watermark values back */
-	for_each_crtc_in_state(state, crtc, cstate, i) {
+	for_each_new_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
 		cs->wm.need_postvbl_update = true;
-- 
2.7.4

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

Re: [PATCH 4/5] drm/i915: Use new atomic iterator macros in display code

[Daniel Vetter]

On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
> Add a big fat warning in __intel_display_resume that the old state is
> invalid, and use the correct state everywhere.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------

This file is too big. Because it's one big mess this patch here is way too
big and one big mess :(

>  1 file changed, 82 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5526a196e8a2..83f86cf44f66 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
>  	if (!state)
>  		return 0;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	/*
> +	 * We've duplicated the state, pointers to the old state are invalid.
> +	 *
> +	 * Don't attempt to use the old state until we commit the duplicated state.
> +	 */
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {

Just a drive-by comment, but why exactly do we need this hack? We read out
full hw state like on boot-up, assuming our commit machinery computes the
diff correctly it should noticed that we have to do a full commit here.

Hacks like this imo show that we still have a fairly significant design
issue in our fastbook code ...

>  		/*
>  		 * Force recalculation even if we restore
>  		 * current state. With fast modeset this may not result
> @@ -5002,13 +5007,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  	}
>  }
>  
> -static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> +				   struct intel_crtc_state *pipe_config)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc_state *pipe_config =
> -		to_intel_crtc_state(crtc->base.state);
>  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_pri_state =
> @@ -5105,12 +5109,11 @@ static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
>  					  struct intel_crtc_state *crtc_state,
>  					  struct drm_atomic_state *old_state)
>  {
> -	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *conn_state;
>  	struct drm_connector *conn;
>  	int i;
>  
> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> -		struct drm_connector_state *conn_state = conn->state;
> +	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>  		struct intel_encoder *encoder =
>  			to_intel_encoder(conn_state->best_encoder);
>  
> @@ -5126,12 +5129,11 @@ static void intel_encoders_pre_enable(struct drm_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state,
>  				      struct drm_atomic_state *old_state)
>  {
> -	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *conn_state;
>  	struct drm_connector *conn;
>  	int i;
>  
> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> -		struct drm_connector_state *conn_state = conn->state;
> +	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>  		struct intel_encoder *encoder =
>  			to_intel_encoder(conn_state->best_encoder);
>  
> @@ -5147,12 +5149,11 @@ static void intel_encoders_enable(struct drm_crtc *crtc,
>  				  struct intel_crtc_state *crtc_state,
>  				  struct drm_atomic_state *old_state)
>  {
> -	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *conn_state;
>  	struct drm_connector *conn;
>  	int i;
>  
> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> -		struct drm_connector_state *conn_state = conn->state;
> +	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>  		struct intel_encoder *encoder =
>  			to_intel_encoder(conn_state->best_encoder);
>  
> @@ -5172,7 +5173,7 @@ static void intel_encoders_disable(struct drm_crtc *crtc,
>  	struct drm_connector *conn;
>  	int i;
>  
> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>  		struct intel_encoder *encoder =
>  			to_intel_encoder(old_conn_state->best_encoder);
>  
> @@ -5192,7 +5193,7 @@ static void intel_encoders_post_disable(struct drm_crtc *crtc,
>  	struct drm_connector *conn;
>  	int i;
>  
> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>  		struct intel_encoder *encoder =
>  			to_intel_encoder(old_conn_state->best_encoder);
>  
> @@ -5212,7 +5213,7 @@ static void intel_encoders_post_pll_disable(struct drm_crtc *crtc,
>  	struct drm_connector *conn;
>  	int i;
>  
> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>  		struct intel_encoder *encoder =
>  			to_intel_encoder(old_conn_state->best_encoder);
>  
> @@ -10877,7 +10878,7 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
>  	struct drm_connector_state *connector_state;
>  	int i;
>  
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> @@ -11051,7 +11052,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  	state = pipe_config->base.state;
>  
>  	/* Clamp display bpp to EDID value */
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> @@ -11324,7 +11325,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  			       &pipe_config->pipe_src_w,
>  			       &pipe_config->pipe_src_h);
>  
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc)
>  			continue;
>  
> @@ -11355,7 +11356,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
>  	 */
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != crtc)
>  			continue;
>  
> @@ -11407,16 +11408,16 @@ static void
>  intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *new_crtc_state;
>  	int i;
>  
>  	/* Double check state. */
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
>  
>  		/* Update hwmode for vblank functions */
> -		if (crtc->state->active)
> -			crtc->hwmode = crtc->state->adjusted_mode;
> +		if (new_crtc_state->active)
> +			crtc->hwmode = new_crtc_state->adjusted_mode;
>  		else
>  			crtc->hwmode.crtc_clock = 0;
>  
> @@ -11889,19 +11890,18 @@ verify_connector_state(struct drm_device *dev,
>  		       struct drm_crtc *crtc)
>  {
>  	struct drm_connector *connector;
> -	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *old_conn_state, *conn_state;
>  	int i;
>  
> -	for_each_connector_in_state(state, connector, old_conn_state, i) {
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) {

Giving conn_state the new_ prefix would have been nice, like you do below.

>  		struct drm_encoder *encoder = connector->encoder;
> -		struct drm_connector_state *state = connector->state;
>  
> -		if (state->crtc != crtc)
> +		if (conn_state->crtc != crtc)
>  			continue;
>  
>  		intel_connector_verify_state(to_intel_connector(connector));
>  
> -		I915_STATE_WARN(state->best_encoder != encoder,
> +		I915_STATE_WARN(conn_state->best_encoder != encoder,
>  		     "connector's atomic encoder doesn't match legacy encoder\n");
>  	}
>  }
> @@ -12187,21 +12187,21 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	int i;
>  
>  	if (!dev_priv->display.crtc_compute_clock)
>  		return;
>  
> -	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) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		struct intel_shared_dpll *old_dpll =
> -			to_intel_crtc_state(crtc->state)->shared_dpll;
> +			to_intel_crtc_state(old_crtc_state)->shared_dpll;
>  
> -		if (!needs_modeset(crtc_state))
> +		if (!needs_modeset(new_crtc_state))
>  			continue;
>  
> -		to_intel_crtc_state(crtc_state)->shared_dpll = NULL;
> +		to_intel_crtc_state(new_crtc_state)->shared_dpll = NULL;
>  
>  		if (!old_dpll)
>  			continue;
> @@ -12227,7 +12227,7 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
>  	int i;
>  
>  	/* look at all crtc's that are going to be enabled in during modeset */
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		intel_crtc = to_intel_crtc(crtc);
>  
>  		if (!crtc_state->active || !needs_modeset(crtc_state))
> @@ -12329,7 +12329,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	int ret = 0, i;
>  
>  	if (!check_digital_port_conflicts(state)) {
> @@ -12342,13 +12342,13 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
>  	intel_state->cdclk.actual = dev_priv->cdclk.actual;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (crtc_state->active)
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		if (new_crtc_state->active)
>  			intel_state->active_crtcs |= 1 << i;
>  		else
>  			intel_state->active_crtcs &= ~(1 << i);
>  
> -		if (crtc_state->active != crtc->state->active)
> +		if (old_crtc_state->active != new_crtc_state->active)
>  			intel_state->active_pipe_changes |= drm_crtc_mask(crtc);
>  	}
>  
> @@ -12427,7 +12427,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *crtc_state;
>  	int ret, i;
>  	bool any_ms = false;
>  
> @@ -12435,12 +12435,12 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		return 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) {
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc_state);
>  
>  		/* Catch I915_MODE_FLAG_INHERITED */
> -		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
> +		if (crtc_state->mode.private_flags != old_crtc_state->mode.private_flags)
>  			crtc_state->mode_changed = true;
>  
>  		if (!needs_modeset(crtc_state))
> @@ -12467,10 +12467,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  
>  		if (i915.fastboot &&
>  		    intel_pipe_config_compare(dev_priv,
> -					to_intel_crtc_state(crtc->state),
> +					to_intel_crtc_state(old_crtc_state),
>  					pipe_config, true)) {
>  			crtc_state->mode_changed = false;
> -			to_intel_crtc_state(crtc_state)->update_pipe = true;
> +			pipe_config->update_pipe = true;
>  		}
>  
>  		if (needs_modeset(crtc_state))
> @@ -12510,7 +12510,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int i, ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (state->legacy_cursor_update)
>  			continue;
>  
> @@ -12607,19 +12607,21 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>  static void intel_update_crtc(struct drm_crtc *crtc,
>  			      struct drm_atomic_state *state,
>  			      struct drm_crtc_state *old_crtc_state,
> +			      struct drm_crtc_state *new_crtc_state,
>  			      unsigned int *crtc_vblank_mask)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->state);
> -	bool modeset = needs_modeset(crtc->state);
> +	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> +	bool modeset = needs_modeset(new_crtc_state);
>  
>  	if (modeset) {
>  		update_scanline_offset(intel_crtc);
>  		dev_priv->display.crtc_enable(pipe_config, state);
>  	} else {
> -		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
> +		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
> +				       pipe_config);
>  	}
>  
>  	if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
> @@ -12638,15 +12640,15 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>  			       unsigned int *crtc_vblank_mask)
>  {
>  	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_crtc_in_state(state, crtc, old_crtc_state, i) {
> -		if (!crtc->state->active)
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		if (!new_crtc_state->active)
>  			continue;
>  
>  		intel_update_crtc(crtc, state, old_crtc_state,
> -				  crtc_vblank_mask);
> +				  new_crtc_state, crtc_vblank_mask);
>  	}
>  }
>  
> @@ -12657,7 +12659,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_crtc *crtc;
>  	struct intel_crtc *intel_crtc;
> -	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct intel_crtc_state *cstate;
>  	unsigned int updated = 0;
>  	bool progress;
> @@ -12666,9 +12668,9 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  
>  	const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
>  
> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i)
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
>  		/* ignore allocations for crtc's that have been turned off. */
> -		if (crtc->state->active)
> +		if (new_crtc_state->active)
>  			entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>  
>  	/*
> @@ -12680,7 +12682,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  	do {
>  		progress = false;
>  
> -		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  			bool vbl_wait = false;
>  			unsigned int cmask = drm_crtc_mask(crtc);
>  
> @@ -12705,12 +12707,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  			 */
>  			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
>  						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
> -			    !crtc->state->active_changed &&
> +			    !new_crtc_state->active_changed &&
>  			    intel_state->wm_results.dirty_pipes != updated)
>  				vbl_wait = true;
>  
>  			intel_update_crtc(crtc, state, old_crtc_state,
> -					  crtc_vblank_mask);
> +					  new_crtc_state, crtc_vblank_mask);
>  
>  			if (vbl_wait)
>  				intel_wait_for_vblank(dev_priv, pipe);
> @@ -12743,7 +12745,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct intel_crtc_state *intel_cstate;
>  	bool hw_check = intel_state->modeset;
> @@ -12756,22 +12758,23 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	if (intel_state->modeset)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>  
> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -		if (needs_modeset(crtc->state) ||
> -		    to_intel_crtc_state(crtc->state)->update_pipe) {
> +		if (needs_modeset(new_crtc_state) ||
> +		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
>  			hw_check = true;
>  
>  			put_domains[to_intel_crtc(crtc)->pipe] =
>  				modeset_get_crtc_power_domains(crtc,
> -					to_intel_crtc_state(crtc->state));
> +					to_intel_crtc_state(new_crtc_state));
>  		}
>  
> -		if (!needs_modeset(crtc->state))
> +		if (!needs_modeset(new_crtc_state))
>  			continue;
>  
> -		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
> +		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
> +				       to_intel_crtc_state(new_crtc_state));
>  
>  		if (old_crtc_state->active) {
>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> @@ -12821,16 +12824,16 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	/* Complete the events for pipes that have now been disabled */
> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> -		bool modeset = needs_modeset(crtc->state);
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		bool modeset = needs_modeset(new_crtc_state);
>  
>  		/* Complete events for now disable pipes here. */
> -		if (modeset && !crtc->state->active && crtc->state->event) {
> +		if (modeset && !new_crtc_state->active && new_crtc_state->event) {
>  			spin_lock_irq(&dev->event_lock);
> -			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +			drm_crtc_send_vblank_event(crtc, new_crtc_state->event);
>  			spin_unlock_irq(&dev->event_lock);
>  
> -			crtc->state->event = NULL;
> +			new_crtc_state->event = NULL;
>  		}
>  	}
>  
> @@ -12856,21 +12859,21 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	 *
>  	 * TODO: Move this (and other cleanup) to an async worker eventually.
>  	 */
> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> -		intel_cstate = to_intel_crtc_state(crtc->state);
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		intel_cstate = to_intel_crtc_state(new_crtc_state);
>  
>  		if (dev_priv->display.optimize_watermarks)
>  			dev_priv->display.optimize_watermarks(intel_state,
>  							      intel_cstate);
>  	}
>  
> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		intel_post_plane_update(to_intel_crtc_state(old_crtc_state));
>  
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv, put_domains[i]);
>  
> -		intel_modeset_verify_crtc(crtc, state, old_crtc_state, crtc->state);
> +		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
>  	}
>  
>  	if (intel_state->modeset && intel_can_enable_sagv(state))
> @@ -12942,13 +12945,13 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,
>  
>  static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>  {
> -	struct drm_plane_state *old_plane_state;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct drm_plane *plane;
>  	int i;
>  
> -	for_each_plane_in_state(state, plane, old_plane_state, i)
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>  		i915_gem_track_fb(intel_fb_obj(old_plane_state->fb),
> -				  intel_fb_obj(plane->state->fb),
> +				  intel_fb_obj(new_plane_state->fb),
>  				  to_intel_plane(plane)->frontbuffer_bit);
>  }
>  
> @@ -14890,7 +14893,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	}
>  
>  	/* Write calculated watermark values back */
> -	for_each_crtc_in_state(state, crtc, cstate, i) {
> +	for_each_new_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>  
>  		cs->wm.need_postvbl_update = true;

Yeah, the new iterators are definitely much, much better. I tried to read
it all carefully (really, intel_display.c should be split a lot), and
didn't find any real bugs. A bit more consistency with old/new, when we
mix them, would be great though.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
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 4/5] drm/i915: Use new atomic iterator macros in display code

[Maarten Lankhorst]

Op 13-03-17 om 11:15 schreef Daniel Vetter:
> On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
>> Add a big fat warning in __intel_display_resume that the old state is
>> invalid, and use the correct state everywhere.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
> This file is too big. Because it's one big mess this patch here is way too
> big and one big mess :(
>
>>  1 file changed, 82 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5526a196e8a2..83f86cf44f66 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
>>  	if (!state)
>>  		return 0;
>>  
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +	/*
>> +	 * We've duplicated the state, pointers to the old state are invalid.
>> +	 *
>> +	 * Don't attempt to use the old state until we commit the duplicated state.
>> +	 */
>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> Just a drive-by comment, but why exactly do we need this hack? We read out
> full hw state like on boot-up, assuming our commit machinery computes the
> diff correctly it should noticed that we have to do a full commit here.
>
> Hacks like this imo show that we still have a fairly significant design
> issue in our fastbook code ...
We read out the full hw state, but we cannot trust the hw state to be correct. In general when we poke a property we set mode_changed,
and the fastboot code may downgrade it to a pipe update if it's compatible enough. We only check on mode_changed because not all
planes and connectors may otherwise be part of the atomic state.
>>  		/*
>>  		 * Force recalculation even if we restore
>>  		 * current state. With fast modeset this may not result
>> @@ -5002,13 +5007,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>  	}
>>  }
>>  
>> -static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>> +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>> +				   struct intel_crtc_state *pipe_config)
>>  {
>>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>> -	struct intel_crtc_state *pipe_config =
>> -		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
>>  	struct drm_plane *primary = crtc->base.primary;
>>  	struct drm_plane_state *old_pri_state =
>> @@ -5105,12 +5109,11 @@ static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
>>  					  struct intel_crtc_state *crtc_state,
>>  					  struct drm_atomic_state *old_state)
>>  {
>> -	struct drm_connector_state *old_conn_state;
>> +	struct drm_connector_state *conn_state;
>>  	struct drm_connector *conn;
>>  	int i;
>>  
>> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
>> -		struct drm_connector_state *conn_state = conn->state;
>> +	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>>  		struct intel_encoder *encoder =
>>  			to_intel_encoder(conn_state->best_encoder);
>>  
>> @@ -5126,12 +5129,11 @@ static void intel_encoders_pre_enable(struct drm_crtc *crtc,
>>  				      struct intel_crtc_state *crtc_state,
>>  				      struct drm_atomic_state *old_state)
>>  {
>> -	struct drm_connector_state *old_conn_state;
>> +	struct drm_connector_state *conn_state;
>>  	struct drm_connector *conn;
>>  	int i;
>>  
>> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
>> -		struct drm_connector_state *conn_state = conn->state;
>> +	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>>  		struct intel_encoder *encoder =
>>  			to_intel_encoder(conn_state->best_encoder);
>>  
>> @@ -5147,12 +5149,11 @@ static void intel_encoders_enable(struct drm_crtc *crtc,
>>  				  struct intel_crtc_state *crtc_state,
>>  				  struct drm_atomic_state *old_state)
>>  {
>> -	struct drm_connector_state *old_conn_state;
>> +	struct drm_connector_state *conn_state;
>>  	struct drm_connector *conn;
>>  	int i;
>>  
>> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
>> -		struct drm_connector_state *conn_state = conn->state;
>> +	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>>  		struct intel_encoder *encoder =
>>  			to_intel_encoder(conn_state->best_encoder);
>>  
>> @@ -5172,7 +5173,7 @@ static void intel_encoders_disable(struct drm_crtc *crtc,
>>  	struct drm_connector *conn;
>>  	int i;
>>  
>> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
>> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>>  		struct intel_encoder *encoder =
>>  			to_intel_encoder(old_conn_state->best_encoder);
>>  
>> @@ -5192,7 +5193,7 @@ static void intel_encoders_post_disable(struct drm_crtc *crtc,
>>  	struct drm_connector *conn;
>>  	int i;
>>  
>> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
>> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>>  		struct intel_encoder *encoder =
>>  			to_intel_encoder(old_conn_state->best_encoder);
>>  
>> @@ -5212,7 +5213,7 @@ static void intel_encoders_post_pll_disable(struct drm_crtc *crtc,
>>  	struct drm_connector *conn;
>>  	int i;
>>  
>> -	for_each_connector_in_state(old_state, conn, old_conn_state, i) {
>> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>>  		struct intel_encoder *encoder =
>>  			to_intel_encoder(old_conn_state->best_encoder);
>>  
>> @@ -10877,7 +10878,7 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
>>  	struct drm_connector_state *connector_state;
>>  	int i;
>>  
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>>  		if (connector_state->crtc != &crtc->base)
>>  			continue;
>>  
>> @@ -11051,7 +11052,7 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>>  	state = pipe_config->base.state;
>>  
>>  	/* Clamp display bpp to EDID value */
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>>  		if (connector_state->crtc != &crtc->base)
>>  			continue;
>>  
>> @@ -11324,7 +11325,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>>  			       &pipe_config->pipe_src_w,
>>  			       &pipe_config->pipe_src_h);
>>  
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>>  		if (connector_state->crtc != crtc)
>>  			continue;
>>  
>> @@ -11355,7 +11356,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>>  	 * adjust it according to limitations or connector properties, and also
>>  	 * a chance to reject the mode entirely.
>>  	 */
>> -	for_each_connector_in_state(state, connector, connector_state, i) {
>> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
>>  		if (connector_state->crtc != crtc)
>>  			continue;
>>  
>> @@ -11407,16 +11408,16 @@ static void
>>  intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>>  {
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *new_crtc_state;
>>  	int i;
>>  
>>  	/* Double check state. */
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> +		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
>>  
>>  		/* Update hwmode for vblank functions */
>> -		if (crtc->state->active)
>> -			crtc->hwmode = crtc->state->adjusted_mode;
>> +		if (new_crtc_state->active)
>> +			crtc->hwmode = new_crtc_state->adjusted_mode;
>>  		else
>>  			crtc->hwmode.crtc_clock = 0;
>>  
>> @@ -11889,19 +11890,18 @@ verify_connector_state(struct drm_device *dev,
>>  		       struct drm_crtc *crtc)
>>  {
>>  	struct drm_connector *connector;
>> -	struct drm_connector_state *old_conn_state;
>> +	struct drm_connector_state *old_conn_state, *conn_state;
>>  	int i;
>>  
>> -	for_each_connector_in_state(state, connector, old_conn_state, i) {
>> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) {
> Giving conn_state the new_ prefix would have been nice, like you do below.
>
Fixed this one, thanks.
>>  		struct drm_encoder *encoder = connector->encoder;
>> -		struct drm_connector_state *state = connector->state;
>>  
>> -		if (state->crtc != crtc)
>> +		if (conn_state->crtc != crtc)
>>  			continue;
>>  
>>  		intel_connector_verify_state(to_intel_connector(connector));
>>  
>> -		I915_STATE_WARN(state->best_encoder != encoder,
>> +		I915_STATE_WARN(conn_state->best_encoder != encoder,
>>  		     "connector's atomic encoder doesn't match legacy encoder\n");
>>  	}
>>  }
>> @@ -12187,21 +12187,21 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>>  	struct drm_device *dev = state->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>  	int i;
>>  
>>  	if (!dev_priv->display.crtc_compute_clock)
>>  		return;
>>  
>> -	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) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  		struct intel_shared_dpll *old_dpll =
>> -			to_intel_crtc_state(crtc->state)->shared_dpll;
>> +			to_intel_crtc_state(old_crtc_state)->shared_dpll;
>>  
>> -		if (!needs_modeset(crtc_state))
>> +		if (!needs_modeset(new_crtc_state))
>>  			continue;
>>  
>> -		to_intel_crtc_state(crtc_state)->shared_dpll = NULL;
>> +		to_intel_crtc_state(new_crtc_state)->shared_dpll = NULL;
>>  
>>  		if (!old_dpll)
>>  			continue;
>> @@ -12227,7 +12227,7 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
>>  	int i;
>>  
>>  	/* look at all crtc's that are going to be enabled in during modeset */
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>  		intel_crtc = to_intel_crtc(crtc);
>>  
>>  		if (!crtc_state->active || !needs_modeset(crtc_state))
>> @@ -12329,7 +12329,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>  	int ret = 0, i;
>>  
>>  	if (!check_digital_port_conflicts(state)) {
>> @@ -12342,13 +12342,13 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
>>  	intel_state->cdclk.actual = dev_priv->cdclk.actual;
>>  
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (crtc_state->active)
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>> +		if (new_crtc_state->active)
>>  			intel_state->active_crtcs |= 1 << i;
>>  		else
>>  			intel_state->active_crtcs &= ~(1 << i);
>>  
>> -		if (crtc_state->active != crtc->state->active)
>> +		if (old_crtc_state->active != new_crtc_state->active)
>>  			intel_state->active_pipe_changes |= drm_crtc_mask(crtc);
>>  	}
>>  
>> @@ -12427,7 +12427,7 @@ static int intel_atomic_check(struct drm_device *dev,
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *crtc_state;
>>  	int ret, i;
>>  	bool any_ms = false;
>>  
>> @@ -12435,12 +12435,12 @@ static int intel_atomic_check(struct drm_device *dev,
>>  	if (ret)
>>  		return 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) {
>>  		struct intel_crtc_state *pipe_config =
>>  			to_intel_crtc_state(crtc_state);
>>  
>>  		/* Catch I915_MODE_FLAG_INHERITED */
>> -		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
>> +		if (crtc_state->mode.private_flags != old_crtc_state->mode.private_flags)
>>  			crtc_state->mode_changed = true;
>>  
>>  		if (!needs_modeset(crtc_state))
>> @@ -12467,10 +12467,10 @@ static int intel_atomic_check(struct drm_device *dev,
>>  
>>  		if (i915.fastboot &&
>>  		    intel_pipe_config_compare(dev_priv,
>> -					to_intel_crtc_state(crtc->state),
>> +					to_intel_crtc_state(old_crtc_state),
>>  					pipe_config, true)) {
>>  			crtc_state->mode_changed = false;
>> -			to_intel_crtc_state(crtc_state)->update_pipe = true;
>> +			pipe_config->update_pipe = true;
>>  		}
>>  
>>  		if (needs_modeset(crtc_state))
>> @@ -12510,7 +12510,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>>  	struct drm_crtc *crtc;
>>  	int i, ret;
>>  
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>  		if (state->legacy_cursor_update)
>>  			continue;
>>  
>> @@ -12607,19 +12607,21 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>>  static void intel_update_crtc(struct drm_crtc *crtc,
>>  			      struct drm_atomic_state *state,
>>  			      struct drm_crtc_state *old_crtc_state,
>> +			      struct drm_crtc_state *new_crtc_state,
>>  			      unsigned int *crtc_vblank_mask)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->state);
>> -	bool modeset = needs_modeset(crtc->state);
>> +	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
>> +	bool modeset = needs_modeset(new_crtc_state);
>>  
>>  	if (modeset) {
>>  		update_scanline_offset(intel_crtc);
>>  		dev_priv->display.crtc_enable(pipe_config, state);
>>  	} else {
>> -		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
>> +		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>> +				       pipe_config);
>>  	}
>>  
>>  	if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
>> @@ -12638,15 +12640,15 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>>  			       unsigned int *crtc_vblank_mask)
>>  {
>>  	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_crtc_in_state(state, crtc, old_crtc_state, i) {
>> -		if (!crtc->state->active)
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>> +		if (!new_crtc_state->active)
>>  			continue;
>>  
>>  		intel_update_crtc(crtc, state, old_crtc_state,
>> -				  crtc_vblank_mask);
>> +				  new_crtc_state, crtc_vblank_mask);
>>  	}
>>  }
>>  
>> @@ -12657,7 +12659,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>  	struct drm_crtc *crtc;
>>  	struct intel_crtc *intel_crtc;
>> -	struct drm_crtc_state *old_crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>  	struct intel_crtc_state *cstate;
>>  	unsigned int updated = 0;
>>  	bool progress;
>> @@ -12666,9 +12668,9 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>>  
>>  	const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
>>  
>> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i)
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
>>  		/* ignore allocations for crtc's that have been turned off. */
>> -		if (crtc->state->active)
>> +		if (new_crtc_state->active)
>>  			entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>>  
>>  	/*
>> @@ -12680,7 +12682,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>>  	do {
>>  		progress = false;
>>  
>> -		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>>  			bool vbl_wait = false;
>>  			unsigned int cmask = drm_crtc_mask(crtc);
>>  
>> @@ -12705,12 +12707,12 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>>  			 */
>>  			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
>>  						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
>> -			    !crtc->state->active_changed &&
>> +			    !new_crtc_state->active_changed &&
>>  			    intel_state->wm_results.dirty_pipes != updated)
>>  				vbl_wait = true;
>>  
>>  			intel_update_crtc(crtc, state, old_crtc_state,
>> -					  crtc_vblank_mask);
>> +					  new_crtc_state, crtc_vblank_mask);
>>  
>>  			if (vbl_wait)
>>  				intel_wait_for_vblank(dev_priv, pipe);
>> @@ -12743,7 +12745,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	struct drm_device *dev = state->dev;
>>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>> -	struct drm_crtc_state *old_crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>  	struct drm_crtc *crtc;
>>  	struct intel_crtc_state *intel_cstate;
>>  	bool hw_check = intel_state->modeset;
>> @@ -12756,22 +12758,23 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	if (intel_state->modeset)
>>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>>  
>> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  
>> -		if (needs_modeset(crtc->state) ||
>> -		    to_intel_crtc_state(crtc->state)->update_pipe) {
>> +		if (needs_modeset(new_crtc_state) ||
>> +		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
>>  			hw_check = true;
>>  
>>  			put_domains[to_intel_crtc(crtc)->pipe] =
>>  				modeset_get_crtc_power_domains(crtc,
>> -					to_intel_crtc_state(crtc->state));
>> +					to_intel_crtc_state(new_crtc_state));
>>  		}
>>  
>> -		if (!needs_modeset(crtc->state))
>> +		if (!needs_modeset(new_crtc_state))
>>  			continue;
>>  
>> -		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
>> +		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
>> +				       to_intel_crtc_state(new_crtc_state));
>>  
>>  		if (old_crtc_state->active) {
>>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
>> @@ -12821,16 +12824,16 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	}
>>  
>>  	/* Complete the events for pipes that have now been disabled */
>> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> -		bool modeset = needs_modeset(crtc->state);
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> +		bool modeset = needs_modeset(new_crtc_state);
>>  
>>  		/* Complete events for now disable pipes here. */
>> -		if (modeset && !crtc->state->active && crtc->state->event) {
>> +		if (modeset && !new_crtc_state->active && new_crtc_state->event) {
>>  			spin_lock_irq(&dev->event_lock);
>> -			drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> +			drm_crtc_send_vblank_event(crtc, new_crtc_state->event);
>>  			spin_unlock_irq(&dev->event_lock);
>>  
>> -			crtc->state->event = NULL;
>> +			new_crtc_state->event = NULL;
>>  		}
>>  	}
>>  
>> @@ -12856,21 +12859,21 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	 *
>>  	 * TODO: Move this (and other cleanup) to an async worker eventually.
>>  	 */
>> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> -		intel_cstate = to_intel_crtc_state(crtc->state);
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> +		intel_cstate = to_intel_crtc_state(new_crtc_state);
>>  
>>  		if (dev_priv->display.optimize_watermarks)
>>  			dev_priv->display.optimize_watermarks(intel_state,
>>  							      intel_cstate);
>>  	}
>>  
>> -	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>>  		intel_post_plane_update(to_intel_crtc_state(old_crtc_state));
>>  
>>  		if (put_domains[i])
>>  			modeset_put_power_domains(dev_priv, put_domains[i]);
>>  
>> -		intel_modeset_verify_crtc(crtc, state, old_crtc_state, crtc->state);
>> +		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
>>  	}
>>  
>>  	if (intel_state->modeset && intel_can_enable_sagv(state))
>> @@ -12942,13 +12945,13 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,
>>  
>>  static void intel_atomic_track_fbs(struct drm_atomic_state *state)
>>  {
>> -	struct drm_plane_state *old_plane_state;
>> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>>  	struct drm_plane *plane;
>>  	int i;
>>  
>> -	for_each_plane_in_state(state, plane, old_plane_state, i)
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>>  		i915_gem_track_fb(intel_fb_obj(old_plane_state->fb),
>> -				  intel_fb_obj(plane->state->fb),
>> +				  intel_fb_obj(new_plane_state->fb),
>>  				  to_intel_plane(plane)->frontbuffer_bit);
>>  }
>>  
>> @@ -14890,7 +14893,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>>  	}
>>  
>>  	/* Write calculated watermark values back */
>> -	for_each_crtc_in_state(state, crtc, cstate, i) {
>> +	for_each_new_crtc_in_state(state, crtc, cstate, i) {
>>  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>>  
>>  		cs->wm.need_postvbl_update = true;
> Yeah, the new iterators are definitely much, much better. I tried to read
> it all carefully (really, intel_display.c should be split a lot), and
Agreed, too much code in 1 place.
> didn't find any real bugs. A bit more consistency with old/new, when we
> mix them, would be great though.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


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

Re: [PATCH 4/5] drm/i915: Use new atomic iterator macros in display code

[Daniel Vetter]

On Mon, Mar 13, 2017 at 12:08:19PM +0100, Maarten Lankhorst wrote:
> Op 13-03-17 om 11:15 schreef Daniel Vetter:
> > On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
> >> Add a big fat warning in __intel_display_resume that the old state is
> >> invalid, and use the correct state everywhere.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
> > This file is too big. Because it's one big mess this patch here is way too
> > big and one big mess :(
> >
> >>  1 file changed, 82 insertions(+), 79 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 5526a196e8a2..83f86cf44f66 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
> >>  	if (!state)
> >>  		return 0;
> >>  
> >> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >> +	/*
> >> +	 * We've duplicated the state, pointers to the old state are invalid.
> >> +	 *
> >> +	 * Don't attempt to use the old state until we commit the duplicated state.
> >> +	 */
> >> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > Just a drive-by comment, but why exactly do we need this hack? We read out
> > full hw state like on boot-up, assuming our commit machinery computes the
> > diff correctly it should noticed that we have to do a full commit here.
> >
> > Hacks like this imo show that we still have a fairly significant design
> > issue in our fastbook code ...
> We read out the full hw state, but we cannot trust the hw state to be correct. In general when we poke a property we set mode_changed,
> and the fastboot code may downgrade it to a pipe update if it's compatible enough. We only check on mode_changed because not all
> planes and connectors may otherwise be part of the atomic state.

drm_atomic.c doesn't set mode_changed anywhere, it's all derived state.
And we call the full atomic commit, which mean we do re-compute all the
derived stuff like mode_changed. I still don't get why we need this.

Iirc on the fastboot side we had some hacks to dirty plane states to force
this stuff, the same hw readout functions should do the trick here too.
-Daniel
-- 
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 4/5] drm/i915: Use new atomic iterator macros in display code

[Maarten Lankhorst]

Op 14-03-17 om 07:50 schreef Daniel Vetter:
> On Mon, Mar 13, 2017 at 12:08:19PM +0100, Maarten Lankhorst wrote:
>> Op 13-03-17 om 11:15 schreef Daniel Vetter:
>>> On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
>>>> Add a big fat warning in __intel_display_resume that the old state is
>>>> invalid, and use the correct state everywhere.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
>>> This file is too big. Because it's one big mess this patch here is way too
>>> big and one big mess :(
>>>
>>>>  1 file changed, 82 insertions(+), 79 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 5526a196e8a2..83f86cf44f66 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
>>>>  	if (!state)
>>>>  		return 0;
>>>>  
>>>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>> +	/*
>>>> +	 * We've duplicated the state, pointers to the old state are invalid.
>>>> +	 *
>>>> +	 * Don't attempt to use the old state until we commit the duplicated state.
>>>> +	 */
>>>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> Just a drive-by comment, but why exactly do we need this hack? We read out
>>> full hw state like on boot-up, assuming our commit machinery computes the
>>> diff correctly it should noticed that we have to do a full commit here.
>>>
>>> Hacks like this imo show that we still have a fairly significant design
>>> issue in our fastbook code ...
>> We read out the full hw state, but we cannot trust the hw state to be correct. In general when we poke a property we set mode_changed,
>> and the fastboot code may downgrade it to a pipe update if it's compatible enough. We only check on mode_changed because not all
>> planes and connectors may otherwise be part of the atomic state.
> drm_atomic.c doesn't set mode_changed anywhere, it's all derived state.
> And we call the full atomic commit, which mean we do re-compute all the
> derived stuff like mode_changed. I still don't get why we need this.
In the core resume, all crtc's are disabled on suspend, and are assumed to be off on resume.

But with i915 hw readout, the hw may also be resumed by bios and by chance the connectors from bios match up to the state we
want to set, the mode also being the same.

The only thing different could be (for example) crtc_state->has_audio or lane configuration, the bios might not set the same. We have to
force a modeset (which might be downgraded to a fastset) in order to notice that this has to be restored as well.
> Iirc on the fastboot side we had some hacks to dirty plane states to force
> this stuff, the same hw readout functions should do the trick here too.

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

Re: [PATCH 4/5] drm/i915: Use new atomic iterator macros in display code

[Daniel Vetter]

On Wed, Mar 15, 2017 at 09:23:30AM +0100, Maarten Lankhorst wrote:
> Op 14-03-17 om 07:50 schreef Daniel Vetter:
> > On Mon, Mar 13, 2017 at 12:08:19PM +0100, Maarten Lankhorst wrote:
> >> Op 13-03-17 om 11:15 schreef Daniel Vetter:
> >>> On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
> >>>> Add a big fat warning in __intel_display_resume that the old state is
> >>>> invalid, and use the correct state everywhere.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
> >>> This file is too big. Because it's one big mess this patch here is way too
> >>> big and one big mess :(
> >>>
> >>>>  1 file changed, 82 insertions(+), 79 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index 5526a196e8a2..83f86cf44f66 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
> >>>>  	if (!state)
> >>>>  		return 0;
> >>>>  
> >>>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >>>> +	/*
> >>>> +	 * We've duplicated the state, pointers to the old state are invalid.
> >>>> +	 *
> >>>> +	 * Don't attempt to use the old state until we commit the duplicated state.
> >>>> +	 */
> >>>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >>> Just a drive-by comment, but why exactly do we need this hack? We read out
> >>> full hw state like on boot-up, assuming our commit machinery computes the
> >>> diff correctly it should noticed that we have to do a full commit here.
> >>>
> >>> Hacks like this imo show that we still have a fairly significant design
> >>> issue in our fastbook code ...
> >> We read out the full hw state, but we cannot trust the hw state to be correct. In general when we poke a property we set mode_changed,
> >> and the fastboot code may downgrade it to a pipe update if it's compatible enough. We only check on mode_changed because not all
> >> planes and connectors may otherwise be part of the atomic state.
> > drm_atomic.c doesn't set mode_changed anywhere, it's all derived state.
> > And we call the full atomic commit, which mean we do re-compute all the
> > derived stuff like mode_changed. I still don't get why we need this.
> In the core resume, all crtc's are disabled on suspend, and are assumed to be off on resume.
> 
> But with i915 hw readout, the hw may also be resumed by bios and by chance the connectors from bios match up to the state we
> want to set, the mode also being the same.
> 
> The only thing different could be (for example) crtc_state->has_audio or lane configuration, the bios might not set the same. We have to
> force a modeset (which might be downgraded to a fastset) in order to notice that this has to be restored as well.

But we compare crtc_states and upgrade them to a modeset already when e.g.
has_audio changed. Or at least we should.

This smells like you're papering over a bug in our atomic_check code
still. The state compare stuff /should/ be able to figure this out. If
it's broken, then we might run into this in normal modesets too.
-Daniel
-- 
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 4/5] drm/i915: Use new atomic iterator macros in display code

[Maarten Lankhorst]

Op 16-03-17 om 09:47 schreef Daniel Vetter:
> On Wed, Mar 15, 2017 at 09:23:30AM +0100, Maarten Lankhorst wrote:
>> Op 14-03-17 om 07:50 schreef Daniel Vetter:
>>> On Mon, Mar 13, 2017 at 12:08:19PM +0100, Maarten Lankhorst wrote:
>>>> Op 13-03-17 om 11:15 schreef Daniel Vetter:
>>>>> On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
>>>>>> Add a big fat warning in __intel_display_resume that the old state is
>>>>>> invalid, and use the correct state everywhere.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
>>>>> This file is too big. Because it's one big mess this patch here is way too
>>>>> big and one big mess :(
>>>>>
>>>>>>  1 file changed, 82 insertions(+), 79 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index 5526a196e8a2..83f86cf44f66 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
>>>>>>  	if (!state)
>>>>>>  		return 0;
>>>>>>  
>>>>>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>>>> +	/*
>>>>>> +	 * We've duplicated the state, pointers to the old state are invalid.
>>>>>> +	 *
>>>>>> +	 * Don't attempt to use the old state until we commit the duplicated state.
>>>>>> +	 */
>>>>>> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>>>> Just a drive-by comment, but why exactly do we need this hack? We read out
>>>>> full hw state like on boot-up, assuming our commit machinery computes the
>>>>> diff correctly it should noticed that we have to do a full commit here.
>>>>>
>>>>> Hacks like this imo show that we still have a fairly significant design
>>>>> issue in our fastbook code ...
>>>> We read out the full hw state, but we cannot trust the hw state to be correct. In general when we poke a property we set mode_changed,
>>>> and the fastboot code may downgrade it to a pipe update if it's compatible enough. We only check on mode_changed because not all
>>>> planes and connectors may otherwise be part of the atomic state.
>>> drm_atomic.c doesn't set mode_changed anywhere, it's all derived state.
>>> And we call the full atomic commit, which mean we do re-compute all the
>>> derived stuff like mode_changed. I still don't get why we need this.
>> In the core resume, all crtc's are disabled on suspend, and are assumed to be off on resume.
>>
>> But with i915 hw readout, the hw may also be resumed by bios and by chance the connectors from bios match up to the state we
>> want to set, the mode also being the same.
>>
>> The only thing different could be (for example) crtc_state->has_audio or lane configuration, the bios might not set the same. We have to
>> force a modeset (which might be downgraded to a fastset) in order to notice that this has to be restored as well.
> But we compare crtc_states and upgrade them to a modeset already when e.g.
> has_audio changed. Or at least we should.
>
> This smells like you're papering over a bug in our atomic_check code
> still. The state compare stuff /should/ be able to figure this out. If
> it's broken, then we might run into this in normal modesets too.
We don't run the state comparison until
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Use new atomic iterator macros in cdclk

[Maarten Lankhorst]

Calculating the max pixel rate requires the new state, so use it there.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index de5ce6bfc7d7..c2cc33f3d888 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1470,7 +1470,7 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
 	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
 	       sizeof(intel_state->min_pixclk));
 
-	for_each_crtc_in_state(state, crtc, cstate, i) {
+	for_each_new_crtc_in_state(state, crtc, cstate, i) {
 		int pixel_rate;
 
 		crtc_state = to_intel_crtc_state(cstate);
-- 
2.7.4

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

Re: [PATCH 5/5] drm/i915: Use new atomic iterator macros in cdclk

[Daniel Vetter]

On Thu, Mar 09, 2017 at 03:52:05PM +0100, Maarten Lankhorst wrote:
> Calculating the max pixel rate requires the new state, so use it there.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index de5ce6bfc7d7..c2cc33f3d888 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> @@ -1470,7 +1470,7 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>  	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
>  	       sizeof(intel_state->min_pixclk));
>  
> -	for_each_crtc_in_state(state, crtc, cstate, i) {
> +	for_each_new_crtc_in_state(state, crtc, cstate, i) {
>  		int pixel_rate;
>  
>  		crtc_state = to_intel_crtc_state(cstate);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

✓ Fi.CI.BAT: success for drm/i915: Use new atomic iterator macros.

[Patchwork]

== Series Details ==

Series: drm/i915: Use new atomic iterator macros.
URL   : https://patchwork.freedesktop.org/series/20998/
State : success

== Summary ==

Series 20998v1 drm/i915: Use new atomic iterator macros.
https://patchwork.freedesktop.org/api/1.0/series/20998/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                incomplete -> PASS       (fi-skl-6700k) fdo#100130

fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 462s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 607s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 537s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 592s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 501s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 505s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 443s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 442s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 509s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 476s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 476s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 513s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 597s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 494s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 557s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 555s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 426s

510c200742ced5a91d07e48220b669a3c9b30c0c drm-tip: 2017y-03m-09d-15h-21m-14s UTC integration manifest
d341f75 drm/i915: Use new atomic iterator macros in cdclk
9a7ef81 drm/i915: Use new atomic iterator macros in display code
f1f97ac drm/i915: Use new atomic iterator macros in wm code
931fe38 drm/i915: Use new atomic iterator macros in fbc
2dafc3b drm/i915: Use new atomic iterator macros in ddi

== Logs ==

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

Re: [PATCH 0/5] drm/i915: Use new atomic iterator macros.

[Ville Syrjälä]

On Thu, Mar 09, 2017 at 03:52:00PM +0100, Maarten Lankhorst wrote:
> Some small patchset to deal with the atomic iterator changes.
> 
> Maarten Lankhorst (5):
>   drm/i915: Use new atomic iterator macros in ddi
>   drm/i915: Use new atomic iterator macros in fbc
>   drm/i915: Use new atomic iterator macros in wm code
>   drm/i915: Use new atomic iterator macros in display code
>   drm/i915: Use new atomic iterator macros in cdclk

Quite tedious stuff, but I managed to trawl through it. Didn't spot
any mistakes.

For the series
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
>  drivers/gpu/drm/i915/intel_cdclk.c   |   2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |   4 +-
>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>  drivers/gpu/drm/i915/intel_fbc.c     |   2 +-
>  drivers/gpu/drm/i915/intel_pm.c      |   8 +-
>  6 files changed, 90 insertions(+), 89 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] drm/i915: Use new atomic iterator macros.

[Maarten Lankhorst]

Op 09-03-17 om 19:35 schreef Ville Syrjälä:
> On Thu, Mar 09, 2017 at 03:52:00PM +0100, Maarten Lankhorst wrote:
>> Some small patchset to deal with the atomic iterator changes.
>>
>> Maarten Lankhorst (5):
>>   drm/i915: Use new atomic iterator macros in ddi
>>   drm/i915: Use new atomic iterator macros in fbc
>>   drm/i915: Use new atomic iterator macros in wm code
>>   drm/i915: Use new atomic iterator macros in display code
>>   drm/i915: Use new atomic iterator macros in cdclk
> Quite tedious stuff, but I managed to trawl through it. Didn't spot
> any mistakes.
>
> For the series
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
Series pushed, thanks for review. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx