[PATCH 00/12] Remove intel_crtc->atomic and fix BAT!

[PATCH 00/12] Remove intel_crtc->atomic and fix BAT!

[Maarten Lankhorst]

There are a couple of WARN_ON(!wm_changed); on skl, those should be gone with the skylake wm patch.

intel_crtc->atomic is a remnant from a transitional era, and it's time to kill it. :)

This series applies on top of

[PATCH 0/4] cdclk fixes

and perhaps (didn't check if it needed it)
[PATCH 0/8] Make plane updates use the atomic state.

Maarten Lankhorst (12):
  drm/i915: Move disable_cxsr to the crtc_state.
  drm/i915: Calculate watermark related members in the crtc_state, v3.
  drm/i915/skl: Update watermarks before the crtc is disabled.
  drm/i915: Remove double wait_for_vblank on broadwell.
  drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.
  drm/i915: Remove intel_crtc->atomic.disable_ips.
  drm/i915: Remove atomic.pre_disable_primary.
  drm/i915: Remove update_sprite_watermarks.
  drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.
  drm/i915: Nuke fbc members from intel_crtc->atomic.
  drm/i915: Keep track of the cdclk as if all crtc's were active.
  drm/i915: Calculate visibility in check_plane correctly regardless of
    dpms.

 drivers/gpu/drm/i915/i915_drv.h           |   2 +-
 drivers/gpu/drm/i915/intel_atomic.c       |   4 +
 drivers/gpu/drm/i915/intel_atomic_plane.c |   4 +-
 drivers/gpu/drm/i915/intel_display.c      | 352 +++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h          |  36 +--
 5 files changed, 213 insertions(+), 185 deletions(-)

-- 
2.1.0

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

[PATCH 01/12] drm/i915: Move disable_cxsr to the crtc_state.

[Maarten Lankhorst]

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index c4eadbc928b7..9f0638a37b6d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -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;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f8c332aee1e0..5ee64e67ad8a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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)
 		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)
 		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);
 	}
@@ -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;
 	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;
 			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)) {
 		intel_crtc->atomic.update_wm_pre = true;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 725973ebf49f..dd89342832e2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -370,7 +370,8 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 	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,
@@ -533,7 +534,6 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
 	bool disable_ips;
-	bool disable_cxsr;
 	bool pre_disable_primary;
 	bool update_wm_pre, update_wm_post;
 
-- 
2.1.0

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

Re: [PATCH 01/12] drm/i915: Move disable_cxsr to the crtc_state.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> intel_crtc->atomic will be removed later on, move this member
> to intel_crtc_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index c4eadbc928b7..9f0638a37b6d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -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;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f8c332aee1e0..5ee64e67ad8a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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)
>  		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)
>  		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);
>  	}
> @@ -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;
>  	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;
>  			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)) {
>  		intel_crtc->atomic.update_wm_pre = true;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 725973ebf49f..dd89342832e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -370,7 +370,8 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync
> mode.flags */
>  	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,
> @@ -533,7 +534,6 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
>  	bool disable_ips;
> -	bool disable_cxsr;
>  	bool pre_disable_primary;
>  	bool update_wm_pre, update_wm_post;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 9f0638a37b6d..4625f8a9ba12 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -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;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5ee64e67ad8a..db4995406277 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
 
-	if (crtc->atomic.update_wm_post)
+	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 
 	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)
@@ -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;
 		}
-	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
+	} else if ((was_visible || visible) &&
+		   intel_wm_need_update(plane, plane_state)) {
+		pipe_config->wm_changed = true;
 	}
 
 	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 &&
 	    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);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dd89342832e2..db61c37dbf09 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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;
-- 
2.1.0

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

Re: [PATCH 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 9f0638a37b6d..4625f8a9ba12 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -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;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5ee64e67ad8a..db4995406277 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  
> -	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.

>  
>  	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)
> @@ -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;
>  		}
> -	} 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?

Ander

> +		   intel_wm_need_update(plane, plane_state)) {
> +		pipe_config->wm_changed = true;
>  	}
>  
>  	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 &&
>  	    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);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index dd89342832e2..db61c37dbf09 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.

