Remove intel_crtc->atomic and fix BAT!

intel_crtc->atomic will be removed later on, move this member to intel_crtc_state.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
 3 files changed, 10 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index c4eadbc928b7..9f0638a37b6d 100644
@@ -95,6 +95,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
+	crtc_state->disable_cxsr = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +1 -2

index f8c332aee1e0..5ee64e67ad8a 100644
@@ -4749,8 +4749,7 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
-	if (atomic->disable_cxsr)
-		crtc->wm.cxsr_allowed = true;
+	crtc->wm.cxsr_allowed = true;
 
 	if (crtc->atomic.update_wm_post)

drivers/gpu/drm/i915/intel_display.c +2 -0

 		intel_update_watermarks(&crtc->base);

@@ -4769,6 +4768,8 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 
 	if (atomic->disable_fbc)

drivers/gpu/drm/i915/intel_display.c +1 -1

 		intel_fbc_disable_crtc(crtc);

@@ -4779,7 +4780,7 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
 
-	if (atomic->disable_cxsr) {
+	if (pipe_config->disable_cxsr) {
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);

drivers/gpu/drm/i915/intel_display.c +1 -0

 	}

@@ -11658,6 +11659,7 @@ static bool needs_scaling(struct intel_plane_state
 *state)
 int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 				    struct drm_plane_state *plane_state)
 {
+	struct intel_crtc_state *pipe_config =
 to_intel_crtc_state(crtc_state);
 	struct drm_crtc *crtc = crtc_state->crtc;

drivers/gpu/drm/i915/intel_display.c +1 -1

 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane = plane_state->plane;

@@ -11708,7 +11710,7 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		intel_crtc->atomic.update_wm_pre = true;
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			intel_crtc->atomic.disable_cxsr = true;
+			pipe_config->disable_cxsr = true;
 			/* to potentially re-enable cxsr */
 			intel_crtc->atomic.wait_vblank = true;

drivers/gpu/drm/i915/intel_display.c +1 -1

 			intel_crtc->atomic.update_wm_post = true;

@@ -11719,7 +11721,7 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			if (is_crtc_enabled)
 				intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.disable_cxsr = true;
+			pipe_config->disable_cxsr = true;
 		}
 	} else if (intel_wm_need_update(plane, plane_state)) {

drivers/gpu/drm/i915/intel_display.c +0 -0

 		intel_crtc->atomic.update_wm_pre = true;

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 725973ebf49f..dd89342832e2 100644
@@ -370,7 +370,8 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync

mode.flags */

drivers/gpu/drm/i915/intel_drv.h +2 -1

 	unsigned long quirks;
 
-	bool update_pipe;
+	bool update_pipe; /* can a fast modeset be performed? */
+	bool disable_cxsr;
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,

This removes pre/post_wm_update from intel_crtc->atomic, and creates atomic state for it in intel_crtc.

Changes since v1:
- Rebase on top of wm changes.
Changes since v2:
- Split disable_cxsr into a separate patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 15 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 9f0638a37b6d..4625f8a9ba12 100644
@@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
+	crtc_state->wm_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +2 -0

index 5ee64e67ad8a..db4995406277 100644
@@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

drivers/gpu/drm/i915/intel_display.c +3 -0

 	if (atomic->update_fbc)

@@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
+
+	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+		intel_update_watermarks(&crtc->base);
 }
 

static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
plane_mask)

drivers/gpu/drm/i915/intel_display.c +2 -11

@@ -11706,25 +11711,18 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on) {
-		intel_crtc->atomic.update_wm_pre = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			pipe_config->disable_cxsr = true;
-			/* to potentially re-enable cxsr */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
+	if (turn_on || turn_off) {
+		pipe_config->wm_changed = true;

drivers/gpu/drm/i915/intel_display.c +2 -0

+		   intel_wm_need_update(plane, plane_state)) {
+		pipe_config->wm_changed = true;
 	}
 

drivers/gpu/drm/i915/intel_display.c +1 -1

 	if (visible || was_visible)

@@ -11869,7 +11867,7 @@ static int intel_crtc_atomic_check(struct drm_crtc
 *crtc,
 	}
 
 	if (mode_changed && !crtc_state->active)
-		intel_crtc->atomic.update_wm_post = true;
+		pipe_config->wm_changed = true;
 
 	if (mode_changed && crtc_state->enable &&

drivers/gpu/drm/i915/intel_display.c +0 -3

 	    dev_priv->display.crtc_compute_clock &&

@@ -13762,9 +13760,6 @@ static void intel_begin_crtc_commit(struct drm_crtc
 *crtc,
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
 
-	if (intel_crtc->atomic.update_wm_pre)
-		intel_update_watermarks(crtc);
-
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);

drivers/gpu/drm/i915/intel_drv.h +1 -0

index dd89342832e2..db61c37dbf09 100644
@@ -372,6 +372,7 @@ struct intel_crtc_state {
 
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
+	bool wm_changed; /* watermarks are updated */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,

Quoted 47 lines

This removes pre/post_wm_update from intel_crtc->atomic, and creates atomic state for it in intel_crtc.

Changes since v1:
- Rebase on top of wm changes.
Changes since v2:
- Split disable_cxsr into a separate patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 15 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 9f0638a37b6d..4625f8a9ba12 100644
@@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
+	crtc_state->wm_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +2 -0

index 5ee64e67ad8a..db4995406277 100644
@@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

@@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 
 	crtc->wm.cxsr_allowed = true;
 

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc *crtc) · line 4757

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.c +1 -1

-	if (crtc->atomic.update_wm_post)
+	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);

This adds an extra call to intel_update_watermarks() for the case where previously update_wm_pre would be set. This won't cause extra register writes because of the dirty check, but I think it deserves a note in the commit message.

Quoted 37 lines

drivers/gpu/drm/i915/intel_display.c +3 -0

 	if (atomic->update_fbc)

@@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
+
+	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+		intel_update_watermarks(&crtc->base);
 }
 

static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
plane_mask)

drivers/gpu/drm/i915/intel_display.c +3 -11

@@ -11706,25 +11711,18 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on) {
-		intel_crtc->atomic.update_wm_pre = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			pipe_config->disable_cxsr = true;
-			/* to potentially re-enable cxsr */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
+	if (turn_on || turn_off) {
+		pipe_config->wm_changed = true;
+
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			if (is_crtc_enabled)
 				intel_crtc->atomic.wait_vblank = true;

drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11723

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +1 -2

 			pipe_config->disable_cxsr = true;
 		}
-	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
+	} else if ((was_visible || visible) &&

So this avoids watermark changes when the plane is not visible before or after the update. Wouldn't it be better to fix intel_wm_need_update() instead if returns true in that case?

Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira:

Quoted 49 lines · Maarten Lankhorst

Quoted 48 lines

This removes pre/post_wm_update from intel_crtc->atomic, and creates atomic state for it in intel_crtc.

Changes since v1:
- Rebase on top of wm changes.
Changes since v2:
- Split disable_cxsr into a separate patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 15 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 9f0638a37b6d..4625f8a9ba12 100644
@@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
+	crtc_state->wm_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +2 -0

index 5ee64e67ad8a..db4995406277 100644
@@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

@@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct

intel_crtc
*crtc)

