drm/i915: Enable fastboot, v2!

ips_enabled was used as a variable of whether IPS can be enabled or not, but should be used to test whether IPS is actually enabled.

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

---
 drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 2 files changed, 35 insertions(+), 42 deletions(-)

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

index 3b3dec1e6640..8283e80597bd 100644
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc_state->ips_enabled)
 		return;
 
 	/*

@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->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
-	 * versa.
-	 */
-	hsw_enable_ips(new_crtc_state);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes

@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	intel_check_pch_fifo_underruns(dev_priv);
 }
 
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
 static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
-			  const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);

@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
-	 * So diasble underrun reporting before all the planes get disabled.
-	 * FIXME: Need to fix the logic to work when we turn off all planes
-	 * but leave the pipe running.
+	 * So disable underrun reporting before all the planes get disabled.
 	 */
 	if (IS_GEN2(dev_priv))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
-	/*
-	 * 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
-	 * versa.
-	 */
-	hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */

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

+		hsw_enable_ips(pipe_config);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),

@@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 
+	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+		hsw_disable_ips(old_crtc_state);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(old_intel_state,

@@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 			to_intel_plane_state(old_pri_state);
 
 		intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
-		if (old_primary_state->base.visible &&
+		/*
+		 * Gen2 reports pipe underruns whenever all planes are disabled.
+		 * So disable underrun reporting before all the planes get disabled.
+		 */
+		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
 		    (modeset || !primary_state->base.visible))
-			intel_pre_disable_primary(&crtc->base, old_crtc_state);
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
 	}
 
 	/*

@@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 				     struct intel_crtc_state *pipe_config)
 {
+	u8 visible_planes;
+
 	if (pipe_config->ips_force_disable)
 		return false;
 
 	if (pipe_config->pipe_bpp > 24)
 		return false;
 

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

+
 	/* HSW can handle pixel rate up to cdclk? */
 	if (IS_HASWELL(dev_priv))
 		return true;

@@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	intel_crtc_compute_pixel_rate(pipe_config);
 
-	if (HAS_IPS(dev_priv))
-		hsw_compute_ips_config(crtc, pipe_config);
-
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 

@@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 							 pipe_config);
 	}
 
+	if (HAS_IPS(dev_priv))
+		hsw_compute_ips_config(intel_crtc, pipe_config);
+
 	return ret;
 }
 

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

index 61641d479b93..1f5cd572a7ff 100644
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		 * completely disable it.
 		 */
 		pipe_config->ips_force_disable = enable;
-		if (pipe_config->ips_enabled == enable)
-			pipe_config->base.connectors_changed = true;
 	}
 
 	if (IS_HASWELL(dev_priv)) {

Signature · 6 lines

2.15.0

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

Quoted 79 lines

ips_enabled was used as a variable of whether IPS can be enabled or not, but should be used to test whether IPS is actually enabled.

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

---
 drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 2 files changed, 35 insertions(+), 42 deletions(-)

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

index 3b3dec1e6640..8283e80597bd 100644
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc_state->ips_enabled)
 		return;
 
 	/*

@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->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
-	 * versa.
-	 */
-	hsw_enable_ips(new_crtc_state);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes

@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	intel_check_pch_fifo_underruns(dev_priv);
 }
 
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
 static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
-			  const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);

@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
-	 * So diasble underrun reporting before all the planes get disabled.
-	 * FIXME: Need to fix the logic to work when we turn off all planes
-	 * but leave the pipe running.
+	 * So disable underrun reporting before all the planes get disabled.
 	 */
 	if (IS_GEN2(dev_priv))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
-	/*
-	 * 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
-	 * versa.
-	 */
-	hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;

drivers/gpu/drm/i915/intel_display.c intel_pre_disable_primary(struct drm_crtc *crtc, · line 5000

Quoted 3 lines

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

-
-	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+	hsw_disable_ips(to_intel_crtc_state(crtc->state));

BTW thinking about the lack of readout on BDW. In case the BIOS ever enables IPS, I think we should sanitize ips_enabled to true on BDW. Or maybe just have the redaout code always set ips_enabled=true on BDW? That way the first modeset will try to turn it off regardless of whether it was enabled or not.

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) · line 5033