[Maarten Lankhorst]

Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>> 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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 9f0638a37b6d..4625f8a9ba12 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -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;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 5ee64e67ad8a..db4995406277 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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;
>>  
>> -	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..
>>  
>>  	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)
>> @@ -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;
>>  		}
>> -	} 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).

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

Re: [PATCH 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-24 at 15:55 +0100, Maarten Lankhorst wrote:
> Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira:
> > On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > > 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 <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  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(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > > b/drivers/gpu/drm/i915/intel_atomic.c
> > > index 9f0638a37b6d..4625f8a9ba12 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > @@ -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;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 5ee64e67ad8a..db4995406277 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -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;
> > >  
> > > -	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..
> > >  
> > >  	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)
> > > @@ -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;
> > >  		}
> > > -	} 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).

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.


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

Re: [PATCH 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.

[Maarten Lankhorst]

Op 25-11-15 om 10:22 schreef Ander Conselvan De Oliveira:
> On Tue, 2015-11-24 at 15:55 +0100, Maarten Lankhorst wrote:
>> Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira:
>>> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>>>> 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 <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 9f0638a37b6d..4625f8a9ba12 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -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;
>>>>  }
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index 5ee64e67ad8a..db4995406277 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -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;
>>>>  
>>>> -	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..
>>>>  
>>>>  	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)
>>>> @@ -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;
>>>>  		}
>>>> -	} 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).
> 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 could be fixed there. if (visible != was_visible) return true; if (!visible) return false;  and then remove the fb checks.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 02/12] drm/i915: Calculate watermark related members in the crtc_state, v4.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 9f0638a37b6d..4625f8a9ba12 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -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;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 029777f75de8..b96419fd4c9e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 53405cdc01f4..4f5d0949137e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/12] drm/i915: Calculate watermark related members in the crtc_state, v4.

[Daniel Vetter]

On Thu, Dec 03, 2015 at 01:49:13PM +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 9f0638a37b6d..4625f8a9ba12 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -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;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 029777f75de8..b96419fd4c9e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 53405cdc01f4..4f5d0949137e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/12] drm/i915/skl: Update watermarks before the crtc is disabled.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
Cc: stable@vger.kernel.org
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index db4995406277..5345ffcce51e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 
 	if (atomic->update_fbc)
@@ -13362,6 +13362,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 			dev_priv->display.crtc_disable(crtc);
 			intel_crtc->active = false;
 			intel_disable_shared_dpll(intel_crtc);
+
+			if (!crtc->state->active)
+				intel_update_watermarks(crtc);
 		}
 	}
 
-- 
2.1.0

Re: [PATCH 03/12] drm/i915/skl: Update watermarks before the crtc is disabled.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org
> Cc: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index db4995406277..5345ffcce51e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  
>  	if (atomic->update_fbc)
> @@ -13362,6 +13362,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  			dev_priv->display.crtc_disable(crtc);
>  			intel_crtc->active = false;
>  			intel_disable_shared_dpll(intel_crtc);
> +
> +			if (!crtc->state->active)
> +				intel_update_watermarks(crtc);
>  		}
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/12] drm/i915: Remove double wait_for_vblank on broadwell.

[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 <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5345ffcce51e..60f17bc5f0ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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
-	 * 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
-- 
2.1.0

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

Re: [PATCH 04/12] drm/i915: Remove double wait_for_vblank on broadwell.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5345ffcce51e..60f17bc5f0ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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
> -	 * 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
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915: Remove double wait_for_vblank on broadwell.

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 11:44:48AM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > 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 <maarten.lankhorst@linux.intel.com>
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

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

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 8 --------
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 5345ffcce51e..60f17bc5f0ce 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -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
> > -	 * 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
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 04/12] drm/i915: Remove double wait_for_vblank on broadwell.

[Maarten Lankhorst]

Op 08-12-15 om 15:14 schreef Ville Syrjälä:
> On Wed, Nov 25, 2015 at 11:44:48AM +0200, Ander Conselvan De Oliveira wrote:
>> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>>> 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 <maarten.lankhorst@linux.intel.com>
>> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> I see this one got merged without the r-b tag in the commit message.
>
>
Oops, my bad!