crtc->wm.cxsr_allowed = true;

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc *crtc) · line 4757

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.c +1 -1

-	if (crtc->atomic.update_wm_post)
+	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);

This adds an extra call to intel_update_watermarks() for the case where previously update_wm_pre would be set. This won't cause extra register writes
because of the dirty check, but I think it deserves a note in the commit message.

I think intel_update_watermarks needs to be fixed to take a crtc_state and whether pre/post commit is used.

It looks to me like doing anything else could bug if watermarks are changed with 1 plane becoming visible and the other invisible..

Quoted 39 lines

Quoted 39 lines

drivers/gpu/drm/i915/intel_display.c +3 -1

 	if (atomic->update_fbc)

@@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
+
+	if (!needs_modeset(&pipe_config->base) && pipe_config
->wm_changed)
+		intel_update_watermarks(&crtc->base);
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned

plane_mask)

drivers/gpu/drm/i915/intel_display.c +3 -11

@@ -11706,25 +11711,18 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on) {
-		intel_crtc->atomic.update_wm_pre = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			pipe_config->disable_cxsr = true;
-			/* to potentially re-enable cxsr */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
+	if (turn_on || turn_off) {
+		pipe_config->wm_changed = true;
+
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			if (is_crtc_enabled)
 				intel_crtc->atomic.wait_vblank = true;
 			pipe_config->disable_cxsr = true;

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11723

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +1 -2

 		}
-	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
+	} else if ((was_visible || visible) &&

So this avoids watermark changes when the plane is not visible before or after
the update. Wouldn't it be better to fix intel_wm_need_update() instead if returns true in that case?

If visible is changed wm_changed = true is always set. It would go through the (turn_on || turn_off) case.

I guess checking was_visible || visible is overkill, and could be changed to just visible && intel_wm_need_update(plane, plane_state).

Quoted 113 lines

Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira:

Quoted 49 lines · Maarten Lankhorst

Quoted 48 lines

This removes pre/post_wm_update from intel_crtc->atomic, and creates atomic state for it in intel_crtc.

Changes since v1:
- Rebase on top of wm changes.
Changes since v2:
- Split disable_cxsr into a separate patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 15 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 9f0638a37b6d..4625f8a9ba12 100644
@@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
+	crtc_state->wm_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +2 -0

index 5ee64e67ad8a..db4995406277 100644
@@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

@@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct

intel_crtc
*crtc)

crtc->wm.cxsr_allowed = true;

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc *crtc) · line 4757

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.c +1 -1

-	if (crtc->atomic.update_wm_post)
+	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);

This adds an extra call to intel_update_watermarks() for the case where previously update_wm_pre would be set. This won't cause extra register writes
because of the dirty check, but I think it deserves a note in the commit message.

I think intel_update_watermarks needs to be fixed to take a crtc_state and whether pre/post commit is used.

It looks to me like doing anything else could bug if watermarks are changed with 1 plane becoming visible and the other invisible..

Quoted 39 lines

Quoted 39 lines

drivers/gpu/drm/i915/intel_display.c +3 -1

 	if (atomic->update_fbc)

@@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
+
+	if (!needs_modeset(&pipe_config->base) && pipe_config
->wm_changed)
+		intel_update_watermarks(&crtc->base);
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned

plane_mask)

drivers/gpu/drm/i915/intel_display.c +3 -11

@@ -11706,25 +11711,18 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on) {
-		intel_crtc->atomic.update_wm_pre = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			pipe_config->disable_cxsr = true;
-			/* to potentially re-enable cxsr */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
+	if (turn_on || turn_off) {
+		pipe_config->wm_changed = true;
+
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			if (is_crtc_enabled)
 				intel_crtc->atomic.wait_vblank = true;
 			pipe_config->disable_cxsr = true;

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11723

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +1 -2

 		}
-	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
+	} else if ((was_visible || visible) &&

So this avoids watermark changes when the plane is not visible before or after
the update. Wouldn't it be better to fix intel_wm_need_update() instead if returns true in that case?

If visible is changed wm_changed = true is always set. It would go through the (turn_on || turn_off) case.

Quoted 2 lines

I guess checking was_visible || visible is overkill, and could be changed to just visible && intel_wm_need_update(plane, plane_state).

What I meant is that I find odd that intel_wm_need_update() returns true when both was_visible and visible is false. Since that function is supposed to compare two plane states and tell us if a wm update is needed, I'd rather add the visible check there.

This removes pre/post_wm_update from intel_crtc->atomic, and creates atomic state for it in intel_crtc.

Changes since v1:
- Rebase on top of wm changes.
Changes since v2:
- Split disable_cxsr into a separate patch. Changes since v3:
- Move some of the changes to intel_wm_need_update.

Signed-off-by Maarten Lankhorst <[email protected]>

---

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 9f0638a37b6d..4625f8a9ba12 100644
@@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
+	crtc_state->wm_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +19 -20

index 029777f75de8..b96419fd4c9e 100644
@@ -4767,6 +4767,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

@@ -4777,7 +4779,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 
 	crtc->wm.cxsr_allowed = true;
 
-	if (crtc->atomic.update_wm_post)
+	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 
 	if (atomic->update_fbc)

@@ -4810,6 +4812,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
+
+	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+		intel_update_watermarks(&crtc->base);
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)

