[PATCH v2 1/6] drm/i915: Update watermark state correctly in sanitize_watermarks

[PATCH v2 1/6] drm/i915: Update watermark state correctly in sanitize_watermarks

[Maarten Lankhorst]

We no longer use intel_crtc->wm.active for watermarks any more,
which was incorrect. But this uncovered a bug in sanitize_watermarks(),
which meant that we wrote the correct watermarks, but the next
update would still use the wrong hw watermarks for calculating.
This caused all further updates to fail with -EINVAL and the
log would reveal an error like the one below:

[   10.043902] [drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
[   10.043960] [drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
[   10.044030] [drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: b6b178a77210 ("drm/i915: Calculate ironlake intermediate watermarks correctly, v2.")
Cc: stable@vger.kernel.org #v4.8+
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84817ccc5305..17665ee06c9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14442,6 +14442,8 @@ static void sanitize_watermarks(struct drm_device *dev)
 
 		cs->wm.need_postvbl_update = true;
 		dev_priv->display.optimize_watermarks(intel_state, cs);
+
+		to_intel_crtc_state(crtc->state)->wm = cs->wm;
 	}
 
 put_state:
-- 
2.15.0

[PATCH v2 1/6] drm/i915: Update watermark state correctly in sanitize_watermarks

[Maarten Lankhorst]

We no longer use intel_crtc->wm.active for watermarks any more,
which was incorrect. But this uncovered a bug in sanitize_watermarks(),
which meant that we wrote the correct watermarks, but the next
update would still use the wrong hw watermarks for calculating.
This caused all further updates to fail with -EINVAL and the
log would reveal an error like the one below:

[   10.043902] [drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
[   10.043960] [drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
[   10.044030] [drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: b6b178a77210 ("drm/i915: Calculate ironlake intermediate watermarks correctly, v2.")
Cc: stable@vger.kernel.org #v4.8+
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84817ccc5305..17665ee06c9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14442,6 +14442,8 @@ static void sanitize_watermarks(struct drm_device *dev)
 
 		cs->wm.need_postvbl_update = true;
 		dev_priv->display.optimize_watermarks(intel_state, cs);
+
+		to_intel_crtc_state(crtc->state)->wm = cs->wm;
 	}
 
 put_state:
-- 
2.15.0

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

[PATCH v2 2/6] drm/i915: Handle adjust better in intel_pipe_config_compare

[Maarten Lankhorst]

Some parameters use CHECK_I, but should really use CHECK_BOOL.
Convert all boools to use CHECK_BOOL, and also create a
CHECK_BOOL_INCOMPLETE for has_audio and has_infoframe, we
cannot currently check whether the inherited infoframes and
audio are set up correctly.

Also do not check ips_enabled, it will not be enabled if the
primary plane isn't. It's only a flag indicating IPS can be
enabled. This was actually causing a failure on f2-hsw-4010u
with kms_atomic_transitions.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17665ee06c9a..f5933b0719c9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11076,6 +11076,9 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 			  bool adjust)
 {
 	bool ret = true;
+	bool fixup_inherited = adjust &&
+		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
+		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
 
 #define PIPE_CONF_CHECK_X(name)	\
 	if (current_config->name != pipe_config->name) { \
@@ -11095,6 +11098,26 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 		ret = false; \
 	}
 
+#define PIPE_CONF_CHECK_BOOL(name)	\
+	if (current_config->name != pipe_config->name) { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "(expected %s, found %s)\n", \
+			  yesno(current_config->name), \
+			  yesno(pipe_config->name)); \
+		ret = false; \
+	}
+
+#define PIPE_CONF_CHECK_BOOL_INCOMPLETE(name) \
+	if (!fixup_inherited || (!current_config->name && !pipe_config->name)) { \
+		PIPE_CONF_CHECK_BOOL(name); \
+	} else { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "unable to verify whether state matches exactly, forcing modeset (expected %s, found %s)\n", \
+			  yesno(current_config->name), \
+			  yesno(pipe_config->name)); \
+		ret = false; \
+	}
+
 #define PIPE_CONF_CHECK_P(name)	\
 	if (current_config->name != pipe_config->name) { \
 		pipe_config_err(adjust, __stringify(name), \
@@ -11180,7 +11203,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 
 	PIPE_CONF_CHECK_I(cpu_transcoder);
 
-	PIPE_CONF_CHECK_I(has_pch_encoder);
+	PIPE_CONF_CHECK_BOOL(has_pch_encoder);
 	PIPE_CONF_CHECK_I(fdi_lanes);
 	PIPE_CONF_CHECK_M_N(fdi_m_n);
 
@@ -11212,17 +11235,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
 
 	PIPE_CONF_CHECK_I(pixel_multiplier);
-	PIPE_CONF_CHECK_I(has_hdmi_sink);
+	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
 	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		PIPE_CONF_CHECK_I(limited_color_range);
+		PIPE_CONF_CHECK_BOOL(limited_color_range);
 
-	PIPE_CONF_CHECK_I(hdmi_scrambling);
-	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
-	PIPE_CONF_CHECK_I(has_infoframe);
-	PIPE_CONF_CHECK_I(ycbcr420);
+	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
+	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
+	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
+	PIPE_CONF_CHECK_BOOL(ycbcr420);
 
-	PIPE_CONF_CHECK_I(has_audio);
+	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
 
 	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
 			      DRM_MODE_FLAG_INTERLACE);
@@ -11248,7 +11271,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 		PIPE_CONF_CHECK_I(pipe_src_w);
 		PIPE_CONF_CHECK_I(pipe_src_h);
 
-		PIPE_CONF_CHECK_I(pch_pfit.enabled);
+		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);
 		if (current_config->pch_pfit.enabled) {
 			PIPE_CONF_CHECK_X(pch_pfit.pos);
 			PIPE_CONF_CHECK_X(pch_pfit.size);
@@ -11258,11 +11281,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 		PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate);
 	}
 
-	/* BDW+ don't expose a synchronous way to read the state */
-	if (IS_HASWELL(dev_priv))
-		PIPE_CONF_CHECK_I(ips_enabled);
-
-	PIPE_CONF_CHECK_I(double_wide);
+	PIPE_CONF_CHECK_BOOL(double_wide);
 
 	PIPE_CONF_CHECK_P(shared_dpll);
 	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
@@ -11300,6 +11319,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
+#undef PIPE_CONF_CHECK_BOOL
+#undef PIPE_CONF_CHECK_BOOL_INCOMPLETE
 #undef PIPE_CONF_CHECK_P
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
-- 
2.15.0

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

[PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c   |  4 ++--
 drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index b8315bca852b..c6373febf617 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
 	 */
 	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
 	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
-		hsw_disable_ips(intel_crtc);
+		hsw_disable_ips(intel_crtc, intel_crtc_state);
 		reenable_ips = true;
 	}
 
@@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
 	i9xx_load_luts(crtc_state);
 
 	if (reenable_ips)
-		hsw_enable_ips(intel_crtc);
+		hsw_enable_ips(intel_crtc, intel_crtc_state);
 }
 
 static void bdw_load_degamma_lut(struct drm_crtc_state *state)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f5933b0719c9..13b372e4f06e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-void hsw_enable_ips(struct intel_crtc *crtc)