Waiting on git-patchwork to automate pulling tags. :)

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

Re: [PATCH 04/12] drm/i915: Remove double wait_for_vblank on broadwell.

[Daniel Vetter]

On Tue, Dec 08, 2015 at 04:14:58PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 25, 2015 at 11:44:48AM +0200, Ander Conselvan De Oliveira wrote:
> > On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > > 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 <maarten.lankhorst@linux.intel.com>
> > 
> > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
> I see this one got merged without the r-b tag in the commit message.

I guess we really need your dim xt ;-)
-Daniel

> 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 8 --------
> > >  1 file changed, 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 5345ffcce51e..60f17bc5f0ce 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -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
> > > -	 * 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
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/12] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.

[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 <maarten.lankhorst@linux.intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4625f8a9ba12..8e579a8505ac 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -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;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60f17bc5f0ce..299edbf6f99e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 
 	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;
 		}
 	} 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;
 	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;
-		}
 
 		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
@@ -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));
 
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index db61c37dbf09..66f66740ccaa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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,
@@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit {
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-	bool wait_vblank;
 	bool update_fbc;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
-- 
2.1.0

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

Re: [PATCH 05/12] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 4625f8a9ba12..8e579a8505ac 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -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;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 60f17bc5f0ce..299edbf6f99e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  
>  	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;
>  		}
>  	} 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;
>  	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;
> -		}
>  
>  		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
> @@ -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);

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

>  	}
>  
>  	/* 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);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index db61c37dbf09..66f66740ccaa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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,
> @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;
>  	bool update_fbc;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.

[Imre Deak]

On ke, 2015-11-25 at 14:21 +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > 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 <maarten.lankhorst@linux.intel.com>
> > ---
> >  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(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index 4625f8a9ba12..8e579a8505ac 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -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;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 60f17bc5f0ce..299edbf6f99e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -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);
> >  
> >  	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;
> >  		}
> >  	} 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;
> >  	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;
> > -		}
> >  
> >  		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
> > @@ -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);
> 
> 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().

Yes, that's cleaner, and possibly avoids some unnecessary on/off toggling of
power wells. We could've done this already earlier using the existing any_ms flag..

Btw, not the problem in this patch, but I just noticed we should take the modeset
power domain already earlier. We are disabling at least CRTCs/PLLs already earlier
which is in the set of operations we need to protect with this power domain. That
could be fixed as a follow-up.

--Imre

> 
> Ander
> 
> >  	}
> >  
> >  	/* 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);
> >  	drm_atomic_helper_cleanup_planes(dev, state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index db61c37dbf09..66f66740ccaa 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -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,
> > @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit {
> >  
> >  	/* Sleepable operations to perform after commit */
> >  	unsigned fb_bits;
> > -	bool wait_vblank;
> >  	bool update_fbc;
> >  	bool post_enable_primary;
> >  	unsigned update_sprite_watermarks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.

[Daniel Stone]

Hi,

On 25 November 2015 at 12:21, Ander Conselvan De Oliveira
<conselvan2@gmail.com> wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>> @@ -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);
>> -
>> -             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().

Even better would be if POWER_DOMAIN_MODESET was a part of
put_domains, and all domains were put after completion. I suggested
this during review of Patrik's series, but that seems to have got
lost. :(

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 4625f8a9ba12..8e579a8505ac 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -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;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 60f17bc5f0ce..299edbf6f99e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  
>  	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;
>  		}
>  	} 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;
>  	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;
> -		}

Did you intend to delete the braces on both sides of the if? Sorry, OCD. :)

Other than this and the comment on the other thread were I added Patrik and
Imre, this looks good.

Ander


>  
>  		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
> @@ -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));
>  
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index db61c37dbf09..66f66740ccaa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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,
> @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;
>  	bool update_fbc;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/12] drm/i915: Remove intel_crtc->atomic.disable_ips.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +---------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 299edbf6f99e..1fec49c4490e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
-		}
 
 		/*
 		 * FBC does not work on some platforms for rotated
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 66f66740ccaa..13ffefa3a6c1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -535,7 +535,6 @@ struct intel_mmio_flip {
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
-	bool disable_ips;
 	bool pre_disable_primary;
 
 	/* Sleepable operations to perform after commit */