Quoted 9 lines

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

 	/*
 	 * Vblank time updates from the shadow to live plane control register

@@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->update_wm_post && pipe_config->base.active)
 		intel_update_watermarks(crtc);
 
+	if (!old_crtc_state->ips_enabled ||
+	    needs_modeset(&old_crtc_state->base) ||

Shoudldn't that be needs_modeset(new_crtc_state)?

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) · line 5034

Quoted 1 line

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

+	    pipe_config->update_pipe)

Why do we have to check that?

Quoted 44 lines

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

+		hsw_enable_ips(pipe_config);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),

@@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 
+	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+		hsw_disable_ips(old_crtc_state);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(old_intel_state,

@@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 			to_intel_plane_state(old_pri_state);
 
 		intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
-		if (old_primary_state->base.visible &&
+		/*
+		 * Gen2 reports pipe underruns whenever all planes are disabled.
+		 * So disable underrun reporting before all the planes get disabled.
+		 */
+		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
 		    (modeset || !primary_state->base.visible))
-			intel_pre_disable_primary(&crtc->base, old_crtc_state);
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
 	}
 
 	/*

@@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 				     struct intel_crtc_state *pipe_config)
 {
+	u8 visible_planes;
+
 	if (pipe_config->ips_force_disable)
 		return false;
 
 	if (pipe_config->pipe_bpp > 24)
 		return false;
 

drivers/gpu/drm/i915/intel_display.c static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, · line 6233

Quoted 10 lines

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

+	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
+
+	/*
+	 * 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
+	 * versa.
+	 */
+	if (visible_planes != BIT(PLANE_PRIMARY))
+		return false;

I think that should be '(active_planes & PRIMARY) == 0'. As in we don't care what the other planes are doing, IPS just tracks the primary state (for the time being).

What's missing from this patch is changing the cdclk vs. ips workaround to not assume that cdclk can be increased to accomodate ips.

Quoted 36 lines

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

+
 	/* HSW can handle pixel rate up to cdclk? */
 	if (IS_HASWELL(dev_priv))
 		return true;

@@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	intel_crtc_compute_pixel_rate(pipe_config);
 
-	if (HAS_IPS(dev_priv))
-		hsw_compute_ips_config(crtc, pipe_config);
-
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 

@@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 							 pipe_config);
 	}
 
+	if (HAS_IPS(dev_priv))
+		hsw_compute_ips_config(intel_crtc, pipe_config);
+
 	return ret;
 }
 

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

index 61641d479b93..1f5cd572a7ff 100644
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		 * completely disable it.
 		 */
 		pipe_config->ips_force_disable = enable;
-		if (pipe_config->ips_enabled == enable)
-			pipe_config->base.connectors_changed = true;
 	}
 

Quoted 8 lines

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

 	if (IS_HASWELL(dev_priv)) {

Signature · 6 lines

2.15.0

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

Op 17-11-17 om 16:52 schreef Ville Syrjälä:

Quoted 80 lines · Maarten Lankhorst

Quoted 79 lines

ips_enabled was used as a variable of whether IPS can be enabled or not, but should be used to test whether IPS is actually enabled.

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

---
 drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 2 files changed, 35 insertions(+), 42 deletions(-)

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

index 3b3dec1e6640..8283e80597bd 100644
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc_state->ips_enabled)
 		return;
 
 	/*

@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->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
-	 * versa.
-	 */
-	hsw_enable_ips(new_crtc_state);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes

@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	intel_check_pch_fifo_underruns(dev_priv);
 }
 
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
 static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
-			  const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);

@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
-	 * So diasble underrun reporting before all the planes get disabled.
-	 * FIXME: Need to fix the logic to work when we turn off all planes
-	 * but leave the pipe running.
+	 * So disable underrun reporting before all the planes get disabled.
 	 */
 	if (IS_GEN2(dev_priv))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