+void hsw_enable_ips(struct intel_crtc *crtc, 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;
 
 	/*
@@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	}
 }
 
-void hsw_disable_ips(struct intel_crtc *crtc)
+void hsw_disable_ips(struct intel_crtc *crtc, 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;
 
 	assert_plane_enabled(dev_priv, crtc->plane);
@@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
  * completely hide the primary plane.
  */
 static void
-intel_post_enable_primary(struct drm_crtc *crtc)
+intel_post_enable_primary(struct drm_crtc *crtc,
+			  const struct intel_crtc_state *new_crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 * when going from primary only to sprite only and vice
 	 * versa.
 	 */
-	hsw_enable_ips(intel_crtc);
+	hsw_enable_ips(intel_crtc, new_crtc_state);
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
@@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 
 /* FIXME move all this to pre_plane_update() with proper state tracking */
 static void
-intel_pre_disable_primary(struct drm_crtc *crtc)
+intel_pre_disable_primary(struct drm_crtc *crtc,
+			  const struct intel_crtc_state *old_crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	 * when going from primary only to sprite only and vice
 	 * versa.
 	 */
-	hsw_disable_ips(intel_crtc);
+	hsw_disable_ips(intel_crtc, old_crtc_state);
 }
 
 /* FIXME get rid of this and use pre_plane_update */
@@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 
-	intel_pre_disable_primary(crtc);
+	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
 
 	/*
 	 * Vblank time updates from the shadow to live plane control register
@@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		if (primary_state->base.visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->base.visible))
-			intel_post_enable_primary(&crtc->base);
+			intel_post_enable_primary(&crtc->base, pipe_config);
 	}
 }
 
@@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 
 		if (old_primary_state->base.visible &&
 		    (modeset || !primary_state->base.visible))
-			intel_pre_disable_primary(&crtc->base);
+			intel_pre_disable_primary(&crtc->base, old_crtc_state);
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cddd96b24878..82c33d9cd466 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 	}
 
  out:
-	hsw_enable_ips(intel_crtc);
+	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
 	return ret;
 }
 
@@ -3974,11 +3974,11 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 			return ret;
 	}
 
-	hsw_disable_ips(intel_crtc);
+	hsw_disable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
 
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
 			       buf | DP_TEST_SINK_START) < 0) {
-		hsw_enable_ips(intel_crtc);
+		hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
 		return -EIO;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 00b488688042..2f8b8b2a443b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1490,8 +1490,8 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
 int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
 
 bool intel_crtc_active(struct intel_crtc *crtc);
-void hsw_enable_ips(struct intel_crtc *crtc);
-void hsw_disable_ips(struct intel_crtc *crtc);
+void hsw_enable_ips(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state);
+void hsw_disable_ips(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state);
 enum intel_display_power_domain intel_port_to_power_domain(enum port port);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_state *pipe_config);
-- 
2.15.0

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

[PATCH v2 4/6] drm/i915: Handle ips_enabled in fastset

[Maarten Lankhorst]

pre_plane_disable and post_plane_enable handle set ips correctly,
but if there is no modeset and the ips_enabled value changes
because of force disabling for crc, or hw state readout, then we
don't toggle ips correctly. Handle this special case, which prevents
us from having to do a full modeset when collecting pipe crc.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 13b372e4f06e..3af1e3f74dbb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5037,6 +5037,10 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->base.visible))
 			intel_post_enable_primary(&crtc->base, pipe_config);
+		else if (pipe_config->update_pipe && pipe_config->ips_enabled &&
+			 old_primary_state->base.visible && primary_state->base.visible)
+			/* IPS turned on after fastset or CRC collection disable. */
+			hsw_enable_ips(crtc, pipe_config);
 	}
 }
 
@@ -5066,6 +5070,10 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 		if (old_primary_state->base.visible &&
 		    (modeset || !primary_state->base.visible))
 			intel_pre_disable_primary(&crtc->base, old_crtc_state);
+		else if (pipe_config->update_pipe && !pipe_config->ips_enabled &&
+			 old_primary_state->base.visible && primary_state->base.visible)
+			/* IPS turned off for CRC, disable it. */
+			hsw_disable_ips(crtc, old_crtc_state);
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 899839f2f7c6..cb92befc16d7 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -542,7 +542,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		 */
 		pipe_config->ips_force_disable = enable;
 		if (pipe_config->ips_enabled == enable)
-			pipe_config->base.connectors_changed = true;
+			pipe_config->base.mode_changed = true;
 	}
 
 	if (IS_HASWELL(dev_priv)) {
-- 
2.15.0

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

[PATCH v2 5/6] drm/i915: Enable FIFO underrun reporting after initial fastset, v2.

[Maarten Lankhorst]

The firmware may have set up the pipe correctly, but the FIFO
underrun and CRC interrupts are likely not enabled.

This resulted in debugfs_test.read_all_entries failing on haswell,
because of a timeout when reading the crc debugfs entry.

Solve this by enabling FIFO underrun reporting after the initial
fastset, which lets interrupts be generated as expected.

Changes since v1:
- Always enable CPU FIFO underrun reporting for >GEN2,
  and handle GEN2 correctly.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3af1e3f74dbb..58bce253f4a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12905,6 +12905,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
 				     struct drm_crtc_state *old_crtc_state)
 {
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_crtc_state->state);
@@ -12912,6 +12913,17 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
 		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
 
 	intel_pipe_update_end(new_crtc_state);