@@ -11674,9 +11679,14 @@ static bool intel_wm_need_update(struct drm_plane *plane,
 	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
 
 	/* Update watermarks on tiling or size changes. */
-	if (!plane->state->fb || !state->fb ||
-	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
-	    plane->state->rotation != state->rotation ||
+	if (new->visible != cur->visible)
+		return true;
+
+	if (!cur->base.fb || !new->base.fb)
+		return false;
+
+	if (cur->base.fb->modifier[0] != new->base.fb->modifier[0] ||
+	    cur->base.rotation != new->base.rotation ||
 	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
 	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
 	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||

@@ -11746,17 +11756,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on) {
-		intel_crtc->atomic.update_wm_pre = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			pipe_config->disable_cxsr = true;
-			/* to potentially re-enable cxsr */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
+	if (turn_on || turn_off) {
+		pipe_config->wm_changed = true;
+
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			if (is_crtc_enabled)

@@ -11764,7 +11766,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			pipe_config->disable_cxsr = true;
 		}
 	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
+		pipe_config->wm_changed = true;
 	}
 
 	if (visible || was_visible)

@@ -11909,7 +11911,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	if (mode_changed && !crtc_state->active)
-		intel_crtc->atomic.update_wm_post = true;
+		pipe_config->wm_changed = true;
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&

@@ -13809,9 +13811,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
 
-	if (intel_crtc->atomic.update_wm_pre)
-		intel_update_watermarks(crtc);
-
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
 

drivers/gpu/drm/i915/intel_drv.h +1 -1

index 53405cdc01f4..4f5d0949137e 100644
@@ -372,6 +372,7 @@ struct intel_crtc_state {
 
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
+	bool wm_changed; /* watermarks are updated */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,

@@ -535,7 +536,6 @@ struct intel_crtc_atomic_commit {
 	bool disable_fbc;
 	bool disable_ips;
 	bool pre_disable_primary;
-	bool update_wm_pre, update_wm_post;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On skylake some of the registers are only writable when the correct power wells are enabled. Because of this watermarks have to be updated before the crtc turns off, or you get unclaimed register read and write warnings.

This patch needs to be modified slightly to apply to -fixes.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181

Signed-off-by Maarten Lankhorst <[email protected]>

Cc [email protected]

Cc Matt Roper <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_display.c +1 -1

index db4995406277..5345ffcce51e 100644
@@ -4753,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 
 	crtc->wm.cxsr_allowed = true;
 
-	if (pipe_config->wm_changed)
+	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 	if (atomic->update_fbc)

wait_vblank is already set in intel_plane_atomic_calc_changes for broadwell, waiting for a double vblank is overkill.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 1 file changed, 8 deletions(-)

drivers/gpu/drm/i915/intel_display.c +0 -1

index 5345ffcce51e..60f17bc5f0ce 100644
@@ -4657,14 +4657,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 
 	/*
-	 * BDW signals flip done immediately if the plane

Quoted 4 lines · Maarten Lankhorst

wait_vblank is already set in intel_plane_atomic_calc_changes for broadwell, waiting for a double vblank is overkill.

Signed-off-by Maarten Lankhorst <[email protected]>

Reviewed-by Ander Conselvan de Oliveira <[email protected]>

Quoted 15 lines

---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 1 file changed, 8 deletions(-)

drivers/gpu/drm/i915/intel_display.c +0 -1

index 5345ffcce51e..60f17bc5f0ce 100644
@@ -4657,14 +4657,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 
 	/*
-	 * BDW signals flip done immediately if the plane

drivers/gpu/drm/i915/intel_display.c intel_post_enable_primary(struct drm_crtc *crtc) · line 4662

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +0 -7

-	 * is disabled, even if the plane enable is already
-	 * armed to occur at the next vblank :(
-	 */
-	if (IS_BROADWELL(dev))
-		intel_wait_for_vblank(dev, pipe);
-
-	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
 	 * when going from primary only to sprite only and vice

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 6 lines · Maarten Lankhorst

Quoted 4 lines

wait_vblank is already set in intel_plane_atomic_calc_changes for broadwell, waiting for a double vblank is overkill.

Signed-off-by Maarten Lankhorst <[email protected]>

Reviewed-by Ander Conselvan de Oliveira <[email protected]>

I see this one got merged without the r-b tag in the commit message.

Quoted 7 lines · Ander Conselvan De Oliveira

Quoted 4 lines · Maarten Lankhorst

wait_vblank is already set in intel_plane_atomic_calc_changes for broadwell, waiting for a double vblank is overkill.

Signed-off-by Maarten Lankhorst <[email protected]>

Reviewed-by Ander Conselvan de Oliveira <[email protected]>

I see this one got merged without the r-b tag in the commit message.

Quoted 30 lines

Quoted 16 lines

---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 1 file changed, 8 deletions(-)

drivers/gpu/drm/i915/intel_display.c +0 -1

index 5345ffcce51e..60f17bc5f0ce 100644
@@ -4657,14 +4657,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 
 	/*
-	 * BDW signals flip done immediately if the plane

drivers/gpu/drm/i915/intel_display.c intel_post_enable_primary(struct drm_crtc *crtc) · line 4662

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +0 -7

-	 * is disabled, even if the plane enable is already
-	 * armed to occur at the next vblank :(
-	 */
-	if (IS_BROADWELL(dev))
-		intel_wait_for_vblank(dev, pipe);
-
-	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
 	 * when going from primary only to sprite only and vice

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Currently we perform our own wait in post_plane_update, but the atomic core performs another one in wait_for_vblanks. This means that 2 vblanks are done when a fb is changed, which is a bit overkill.

Merge them by creating a helper function that takes a crtc mask for the planes to wait on.

The broadwell vblank workaround may look gone entirely but this is not the case. pipe_config->wm_changed is set to true when any plane is turned on, which forces a vblank wait.

Changes since v1:
- Removing the double vblank wait on broadwell moved to its own commit.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++---------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 65 insertions(+), 26 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 4625f8a9ba12..8e579a8505ac 100644
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
+	crtc_state->fb_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +0 -3

index 60f17bc5f0ce..299edbf6f99e 100644
@@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 

drivers/gpu/drm/i915/intel_display.c +6 -2

 	crtc->wm.cxsr_allowed = true;

@@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
 
+	if (needs_modeset(&pipe_config->base))
+		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
+
 	memset(atomic, 0, sizeof(*atomic));
 }

@@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 	if (!was_visible && !visible)
 		return 0;
 
+	if (fb != old_plane_state->base.fb)
+		pipe_config->fb_changed = true;
+
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);

@@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;
 			pipe_config->disable_cxsr = true;
 		}

drivers/gpu/drm/i915/intel_display.c +0 -8

 	} else if ((was_visible || visible) &&

@@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		    plane_state->rotation != BIT(DRM_ROTATE_0))
 			intel_crtc->atomic.disable_fbc = true;
 
-		/*
-		 * BDW signals flip done immediately if the plane
-		 * is disabled, even if the plane enable is already
-		 * armed to occur at the next vblank :(
-		 */
-		if (turn_on && IS_BROADWELL(dev))
-			intel_crtc->atomic.wait_vblank = true;
-
 		intel_crtc->atomic.update_fbc |= visible || mode_changed;
 		break;

drivers/gpu/drm/i915/intel_display.c +2 -4

 	case DRM_PLANE_TYPE_CURSOR:

@@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&
 		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)->disable_lp_wm =
 true;
-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
+			pipe_config->disable_lp_wm = true;
+		} else if (turn_off && !mode_changed)
 			intel_crtc->atomic.update_sprite_watermarks |=
 				1 << i;
-		}
 

drivers/gpu/drm/i915/intel_display.c +42 -0

 		break;
 	}

@@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct
 drm_device *dev,
 	return ret;
 }
 