-	/*
-	 * 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
-	 * versa.
-	 */
-	hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c intel_pre_disable_primary(struct drm_crtc *crtc, · line 5000

Quoted 3 lines

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

-
-	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+	hsw_disable_ips(to_intel_crtc_state(crtc->state));

BTW thinking about the lack of readout on BDW. In case the BIOS ever enables IPS, I think we should sanitize ips_enabled to true on BDW. Or maybe just have the redaout code always set ips_enabled=true on BDW? That way the first modeset will try to turn it off regardless of whether it was enabled or not.

Yes, good idea. :)

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) · line 5033

Quoted 9 lines

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

 	/*
 	 * Vblank time updates from the shadow to live plane control register

@@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->update_wm_post && pipe_config->base.active)
 		intel_update_watermarks(crtc);
 
+	if (!old_crtc_state->ips_enabled ||
+	    needs_modeset(&old_crtc_state->base) ||

Shoudldn't that be needs_modeset(new_crtc_state)?

Yes, oops..

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) · line 5034

Quoted 1 line

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

+	    pipe_config->update_pipe)

Why do we have to check that?

In case we take over the hw state from bios..

If we default ips_enabled to true on BDW and read out on HSW again, then this check will make IPS work, regardless of bios setting..

Quoted 45 lines

Quoted 45 lines

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

+		hsw_enable_ips(pipe_config);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),

@@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 
+	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+		hsw_disable_ips(old_crtc_state);
+
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			intel_atomic_get_new_plane_state(old_intel_state,

@@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 			to_intel_plane_state(old_pri_state);
 
 		intel_fbc_pre_update(crtc, pipe_config, primary_state);
-
-		if (old_primary_state->base.visible &&
+		/*
+		 * Gen2 reports pipe underruns whenever all planes are disabled.
+		 * So disable underrun reporting before all the planes get disabled.
+		 */
+		if (IS_GEN2(dev_priv) && old_primary_state->base.visible &&
 		    (modeset || !primary_state->base.visible))
-			intel_pre_disable_primary(&crtc->base, old_crtc_state);
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
 	}
 
 	/*

@@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 				     struct intel_crtc_state *pipe_config)
 {
+	u8 visible_planes;
+
 	if (pipe_config->ips_force_disable)
 		return false;
 
 	if (pipe_config->pipe_bpp > 24)
 		return false;
 

Quoted 13 lines

drivers/gpu/drm/i915/intel_display.c static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, · line 6233

Quoted 10 lines

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

+	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
+
+	/*
+	 * 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
+	 * versa.
+	 */
+	if (visible_planes != BIT(PLANE_PRIMARY))
+		return false;

I think that should be '(active_planes & PRIMARY) == 0'. As in we don't care what the other planes are doing, IPS just tracks the primary state (for the time being).

Oops, faultily assumed from the comment that IPS only worked with the primary plane or sprite plane, not both. I'll correct it.

Quoted 2 lines

What's missing from this patch is changing the cdclk vs. ips workaround to not assume that cdclk can be increased to accomodate ips.

Hm true, I missed that..

Will send a new version in a bit.

Quoted 44 lines

Quoted 44 lines

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

+
 	/* HSW can handle pixel rate up to cdclk? */
 	if (IS_HASWELL(dev_priv))
 		return true;

@@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	intel_crtc_compute_pixel_rate(pipe_config);
 
-	if (HAS_IPS(dev_priv))
-		hsw_compute_ips_config(crtc, pipe_config);
-
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 

@@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 							 pipe_config);
 	}
 
+	if (HAS_IPS(dev_priv))
+		hsw_compute_ips_config(intel_crtc, pipe_config);
+
 	return ret;
 }
 

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

index 61641d479b93..1f5cd572a7ff 100644
@@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		 * completely disable it.
 		 */
 		pipe_config->ips_force_disable = enable;