+
+	if (new_crtc_state->update_pipe &&
+	    !needs_modeset(&new_crtc_state->base) &&
+	    old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) {
+		if (!IS_GEN2(dev_priv) ||
+		    new_crtc_state->active_planes & BIT(PLANE_PRIMARY))
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
+
+		if (new_crtc_state->has_pch_encoder && !HAS_DDI(dev_priv))
+			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
+	}
 }
 
 /**
-- 
2.15.0

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

[PATCH v2 6/6] drm/i915: Re-enable fastboot by default

[Maarten Lankhorst]

This fix was originally reverted because it left a chromebook pixel
black, and no immediate fix was available. This has been fixed in the
meantime.

Rather than trying to remove the parameter, set it to default to true
for now, so we can always back out if required.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Testcase: kms_panel_fitting
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c7292268ed43..b99cb58801e6 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -57,7 +57,7 @@
 	param(bool, alpha_support, IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT)) \
 	param(bool, enable_cmd_parser, true) \
 	param(bool, enable_hangcheck, true) \
-	param(bool, fastboot, false) \
+	param(bool, fastboot, true) \
 	param(bool, prefault_disable, false) \
 	param(bool, load_detect_test, false) \
 	param(bool, force_reset_modeset_test, false) \
-- 
2.15.0

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

✓ Fi.CI.BAT: success for series starting with [v2,1/6] drm/i915: Update watermark state correctly in sanitize_watermarks

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915: Update watermark state correctly in sanitize_watermarks
URL   : https://patchwork.freedesktop.org/series/33531/
State : success

== Summary ==

Series 33531v1 series starting with [v2,1/6] drm/i915: Update watermark state correctly in sanitize_watermarks
https://patchwork.freedesktop.org/api/1.0/series/33531/revisions/1/mbox/

Test gem_sync:
        Subgroup basic-store-all:
                fail       -> PASS       (fi-ivb-3520m) fdo#100007

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:442s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:382s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:524s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:275s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:505s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:493s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:485s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:421s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:262s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:426s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:439s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:430s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:482s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:518s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:538s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:571s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:546s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:559s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:497s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:465s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:555s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:426s
Blacklisted hosts:
fi-cfl-s         total:289  pass:244  dwarn:12  dfail:1   fail:0   skip:32  time:541s
fi-cnl-y         total:74   pass:57   dwarn:0   dfail:0   fail:0   skip:16 
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:491s

65dc54b704d3ee0486f9f5b11f00c28973f783a2 drm-tip: 2017y-11m-09d-08h-53m-46s UTC integration manifest
2cbf97189bf0 drm/i915: Re-enable fastboot by default
c2b1fb174845 drm/i915: Enable FIFO underrun reporting after initial fastset, v2.
2bdfd24af317 drm/i915: Handle ips_enabled in fastset
872225cdccf0 drm/i915: Pass crtc_state to ips toggle functions
4ff8514f60a0 drm/i915: Handle adjust better in intel_pipe_config_compare
b5817941ffa5 drm/i915: Update watermark state correctly in sanitize_watermarks

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7041/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/6] drm/i915: Handle adjust better in intel_pipe_config_compare

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 05:24:54PM +0100, Maarten Lankhorst wrote:
> Some parameters use CHECK_I, but should really use CHECK_BOOL.
> Convert all boools to use CHECK_BOOL, and also create a
> CHECK_BOOL_INCOMPLETE for has_audio and has_infoframe, we
> cannot currently check whether the inherited infoframes and
> audio are set up correctly.

Maybe split the regular BOOL think into prep patch so that we don't
introduce functional changes with it?

> 
> Also do not check ips_enabled, it will not be enabled if the
> primary plane isn't. It's only a flag indicating IPS can be
> enabled. This was actually causing a failure on f2-hsw-4010u
> with kms_atomic_transitions.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 49 +++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17665ee06c9a..f5933b0719c9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11076,6 +11076,9 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  			  bool adjust)
>  {
>  	bool ret = true;
> +	bool fixup_inherited = adjust &&
> +		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> +		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
>  
>  #define PIPE_CONF_CHECK_X(name)	\
>  	if (current_config->name != pipe_config->name) { \
> @@ -11095,6 +11098,26 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		ret = false; \
>  	}
>  
> +#define PIPE_CONF_CHECK_BOOL(name)	\
> +	if (current_config->name != pipe_config->name) { \
> +		pipe_config_err(adjust, __stringify(name), \
> +			  "(expected %s, found %s)\n", \
> +			  yesno(current_config->name), \
> +			  yesno(pipe_config->name)); \
> +		ret = false; \
> +	}
> +
> +#define PIPE_CONF_CHECK_BOOL_INCOMPLETE(name) \
> +	if (!fixup_inherited || (!current_config->name && !pipe_config->name)) { \
> +		PIPE_CONF_CHECK_BOOL(name); \
> +	} else { \
> +		pipe_config_err(adjust, __stringify(name), \
> +			  "unable to verify whether state matches exactly, forcing modeset (expected %s, found %s)\n", \
> +			  yesno(current_config->name), \
> +			  yesno(pipe_config->name)); \
> +		ret = false; \
> +	}
> +
>  #define PIPE_CONF_CHECK_P(name)	\
>  	if (current_config->name != pipe_config->name) { \
>  		pipe_config_err(adjust, __stringify(name), \
> @@ -11180,7 +11203,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  
>  	PIPE_CONF_CHECK_I(cpu_transcoder);
>  
> -	PIPE_CONF_CHECK_I(has_pch_encoder);
> +	PIPE_CONF_CHECK_BOOL(has_pch_encoder);
>  	PIPE_CONF_CHECK_I(fdi_lanes);
>  	PIPE_CONF_CHECK_M_N(fdi_m_n);
>  
> @@ -11212,17 +11235,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>  
>  	PIPE_CONF_CHECK_I(pixel_multiplier);
> -	PIPE_CONF_CHECK_I(has_hdmi_sink);
> +	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		PIPE_CONF_CHECK_I(limited_color_range);
> +		PIPE_CONF_CHECK_BOOL(limited_color_range);
>  
> -	PIPE_CONF_CHECK_I(hdmi_scrambling);
> -	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
> -	PIPE_CONF_CHECK_I(has_infoframe);
> -	PIPE_CONF_CHECK_I(ycbcr420);
> +	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
> +	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
> +	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> +	PIPE_CONF_CHECK_BOOL(ycbcr420);
>  
> -	PIPE_CONF_CHECK_I(has_audio);
> +	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>  
>  	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
>  			      DRM_MODE_FLAG_INTERLACE);
> @@ -11248,7 +11271,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		PIPE_CONF_CHECK_I(pipe_src_w);
>  		PIPE_CONF_CHECK_I(pipe_src_h);
>  
> -		PIPE_CONF_CHECK_I(pch_pfit.enabled);
> +		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);
>  		if (current_config->pch_pfit.enabled) {
>  			PIPE_CONF_CHECK_X(pch_pfit.pos);
>  			PIPE_CONF_CHECK_X(pch_pfit.size);
> @@ -11258,11 +11281,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate);
>  	}
>  
> -	/* BDW+ don't expose a synchronous way to read the state */
> -	if (IS_HASWELL(dev_priv))
> -		PIPE_CONF_CHECK_I(ips_enabled);
> -
> -	PIPE_CONF_CHECK_I(double_wide);
> +	PIPE_CONF_CHECK_BOOL(double_wide);
>  
>  	PIPE_CONF_CHECK_P(shared_dpll);
>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> @@ -11300,6 +11319,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
> +#undef PIPE_CONF_CHECK_BOOL
> +#undef PIPE_CONF_CHECK_BOOL_INCOMPLETE
>  #undef PIPE_CONF_CHECK_P
>  #undef PIPE_CONF_CHECK_FLAGS
>  #undef PIPE_CONF_CHECK_CLOCK_FUZZY
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 05:24:55PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  4 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index b8315bca852b..c6373febf617 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>  	 */
>  	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
>  	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> -		hsw_disable_ips(intel_crtc);
> +		hsw_disable_ips(intel_crtc, intel_crtc_state);