+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private *dev_priv,
+					  unsigned crtc_mask)
+{
+	unsigned last_vblank_count[I915_MAX_PIPES];
+	enum pipe pipe;
+	int ret;
+
+	if (!crtc_mask)
+		return;
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (ret != 0) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
 /**
  * intel_atomic_commit - commit validated state object

* @dev: DRM device

drivers/gpu/drm/i915/intel_display.c +6 -2

@@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
+	unsigned crtc_vblank_mask = 0;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {

@@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)->update_pipe;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config->update_pipe;
 		unsigned long put_domains = 0;
 
 		if (modeset)

@@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device
 *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||

drivers/gpu/drm/i915/intel_display.c +4 -1

 	}
 
 	/* FIXME: add subpixel order */
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		intel_post_plane_update(to_intel_crtc(crtc));
 
 	mutex_lock(&dev->struct_mutex);

drivers/gpu/drm/i915/intel_display.c +0 -0

 	drm_atomic_helper_cleanup_planes(dev, state);

drivers/gpu/drm/i915/intel_drv.h +1 -0

index db61c37dbf09..66f66740ccaa 100644
@@ -373,6 +373,7 @@ struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */
+	bool fb_changed; /* fb on any of the planes is changed */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,

Quoted 202 lines · Maarten Lankhorst

Currently we perform our own wait in post_plane_update, but the atomic core performs another one in wait_for_vblanks. This means that 2 vblanks are done when a fb is changed, which is a bit overkill.

Merge them by creating a helper function that takes a crtc mask for the planes to wait on.

The broadwell vblank workaround may look gone entirely but this is not the case. pipe_config->wm_changed is set to true when any plane is turned on, which forces a vblank wait.

Changes since v1:
- Removing the double vblank wait on broadwell moved to its own commit.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 + drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++--------- -
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-

 3 files changed, 65 insertions(+), 26 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 4625f8a9ba12..8e579a8505ac 100644
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
  	crtc_state->disable_lp_wm = false;
  	crtc_state->disable_cxsr = false;
  	crtc_state->wm_changed = false;
+	crtc_state->fb_changed = false;

  return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +0 -0

index 60f17bc5f0ce..299edbf6f99e 100644
@@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
  	struct drm_device *dev = crtc->base.dev;
  	struct drm_i915_private *dev_priv = dev->dev_private;

drivers/gpu/drm/i915/intel_display.c +0 -3

-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-

  intel_frontbuffer_flip(dev, atomic->fb_bits);
  crtc->wm.cxsr_allowed = true;

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
  	if (atomic->post_enable_primary)
  		intel_post_enable_primary(&crtc->base);

drivers/gpu/drm/i915/intel_display.c +3 -0

+	if (needs_modeset(&pipe_config->base))
+		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
+

  memset(atomic, 0, sizeof(*atomic));
 }

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  	if (!was_visible && !visible)
  		return 0;

drivers/gpu/drm/i915/intel_display.c +3 -0

+	if (fb != old_plane_state->base.fb)
+		pipe_config->fb_changed = true;
+

  turn_off = was_visible && (!visible || mode_changed); turn_on = visible && (!was_visible || mode_changed);

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,

  /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) {

drivers/gpu/drm/i915/intel_display.c +0 -2

-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;

  pipe_config->disable_cxsr = true; }
  } else if ((was_visible || visible) &&

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  		    plane_state->rotation != BIT(DRM_ROTATE_0))
  			intel_crtc->atomic.disable_fbc = true;

drivers/gpu/drm/i915/intel_display.c +0 -8

-		/*
-		 * BDW signals flip done immediately if the plane
-		 * is disabled, even if the plane enable is already
-		 * armed to occur at the next vblank :(
-		 */
-		if (turn_on && IS_BROADWELL(dev))
-			intel_crtc->atomic.wait_vblank = true;
-

  intel_crtc->atomic.update_fbc |= visible || mode_changed; break;
  case DRM_PLANE_TYPE_CURSOR:

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,

  if (IS_IVYBRIDGE(dev) && needs_scaling(to_intel_plane_state(plane_state)) && !needs_scaling(old_plane_state)) {

drivers/gpu/drm/i915/intel_display.c +0 -1

-			to_intel_crtc_state(crtc_state)->disable_lp_wm =

true;

drivers/gpu/drm/i915/intel_display.c +2 -2

-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
+			pipe_config->disable_lp_wm = true;
+		} else if (turn_off && !mode_changed)

  intel_crtc->atomic.update_sprite_watermarks |= 1 << i;

drivers/gpu/drm/i915/intel_display.c +0 -1

-		}

  break;
  }

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct
 drm_device *dev,
  	return ret;
  }

drivers/gpu/drm/i915/intel_display.c +42 -0

+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private *dev_priv,
+					  unsigned crtc_mask)
+{
+	unsigned last_vblank_count[I915_MAX_PIPES];
+	enum pipe pipe;
+	int ret;
+
+	if (!crtc_mask)
+		return;
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (ret != 0) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+

 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev,
  	struct drm_crtc *crtc;
  	int ret = 0, i;
  	bool hw_check = intel_state->modeset;
+	unsigned crtc_vblank_mask = 0;

  ret = intel_atomic_prepare_commit(dev, state, async); if (ret) {

drivers/gpu/drm/i915/intel_display.c +3 -2

@@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev,
  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
  		bool modeset = needs_modeset(crtc->state);
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)->update_pipe;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config->update_pipe;
  		unsigned long put_domains = 0;

  if (modeset)

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device
 *dev,
  		    (crtc->state->planes_changed || update_pipe))
  			drm_atomic_helper_commit_planes_on_crtc(crtc_state);

drivers/gpu/drm/i915/intel_display.c +5 -0

+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||
+		     pipe_config->fb_changed))
+			crtc_vblank_mask |= 1 << i;
+

  if (put_domains) modeset_put_power_domains(dev_priv, put_domains);

drivers/gpu/drm/i915/intel_display.c +0 -3

-
-		intel_post_plane_update(intel_crtc);
-

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13410

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +0 -2

-		if (modeset)
-			intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);

Would it be too evil to move the power domain get/put out of the loop? If I understand correctly, intel_state->modeset already tells us if there is any crtc that needs a modeset, so we could just protect the calls with that. I'm not sure what is the impact, but at least we could avoid having the get in intel_atomic_commit() and the put in intel_post_plane_enable().

Ander

Quoted 25 lines

  }

  /* FIXME: add subpixel order */

drivers/gpu/drm/i915/intel_display.c +4 -1

-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		intel_post_plane_update(to_intel_crtc(crtc));

  mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state);

drivers/gpu/drm/i915/intel_drv.h +0 -0