-- 
2.1.0

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

Re: [PATCH 06/12] drm/i915: Remove intel_crtc->atomic.disable_ips.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 +---------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 299edbf6f99e..1fec49c4490e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
> -		}
>  
>  		/*
>  		 * FBC does not work on some platforms for rotated
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 66f66740ccaa..13ffefa3a6c1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -535,7 +535,6 @@ struct intel_mmio_flip {
>  struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
> -	bool disable_ips;
>  	bool pre_disable_primary;
>  
>  	/* Sleepable operations to perform after commit */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/12] drm/i915: Remove atomic.pre_disable_primary.

[Maarten Lankhorst]

This can be derived from the atomic state in pre_plane_update,
which makes it more clear when it's supposed to be called.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fec49c4490e..52a0058cf145 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4757,26 +4757,40 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	memset(atomic, 0, sizeof(*atomic));
 }
 
-static void intel_pre_plane_update(struct intel_crtc *crtc)
+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);
+	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 =
+		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 (atomic->pre_disable_primary)
-		intel_pre_disable_primary(&crtc->base);
+	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 (old_primary_state->visible &&
+		    (modeset || !primary_state->visible))
+			intel_pre_disable_primary(&crtc->base);
+	}
 
 	if (pipe_config->disable_cxsr) {
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
 
-	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+	if (!modeset && pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 }
 
@@ -11721,7 +11735,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
 
 		if (turn_off)
@@ -13367,7 +13380,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!needs_modeset(crtc->state))
 			continue;
 
-		intel_pre_plane_update(intel_crtc);
+		intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
 		if (crtc_state->active) {
 			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
@@ -13393,7 +13406,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	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);
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc->state);
@@ -13416,7 +13428,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		}
 
 		if (!modeset)
-			intel_pre_plane_update(intel_crtc);
+			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
 		if (crtc->state->active &&
 		    (crtc->state->planes_changed || update_pipe))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 13ffefa3a6c1..ee550a93bb9f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -535,7 +535,6 @@ struct intel_mmio_flip {
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
-	bool pre_disable_primary;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-- 
2.1.0

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

[PATCH 08/12] drm/i915: Remove update_sprite_watermarks.

[Maarten Lankhorst]

Commit 791a32be6eb2 ("drm/i915: Drop intel_update_sprite_watermarks")
removes the use of this variable, but forgot to remove it.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +-----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 52a0058cf145..d539703de367 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11676,7 +11676,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
-	int i = drm_plane_index(plane);
 	bool mode_changed = needs_modeset(crtc_state);
 	bool was_crtc_enabled = crtc->state->active;
 	bool is_crtc_enabled = crtc_state->active;
@@ -11770,11 +11769,8 @@ 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)) {
+		    !needs_scaling(old_plane_state))
 			pipe_config->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed)
-			intel_crtc->atomic.update_sprite_watermarks |=
-				1 << i;
 
 		break;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ee550a93bb9f..e4ae629e7009 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -540,7 +540,6 @@ struct intel_crtc_atomic_commit {
 	unsigned fb_bits;
 	bool update_fbc;
 	bool post_enable_primary;
-	unsigned update_sprite_watermarks;
 };
 
 struct intel_crtc {
-- 
2.1.0

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

[PATCH 09/12] drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 8e579a8505ac..9a45cff26767 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -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;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d539703de367..95501aba7d23 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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)
 	if (atomic->update_fbc)
 		intel_fbc_update(dev_priv);
 
-	if (atomic->post_enable_primary)
-		intel_post_enable_primary(&crtc->base);
+	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 &&
+		    (needs_modeset(&pipe_config->base) ||
+		     !old_primary_state->visible))
+			intel_post_enable_primary(&crtc->base);
+	}
 
 	if (needs_modeset(&pipe_config->base))
 		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);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e4ae629e7009..35ae827ab923 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -370,6 +370,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 	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 */