I would suggest simplifying more and passing only the state.
The callee can always dig out the crtc if it needs it.

>  		reenable_ips = true;
>  	}
>  
> @@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>  	i9xx_load_luts(crtc_state);
>  
>  	if (reenable_ips)
> -		hsw_enable_ips(intel_crtc);
> +		hsw_enable_ips(intel_crtc, intel_crtc_state);
>  }
>  
>  static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f5933b0719c9..13b372e4f06e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>  	}
>  }
>  
> -void hsw_enable_ips(struct intel_crtc *crtc)
> +void hsw_enable_ips(struct intel_crtc *crtc, 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;
>  
>  	/*
> @@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>  	}
>  }
>  
> -void hsw_disable_ips(struct intel_crtc *crtc)
> +void hsw_disable_ips(struct intel_crtc *crtc, 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;
>  
>  	assert_plane_enabled(dev_priv, crtc->plane);

Hmm. Confusied. Oh right, my plane assert cleanup didn't manage to go in yet :P

> @@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
>   * completely hide the primary plane.
>   */
>  static void
> -intel_post_enable_primary(struct drm_crtc *crtc)
> +intel_post_enable_primary(struct drm_crtc *crtc,
> +			  const struct intel_crtc_state *new_crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	 * when going from primary only to sprite only and vice
>  	 * versa.
>  	 */
> -	hsw_enable_ips(intel_crtc);
> +	hsw_enable_ips(intel_crtc, new_crtc_state);
>  
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> @@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  
>  /* FIXME move all this to pre_plane_update() with proper state tracking */
>  static void
> -intel_pre_disable_primary(struct drm_crtc *crtc)
> +intel_pre_disable_primary(struct drm_crtc *crtc,
> +			  const struct intel_crtc_state *old_crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  	 * when going from primary only to sprite only and vice
>  	 * versa.
>  	 */
> -	hsw_disable_ips(intel_crtc);
> +	hsw_disable_ips(intel_crtc, old_crtc_state);
>  }
>  
>  /* FIXME get rid of this and use pre_plane_update */
> @@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
>  
> -	intel_pre_disable_primary(crtc);
> +	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>  
>  	/*
>  	 * Vblank time updates from the shadow to live plane control register
> @@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		if (primary_state->base.visible &&
>  		    (needs_modeset(&pipe_config->base) ||
>  		     !old_primary_state->base.visible))
> -			intel_post_enable_primary(&crtc->base);
> +			intel_post_enable_primary(&crtc->base, pipe_config);
>  	}
>  }
>  
> @@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  
>  		if (old_primary_state->base.visible &&
>  		    (modeset || !primary_state->base.visible))
> -			intel_pre_disable_primary(&crtc->base);
> +			intel_pre_disable_primary(&crtc->base, old_crtc_state);
>  	}
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cddd96b24878..82c33d9cd466 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  	}
>  
>   out:
> -	hsw_enable_ips(intel_crtc);
> +	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));

crtc->config would seem like the slightly more correct thing to pass
here. It's also easier to grep for when someone sets out to fix this
mess properly.

>  	return ret;
>  }
>  
> @@ -3974,11 +3974,11 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  			return ret;
>  	}
>  
> -	hsw_disable_ips(intel_crtc);
> +	hsw_disable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
>  
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>  			       buf | DP_TEST_SINK_START) < 0) {
> -		hsw_enable_ips(intel_crtc);
> +		hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
>  		return -EIO;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 00b488688042..2f8b8b2a443b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1490,8 +1490,8 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
>  int chv_calc_dpll_params(int refclk, struct dpll *pll_clock);
>  
>  bool intel_crtc_active(struct intel_crtc *crtc);
> -void hsw_enable_ips(struct intel_crtc *crtc);
> -void hsw_disable_ips(struct intel_crtc *crtc);
> +void hsw_enable_ips(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state);
> +void hsw_disable_ips(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state);
>  enum intel_display_power_domain intel_port_to_power_domain(enum port port);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state *pipe_config);
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 5/6] drm/i915: Enable FIFO underrun reporting after initial fastset, v2.

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 05:24:57PM +0100, Maarten Lankhorst wrote:
> The firmware may have set up the pipe correctly, but the FIFO
> underrun and CRC interrupts are likely not enabled.
> 
> This resulted in debugfs_test.read_all_entries failing on haswell,
> because of a timeout when reading the crc debugfs entry.
> 
> Solve this by enabling FIFO underrun reporting after the initial
> fastset, which lets interrupts be generated as expected.
> 
> Changes since v1:
> - Always enable CPU FIFO underrun reporting for >GEN2,
>   and handle GEN2 correctly.

"Correct" is a strong word to use here. I think I should have the
actually correct thing somewhere...

git://github.com/vsyrjala/linux.git gen2_fifo_underrun

Though it seems I haven't managed to write proper commit messages for
it yet.

> 
> Testcase: debugfs_test.read_all_entries
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3af1e3f74dbb..58bce253f4a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12905,6 +12905,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>  				     struct drm_crtc_state *old_crtc_state)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_atomic_state *old_intel_state =
>  		to_intel_atomic_state(old_crtc_state->state);
> @@ -12912,6 +12913,17 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>  		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
>  
>  	intel_pipe_update_end(new_crtc_state);
> +
> +	if (new_crtc_state->update_pipe &&
> +	    !needs_modeset(&new_crtc_state->base) &&
> +	    old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) {
> +		if (!IS_GEN2(dev_priv) ||
> +		    new_crtc_state->active_planes & BIT(PLANE_PRIMARY))
> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> +
> +		if (new_crtc_state->has_pch_encoder && !HAS_DDI(dev_priv))
> +			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);

I quite dislike having so many platform checks in high level
common code. Maybe we should have some kind of
intel_initial_modeset_done() thing or something?

I don't quite see why that !DDI check is here either.

> +	}
>  }
>  
>  /**
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 4/6] drm/i915: Handle ips_enabled in fastset

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 05:24:56PM +0100, Maarten Lankhorst wrote:
> pre_plane_disable and post_plane_enable handle set ips correctly,
> but if there is no modeset and the ips_enabled value changes
> because of force disabling for crc, or hw state readout, then we
> don't toggle ips correctly. Handle this special case, which prevents
> us from having to do a full modeset when collecting pipe crc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 8 ++++++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 13b372e4f06e..3af1e3f74dbb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5037,6 +5037,10 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		    (needs_modeset(&pipe_config->base) ||
>  		     !old_primary_state->base.visible))
>  			intel_post_enable_primary(&crtc->base, pipe_config);
> +		else if (pipe_config->update_pipe && pipe_config->ips_enabled &&

What's with the update_pipe check here? Shouldn't we just check

if (ips off -> ips on)
	enable_ips()

?

> +			 old_primary_state->base.visible && primary_state->base.visible)
> +			/* IPS turned on after fastset or CRC collection disable. */
> +			hsw_enable_ips(crtc, pipe_config);
>  	}
>  }
>  
> @@ -5066,6 +5070,10 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  		if (old_primary_state->base.visible &&
>  		    (modeset || !primary_state->base.visible))
>  			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> +		else if (pipe_config->update_pipe && !pipe_config->ips_enabled &&
> +			 old_primary_state->base.visible && primary_state->base.visible)
> +			/* IPS turned off for CRC, disable it. */
> +			hsw_disable_ips(crtc, old_crtc_state);