index db61c37dbf09..66f66740ccaa 100644
@@ -373,6 +373,7 @@ struct intel_crtc_state {

  bool update_pipe; /* can a fast modeset be performed? */ bool disable_cxsr;
  bool wm_changed; /* watermarks are updated */

drivers/gpu/drm/i915/intel_drv.h +1 -0

+	bool fb_changed; /* fb on any of the planes is changed */

  /* Pipe source size (ie. panel fitter input size)
   * All planes will be positioned inside this space,

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +0 -0

@@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit {

  /* Sleepable operations to perform after commit */ unsigned fb_bits;

drivers/gpu/drm/i915/intel_drv.h +0 -1

-	bool wait_vblank;

  bool update_fbc;
  bool post_enable_primary;
  unsigned update_sprite_watermarks;

Quoted 38 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_display.c +11 -5

@@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
      if (atomic->post_enable_primary)
              intel_post_enable_primary(&crtc->base);
 
+     if (needs_modeset(&pipe_config->base))
+             intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
+
      memset(atomic, 0, sizeof(*atomic));
 }

@@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev,
      for_each_crtc_in_state(state, crtc, crtc_state, i) {
              struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
              bool modeset = needs_modeset(crtc->state);
-             bool update_pipe = !modeset &&
-                     to_intel_crtc_state(crtc->state)->update_pipe;
+             struct intel_crtc_state *pipe_config =
+                     to_intel_crtc_state(crtc->state);
+             bool update_pipe = !modeset && pipe_config->update_pipe;
              unsigned long put_domains = 0;
 
              if (modeset)

@@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device
 *dev,
                  (crtc->state->planes_changed || update_pipe))
                      drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
+             if (pipe_config->base.active &&
+                 (pipe_config->wm_changed || pipe_config->disable_cxsr ||
+                  pipe_config->fb_changed))
+                     crtc_vblank_mask |= 1 << i;
+
              if (put_domains)
                      modeset_put_power_domains(dev_priv, put_domains);
-
-             intel_post_plane_update(intel_crtc);
-

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13410

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +0 -2

-             if (modeset)
-                     intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);

Would it be too evil to move the power domain get/put out of the loop? If I understand correctly, intel_state->modeset already tells us if there is any crtc that needs a modeset, so we could just protect the calls with that. I'm not sure what is the impact, but at least we could avoid having the get in intel_atomic_commit() and the put in intel_post_plane_enable().

Currently we perform our own wait in post_plane_update, but the atomic core performs another one in wait_for_vblanks. This means that 2 vblanks are done when a fb is changed, which is a bit overkill.

Merge them by creating a helper function that takes a crtc mask for the planes to wait on.

The broadwell vblank workaround may look gone entirely but this is not the case. pipe_config->wm_changed is set to true when any plane is turned on, which forces a vblank wait.

Changes since v1:
- Removing the double vblank wait on broadwell moved to its own commit.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++---------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 65 insertions(+), 26 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 4625f8a9ba12..8e579a8505ac 100644
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
+	crtc_state->fb_changed = false;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +0 -3

index 60f17bc5f0ce..299edbf6f99e 100644
@@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 

drivers/gpu/drm/i915/intel_display.c +6 -2

 	crtc->wm.cxsr_allowed = true;

@@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
 
+	if (needs_modeset(&pipe_config->base))
+		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
+
 	memset(atomic, 0, sizeof(*atomic));
 }

@@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 	if (!was_visible && !visible)
 		return 0;
 
+	if (fb != old_plane_state->base.fb)
+		pipe_config->fb_changed = true;
+
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);

@@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;
 			pipe_config->disable_cxsr = true;
 		}

drivers/gpu/drm/i915/intel_display.c +0 -8

 	} else if ((was_visible || visible) &&

@@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		    plane_state->rotation != BIT(DRM_ROTATE_0))
 			intel_crtc->atomic.disable_fbc = true;
 
-		/*
-		 * BDW signals flip done immediately if the plane
-		 * is disabled, even if the plane enable is already
-		 * armed to occur at the next vblank :(
-		 */
-		if (turn_on && IS_BROADWELL(dev))
-			intel_crtc->atomic.wait_vblank = true;
-
 		intel_crtc->atomic.update_fbc |= visible || mode_changed;
 		break;

drivers/gpu/drm/i915/intel_display.c +0 -0

 	case DRM_PLANE_TYPE_CURSOR:

@@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&

drivers/gpu/drm/i915/intel_display.c +0 -0

 		break;

drivers/gpu/drm/i915/intel_display.c +42 -0

 	}

@@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct
 drm_device *dev,
 	return ret;
 }
 
+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private *dev_priv,
+					  unsigned crtc_mask)
+{
+	unsigned last_vblank_count[I915_MAX_PIPES];
+	enum pipe pipe;
+	int ret;
+
+	if (!crtc_mask)
+		return;
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (ret != 0) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
 /**
  * intel_atomic_commit - commit validated state object

* @dev: DRM device

drivers/gpu/drm/i915/intel_display.c +13 -8

@@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
+	unsigned crtc_vblank_mask = 0;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {

@@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)->update_pipe;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config->update_pipe;
 		unsigned long put_domains = 0;
 
 		if (modeset)

@@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device
 *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||
+		     pipe_config->fb_changed))
+			crtc_vblank_mask |= 1 << i;
+
 		if (put_domains)
 			modeset_put_power_domains(dev_priv, put_domains);
-
-		intel_post_plane_update(intel_crtc);
-
-		if (modeset)
-			intel_display_power_put(dev_priv,
 POWER_DOMAIN_MODESET);
 	}
 
 	/* FIXME: add subpixel order */
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		intel_post_plane_update(to_intel_crtc(crtc));
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);

drivers/gpu/drm/i915/intel_drv.h +1 -0

index db61c37dbf09..66f66740ccaa 100644
@@ -373,6 +373,7 @@ struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */
+	bool fb_changed; /* fb on any of the planes is changed */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,

This is a revert of commit 066cf55b9ce3 "drm/i915: Fix IPS related flicker". intel_pre_disable_primary already handles this, and now everything goes through the atomic path there's no need to try to disable ips twice.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 16 +---------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 1 insertion(+), 16 deletions(-)

drivers/gpu/drm/i915/intel_display.c +1 -15

index 299edbf6f99e..1fec49c4490e 100644
@@ -4768,9 +4768,6 @@ static void intel_pre_plane_update(struct intel_crtc
 *crtc)
 	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
 
-	if (crtc->atomic.disable_ips)
-		hsw_disable_ips(crtc);
-
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);

@@ -11727,19 +11724,8 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
 