@@ -537,9 +538,7 @@ struct intel_crtc_atomic_commit {
 	bool disable_fbc;
 
 	/* Sleepable operations to perform after commit */
-	unsigned fb_bits;
 	bool update_fbc;
-	bool post_enable_primary;
 };
 
 struct intel_crtc {
-- 
2.1.0

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

Re: [PATCH 09/12] drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 8e579a8505ac..9a45cff26767 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -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;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index d539703de367..95501aba7d23 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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)
>  	if (atomic->update_fbc)
>  		intel_fbc_update(dev_priv);
>  
> -	if (atomic->post_enable_primary)
> -		intel_post_enable_primary(&crtc->base);
> +	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);

Hmm, old_pri_state and old_primary_state. I wonder if it is worth creating a
intel_atomic_get_existing_plane_state() helper to avoid this kind of stuff.
Something to check out later.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


> +
> +		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))
>  		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);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e4ae629e7009..35ae827ab923 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -370,6 +370,7 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync
> mode.flags */
>  	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 */
> @@ -537,9 +538,7 @@ struct intel_crtc_atomic_commit {
>  	bool disable_fbc;
>  
>  	/* Sleepable operations to perform after commit */
> -	unsigned fb_bits;
>  	bool update_fbc;
> -	bool post_enable_primary;
>  };
>  
>  struct intel_crtc {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/12] drm/i915: Nuke fbc members from intel_crtc->atomic.

[Maarten Lankhorst]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 95501aba7d23..7f69f98d8b23 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 	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) ||
 		     !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)
@@ -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);
 	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 =
 			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) &&
+			 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
+			 * 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);
 	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;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 35ae827ab923..d2e1dc698fca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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.
- */
-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;
@@ -601,8 +587,6 @@ struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;
 
-- 
2.1.0

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

Re: [PATCH 10/12] drm/i915: Nuke fbc members from intel_crtc->atomic.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> This leaves intel_crtc->atomic empty, so zap it as well.

I made some comments on this patch already. I think those are still valid:

http://lists.freedesktop.org/archives/intel-gfx/2015-November/079829.html

Ander

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 87 ++++++++++++++---------------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     | 16 -------
>  2 files changed, 33 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 95501aba7d23..7f69f98d8b23 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  	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) ||
>  		     !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)
> @@ -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);
>  	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 =
>  			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)
> &&
> +			 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
> +			 * 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);
>  	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;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 35ae827ab923..d2e1dc698fca 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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.
> - */
> -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;
> @@ -601,8 +587,6 @@ struct intel_crtc {
>  		int scanline_start;
>  	} debug;
>  
> -	struct intel_crtc_atomic_commit atomic;
> -
>  	/* scalers available on this crtc */
>  	int num_scalers;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/12] drm/i915: Keep track of the cdclk as if all crtc's were active.