-		if (pipe_config->ips_enabled == enable)
-			pipe_config->base.connectors_changed = true;
 	}
 
 	if (IS_HASWELL(dev_priv)) {

Signature · 6 lines

2.15.0

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

Quoted 105 lines

Op 17-11-17 om 16:52 schreef Ville Syrjälä:

Quoted 80 lines · Maarten Lankhorst

Quoted 79 lines

ips_enabled was used as a variable of whether IPS can be enabled or not, but should be used to test whether IPS is actually enabled.

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

---
 drivers/gpu/drm/i915/intel_display.c  | 75 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 2 files changed, 35 insertions(+), 42 deletions(-)

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

index 3b3dec1e6640..8283e80597bd 100644
@@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc_state->ips_enabled)
 		return;
 
 	/*

@@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->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
-	 * versa.
-	 */
-	hsw_enable_ips(new_crtc_state);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes

@@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc,
 	intel_check_pch_fifo_underruns(dev_priv);
 }
 
-/* FIXME move all this to pre_plane_update() with proper state tracking */
+/* FIXME get rid of this and use pre_plane_update */
 static void
-intel_pre_disable_primary(struct drm_crtc *crtc,
-			  const struct intel_crtc_state *old_crtc_state)
+intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);

@@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc,
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
-	 * So diasble underrun reporting before all the planes get disabled.
-	 * FIXME: Need to fix the logic to work when we turn off all planes
-	 * but leave the pipe running.
+	 * So disable underrun reporting before all the planes get disabled.
 	 */
 	if (IS_GEN2(dev_priv))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
-	/*
-	 * 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
-	 * versa.
-	 */
-	hsw_disable_ips(old_crtc_state);
-}
-
-/* FIXME get rid of this and use pre_plane_update */
-static void
-intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c intel_pre_disable_primary(struct drm_crtc *crtc, · line 5000

Quoted 3 lines

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

-
-	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
+	hsw_disable_ips(to_intel_crtc_state(crtc->state));

BTW thinking about the lack of readout on BDW. In case the BIOS ever enables IPS, I think we should sanitize ips_enabled to true on BDW. Or maybe just have the redaout code always set ips_enabled=true on BDW? That way the first modeset will try to turn it off regardless of whether it was enabled or not.

Yes, good idea. :)

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) · line 5033

Quoted 9 lines

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

 	/*
 	 * Vblank time updates from the shadow to live plane control register

@@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->update_wm_post && pipe_config->base.active)
 		intel_update_watermarks(crtc);
 
+	if (!old_crtc_state->ips_enabled ||
+	    needs_modeset(&old_crtc_state->base) ||

Shoudldn't that be needs_modeset(new_crtc_state)?

Yes, oops..

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) · line 5034

Quoted 1 line

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

+	    pipe_config->update_pipe)

Why do we have to check that?

In case we take over the hw state from bios..

Quoted 2 lines

If we default ips_enabled to true on BDW and read out on HSW again, then this check will make IPS work, regardless of bios setting..

Why aren't we just checking something like
if ((!old_state->ips_enabled || needs_modeset()) && new_state->ips_enabled) ?

IPS state may have to change whenever plane visibility changes, so I don't see any reson to involve ->update_pipe in these decision.

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

---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

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

index 8283e80597bd..38a1cdb3dbb2 100644
@@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		intel_fbc_post_update(crtc);
 
 		if (primary_state->base.visible &&
-		    (needs_modeset(&pipe_config->base) ||
+		    (pipe_config->disable_cxsr ||
 		     !old_primary_state->base.visible))
 			intel_post_enable_primary(&crtc->base, pipe_config);
 	}

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

 		hsw_disable_ips(old_crtc_state);
 
 	if (old_pri_state) {

@@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
 
 	/*
-	 * FIXME IPS should be fine as long as one plane is

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

 	/* HSW can handle pixel rate up to cdclk? */

Signature · 6 lines

2.15.0

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