-		if (turn_off) {
-			/*
-			 * FIXME: Actually if we will still have any other
-			 * plane enabled on the pipe we could let IPS enabled
-			 * still, but for now lets consider that when we make
-			 * primary invisible by setting DSPCNTR to 0 on
-			 * update_primary_plane function IPS needs to be
-			 * disable.
-			 */
-			intel_crtc->atomic.disable_ips = true;
-
+		if (turn_off)
 			intel_crtc->atomic.disable_fbc = true;
-		}
 
 		/*

drivers/gpu/drm/i915/intel_display.c +0 -0

 		 * FBC does not work on some platforms for rotated

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 66f66740ccaa..13ffefa3a6c1 100644

fb_bits is useful to have in the crtc_state for cs flips when the code is updated to use intel_frontbuffer_flip_prepare/complete. So calculate it in advance and move it to crtc_state. The other stuff can be calculated in post_plane_update, and aren't useful elsewhere.

Changes since v1:
- Changing wording, remove comment about loop.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 3 files changed, 22 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 8e579a8505ac..9a45cff26767 100644
@@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
 	crtc_state->fb_changed = false;
+	crtc_state->fb_bits = 0;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +7 -2

index d539703de367..95501aba7d23 100644
@@ -4730,15 +4730,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_post_plane_update(struct intel_crtc *crtc)
+static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+	struct drm_atomic_state *old_state = old_crtc_state->base.state;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *old_pri_state =
+		drm_atomic_get_existing_plane_state(old_state, primary);
 
-	intel_frontbuffer_flip(dev, atomic->fb_bits);
+	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
 

@@ -4748,8 +4753,17 @@ static void intel_post_plane_update(struct intel_crtc
 *crtc)

drivers/gpu/drm/i915/intel_display.c +6 -0

+
+		if (primary_state->visible &&
+		    (needs_modeset(&pipe_config->base) ||
+		     !old_primary_state->visible))
+			intel_post_enable_primary(&crtc->base);
+	}
 
 	if (needs_modeset(&pipe_config->base))

drivers/gpu/drm/i915/intel_display.c +2 -6

 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);

@@ -11729,13 +11743,10 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 	}
 
 	if (visible || was_visible)
-		intel_crtc->atomic.fb_bits |=
-			to_intel_plane(plane)->frontbuffer_bit;
+		pipe_config->fb_bits |= to_intel_plane(plane)
->frontbuffer_bit;
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.post_enable_primary = turn_on;
-
 		if (turn_off)
 			intel_crtc->atomic.disable_fbc = true;

@@ -13444,7 +13455,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		intel_post_plane_update(to_intel_crtc(crtc));
+		intel_post_plane_update(to_intel_crtc_state(crtc_state));
 
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);

drivers/gpu/drm/i915/intel_drv.h +0 -0

index e4ae629e7009..35ae827ab923 100644
@@ -370,6 +370,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync

mode.flags */

drivers/gpu/drm/i915/intel_drv.h +1 -0

 	unsigned long quirks;
 
+	unsigned fb_bits; /* framebuffers to flip */
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */

This leaves intel_crtc->atomic empty, so zap it as well.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 87 ++++++++++++++---------------------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     | 16 -------
 2 files changed, 33 insertions(+), 70 deletions(-)

drivers/gpu/drm/i915/intel_display.c +0 -1

index 95501aba7d23..7f69f98d8b23 100644
@@ -4734,7 +4734,6 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);

drivers/gpu/drm/i915/intel_display.c +3 -3

 	struct drm_device *dev = crtc->base.dev;

@@ -4750,15 +4749,15 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_update(dev_priv);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		if (primary_state->visible)
+			intel_fbc_update(dev_priv);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||

drivers/gpu/drm/i915/intel_display.c +0 -2

 		     !old_primary_state->visible))

@@ -4767,8 +4766,6 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
 
 	if (needs_modeset(&pipe_config->base))
 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 

static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)

drivers/gpu/drm/i915/intel_display.c +0 -1

@@ -4776,7 +4773,6 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
 	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 = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);

drivers/gpu/drm/i915/intel_display.c +0 -3

 	struct drm_atomic_state *old_state = old_crtc_state->base.state;

@@ -4785,9 +4781,6 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state, primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
-	if (atomic->disable_fbc)
-		intel_fbc_disable_crtc(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =

drivers/gpu/drm/i915/intel_display.c +5 -1

 			to_intel_plane_state(primary->state);

@@ -4795,8 +4788,27 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
 			to_intel_plane_state(old_pri_state);
 
 		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		    (modeset || !primary_state->visible)) {
+			intel_fbc_disable_crtc(crtc);
+
 			intel_pre_disable_primary(&crtc->base);
+		} else if (primary_state->visible &&
+			 INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv)

&&

drivers/gpu/drm/i915/intel_display.c +8 -0

+			 dev_priv->fbc.crtc == crtc &&
+			 primary_state->base.rotation != BIT(DRM_ROTATE_0)) {
+			/*
+			 * FBC does not work on some platforms for rotated
+			 * planes, so disable it when rotation is not 0 and
+			 * update it when rotation is set back to 0.
+			 *
+			 * FIXME: This is redundant with the fbc update done

in

drivers/gpu/drm/i915/intel_display.c +7 -1

+			 * the primary plane enable function except that that
+			 * one is done too late. We eventually need to unify
+			 * this.
+			 */
+
+			intel_fbc_disable_crtc(crtc);
+		}
 	}
 
 	if (pipe_config->disable_cxsr) {

@@ -11686,7 +11698,6 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane = plane_state->plane;
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);

drivers/gpu/drm/i915/intel_display.c +10 -44

 	int idx = intel_crtc->base.base.id, ret;

@@ -11745,46 +11756,17 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)
->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		if (turn_off)
-			intel_crtc->atomic.disable_fbc = true;
-
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-
-		if (visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    plane_state->rotation != BIT(DRM_ROTATE_0))
-			intel_crtc->atomic.disable_fbc = true;
-
-		intel_crtc->atomic.update_fbc |= visible || mode_changed;
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }

@@ -13166,9 +13148,6 @@ static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state
->mode.private_flags)
 			crtc_state->mode_changed = true;

drivers/gpu/drm/i915/intel_drv.h +0 -2

index 35ae827ab923..d2e1dc698fca 100644
@@ -527,20 +527,6 @@ struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of

an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */

drivers/gpu/drm/i915/intel_drv.h +0 -8

-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-	bool disable_fbc;
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;

On skylake when calculating plane visibility with the crtc in dpms off mode the real cdclk may be different from what it would be if the crtc was active. This may result in a WARN_ON(cdclk < crtc_clock) from skl_max_scale. The fix is to keep a atomic_cdclk that would be true if all crtc's were active.

This is required to get the same calculations done correctly regardless of dpms mode.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++-----------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
 3 files changed, 44 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -1

