[11/42] drm/i915: Update less state during modeset.
diff mbox

Message ID 1431354318-11995-12-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 11, 2015, 2:24 p.m. UTC
No need to repeatedly call update_watermarks, or update_fbc.
For update_watermarks once should be enough after disabling crtc's
and swapping the state.

Down to a single call to update_watermarks in .crtc_enable

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

Comments

Daniel Vetter May 12, 2015, 8:22 a.m. UTC | #1
On Mon, May 11, 2015 at 04:24:47PM +0200, Maarten Lankhorst wrote:
> No need to repeatedly call update_watermarks, or update_fbc.
> For update_watermarks once should be enough after disabling crtc's
> and swapping the state.
> 
> Down to a single call to update_watermarks in .crtc_enable
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

In theory we shouldn't even need that one, but only a two-stage wm update
in our ->atomic_begin/->atomic_flush crtc callbacks. But I wonder whether
reducing the amount of wm updates as long as we don't yet have the
two-stage thing will increase our risk for underruns. I guess for now we'd
at least need the call in crtc_enable to reduce that risk.

Imo the fbc changes would look great as it's own patch ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 79 +++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9610f76a2489..a90ebc8eff6f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4719,10 +4719,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	 */
>  	hsw_enable_ips(intel_crtc);
>  
> -	mutex_lock(&dev->struct_mutex);
> -	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
> -
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>  	 * So don't enable underrun reporting before at least some planes
> @@ -4777,11 +4773,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  	if (HAS_GMCH_DISPLAY(dev))
>  		intel_set_memory_cxsr(dev_priv, false);
>  
> -	mutex_lock(&dev->struct_mutex);
> -	if (dev_priv->fbc.crtc == intel_crtc)
> -		intel_fbc_disable(dev);
> -	mutex_unlock(&dev->struct_mutex);
> -
>  	/*
>  	 * FIXME IPS should be fine as long as one plane is
>  	 * enabled, but in practice it seems to have problems
> @@ -5009,9 +5000,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	u32 reg, temp;
>  
> -	if (!intel_crtc->active)
> -		return;
> -
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
> @@ -5049,18 +5037,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  			I915_WRITE(PCH_DPLL_SEL, temp);
>  		}
>  
> -		/* disable PCH DPLL */
> -		intel_disable_shared_dpll(intel_crtc);
> -
>  		ironlake_fdi_pll_disable(intel_crtc);
>  	}
> -
> -	intel_crtc->active = false;
> -	intel_update_watermarks(crtc);
> -
> -	mutex_lock(&dev->struct_mutex);
> -	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  static void haswell_crtc_disable(struct drm_crtc *crtc)
> @@ -5071,9 +5049,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
> -	if (!intel_crtc->active)
> -		return;
> -
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		intel_opregion_notify_encoder(encoder, false);
>  		encoder->disable(encoder);
> @@ -5109,16 +5084,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
> -
> -	intel_crtc->active = false;
> -	intel_update_watermarks(crtc);
> -
> -	mutex_lock(&dev->struct_mutex);
> -	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	if (intel_crtc_to_shared_dpll(intel_crtc))
> -		intel_disable_shared_dpll(intel_crtc);
>  }
>  
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> @@ -5943,9 +5908,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	if (!intel_crtc->active)
> -		return;
> -
>  	/*
>  	 * On gen2 planes are double buffered but the pipe isn't, so we must
>  	 * wait for planes to fully turn off before disabling the pipe.
> @@ -5979,13 +5941,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  
>  	if (!IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> -
> -	intel_crtc->active = false;
> -	intel_update_watermarks(crtc);
> -
> -	mutex_lock(&dev->struct_mutex);
> -	intel_fbc_update(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  /* Master function to enable/disable CRTC and corresponding power wells */
> @@ -12347,11 +12302,21 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  	__intel_set_mode_update_planes(dev, state);
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
>  		if (!needs_modeset(crtc_state))
>  			continue;
>  
> +		if (!crtc->state->active)
> +			continue;
> +
>  		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
>  		dev_priv->display.crtc_disable(crtc);
> +
> +		intel_crtc->active = false;
> +
> +		if (intel_crtc_to_shared_dpll(intel_crtc))
> +			intel_disable_shared_dpll(intel_crtc);
>  	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
> @@ -12365,8 +12330,11 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!crtc->state->active)
> +		if (!crtc->state->active) {
> +			if (needs_modeset(crtc->state))
> +				intel_update_watermarks(crtc);
>  			continue;
> +		}
>  
>  		update_scanline_offset(to_intel_crtc(crtc));
>  
> @@ -13030,6 +12998,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
>  
>  	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
>  	intel_crtc->atomic.update_wm = mode_changed;
> +	intel_crtc->atomic.disable_fbc = mode_changed;
>  
>  	idx = crtc->base.id;
>  	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> @@ -13165,6 +13134,9 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
>  			break;
>  		}
>  	}
> +
> +	if (mode_changed)
> +		intel_crtc->atomic.update_wm = false;
>  	return 0;
>  }
>  
> @@ -13204,8 +13176,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	if (intel_crtc->atomic.wait_for_flips)
>  		intel_crtc_wait_for_pending_flips(crtc);
>  
> -	if (intel_crtc->atomic.disable_fbc)
> -		intel_fbc_disable(dev);
> +	if (intel_crtc->atomic.disable_fbc &&
> +	    dev_priv->fbc.crtc == intel_crtc) {
> +		mutex_lock(&dev->struct_mutex);
> +		if (dev_priv->fbc.crtc == intel_crtc)
> +			intel_fbc_disable(dev);
> +		mutex_unlock(&dev->struct_mutex);
> +	}
>  
>  	if (intel_crtc->atomic.pre_disable_primary)
>  		intel_pre_disable_primary(crtc);
> @@ -13244,15 +13221,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  
>  	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
>  
> +	if (intel_crtc->atomic.post_enable_primary)
> +		intel_post_enable_primary(crtc);
> +
>  	if (intel_crtc->atomic.update_fbc) {
>  		mutex_lock(&dev->struct_mutex);
>  		intel_fbc_update(dev);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
>  
> -	if (intel_crtc->atomic.post_enable_primary)
> -		intel_post_enable_primary(crtc);
> -
>  	plane_mask = intel_crtc->atomic.update_sprite_watermarks;
>  	drm_for_each_plane_mask(p, dev, plane_mask)
>  		intel_update_sprite_watermarks(p, crtc, 0, 0, 0, false, false);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9610f76a2489..a90ebc8eff6f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4719,10 +4719,6 @@  intel_post_enable_primary(struct drm_crtc *crtc)
 	 */
 	hsw_enable_ips(intel_crtc);
 
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes
@@ -4777,11 +4773,6 @@  intel_pre_disable_primary(struct drm_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		intel_set_memory_cxsr(dev_priv, false);
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->fbc.crtc == intel_crtc)
-		intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
@@ -5009,9 +5000,6 @@  static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	if (!intel_crtc->active)
-		return;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
@@ -5049,18 +5037,8 @@  static void ironlake_crtc_disable(struct drm_crtc *crtc)
 			I915_WRITE(PCH_DPLL_SEL, temp);
 		}
 