Quoted 22 lines

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

---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

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

index 8283e80597bd..38a1cdb3dbb2 100644
@@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		intel_fbc_post_update(crtc);
 
 		if (primary_state->base.visible &&
-		    (needs_modeset(&pipe_config->base) ||
+		    (pipe_config->disable_cxsr ||
 		     !old_primary_state->base.visible))
 			intel_post_enable_primary(&crtc->base, pipe_config);
 	}

@@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 

drivers/gpu/drm/i915/intel_display.c static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, · line 5067

Quoted 2 lines

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

-	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)

What does IPS have to do with cxsr?

Quoted 8 lines

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

 		hsw_disable_ips(old_crtc_state);
 
 	if (old_pri_state) {

@@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
 
 	/*
-	 * FIXME IPS should be fine as long as one plane is

drivers/gpu/drm/i915/intel_display.c static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, · line 6232

Quoted 10 lines

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

-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
+	 * IPS should be fine as long as one plane is enabled, but
+	 * temporarily disable it when when going from primary only
+	 * to sprite only and vice versa.
 	 */
-	if (visible_planes != BIT(PLANE_PRIMARY))
+	if (hweight32(visible_planes) != 1)
 		return false;

That should just be
if (active_planes == 0)

	return false;

assuming we have no problems with the toggling between primary only and sprite only.

I can't recall how the cursor affecrs IPS. But I think IPS should work as long as any plane (including the cursor) is enabled.

Op 17-11-17 om 16:55 schreef Ville Syrjälä:

Quoted 23 lines · Maarten Lankhorst

Quoted 22 lines

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

---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

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

index 8283e80597bd..38a1cdb3dbb2 100644
@@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		intel_fbc_post_update(crtc);
 
 		if (primary_state->base.visible &&
-		    (needs_modeset(&pipe_config->base) ||
+		    (pipe_config->disable_cxsr ||
 		     !old_primary_state->base.visible))
 			intel_post_enable_primary(&crtc->base, pipe_config);
 	}

@@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 

Quoted 3 lines

drivers/gpu/drm/i915/intel_display.c static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, · line 5067

Quoted 2 lines

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

-	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
+	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)

What does IPS have to do with cxsr?

Nothing, laziness. disable cxsr is set when planes get disabled.

Quoted 22 lines

Quoted 8 lines

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

 		hsw_disable_ips(old_crtc_state);
 
 	if (old_pri_state) {

@@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
 
 	/*
-	 * FIXME IPS should be fine as long as one plane is

drivers/gpu/drm/i915/intel_display.c static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, · line 6232

Quoted 10 lines

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

-	 * enabled, but in practice it seems to have problems
-	 * when going from primary only to sprite only and vice
-	 * versa.
+	 * IPS should be fine as long as one plane is enabled, but
+	 * temporarily disable it when when going from primary only
+	 * to sprite only and vice versa.
 	 */
-	if (visible_planes != BIT(PLANE_PRIMARY))
+	if (hweight32(visible_planes) != 1)
 		return false;

That should just be
if (active_planes == 0)

	return false;

Quoted 5 lines

assuming we have no problems with the toggling between primary only and sprite only.

I can't recall how the cursor affecrs IPS. But I think IPS should work as long as any plane (including the cursor) is enabled.

But cursor visibility can change without the full atomic commit, so it's too unreliable to take into account with the calculations.

What happens when it doesn't work well? Would it be caught by any tests?

Small fixes for IPS, and then we flip the switch! :-)

Maarten Lankhorst (3):
drm/i915: Make ips_enabled a property depending on whether IPS is
enabled.
drm/i915: Enable IPS for sprite plane
drm/i915: Re-enable fastboot by default

 drivers/gpu/drm/i915/i915_params.h    |  2 +-
 drivers/gpu/drm/i915/intel_display.c  | 76 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_pipe_crc.c |  2 -
 3 files changed, 36 insertions(+), 44 deletions(-)

Signature · 6 lines

2.15.0

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