index 3caecf896f17..f3ad72abeea7 100644
@@ -1776,7 +1776,7 @@ struct drm_i915_private {
 
 	unsigned int fsb_freq, mem_freq, is_ddr3;
 	unsigned int skl_boot_cdclk;
-	unsigned int cdclk_freq, max_cdclk_freq;
+	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
 	unsigned int max_dotclk_freq;
 	unsigned int hpll_freq;
 	unsigned int czclk_freq;

drivers/gpu/drm/i915/intel_display.c +1 -0

index 7f69f98d8b23..b2bf92a3b701 100644
@@ -5312,6 +5312,7 @@ static void modeset_put_power_domains(struct
 drm_i915_private *dev_priv,
 
 static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 {
+	struct intel_atomic_state *intel_state =
 to_intel_atomic_state(state);
 	struct drm_device *dev = state->dev;

drivers/gpu/drm/i915/intel_display.c +3 -7

 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long put_domains[I915_MAX_PIPES] = {};

@@ -5325,13 +5326,9 @@ static void modeset_update_crtc_power_domains(struct
 drm_atomic_state *state)
 				modeset_get_crtc_power_domains(crtc);
 	}
 
-	if (dev_priv->display.modeset_commit_cdclk) {
-		unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
-
-		if (cdclk != dev_priv->cdclk_freq &&
-		    !WARN_ON(!state->allow_modeset))
-			dev_priv->display.modeset_commit_cdclk(state);
-	}
+	if (dev_priv->display.modeset_commit_cdclk &&
+	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
+		dev_priv->display.modeset_commit_cdclk(state);
 
 	for (i = 0; i < I915_MAX_PIPES; i++)

drivers/gpu/drm/i915/intel_display.c +15 -3

 		if (put_domains[i])

@@ -6039,13 +6036,18 @@ static int valleyview_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_pixclk = intel_mode_max_pixclk(dev, state);
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(state);
 
 	if (max_pixclk < 0)
 		return max_pixclk;
 
-	to_intel_atomic_state(state)->cdclk =
+	intel_state->cdclk = intel_state->dev_cdclk =
 		valleyview_calc_cdclk(dev_priv, max_pixclk);
 
+	if (!intel_state->active_crtcs)
+		intel_state->dev_cdclk = valleyview_calc_cdclk(dev_priv, 0);
+
 	return 0;
 }

@@ -6054,13 +6056,18 @@ static int broxton_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_pixclk = intel_mode_max_pixclk(dev, state);
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(state);
 
 	if (max_pixclk < 0)
 		return max_pixclk;
 
-	to_intel_atomic_state(state)->cdclk =
+	intel_state->cdclk = intel_state->dev_cdclk =
 		broxton_calc_cdclk(dev_priv, max_pixclk);
 
+	if (!intel_state->active_crtcs)
+		intel_state->dev_cdclk = broxton_calc_cdclk(dev_priv, 0);
+
 	return 0;
 }

@@ -6103,8 +6110,10 @@ static void vlv_program_pfi_credits(struct
 drm_i915_private *dev_priv)
 static void valleyview_modeset_commit_cdclk(struct drm_atomic_state
 *old_state)
 {
 	struct drm_device *dev = old_state->dev;
-	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
+	unsigned req_cdclk = old_intel_state->dev_cdclk;
 

drivers/gpu/drm/i915/intel_display.c +4 -1

 	/*
 	 * FIXME: We can end up here with all power domains off, yet

@@ -9574,7 +9583,9 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
-	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
+	unsigned int req_cdclk = old_intel_state->dev_cdclk;
 
 	broxton_set_cdclk(dev, req_cdclk);
 }

@@ -9700,6 +9711,7 @@ static void broadwell_set_cdclk(struct drm_device *dev,
 int cdclk)
 static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_atomic_state *intel_state =
 to_intel_atomic_state(state);
 	int max_pixclk = ilk_max_pixel_rate(state);

drivers/gpu/drm/i915/intel_display.c +3 -1

 	int cdclk;

@@ -9722,7 +9734,9 @@ static int broadwell_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 		return -EINVAL;
 	}
 
-	to_intel_atomic_state(state)->cdclk = cdclk;
+	intel_state->cdclk = intel_state->dev_cdclk = cdclk;
+	if (!intel_state->active_crtcs)
+		intel_state->dev_cdclk = 337500;
 
 	return 0;

}

drivers/gpu/drm/i915/intel_display.c +3 -1

@@ -9730,7 +9744,9 @@ static int broadwell_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 static void broadwell_modeset_commit_cdclk(struct drm_atomic_state
 *old_state)
 {
 	struct drm_device *dev = old_state->dev;
-	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
+	unsigned req_cdclk = old_intel_state->dev_cdclk;
 

drivers/gpu/drm/i915/intel_display.c +8 -6

 	broadwell_set_cdclk(dev, req_cdclk);
 }

@@ -13066,18 +13082,15 @@ static int intel_modeset_checks(struct
 drm_atomic_state *state)
 	 * adjusted_mode bits in the crtc directly.
 	 */
 	if (dev_priv->display.modeset_calc_cdclk) {
-		unsigned int cdclk;
-
 		ret = dev_priv->display.modeset_calc_cdclk(state);
 
-		cdclk = to_intel_atomic_state(state)->cdclk;
-		if (!ret && cdclk != dev_priv->cdclk_freq)
+		if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)
 			ret = intel_modeset_all_pipes(state);
 
 		if (ret < 0)
 			return ret;
 	} else
-		to_intel_atomic_state(state)->cdclk = dev_priv->cdclk_freq;
+		to_intel_atomic_state(state)->cdclk = dev_priv
->atomic_cdclk_freq;
 
 	intel_modeset_clear_plls(state);

@@ -13358,6 +13371,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
 		       sizeof(intel_state->min_pixclk));
 		dev_priv->active_crtcs = intel_state->active_crtcs;