-		/* disable PCH DPLL */
-		intel_disable_shared_dpll(intel_crtc);
-
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5071,9 +5049,6 @@  static void haswell_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
-	if (!intel_crtc->active)
-		return;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
@@ -5109,16 +5084,6 @@  static void haswell_crtc_disable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	if (intel_crtc_to_shared_dpll(intel_crtc))
-		intel_disable_shared_dpll(intel_crtc);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
@@ -5943,9 +5908,6 @@  static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (!intel_crtc->active)
-		return;
-
 	/*
 	 * On gen2 planes are double buffered but the pipe isn't, so we must
 	 * wait for planes to fully turn off before disabling the pipe.
@@ -5979,13 +5941,6 @@  static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	if (!IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
@@ -12347,11 +12302,21 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	__intel_set_mode_update_planes(dev, state);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
 		if (!needs_modeset(crtc_state))
 			continue;
 
+		if (!crtc->state->active)
+			continue;
+
 		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
 		dev_priv->display.crtc_disable(crtc);
+
+		intel_crtc->active = false;
+
+		if (intel_crtc_to_shared_dpll(intel_crtc))
+			intel_disable_shared_dpll(intel_crtc);
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -12365,8 +12330,11 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc->state->active)
+		if (!crtc->state->active) {
+			if (needs_modeset(crtc->state))
+				intel_update_watermarks(crtc);
 			continue;
+		}
 
 		update_scanline_offset(to_intel_crtc(crtc));
 
@@ -13030,6 +12998,7 @@  static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 	intel_crtc->atomic.update_wm = mode_changed;
+	intel_crtc->atomic.disable_fbc = mode_changed;
 
 	idx = crtc->base.id;
 	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
@@ -13165,6 +13134,9 @@  static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 			break;
 		}
 	}
+
+	if (mode_changed)
+		intel_crtc->atomic.update_wm = false;
 	return 0;
 }
 
@@ -13204,8 +13176,13 @@  static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.wait_for_flips)
 		intel_crtc_wait_for_pending_flips(crtc);
 
-	if (intel_crtc->atomic.disable_fbc)
-		intel_fbc_disable(dev);
+	if (intel_crtc->atomic.disable_fbc &&
+	    dev_priv->fbc.crtc == intel_crtc) {
+		mutex_lock(&dev->struct_mutex);
+		if (dev_priv->fbc.crtc == intel_crtc)
+			intel_fbc_disable(dev);
+		mutex_unlock(&dev->struct_mutex);
+	}
 
 	if (intel_crtc->atomic.pre_disable_primary)
 		intel_pre_disable_primary(crtc);
@@ -13244,15 +13221,15 @@  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
 
+	if (intel_crtc->atomic.post_enable_primary)
+		intel_post_enable_primary(crtc);
+
 	if (intel_crtc->atomic.update_fbc) {
 		mutex_lock(&dev->struct_mutex);
 		intel_fbc_update(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	if (intel_crtc->atomic.post_enable_primary)
-		intel_post_enable_primary(crtc);
-
 	plane_mask = intel_crtc->atomic.update_sprite_watermarks;
 	drm_for_each_plane_mask(p, dev, plane_mask)
 		intel_update_sprite_watermarks(p, crtc, 0, 0, 0, false, false);