[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 <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3caecf896f17..f3ad72abeea7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7f69f98d8b23..b2bf92a3b701 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
 	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++)
 		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;
 
 	/*
 	 * 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);
 	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;
 }
@@ -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;
 
 	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);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d2e1dc698fca..7fd025f64f4c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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;
 
 	unsigned int active_crtcs;
-- 
2.1.0

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

Re: [PATCH 11/12] drm/i915: Keep track of the cdclk as if all crtc's were active.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++-----------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
>  3 files changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3caecf896f17..f3ad72abeea7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7f69f98d8b23..b2bf92a3b701 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  	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++)
>  		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;
>  
>  	/*
>  	 * 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);
>  	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;
>  }
> @@ -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;
>  
>  	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);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d2e1dc698fca..7fd025f64f4c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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;
>  
>  	unsigned int active_crtcs;

So state->cdclk and dev_priv->atomic_cdclk_freq hold the value for when all
CRTCs are enabled and state->dev_cdclkand dev_priv->cdclk_freq hold the actual
cdclk value (which changes for DPMS off). In intel_atomic_check(), there is the
following assignment:

if (any_ms) {
	...
} else
	intel_state->cdclk = to_i915(state->dev)->cdclk_freq;

Can an update during DPMS off cause the value of cdclk for when all CRTCs are
enabled to be overwritten with the DPMS off value?

Patch looks fine other than that, but maybe as a followup we could rename those
fields so the mapping between them is a bit clearer. Maybe current_cdclk for
both state and dev_priv for the value currently programmed to the hardware. Not
sure about the atomic_cdclk. How about dpms_on_cdclk or active_cdclk?

Ander



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

Re: [PATCH 11/12] drm/i915: Keep track of the cdclk as if all crtc's were active.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-26 at 15:31 +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > 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 <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++---------
> > --
> > -
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
> >  3 files changed, 44 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3caecf896f17..f3ad72abeea7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -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;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 7f69f98d8b23..b2bf92a3b701 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -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;
> >  	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++)
> >  		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;
> >  
> >  	/*
> >  	 * 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);
> >  	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;
> >  }
> > @@ -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;
> >  
> >  	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);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d2e1dc698fca..7fd025f64f4c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -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;
> >  
> >  	unsigned int active_crtcs;
> 
> So state->cdclk and dev_priv->atomic_cdclk_freq hold the value for when all
> CRTCs are enabled and state->dev_cdclkand dev_priv->cdclk_freq hold the actual

s/enabled/active/

> cdclk value (which changes for DPMS off). In intel_atomic_check(), there is
> the
> following assignment:
> 
> if (any_ms) {
> 	...
> } else
> 	intel_state->cdclk = to_i915(state->dev)->cdclk_freq;
> 
> Can an update during DPMS off cause the value of cdclk for when all CRTCs are
> enabled to be overwritten with the DPMS off value?
> 
> Patch looks fine other than that, but maybe as a followup we could rename
> those
> fields so the mapping between them is a bit clearer. Maybe current_cdclk for
> both state and dev_priv for the value currently programmed to the hardware.
> Not
> sure about the atomic_cdclk. How about dpms_on_cdclk or active_cdclk?
> 
> Ander
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Keep track of the cdclk as if all crtc's were active.

[Mika Kahola]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> 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.
> 
Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
>  3 files changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3caecf896f17..f3ad72abeea7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7f69f98d8b23..b2bf92a3b701 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  	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++)
>  		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;
>  
>  	/*
>  	 * 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);
>  	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;
>  }
> @@ -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;
>  
>  	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);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d2e1dc698fca..7fd025f64f4c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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;
>  
>  	unsigned int active_crtcs;


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

[PATCH 12/12] drm/i915: Calculate visibility in check_plane correctly regardless of dpms.

[Maarten Lankhorst]

When the crtc is configured but not active we currently clip to (0,0)x(0,0).
This results in differences in calculations depending on dpms setting.
When the crtc is enabled but not active run check_plane as if it were on,
but afterwards set plane_state->visible = false for the checks.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index a5a336863109..e0b851a0004a 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -152,9 +152,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
 	intel_state->clip.x2 =
-		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
+		crtc_state->base.enable ? crtc_state->pipe_src_w : 0;
 	intel_state->clip.y2 =
-		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
+		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
 
 	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b2bf92a3b701..9db322182b15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11738,8 +11738,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (!was_crtc_enabled && WARN_ON(was_visible))
 		was_visible = false;
 
-	if (!is_crtc_enabled && WARN_ON(visible))
-		visible = false;
+	/*
+	 * During visibility is calculated as if the crtc was on,
+	 * but after scaler setup everything depends on it being off
+	 * when the crtc isn't active.
+	 */
+	if (!is_crtc_enabled)
+		to_intel_plane_state(plane_state)->visible = visible = false;
 
 	if (!was_visible && !visible)
 		return 0;
-- 
2.1.0

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

Re: [PATCH 12/12] drm/i915: Calculate visibility in check_plane correctly regardless of dpms.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> When the crtc is configured but not active we currently clip to (0,0)x(0,0).
> This results in differences in calculations depending on dpms setting.

Is this part of "and fix BAT!"?