+		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
 	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {

@@ -15026,7 +15040,12 @@ static void i915_disable_vga(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	intel_update_cdclk(dev);
+
+	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
+
 	intel_prepare_ddi(dev);
 	intel_init_clock_gating(dev);
 	intel_enable_gt_powersave(dev);

drivers/gpu/drm/i915/intel_drv.h +0 -0

index d2e1dc698fca..7fd025f64f4c 100644
@@ -247,6 +247,12 @@ struct intel_atomic_state {
 
 	unsigned int cdclk;

Quoted 235 lines · Maarten Lankhorst

On skylake when calculating plane visibility with the crtc in dpms off mode the real cdclk may be different from what it would be if the crtc was active. This may result in a WARN_ON(cdclk < crtc_clock) from skl_max_scale. The fix is to keep a atomic_cdclk that would be true if all crtc's were active.

This is required to get the same calculations done correctly regardless of dpms mode.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++---------

--

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
 3 files changed, 44 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -1

index 3caecf896f17..f3ad72abeea7 100644
@@ -1776,7 +1776,7 @@ struct drm_i915_private {
 
 	unsigned int fsb_freq, mem_freq, is_ddr3;
 	unsigned int skl_boot_cdclk;
-	unsigned int cdclk_freq, max_cdclk_freq;
+	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
 	unsigned int max_dotclk_freq;
 	unsigned int hpll_freq;
 	unsigned int czclk_freq;

drivers/gpu/drm/i915/intel_display.c +1 -0

index 7f69f98d8b23..b2bf92a3b701 100644
@@ -5312,6 +5312,7 @@ static void modeset_put_power_domains(struct
 drm_i915_private *dev_priv,
 
 static void modeset_update_crtc_power_domains(struct drm_atomic_state
 *state)
 {
+	struct intel_atomic_state *intel_state =
 to_intel_atomic_state(state);

drivers/gpu/drm/i915/intel_display.c +3 -7

 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long put_domains[I915_MAX_PIPES] = {};

@@ -5325,13 +5326,9 @@ static void modeset_update_crtc_power_domains(struct
 drm_atomic_state *state)
 				modeset_get_crtc_power_domains(crtc);
 	}
 
-	if (dev_priv->display.modeset_commit_cdclk) {
-		unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
-
-		if (cdclk != dev_priv->cdclk_freq &&
-		    !WARN_ON(!state->allow_modeset))
-			dev_priv->display.modeset_commit_cdclk(state);
-	}
+	if (dev_priv->display.modeset_commit_cdclk &&
+	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
+		dev_priv->display.modeset_commit_cdclk(state);
 
 	for (i = 0; i < I915_MAX_PIPES; i++)

drivers/gpu/drm/i915/intel_display.c +6 -1

 		if (put_domains[i])

@@ -6039,13 +6036,18 @@ static int valleyview_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_pixclk = intel_mode_max_pixclk(dev, state);
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(state);
 
 	if (max_pixclk < 0)
 		return max_pixclk;
 
-	to_intel_atomic_state(state)->cdclk =
+	intel_state->cdclk = intel_state->dev_cdclk =
 		valleyview_calc_cdclk(dev_priv, max_pixclk);
 
+	if (!intel_state->active_crtcs)
+		intel_state->dev_cdclk = valleyview_calc_cdclk(dev_priv,
 0);
+
 	return 0;

}

drivers/gpu/drm/i915/intel_display.c +9 -2

@@ -6054,13 +6056,18 @@ static int broxton_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_pixclk = intel_mode_max_pixclk(dev, state);
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(state);
 
 	if (max_pixclk < 0)
 		return max_pixclk;
 
-	to_intel_atomic_state(state)->cdclk =
+	intel_state->cdclk = intel_state->dev_cdclk =
 		broxton_calc_cdclk(dev_priv, max_pixclk);
 
+	if (!intel_state->active_crtcs)
+		intel_state->dev_cdclk = broxton_calc_cdclk(dev_priv, 0);
+
 	return 0;
 }

@@ -6103,8 +6110,10 @@ static void vlv_program_pfi_credits(struct
 drm_i915_private *dev_priv)
 static void valleyview_modeset_commit_cdclk(struct drm_atomic_state
 *old_state)
 {
 	struct drm_device *dev = old_state->dev;
-	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
+	unsigned req_cdclk = old_intel_state->dev_cdclk;
 

drivers/gpu/drm/i915/intel_display.c +3 -1

 	/*
 	 * FIXME: We can end up here with all power domains off, yet

@@ -9574,7 +9583,9 @@ void hsw_disable_pc8(struct drm_i915_private
 *dev_priv)
 static void broxton_modeset_commit_cdclk(struct drm_atomic_state
 *old_state)
 {
 	struct drm_device *dev = old_state->dev;
-	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
+	unsigned int req_cdclk = old_intel_state->dev_cdclk;
 

drivers/gpu/drm/i915/intel_display.c +1 -0

 	broxton_set_cdclk(dev, req_cdclk);
 }

@@ -9700,6 +9711,7 @@ static void broadwell_set_cdclk(struct drm_device
 *dev,
 int cdclk)
 static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_atomic_state *intel_state =
 to_intel_atomic_state(state);

drivers/gpu/drm/i915/intel_display.c +3 -1

 	int max_pixclk = ilk_max_pixel_rate(state);
 	int cdclk;

@@ -9722,7 +9734,9 @@ static int broadwell_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 		return -EINVAL;
 	}
 
-	to_intel_atomic_state(state)->cdclk = cdclk;
+	intel_state->cdclk = intel_state->dev_cdclk = cdclk;
+	if (!intel_state->active_crtcs)
+		intel_state->dev_cdclk = 337500;
 
 	return 0;

}

drivers/gpu/drm/i915/intel_display.c +3 -1

@@ -9730,7 +9744,9 @@ static int broadwell_modeset_calc_cdclk(struct
 drm_atomic_state *state)
 static void broadwell_modeset_commit_cdclk(struct drm_atomic_state
 *old_state)
 {
 	struct drm_device *dev = old_state->dev;
-	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
+	unsigned req_cdclk = old_intel_state->dev_cdclk;
 

drivers/gpu/drm/i915/intel_display.c +3 -6

 	broadwell_set_cdclk(dev, req_cdclk);
 }

@@ -13066,18 +13082,15 @@ static int intel_modeset_checks(struct
 drm_atomic_state *state)
 	 * adjusted_mode bits in the crtc directly.
 	 */
 	if (dev_priv->display.modeset_calc_cdclk) {
-		unsigned int cdclk;
-
 		ret = dev_priv->display.modeset_calc_cdclk(state);
 
-		cdclk = to_intel_atomic_state(state)->cdclk;
-		if (!ret && cdclk != dev_priv->cdclk_freq)
+		if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)
 			ret = intel_modeset_all_pipes(state);
 
 		if (ret < 0)
 			return ret;
 	} else
-		to_intel_atomic_state(state)->cdclk = dev_priv->cdclk_freq;
+		to_intel_atomic_state(state)->cdclk = dev_priv
->atomic_cdclk_freq;
 
 	intel_modeset_clear_plls(state);

@@ -13358,6 +13371,7 @@ static int intel_atomic_commit(struct drm_device
 *dev,
 		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
 		       sizeof(intel_state->min_pixclk));
 		dev_priv->active_crtcs = intel_state->active_crtcs;
+		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
 	}
 

drivers/gpu/drm/i915/intel_display.c +5 -0

 	for_each_crtc_in_state(state, crtc, crtc_state, i) {

@@ -15026,7 +15040,12 @@ static void i915_disable_vga(struct drm_device
 *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	intel_update_cdclk(dev);
+
+	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
+
 	intel_prepare_ddi(dev);
 	intel_init_clock_gating(dev);

drivers/gpu/drm/i915/intel_display.c +0 -0

 	intel_enable_gt_powersave(dev);

drivers/gpu/drm/i915/intel_drv.h +6 -0

index d2e1dc698fca..7fd025f64f4c 100644
@@ -247,6 +247,12 @@ struct intel_atomic_state {
 
 	unsigned int cdclk;
 
+	/*
+	 * Calculated device cdclk, can be different from cdclk
+	 * only when all crtc's are DPMS off.
+	 */
+	unsigned int dev_cdclk;
+
 	bool dpll_set, modeset;