same here.

>  	}
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 899839f2f7c6..cb92befc16d7 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -542,7 +542,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
>  		 */
>  		pipe_config->ips_force_disable = enable;
>  		if (pipe_config->ips_enabled == enable)
> -			pipe_config->base.connectors_changed = true;
> +			pipe_config->base.mode_changed = true;
>  	}
>  
>  	if (IS_HASWELL(dev_priv)) {
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

✓ Fi.CI.IGT: success for series starting with [v2,1/6] drm/i915: Update watermark state correctly in sanitize_watermarks

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915: Update watermark state correctly in sanitize_watermarks
URL   : https://patchwork.freedesktop.org/series/33531/
State : success

== Summary ==

shard-hsw        total:2584 pass:1452 dwarn:2   dfail:2   fail:10  skip:1118 time:9359s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7041/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

[Maarten Lankhorst]

Op 09-11-17 om 18:09 schreef Ville Syrjälä:
> On Thu, Nov 09, 2017 at 05:24:55PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
>>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
>>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
>>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>>  4 files changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index b8315bca852b..c6373febf617 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>>  	 */
>>  	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
>>  	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
>> -		hsw_disable_ips(intel_crtc);
>> +		hsw_disable_ips(intel_crtc, intel_crtc_state);
> I would suggest simplifying more and passing only the state.
> The callee can always dig out the crtc if it needs it.
>
>>  		reenable_ips = true;
>>  	}
>>  
>> @@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>>  	i9xx_load_luts(crtc_state);
>>  
>>  	if (reenable_ips)
>> -		hsw_enable_ips(intel_crtc);
>> +		hsw_enable_ips(intel_crtc, intel_crtc_state);
>>  }
>>  
>>  static void bdw_load_degamma_lut(struct drm_crtc_state *state)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f5933b0719c9..13b372e4f06e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>>  	}
>>  }
>>  
>> -void hsw_enable_ips(struct intel_crtc *crtc)
>> +void hsw_enable_ips(struct intel_crtc *crtc, 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;
>>  
>>  	/*
>> @@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>>  	}
>>  }
>>  
>> -void hsw_disable_ips(struct intel_crtc *crtc)
>> +void hsw_disable_ips(struct intel_crtc *crtc, 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;
>>  
>>  	assert_plane_enabled(dev_priv, crtc->plane);
> Hmm. Confusied. Oh right, my plane assert cleanup didn't manage to go in yet :P
>
>> @@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
>>   * completely hide the primary plane.
>>   */
>>  static void
>> -intel_post_enable_primary(struct drm_crtc *crtc)
>> +intel_post_enable_primary(struct drm_crtc *crtc,
>> +			  const struct intel_crtc_state *new_crtc_state)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>>  	 * when going from primary only to sprite only and vice
>>  	 * versa.
>>  	 */
>> -	hsw_enable_ips(intel_crtc);
>> +	hsw_enable_ips(intel_crtc, new_crtc_state);
>>  
>>  	/*
>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>> @@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>>  
>>  /* FIXME move all this to pre_plane_update() with proper state tracking */
>>  static void
>> -intel_pre_disable_primary(struct drm_crtc *crtc)
>> +intel_pre_disable_primary(struct drm_crtc *crtc,
>> +			  const struct intel_crtc_state *old_crtc_state)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>>  	 * when going from primary only to sprite only and vice
>>  	 * versa.
>>  	 */
>> -	hsw_disable_ips(intel_crtc);
>> +	hsw_disable_ips(intel_crtc, old_crtc_state);
>>  }
>>  
>>  /* FIXME get rid of this and use pre_plane_update */
>> @@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	int pipe = intel_crtc->pipe;
>>  
>> -	intel_pre_disable_primary(crtc);
>> +	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>>  
>>  	/*
>>  	 * Vblank time updates from the shadow to live plane control register
>> @@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>  		if (primary_state->base.visible &&
>>  		    (needs_modeset(&pipe_config->base) ||
>>  		     !old_primary_state->base.visible))
>> -			intel_post_enable_primary(&crtc->base);
>> +			intel_post_enable_primary(&crtc->base, pipe_config);
>>  	}
>>  }
>>  
>> @@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>>  
>>  		if (old_primary_state->base.visible &&
>>  		    (modeset || !primary_state->base.visible))
>> -			intel_pre_disable_primary(&crtc->base);
>> +			intel_pre_disable_primary(&crtc->base, old_crtc_state);
>>  	}
>>  
>>  	/*
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index cddd96b24878..82c33d9cd466 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>>  	}
>>  
>>   out:
>> -	hsw_enable_ips(intel_crtc);
>> +	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
> crtc->config would seem like the slightly more correct thing to pass
> here. It's also easier to grep for when someone sets out to fix this
> mess properly.
Considered it, but our crtc->config usage is a mess. I'd rather get rid of the intel_dp_sink_crc code tbh. :/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 06:31:02PM +0100, Maarten Lankhorst wrote:
> Op 09-11-17 om 18:09 schreef Ville Syrjälä:
> > On Thu, Nov 09, 2017 at 05:24:55PM +0100, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
> >>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
> >>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
> >>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
> >>  4 files changed, 20 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >> index b8315bca852b..c6373febf617 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.c
> >> +++ b/drivers/gpu/drm/i915/intel_color.c
> >> @@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
> >>  	 */
> >>  	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
> >>  	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> >> -		hsw_disable_ips(intel_crtc);
> >> +		hsw_disable_ips(intel_crtc, intel_crtc_state);
> > I would suggest simplifying more and passing only the state.
> > The callee can always dig out the crtc if it needs it.
> >
> >>  		reenable_ips = true;
> >>  	}
> >>  
> >> @@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
> >>  	i9xx_load_luts(crtc_state);
> >>  
> >>  	if (reenable_ips)
> >> -		hsw_enable_ips(intel_crtc);
> >> +		hsw_enable_ips(intel_crtc, intel_crtc_state);
> >>  }
> >>  
> >>  static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index f5933b0719c9..13b372e4f06e 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
> >>  	}
> >>  }
> >>  
> >> -void hsw_enable_ips(struct intel_crtc *crtc)
> >> +void hsw_enable_ips(struct intel_crtc *crtc, 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;
> >>  
> >>  	/*
> >> @@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
> >>  	}
> >>  }
> >>  
> >> -void hsw_disable_ips(struct intel_crtc *crtc)
> >> +void hsw_disable_ips(struct intel_crtc *crtc, 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;
> >>  
> >>  	assert_plane_enabled(dev_priv, crtc->plane);
> > Hmm. Confusied. Oh right, my plane assert cleanup didn't manage to go in yet :P
> >
> >> @@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
> >>   * completely hide the primary plane.
> >>   */
> >>  static void
> >> -intel_post_enable_primary(struct drm_crtc *crtc)
> >> +intel_post_enable_primary(struct drm_crtc *crtc,
> >> +			  const struct intel_crtc_state *new_crtc_state)
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >> @@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>  	 * when going from primary only to sprite only and vice
> >>  	 * versa.
> >>  	 */
> >> -	hsw_enable_ips(intel_crtc);
> >> +	hsw_enable_ips(intel_crtc, new_crtc_state);
> >>  
> >>  	/*
> >>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> >> @@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>  
> >>  /* FIXME move all this to pre_plane_update() with proper state tracking */
> >>  static void
> >> -intel_pre_disable_primary(struct drm_crtc *crtc)
> >> +intel_pre_disable_primary(struct drm_crtc *crtc,
> >> +			  const struct intel_crtc_state *old_crtc_state)
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >> @@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >>  	 * when going from primary only to sprite only and vice
> >>  	 * versa.
> >>  	 */
> >> -	hsw_disable_ips(intel_crtc);
> >> +	hsw_disable_ips(intel_crtc, old_crtc_state);
> >>  }
> >>  
> >>  /* FIXME get rid of this and use pre_plane_update */
> >> @@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>  	int pipe = intel_crtc->pipe;
> >>  
> >> -	intel_pre_disable_primary(crtc);
> >> +	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> >>  
> >>  	/*
> >>  	 * Vblank time updates from the shadow to live plane control register
> >> @@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >>  		if (primary_state->base.visible &&
> >>  		    (needs_modeset(&pipe_config->base) ||
> >>  		     !old_primary_state->base.visible))
> >> -			intel_post_enable_primary(&crtc->base);
> >> +			intel_post_enable_primary(&crtc->base, pipe_config);
> >>  	}
> >>  }
> >>  
> >> @@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >>  
> >>  		if (old_primary_state->base.visible &&
> >>  		    (modeset || !primary_state->base.visible))
> >> -			intel_pre_disable_primary(&crtc->base);
> >> +			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> >>  	}
> >>  
> >>  	/*
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index cddd96b24878..82c33d9cd466 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>  	}
> >>  
> >>   out:
> >> -	hsw_enable_ips(intel_crtc);
> >> +	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
> > crtc->config would seem like the slightly more correct thing to pass
> > here. It's also easier to grep for when someone sets out to fix this
> > mess properly.
> Considered it, but our crtc->config usage is a mess. I'd rather get rid of the intel_dp_sink_crc code tbh. :/

I'd rather not spread more ->state uses when I just recently
managed to clean out many of them. We still have some left for
sure, but adding more seems like going backwards.

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

Re: [PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

[Maarten Lankhorst]

Op 09-11-17 om 18:51 schreef Ville Syrjälä:
> On Thu, Nov 09, 2017 at 06:31:02PM +0100, Maarten Lankhorst wrote:
>> Op 09-11-17 om 18:09 schreef Ville Syrjälä:
>>> On Thu, Nov 09, 2017 at 05:24:55PM +0100, Maarten Lankhorst wrote:
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
>>>>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
>>>>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
>>>>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>>>>  4 files changed, 20 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>> index b8315bca852b..c6373febf617 100644
>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>> @@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>>>>  	 */
>>>>  	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
>>>>  	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
>>>> -		hsw_disable_ips(intel_crtc);
>>>> +		hsw_disable_ips(intel_crtc, intel_crtc_state);
>>> I would suggest simplifying more and passing only the state.
>>> The callee can always dig out the crtc if it needs it.
>>>
>>>>  		reenable_ips = true;
>>>>  	}
>>>>  
>>>> @@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
>>>>  	i9xx_load_luts(crtc_state);
>>>>  
>>>>  	if (reenable_ips)
>>>> -		hsw_enable_ips(intel_crtc);
>>>> +		hsw_enable_ips(intel_crtc, intel_crtc_state);
>>>>  }
>>>>  
>>>>  static void bdw_load_degamma_lut(struct drm_crtc_state *state)
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index f5933b0719c9..13b372e4f06e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>>>>  	}
>>>>  }
>>>>  
>>>> -void hsw_enable_ips(struct intel_crtc *crtc)
>>>> +void hsw_enable_ips(struct intel_crtc *crtc, 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;
>>>>  
>>>>  	/*
>>>> @@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>>>>  	}
>>>>  }
>>>>  
>>>> -void hsw_disable_ips(struct intel_crtc *crtc)
>>>> +void hsw_disable_ips(struct intel_crtc *crtc, 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;
>>>>  
>>>>  	assert_plane_enabled(dev_priv, crtc->plane);
>>> Hmm. Confusied. Oh right, my plane assert cleanup didn't manage to go in yet :P
>>>
>>>> @@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
>>>>   * completely hide the primary plane.
>>>>   */
>>>>  static void
>>>> -intel_post_enable_primary(struct drm_crtc *crtc)
>>>> +intel_post_enable_primary(struct drm_crtc *crtc,
>>>> +			  const struct intel_crtc_state *new_crtc_state)
>>>>  {
>>>>  	struct drm_device *dev = crtc->dev;
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> @@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>>>>  	 * when going from primary only to sprite only and vice
>>>>  	 * versa.
>>>>  	 */
>>>> -	hsw_enable_ips(intel_crtc);
>>>> +	hsw_enable_ips(intel_crtc, new_crtc_state);
>>>>  
>>>>  	/*
>>>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>>>> @@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>>>>  
>>>>  /* FIXME move all this to pre_plane_update() with proper state tracking */
>>>>  static void
>>>> -intel_pre_disable_primary(struct drm_crtc *crtc)
>>>> +intel_pre_disable_primary(struct drm_crtc *crtc,
>>>> +			  const struct intel_crtc_state *old_crtc_state)
>>>>  {
>>>>  	struct drm_device *dev = crtc->dev;
>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> @@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>>>>  	 * when going from primary only to sprite only and vice
>>>>  	 * versa.
>>>>  	 */
>>>> -	hsw_disable_ips(intel_crtc);
>>>> +	hsw_disable_ips(intel_crtc, old_crtc_state);
>>>>  }
>>>>  
>>>>  /* FIXME get rid of this and use pre_plane_update */
>>>> @@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>>  	int pipe = intel_crtc->pipe;
>>>>  
>>>> -	intel_pre_disable_primary(crtc);
>>>> +	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
>>>>  
>>>>  	/*
>>>>  	 * Vblank time updates from the shadow to live plane control register
>>>> @@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>>>  		if (primary_state->base.visible &&
>>>>  		    (needs_modeset(&pipe_config->base) ||
>>>>  		     !old_primary_state->base.visible))
>>>> -			intel_post_enable_primary(&crtc->base);
>>>> +			intel_post_enable_primary(&crtc->base, pipe_config);
>>>>  	}
>>>>  }
>>>>  
>>>> @@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>>>>  
>>>>  		if (old_primary_state->base.visible &&
>>>>  		    (modeset || !primary_state->base.visible))
>>>> -			intel_pre_disable_primary(&crtc->base);
>>>> +			intel_pre_disable_primary(&crtc->base, old_crtc_state);
>>>>  	}
>>>>  
>>>>  	/*
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index cddd96b24878..82c33d9cd466 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>>>>  	}
>>>>  
>>>>   out:
>>>> -	hsw_enable_ips(intel_crtc);
>>>> +	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
>>> crtc->config would seem like the slightly more correct thing to pass
>>> here. It's also easier to grep for when someone sets out to fix this
>>> mess properly.
>> Considered it, but our crtc->config usage is a mess. I'd rather get rid of the intel_dp_sink_crc code tbh. :/
> I'd rather not spread more ->state uses when I just recently
> managed to clean out many of them. We still have some left for
> sure, but adding more seems like going backwards.
>
Maybe export hsw_pipe_A_crc_wa ?

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

Re: [PATCH v2 3/6] drm/i915: Pass crtc_state to ips toggle functions

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 06:56:36PM +0100, Maarten Lankhorst wrote:
> Op 09-11-17 om 18:51 schreef Ville Syrjälä:
> > On Thu, Nov 09, 2017 at 06:31:02PM +0100, Maarten Lankhorst wrote:
> >> Op 09-11-17 om 18:09 schreef Ville Syrjälä:
> >>> On Thu, Nov 09, 2017 at 05:24:55PM +0100, Maarten Lankhorst wrote:
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
> >>>>  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++-----------
> >>>>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
> >>>>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
> >>>>  4 files changed, 20 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >>>> index b8315bca852b..c6373febf617 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_color.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_color.c
> >>>> @@ -370,7 +370,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
> >>>>  	 */
> >>>>  	if (IS_HASWELL(dev_priv) && intel_crtc_state->ips_enabled &&
> >>>>  	    (intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> >>>> -		hsw_disable_ips(intel_crtc);
> >>>> +		hsw_disable_ips(intel_crtc, intel_crtc_state);
> >>> I would suggest simplifying more and passing only the state.
> >>> The callee can always dig out the crtc if it needs it.
> >>>
> >>>>  		reenable_ips = true;
> >>>>  	}
> >>>>  
> >>>> @@ -380,7 +380,7 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
> >>>>  	i9xx_load_luts(crtc_state);
> >>>>  
> >>>>  	if (reenable_ips)
> >>>> -		hsw_enable_ips(intel_crtc);
> >>>> +		hsw_enable_ips(intel_crtc, intel_crtc_state);
> >>>>  }
> >>>>  
> >>>>  static void bdw_load_degamma_lut(struct drm_crtc_state *state)
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index f5933b0719c9..13b372e4f06e 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -4832,12 +4832,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -void hsw_enable_ips(struct intel_crtc *crtc)
> >>>> +void hsw_enable_ips(struct intel_crtc *crtc, 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;
> >>>>  
> >>>>  	/*
> >>>> @@ -4871,12 +4871,12 @@ void hsw_enable_ips(struct intel_crtc *crtc)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -void hsw_disable_ips(struct intel_crtc *crtc)
> >>>> +void hsw_disable_ips(struct intel_crtc *crtc, 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;
> >>>>  
> >>>>  	assert_plane_enabled(dev_priv, crtc->plane);
> >>> Hmm. Confusied. Oh right, my plane assert cleanup didn't manage to go in yet :P
> >>>
> >>>> @@ -4924,7 +4924,8 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
> >>>>   * completely hide the primary plane.
> >>>>   */
> >>>>  static void
> >>>> -intel_post_enable_primary(struct drm_crtc *crtc)
> >>>> +intel_post_enable_primary(struct drm_crtc *crtc,
> >>>> +			  const struct intel_crtc_state *new_crtc_state)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->dev;
> >>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>> @@ -4937,7 +4938,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>>>  	 * when going from primary only to sprite only and vice
> >>>>  	 * versa.
> >>>>  	 */
> >>>> -	hsw_enable_ips(intel_crtc);
> >>>> +	hsw_enable_ips(intel_crtc, new_crtc_state);
> >>>>  
> >>>>  	/*
> >>>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> >>>> @@ -4956,7 +4957,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>>>  
> >>>>  /* FIXME move all this to pre_plane_update() with proper state tracking */
> >>>>  static void
> >>>> -intel_pre_disable_primary(struct drm_crtc *crtc)
> >>>> +intel_pre_disable_primary(struct drm_crtc *crtc,
> >>>> +			  const struct intel_crtc_state *old_crtc_state)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->dev;
> >>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>> @@ -4978,7 +4980,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >>>>  	 * when going from primary only to sprite only and vice
> >>>>  	 * versa.
> >>>>  	 */
> >>>> -	hsw_disable_ips(intel_crtc);
> >>>> +	hsw_disable_ips(intel_crtc, old_crtc_state);
> >>>>  }
> >>>>  
> >>>>  /* FIXME get rid of this and use pre_plane_update */
> >>>> @@ -4990,7 +4992,7 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> >>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>>  	int pipe = intel_crtc->pipe;
> >>>>  
> >>>> -	intel_pre_disable_primary(crtc);
> >>>> +	intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state));
> >>>>  
> >>>>  	/*
> >>>>  	 * Vblank time updates from the shadow to live plane control register
> >>>> @@ -5034,7 +5036,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >>>>  		if (primary_state->base.visible &&
> >>>>  		    (needs_modeset(&pipe_config->base) ||
> >>>>  		     !old_primary_state->base.visible))
> >>>> -			intel_post_enable_primary(&crtc->base);
> >>>> +			intel_post_enable_primary(&crtc->base, pipe_config);
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> @@ -5063,7 +5065,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >>>>  
> >>>>  		if (old_primary_state->base.visible &&
> >>>>  		    (modeset || !primary_state->base.visible))
> >>>> -			intel_pre_disable_primary(&crtc->base);
> >>>> +			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> >>>>  	}
> >>>>  
> >>>>  	/*
> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>> index cddd96b24878..82c33d9cd466 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>> @@ -3947,7 +3947,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>>>  	}
> >>>>  
> >>>>   out:
> >>>> -	hsw_enable_ips(intel_crtc);
> >>>> +	hsw_enable_ips(intel_crtc, to_intel_crtc_state(intel_crtc->base.state));
> >>> crtc->config would seem like the slightly more correct thing to pass
> >>> here. It's also easier to grep for when someone sets out to fix this
> >>> mess properly.
> >> Considered it, but our crtc->config usage is a mess. I'd rather get rid of the intel_dp_sink_crc code tbh. :/
> > I'd rather not spread more ->state uses when I just recently
> > managed to clean out many of them. We still have some left for
> > sure, but adding more seems like going backwards.
> >
> Maybe export hsw_pipe_A_crc_wa ?

Yeah, I suppose that would the proper fix. I didn't actually bother
thinking about it that far when I was last looking at this. I guess the
only downside is that we'd also get the panel fitter thing which we
don't really need here. But not sure anyone would actually notice.

Looks like the locking would have to be reworked though.
i915_sink_crc() still uses drm_modeset_lock_all().

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

Re: [PATCH v2 5/6] drm/i915: Enable FIFO underrun reporting after initial fastset, v2.

[Maarten Lankhorst]

Op 09-11-17 om 18:15 schreef Ville Syrjälä:
> On Thu, Nov 09, 2017 at 05:24:57PM +0100, Maarten Lankhorst wrote:
>> The firmware may have set up the pipe correctly, but the FIFO
>> underrun and CRC interrupts are likely not enabled.
>>
>> This resulted in debugfs_test.read_all_entries failing on haswell,
>> because of a timeout when reading the crc debugfs entry.
>>
>> Solve this by enabling FIFO underrun reporting after the initial
>> fastset, which lets interrupts be generated as expected.
>>
>> Changes since v1:
>> - Always enable CPU FIFO underrun reporting for >GEN2,
>>   and handle GEN2 correctly.
> "Correct" is a strong word to use here. I think I should have the
> actually correct thing somewhere...
>
> git://github.com/vsyrjala/linux.git gen2_fifo_underrun
>
> Though it seems I haven't managed to write proper commit messages for
> it yet.
>
>> Testcase: debugfs_test.read_all_entries
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3af1e3f74dbb..58bce253f4a8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12905,6 +12905,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>>  				     struct drm_crtc_state *old_crtc_state)
>>  {
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	struct intel_atomic_state *old_intel_state =
>>  		to_intel_atomic_state(old_crtc_state->state);
>> @@ -12912,6 +12913,17 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>>  		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
>>  
>>  	intel_pipe_update_end(new_crtc_state);
>> +
>> +	if (new_crtc_state->update_pipe &&
>> +	    !needs_modeset(&new_crtc_state->base) &&
>> +	    old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) {
>> +		if (!IS_GEN2(dev_priv) ||
>> +		    new_crtc_state->active_planes & BIT(PLANE_PRIMARY))
>> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
>> +
>> +		if (new_crtc_state->has_pch_encoder && !HAS_DDI(dev_priv))
>> +			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> I quite dislike having so many platform checks in high level
> common code. Maybe we should have some kind of
> intel_initial_modeset_done() thing or something?

We have nothing better atm, I'm ok with completely ignoring FIFO underruns
when inheriting on GEN2 as well, would that work for you for now?

> I don't quite see why that !DDI check is here either.

Oh that one can be nuked I think, was under the mistaken impression it could be set
with DDI, but didn't see anywhere in the code the pch fifo underruns were set for DDI.

The has_pch_encoder check should be enough on its own after more careful inspection. :-)

>> +	}
>>  }
>>  
>>  /**
>> -- 
>> 2.15.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

Re: [PATCH v2 5/6] drm/i915: Enable FIFO underrun reporting after initial fastset, v2.

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 09:02:18AM +0100, Maarten Lankhorst wrote:
> Op 09-11-17 om 18:15 schreef Ville Syrjälä:
> > On Thu, Nov 09, 2017 at 05:24:57PM +0100, Maarten Lankhorst wrote:
> >> The firmware may have set up the pipe correctly, but the FIFO
> >> underrun and CRC interrupts are likely not enabled.
> >>
> >> This resulted in debugfs_test.read_all_entries failing on haswell,
> >> because of a timeout when reading the crc debugfs entry.
> >>
> >> Solve this by enabling FIFO underrun reporting after the initial
> >> fastset, which lets interrupts be generated as expected.
> >>
> >> Changes since v1:
> >> - Always enable CPU FIFO underrun reporting for >GEN2,
> >>   and handle GEN2 correctly.
> > "Correct" is a strong word to use here. I think I should have the
> > actually correct thing somewhere...
> >
> > git://github.com/vsyrjala/linux.git gen2_fifo_underrun
> >
> > Though it seems I haven't managed to write proper commit messages for
> > it yet.
> >
> >> Testcase: debugfs_test.read_all_entries
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 3af1e3f74dbb..58bce253f4a8 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12905,6 +12905,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> >>  				     struct drm_crtc_state *old_crtc_state)
> >>  {
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>  	struct intel_atomic_state *old_intel_state =
> >>  		to_intel_atomic_state(old_crtc_state->state);
> >> @@ -12912,6 +12913,17 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> >>  		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
> >>  
> >>  	intel_pipe_update_end(new_crtc_state);
> >> +
> >> +	if (new_crtc_state->update_pipe &&
> >> +	    !needs_modeset(&new_crtc_state->base) &&
> >> +	    old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) {
> >> +		if (!IS_GEN2(dev_priv) ||
> >> +		    new_crtc_state->active_planes & BIT(PLANE_PRIMARY))
> >> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> >> +
> >> +		if (new_crtc_state->has_pch_encoder && !HAS_DDI(dev_priv))
> >> +			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> > I quite dislike having so many platform checks in high level
> > common code. Maybe we should have some kind of
> > intel_initial_modeset_done() thing or something?
> 
> We have nothing better atm, I'm ok with completely ignoring FIFO underruns
> when inheriting on GEN2 as well, would that work for you for now?
> 
> > I don't quite see why that !DDI check is here either.
> 
> Oh that one can be nuked I think, was under the mistaken impression it could be set
> with DDI, but didn't see anywhere in the code the pch fifo underruns were set for DDI.
> 
> The has_pch_encoder check should be enough on its own after more careful inspection. :-)

Actually the crtc->pipe you pass in will be a problem. You'd have to use
intel_crtc_pch_transcoder(), or maybe we should just stop playing tricks
and store the pch transcoder in the crtc state?

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