> When the crtc is enabled but not active run check_plane as if it were on,
> but afterwards set plane_state->visible = false for the checks.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++--
>  drivers/gpu/drm/i915/intel_display.c      | 9 +++++++--
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index a5a336863109..e0b851a0004a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -152,9 +152,9 @@ static int intel_plane_atomic_check(struct drm_plane
> *plane,
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
>  	intel_state->clip.x2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> +		crtc_state->base.enable ? crtc_state->pipe_src_w : 0;
>  	intel_state->clip.y2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> +		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>  
>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b2bf92a3b701..9db322182b15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11738,8 +11738,13 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (!was_crtc_enabled && WARN_ON(was_visible))
>  		was_visible = false;
>  
> -	if (!is_crtc_enabled && WARN_ON(visible))
> -		visible = false;
> +	/*
> +	 * During visibility is calculated as if the crtc was on,

s/During// ?

Ander

> +	 * but after scaler setup everything depends on it being off
> +	 * when the crtc isn't active.
> +	 */
> +	if (!is_crtc_enabled)
> +		to_intel_plane_state(plane_state)->visible = visible = false;
>  
>  	if (!was_visible && !visible)
>  		return 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Calculate visibility in check_plane correctly regardless of dpms.

[Maarten Lankhorst]

Op 26-11-15 om 14:48 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>> When the crtc is configured but not active we currently clip to (0,0)x(0,0).
>> This results in differences in calculations depending on dpms setting.
> Is this part of "and fix BAT!"?
I'm not 100% sure if there is a BAT associated with it, but it will fix situations where
you can commit a state that's valid while the crtc is inactive, but not when turning it on again.

So for example using a state with 4 planes that all require scalers would be valid with dpms off,
but attempting to turn dpms on will fail with -EINVAL... If there's no BAT for it there should be one imo..
>> When the crtc is enabled but not active run check_plane as if it were on,
>> but afterwards set plane_state->visible = false for the checks.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++--
>>  drivers/gpu/drm/i915/intel_display.c      | 9 +++++++--
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index a5a336863109..e0b851a0004a 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -152,9 +152,9 @@ static int intel_plane_atomic_check(struct drm_plane
>> *plane,
>>  	intel_state->clip.x1 = 0;
>>  	intel_state->clip.y1 = 0;
>>  	intel_state->clip.x2 =
>> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
>> +		crtc_state->base.enable ? crtc_state->pipe_src_w : 0;
>>  	intel_state->clip.y2 =
>> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
>> +		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>>  
>>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index b2bf92a3b701..9db322182b15 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11738,8 +11738,13 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  	if (!was_crtc_enabled && WARN_ON(was_visible))
>>  		was_visible = false;
>>  
>> -	if (!is_crtc_enabled && WARN_ON(visible))
>> -		visible = false;
>> +	/*
>> +	 * During visibility is calculated as if the crtc was on,
> s/During// ?
>
> Ander
>
>> +	 * but after scaler setup everything depends on it being off
>> +	 * when the crtc isn't active.
>> +	 */
>> +	if (!is_crtc_enabled)
>> +		to_intel_plane_state(plane_state)->visible = visible = false;
>>  
>>  	if (!was_visible && !visible)
>>  		return 0;

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

Re: [PATCH 12/12] drm/i915: Calculate visibility in check_plane correctly regardless of dpms.

[Mika Kahola]

Ander had a comment s/During//

Other than this is

On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> When the crtc is configured but not active we currently clip to (0,0)x(0,0).
> This results in differences in calculations depending on dpms setting.
> When the crtc is enabled but not active run check_plane as if it were on,
> but afterwards set plane_state->visible = false for the checks.
> 
Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++--
>  drivers/gpu/drm/i915/intel_display.c      | 9 +++++++--
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index a5a336863109..e0b851a0004a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -152,9 +152,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
>  	intel_state->clip.x2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> +		crtc_state->base.enable ? crtc_state->pipe_src_w : 0;
>  	intel_state->clip.y2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> +		crtc_state->base.enable ? crtc_state->pipe_src_h : 0;
>  
>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b2bf92a3b701..9db322182b15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11738,8 +11738,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	if (!was_crtc_enabled && WARN_ON(was_visible))
>  		was_visible = false;
>  
> -	if (!is_crtc_enabled && WARN_ON(visible))
> -		visible = false;
> +	/*
> +	 * During visibility is calculated as if the crtc was on,
> +	 * but after scaler setup everything depends on it being off
> +	 * when the crtc isn't active.
> +	 */
> +	if (!is_crtc_enabled)
> +		to_intel_plane_state(plane_state)->visible = visible = false;
>  
>  	if (!was_visible && !visible)
>  		return 0;


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