[PATCH v3 01/11] drm/i915: Update watermark state correctly in sanitize_watermarks

[PATCH v3 01/11] 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 v3 02/11] drm/i915: Remove bogus ips_enabled check.

[Maarten Lankhorst]

The flag just tells us IPS can be enabled, if the primary plane
is not enabled it means IPS might not be. This never triggered
in CI because we don't have a haswell ULT there, but can be
reproduced easily with kms_atomic_transitions.plane-all-modeset-transition

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17665ee06c9a..8d2e1111ef44 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11258,10 +11258,6 @@ 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_P(shared_dpll);
-- 
2.15.0

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

[PATCH v3 03/11] drm/i915: Check boolean options in intel_pipe_config_compare with its own macro

[Maarten Lankhorst]

Add PIPE_CONF_CHECK_BOOL for boolean options, which are printed with yesno.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d2e1111ef44..425167da560b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11095,6 +11095,15 @@ 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_P(name)	\
 	if (current_config->name != pipe_config->name) { \
 		pipe_config_err(adjust, __stringify(name), \
@@ -11180,7 +11189,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 +11221,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(has_infoframe);
+	PIPE_CONF_CHECK_BOOL(ycbcr420);
 
-	PIPE_CONF_CHECK_I(has_audio);
+	PIPE_CONF_CHECK_BOOL(has_audio);
 
 	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
 			      DRM_MODE_FLAG_INTERLACE);
@@ -11248,7 +11257,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,7 +11267,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 		PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate);
 	}
 
-	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);
@@ -11296,6 +11305,7 @@ 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_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 v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

[Maarten Lankhorst]

Some parameters use CHECK_BOOLL, but should really use
CHECK_BOOL_INCOMPLETE. We cannot currently check whether
the inherited infoframes and audio are set up correctly.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 425167da560b..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) { \
@@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 		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), \
@@ -11228,10 +11242,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 
 	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
 	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
-	PIPE_CONF_CHECK_BOOL(has_infoframe);
+	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
 	PIPE_CONF_CHECK_BOOL(ycbcr420);
 
-	PIPE_CONF_CHECK_BOOL(has_audio);
+	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
 
 	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
 			      DRM_MODE_FLAG_INTERLACE);
@@ -11306,6 +11320,7 @@ 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 v3 05/11] drm/i915: Only enable IPS when primary plane is visible

[Maarten Lankhorst]

This prevents us from having to open code those checks in
intel_dp_sink_crc.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f5933b0719c9..1b3f258038b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4836,8 +4836,10 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc_state *crtc_state = crtc->config;
 
-	if (!crtc->config->ips_enabled)
+	if (!crtc_state->ips_enabled ||
+	    !(crtc_state->active_planes & BIT(PLANE_PRIMARY)))
 		return;
 
 	/*
-- 
2.15.0

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

[PATCH v3 06/11] drm/i915: Handle locking better in i915_sink_crc.

[Maarten Lankhorst]

Lock the bare minimum, instead of the entire world, and
use interruptible locking because we can.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 39883cd915db..7e8f40eb970d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2736,39 +2736,63 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	struct intel_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp = NULL;
+	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 	u8 crc[6];
 
-	drm_modeset_lock_all(dev);
+	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
+
 	drm_connector_list_iter_begin(dev, &conn_iter);
+
 	for_each_intel_connector_iter(connector, &conn_iter) {
 		struct drm_crtc *crtc;
+		struct drm_connector_state *state;
 
-		if (!connector->base.state->best_encoder)
+		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
 			continue;
 
-		crtc = connector->base.state->crtc;
-		if (!crtc->state->active)
+retry:
+		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
+		if (ret)
+			goto err;
+
+		state = connector->base.state;
+		if (!state->best_encoder)
 			continue;
 
-		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
+		crtc = state->crtc;
+		ret = drm_modeset_lock(&crtc->mutex, &ctx);
+		if (ret)
+			goto err;
+
+		if (!crtc->state->active)
 			continue;
 
-		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
+		intel_dp = enc_to_intel_dp(state->best_encoder);
 
 		ret = intel_dp_sink_crc(intel_dp, crc);
 		if (ret)
-			goto out;
+			goto err;
 
 		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
 			   crc[0], crc[1], crc[2],
 			   crc[3], crc[4], crc[5]);
 		goto out;
+
+err:
+		if (ret == -EDEADLK) {
+			ret = drm_modeset_backoff(&ctx);
+			if (!ret)
+				goto retry;
+		}
+		goto out;
 	}
 	ret = -ENODEV;
 out:
 	drm_connector_list_iter_end(&conn_iter);
-	drm_modeset_unlock_all(dev);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
 	return ret;
 }
 
-- 
2.15.0

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

[PATCH v3 07/11] drm/i915: Pass idle crtc_state to intel_dp_sink_crc

[Maarten Lankhorst]

IPS can only be enabled if the primary plane is visible, so
first make sure sw state matches hw state by waiting for hw_done.

After this pass crtc_state to intel_dp_sink_crc() so that can be used,
instead of using legacy pointers.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c     | 23 +++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h    |  3 ++-
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7e8f40eb970d..31db026494e0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2747,6 +2747,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	for_each_intel_connector_iter(connector, &conn_iter) {
 		struct drm_crtc *crtc;
 		struct drm_connector_state *state;
+		struct intel_crtc_state *crtc_state;
 
 		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
 			continue;
@@ -2765,12 +2766,24 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 		if (ret)
 			goto err;
 
-		if (!crtc->state->active)
+		crtc_state = to_intel_crtc_state(crtc->state);
+		if (!crtc_state->base.active)
 			continue;
 
+		/*
+		 * We need to wait for all crtc updates to complete, to make
+		 * sure any pending modesets and plane updates are completed.
+		 */
+		if (crtc_state->base.commit) {
+			ret = wait_for_completion_interruptible(&crtc_state->base.commit->hw_done);
+
+			if (ret)
+				goto err;
+		}
+
 		intel_dp = enc_to_intel_dp(state->best_encoder);
 
-		ret = intel_dp_sink_crc(intel_dp, crc);
+		ret = intel_dp_sink_crc(intel_dp, crtc_state, crc);
 		if (ret)
 			goto err;
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cddd96b24878..abef0392abb9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3907,11 +3907,12 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
 					intel_dp->is_mst);
 }
 
-static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
+static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
+				  struct intel_crtc_state *crtc_state, bool disable_wa)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	u8 buf;
 	int ret = 0;
 	int count = 0;
@@ -3947,15 +3948,17 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 	}
 
  out:
-	hsw_enable_ips(intel_crtc);
+	if (disable_wa)
+		hsw_enable_ips(intel_crtc);
 	return ret;
 }
 
-static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
+static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
+				   struct intel_crtc_state *crtc_state)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	u8 buf;
 	int ret;
 
@@ -3969,7 +3972,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 		return -EIO;
 
 	if (buf & DP_TEST_SINK_START) {
-		ret = intel_dp_sink_crc_stop(intel_dp);
+		ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false);
 		if (ret)
 			return ret;
 	}
@@ -3986,16 +3989,16 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	return 0;
 }
 
-int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state, u8 *crc)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	u8 buf;
 	int count, ret;
 	int attempts = 6;
 
-	ret = intel_dp_sink_crc_start(intel_dp);
+	ret = intel_dp_sink_crc_start(intel_dp, crtc_state);
 	if (ret)
 		return ret;
 
@@ -4023,7 +4026,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	}
 
 stop:
-	intel_dp_sink_crc_stop(intel_dp);
+	intel_dp_sink_crc_stop(intel_dp, crtc_state, true);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 00b488688042..6abe52161437 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1534,7 +1534,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
-int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
+int intel_dp_sink_crc(struct intel_dp *intel_dp,
+		      struct intel_crtc_state *crtc_state, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);
-- 
2.15.0

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

[PATCH v3 08/11] drm/i915: Pass crtc_state to ips toggle functions, v2

[Maarten Lankhorst]

Changes since v1:
- Only pass crtc_state, not crtc.

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 | 25 ++++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
 4 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index b8315bca852b..aa66e952a95d 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_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_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 1b3f258038b2..4aa02e27985d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4832,11 +4832,11 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-void hsw_enable_ips(struct intel_crtc *crtc)
+void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc_state *crtc_state = crtc->config;
 
 	if (!crtc_state->ips_enabled ||
 	    !(crtc_state->active_planes & BIT(PLANE_PRIMARY)))
@@ -4873,12 +4873,13 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	}
 }
 
-void hsw_disable_ips(struct intel_crtc *crtc)
+void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	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);
@@ -4926,7 +4927,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);
@@ -4939,7 +4941,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(new_crtc_state);
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
@@ -4958,7 +4960,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);
@@ -4980,7 +4983,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(old_crtc_state);
 }
 
 /* FIXME get rid of this and use pre_plane_update */
@@ -4992,7 +4995,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
@@ -5036,7 +5039,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);
 	}
 }
 
@@ -5065,7 +5068,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 abef0392abb9..8b57e4c064e1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3949,7 +3949,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
 
  out:
 	if (disable_wa)
-		hsw_enable_ips(intel_crtc);
+		hsw_enable_ips(crtc_state);
 	return ret;
 }
 
@@ -3977,11 +3977,11 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
 			return ret;
 	}
 
-	hsw_disable_ips(intel_crtc);
+	hsw_disable_ips(crtc_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(crtc_state);
 		return -EIO;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6abe52161437..b5db6bbef7b9 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(const struct intel_crtc_state *crtc_state);
+void hsw_disable_ips(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 v3 09/11] drm/i915: Handle ips_enabled in fastset, v2.

[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.

Changes since v1:
- Simplify conditions.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4aa02e27985d..1b75af773ef7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5040,6 +5040,9 @@ 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)
+			/* IPS turned on after fastset or CRC collection disable. */
+			hsw_enable_ips(pipe_config);
 	}
 }
 
@@ -5069,6 +5072,9 @@ 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)
+			/* IPS turned off for CRC, disable it. */
+			hsw_disable_ips(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 v3 10/11] drm/i915: Enable FIFO underrun reporting after initial fastset, v3.

[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.
Changes since v2:
- Remove unneeded HAS_DDI, simplify GEN2 case.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b75af773ef7..f0dc33fb3390 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12906,6 +12906,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);
@@ -12913,6 +12914,16 @@ 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))
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
+
+		if (new_crtc_state->has_pch_encoder)
+			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 v3 11/11] 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 [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks
URL   : https://patchwork.freedesktop.org/series/33595/
State : success

== Summary ==

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

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:378s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:537s
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:508s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:505s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:498s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:488s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:427s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:263s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:543s
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:437s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:424s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:484s
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:479s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:527s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:542s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:567s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:463s
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:520s
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:455s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:557s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:420s
Blacklisted hosts:
fi-cfl-s         total:289  pass:254  dwarn:3   dfail:0   fail:0   skip:32  time:532s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:558s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:495s

c36994b067f0771eee4d3136f40c0c6040a8fa77 drm-tip: 2017y-11m-10d-11h-48m-19s UTC integration manifest
232f9d117a27 drm/i915: Re-enable fastboot by default
ae57e2cf1505 drm/i915: Enable FIFO underrun reporting after initial fastset, v3.
e11f70002950 drm/i915: Handle ips_enabled in fastset, v2.
fdedc2849b3b drm/i915: Pass crtc_state to ips toggle functions, v2
74425eb99311 drm/i915: Pass idle crtc_state to intel_dp_sink_crc
1bb4c8da357e drm/i915: Handle locking better in i915_sink_crc.
961ab504cb9b drm/i915: Only enable IPS when primary plane is visible
9b5a5ee42197 drm/i915: Handle adjust better in intel_pipe_config_compare
2525985b92d8 drm/i915: Check boolean options in intel_pipe_config_compare with its own macro
600555e30b75 drm/i915: Remove bogus ips_enabled check.
200983c792be drm/i915: Update watermark state correctly in sanitize_watermarks

== Logs ==

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

Re: [PATCH v3 02/11] drm/i915: Remove bogus ips_enabled check.

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:34:54PM +0100, Maarten Lankhorst wrote:
> The flag just tells us IPS can be enabled, if the primary plane
> is not enabled it means IPS might not be. This never triggered
> in CI because we don't have a haswell ULT there, but can be
> reproduced easily with kms_atomic_transitions.plane-all-modeset-transition
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17665ee06c9a..8d2e1111ef44 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11258,10 +11258,6 @@ 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);

Hm ... looking at this ips business I think we got it all wrong. For this
patch I think you should also remove the hw state readout for ips_enabled
in haswell_get_pipe_config, it's effectively dead code. With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -
>  	PIPE_CONF_CHECK_I(double_wide);
>  
>  	PIPE_CONF_CHECK_P(shared_dpll);
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 03/11] drm/i915: Check boolean options in intel_pipe_config_compare with its own macro

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:34:55PM +0100, Maarten Lankhorst wrote:
> Add PIPE_CONF_CHECK_BOOL for boolean options, which are printed with yesno.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8d2e1111ef44..425167da560b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11095,6 +11095,15 @@ 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_P(name)	\
>  	if (current_config->name != pipe_config->name) { \
>  		pipe_config_err(adjust, __stringify(name), \
> @@ -11180,7 +11189,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 +11221,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(has_infoframe);
> +	PIPE_CONF_CHECK_BOOL(ycbcr420);
>  
> -	PIPE_CONF_CHECK_I(has_audio);
> +	PIPE_CONF_CHECK_BOOL(has_audio);
>  
>  	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
>  			      DRM_MODE_FLAG_INTERLACE);
> @@ -11248,7 +11257,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,7 +11267,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate);
>  	}
>  
> -	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);
> @@ -11296,6 +11305,7 @@ 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_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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:34:56PM +0100, Maarten Lankhorst wrote:
> Some parameters use CHECK_BOOLL, but should really use
> CHECK_BOOL_INCOMPLETE. We cannot currently check whether
> the inherited infoframes and audio are set up correctly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 425167da560b..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) { \
> @@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		ret = false; \
>  	}
>  

Maybe add a comment here like

/*
 * Checks state where we only read out the enabling, but not the entire
 * state itself (like full infoframes or ELD for audio). These states
 * require a full modeset on bootup to fix up.
 */

With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +#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), \
> @@ -11228,10 +11242,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  
>  	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>  	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
> -	PIPE_CONF_CHECK_BOOL(has_infoframe);
> +	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
>  	PIPE_CONF_CHECK_BOOL(ycbcr420);
>  
> -	PIPE_CONF_CHECK_BOOL(has_audio);
> +	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>  
>  	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
>  			      DRM_MODE_FLAG_INTERLACE);
> @@ -11306,6 +11320,7 @@ 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 05/11] drm/i915: Only enable IPS when primary plane is visible

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:34:57PM +0100, Maarten Lankhorst wrote:
> This prevents us from having to open code those checks in
> intel_dp_sink_crc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f5933b0719c9..1b3f258038b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4836,8 +4836,10 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc_state *crtc_state = crtc->config;
>  
> -	if (!crtc->config->ips_enabled)
> +	if (!crtc_state->ips_enabled ||
> +	    !(crtc_state->active_planes & BIT(PLANE_PRIMARY)))

I think time to fix up how we compute this, maybe even make it a plane
property, or at least wm property?

I think putting this into the wm state would clarify a bunch of things in
the code flow ... Waiting with more r-bs for ips patches until we know
what we should be doing here.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 06/11] drm/i915: Handle locking better in i915_sink_crc.

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:34:58PM +0100, Maarten Lankhorst wrote:
> Lock the bare minimum, instead of the entire world, and
> use interruptible locking because we can.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 39883cd915db..7e8f40eb970d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2736,39 +2736,63 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>  	struct intel_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp = NULL;
> +	struct drm_modeset_acquire_ctx ctx;
>  	int ret;
>  	u8 crc[6];
>  
> -	drm_modeset_lock_all(dev);
> +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> +
>  	drm_connector_list_iter_begin(dev, &conn_iter);

I kinda expect this funny locking nesting to upset lockdep, I think we
have a bunch of places where we nest list_iter and modeset locks
differently.

I think the correct nesting is list_iter entirely within the ww_mutex
stuff, which means you'd need to terminate the loop (and remember the
connector), then after list_iter_end do the ww_mutex dance. Of course
that's all assuming CI shows I'm right (hopefully it does since we no
longer reboot, in the past finding these took a few runs until you had the
right test combination to trigger this stuff).

Even if we don't yet have such a case I'd really prefer that list_iter
isn't nested between the acquire_ctx and modeset ww_mutex lockdep
contexts.
-Daniel

> +
>  	for_each_intel_connector_iter(connector, &conn_iter) {
>  		struct drm_crtc *crtc;
> +		struct drm_connector_state *state;
>  
> -		if (!connector->base.state->best_encoder)
> +		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
>  			continue;
>  
> -		crtc = connector->base.state->crtc;
> -		if (!crtc->state->active)
> +retry:
> +		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> +		if (ret)
> +			goto err;
> +
> +		state = connector->base.state;
> +		if (!state->best_encoder)
>  			continue;
>  
> -		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> +		crtc = state->crtc;
> +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> +		if (ret)
> +			goto err;
> +
> +		if (!crtc->state->active)
>  			continue;
>  
> -		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
> +		intel_dp = enc_to_intel_dp(state->best_encoder);
>  
>  		ret = intel_dp_sink_crc(intel_dp, crc);
>  		if (ret)
> -			goto out;
> +			goto err;
>  
>  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
>  			   crc[0], crc[1], crc[2],
>  			   crc[3], crc[4], crc[5]);
>  		goto out;
> +
> +err:
> +		if (ret == -EDEADLK) {
> +			ret = drm_modeset_backoff(&ctx);
> +			if (!ret)
> +				goto retry;
> +		}
> +		goto out;
>  	}
>  	ret = -ENODEV;
>  out:
>  	drm_connector_list_iter_end(&conn_iter);
> -	drm_modeset_unlock_all(dev);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
>  	return ret;
>  }
>  
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 09/11] drm/i915: Handle ips_enabled in fastset, v2.

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:35:01PM +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.
> 
> Changes since v1:
> - Simplify conditions.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Hm yeah, more hints that maybe ips should be a plane, or at least a wm
state thing?

This turns a bit into a rabbit hole, so if Ville thinks this is all still
reasonably clean-ish I think we can go with this here too ... I'm just
wondering a bit.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c  | 6 ++++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4aa02e27985d..1b75af773ef7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5040,6 +5040,9 @@ 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)
> +			/* IPS turned on after fastset or CRC collection disable. */
> +			hsw_enable_ips(pipe_config);
>  	}
>  }
>  
> @@ -5069,6 +5072,9 @@ 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)
> +			/* IPS turned off for CRC, disable it. */
> +			hsw_disable_ips(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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 06/11] drm/i915: Handle locking better in i915_sink_crc.

[Daniel Vetter]

On Fri, Nov 10, 2017 at 02:13:39PM +0100, Daniel Vetter wrote:
> On Fri, Nov 10, 2017 at 12:34:58PM +0100, Maarten Lankhorst wrote:
> > Lock the bare minimum, instead of the entire world, and
> > use interruptible locking because we can.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++--------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 39883cd915db..7e8f40eb970d 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2736,39 +2736,63 @@ static int i915_sink_crc(struct seq_file *m, void *data)
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	struct intel_dp *intel_dp = NULL;
> > +	struct drm_modeset_acquire_ctx ctx;
> >  	int ret;
> >  	u8 crc[6];
> >  
> > -	drm_modeset_lock_all(dev);
> > +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > +
> >  	drm_connector_list_iter_begin(dev, &conn_iter);
> 
> I kinda expect this funny locking nesting to upset lockdep, I think we
> have a bunch of places where we nest list_iter and modeset locks
> differently.
> 
> I think the correct nesting is list_iter entirely within the ww_mutex
> stuff, which means you'd need to terminate the loop (and remember the
> connector), then after list_iter_end do the ww_mutex dance. Of course
> that's all assuming CI shows I'm right (hopefully it does since we no
> longer reboot, in the past finding these took a few runs until you had the
> right test combination to trigger this stuff).
> 
> Even if we don't yet have such a case I'd really prefer that list_iter
> isn't nested between the acquire_ctx and modeset ww_mutex lockdep
> contexts.

Correction, this exact locking pattern already exists in
drm_atomic_add_affected_connectors, changing to what I suggested would
actually upset lockdep. Hence

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

on the patch as-is.
-Daniel

> -Daniel
> 
> > +
> >  	for_each_intel_connector_iter(connector, &conn_iter) {
> >  		struct drm_crtc *crtc;
> > +		struct drm_connector_state *state;
> >  
> > -		if (!connector->base.state->best_encoder)
> > +		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> >  			continue;
> >  
> > -		crtc = connector->base.state->crtc;
> > -		if (!crtc->state->active)
> > +retry:
> > +		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> > +		if (ret)
> > +			goto err;
> > +
> > +		state = connector->base.state;
> > +		if (!state->best_encoder)
> >  			continue;
> >  
> > -		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		crtc = state->crtc;
> > +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> > +		if (ret)
> > +			goto err;
> > +
> > +		if (!crtc->state->active)
> >  			continue;
> >  
> > -		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
> > +		intel_dp = enc_to_intel_dp(state->best_encoder);
> >  
> >  		ret = intel_dp_sink_crc(intel_dp, crc);
> >  		if (ret)
> > -			goto out;
> > +			goto err;
> >  
> >  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> >  			   crc[0], crc[1], crc[2],
> >  			   crc[3], crc[4], crc[5]);
> >  		goto out;
> > +
> > +err:
> > +		if (ret == -EDEADLK) {
> > +			ret = drm_modeset_backoff(&ctx);
> > +			if (!ret)
> > +				goto retry;
> > +		}
> > +		goto out;
> >  	}
> >  	ret = -ENODEV;
> >  out:
> >  	drm_connector_list_iter_end(&conn_iter);
> > -	drm_modeset_unlock_all(dev);
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.15.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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 [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks
URL   : https://patchwork.freedesktop.org/series/33595/
State : success

== Summary ==

Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-c:
                dmesg-warn -> PASS       (shard-hsw) fdo#102249
        Subgroup extended-modeset-hang-newfb-with-reset-render-b:
                pass       -> DMESG-WARN (shard-hsw) fdo#103038

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

shard-hsw        total:2584 pass:1451 dwarn:3   dfail:2   fail:10  skip:1118 time:9299s
Blacklisted hosts:
shard-apl        total:2584 pass:1600 dwarn:4   dfail:2   fail:23  skip:955 time:12994s
shard-kbl        total:2575 pass:1694 dwarn:12  dfail:2   fail:22  skip:844 time:9995s
shard-snb        total:2584 pass:1198 dwarn:2   dfail:2   fail:10  skip:1372 time:7672s

== Logs ==

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

Re: [PATCH v3 10/11] drm/i915: Enable FIFO underrun reporting after initial fastset, v3.

[Daniel Vetter]

On Fri, Nov 10, 2017 at 12:35:02PM +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.
> Changes since v2:
> - Remove unneeded HAS_DDI, simplify GEN2 case.
> 
> Testcase: debugfs_test.read_all_entries
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b75af773ef7..f0dc33fb3390 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12906,6 +12906,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);
> @@ -12913,6 +12914,16 @@ 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) {

Enabling fifo underruns multiple times is totally fine, we need to cope
with that (since you can a bunch of underruns, or might re-enable, or
whatever). This means you can simplify this to (old_state & INHERITED).

With that bikeshed applied:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		if (!IS_GEN2(dev_priv))
> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> +
> +		if (new_crtc_state->has_pch_encoder)
> +			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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 01/11] drm/i915: Update watermark state correctly in sanitize_watermarks

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 12:34:53PM +0100, Maarten Lankhorst wrote:
> 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;

Hmm. Oh we don't swap the state here. Looks like we shouldn't be using
anything else from the crtc state, so this should work AFAICS.

Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

>  	}
>  
>  put_state:
> -- 
> 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

Re: [Intel-gfx] [PATCH v3 01/11] drm/i915: Update watermark state correctly in sanitize_watermarks

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 12:34:53PM +0100, Maarten Lankhorst wrote:
> 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;

Hmm. Oh we don't swap the state here. Looks like we shouldn't be using
anything else from the crtc state, so this should work AFAICS.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	}
>  
>  put_state:
> -- 
> 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

Re: [PATCH v3 10/11] drm/i915: Enable FIFO underrun reporting after initial fastset, v3.

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 12:35:02PM +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.
> Changes since v2:
> - Remove unneeded HAS_DDI, simplify GEN2 case.
> 
> Testcase: debugfs_test.read_all_entries
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b75af773ef7..f0dc33fb3390 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12906,6 +12906,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);
> @@ -12913,6 +12914,16 @@ 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))
> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> +
> +		if (new_crtc_state->has_pch_encoder)
> +			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);

Still using the wrong transcoder for HSW/BDW.

> +	}
>  }
>  
>  /**
> -- 
> 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 v3 09/11] drm/i915: Handle ips_enabled in fastset, v2.

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 02:15:04PM +0100, Daniel Vetter wrote:
> On Fri, Nov 10, 2017 at 12:35:01PM +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.
> > 
> > Changes since v1:
> > - Simplify conditions.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Hm yeah, more hints that maybe ips should be a plane, or at least a wm
> state thing?

I think we would have to write this as something like:

pre_plane_update()
if (old_crtc_state->ips && old_crtc_state->active_planes != 0 &&
    (!new_crtc_state->ips || new_crtc_state->active_planes == 0)
    disable_ips();
 
post_plane_update()
if ((!old_crtc_state->ips || old_crtc_state->active_planes == 0) &&
    new_crtc_state->ips && new_crtc_state->active_planes != 0)
    enable_ips();

Probably needs a few mode needs_modeset() and crtc_state->active
checks etc. to account for all the disabling pipe, enabling pipe,
toggling pipe, and doing nothing to the pipe cases.

To simplify maybe we should probably just account for active_planes
in crtc_state->ips_enabled already. Ie. call hsw_compute_ips_config()
even if we don't do a modeset.

We'd have to change the bdw cdclk vs. ips workaround to work the
other way around though since currently hsw_compute_ips_config() assumes
that we can do a modeset to increase cdclk (assuming it's not already
maxed out). But I think it would be a worthwile change to make the code
less convoluted.

> 
> This turns a bit into a rabbit hole, so if Ville thinks this is all still
> reasonably clean-ish I think we can go with this here too ... I'm just
> wondering a bit.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 6 ++++++
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4aa02e27985d..1b75af773ef7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5040,6 +5040,9 @@ 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)
> > +			/* IPS turned on after fastset or CRC collection disable. */
> > +			hsw_enable_ips(pipe_config);
> >  	}
> >  }
> >  
> > @@ -5069,6 +5072,9 @@ 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)
> > +			/* IPS turned off for CRC, disable it. */
> > +			hsw_disable_ips(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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> 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: [Intel-gfx] [PATCH v3 01/11] drm/i915: Update watermark state correctly in sanitize_watermarks

[Maarten Lankhorst]

Op 10-11-17 om 16:28 schreef Ville Syrjälä:
> On Fri, Nov 10, 2017 at 12:34:53PM +0100, Maarten Lankhorst wrote:
>> 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;
> Hmm. Oh we don't swap the state here. Looks like we shouldn't be using
> anything else from the crtc state, so this should work AFAICS.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
We should, but I don't trust that we keep the swapped state completely identical to the original.
For  this reason I felt it was safer to only copy the wm part of the state. Thanks, pushed. :)

Re: [PATCH v3 10/11] drm/i915: Enable FIFO underrun reporting after initial fastset, v3.

[Maarten Lankhorst]

Op 10-11-17 om 20:48 schreef Ville Syrjälä:
> On Fri, Nov 10, 2017 at 12:35:02PM +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.
>> Changes since v2:
>> - Remove unneeded HAS_DDI, simplify GEN2 case.
>>
>> Testcase: debugfs_test.read_all_entries
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1b75af773ef7..f0dc33fb3390 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12906,6 +12906,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);
>> @@ -12913,6 +12914,16 @@ 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))
>> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
>> +
>> +		if (new_crtc_state->has_pch_encoder)
>> +			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> Still using the wrong transcoder for HSW/BDW.
Would it be better with a call to intel_crtc_pch_transcoder?

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

Re: [PATCH v3 06/11] drm/i915: Handle locking better in i915_sink_crc.

[Maarten Lankhorst]

Op 10-11-17 om 14:24 schreef Daniel Vetter:
> On Fri, Nov 10, 2017 at 02:13:39PM +0100, Daniel Vetter wrote:
>> On Fri, Nov 10, 2017 at 12:34:58PM +0100, Maarten Lankhorst wrote:
>>> Lock the bare minimum, instead of the entire world, and
>>> use interruptible locking because we can.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++--------
>>>  1 file changed, 32 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 39883cd915db..7e8f40eb970d 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2736,39 +2736,63 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>>>  	struct intel_connector *connector;
>>>  	struct drm_connector_list_iter conn_iter;
>>>  	struct intel_dp *intel_dp = NULL;
>>> +	struct drm_modeset_acquire_ctx ctx;
>>>  	int ret;
>>>  	u8 crc[6];
>>>  
>>> -	drm_modeset_lock_all(dev);
>>> +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>>> +
>>>  	drm_connector_list_iter_begin(dev, &conn_iter);
>> I kinda expect this funny locking nesting to upset lockdep, I think we
>> have a bunch of places where we nest list_iter and modeset locks
>> differently.
>>
>> I think the correct nesting is list_iter entirely within the ww_mutex
>> stuff, which means you'd need to terminate the loop (and remember the
>> connector), then after list_iter_end do the ww_mutex dance. Of course
>> that's all assuming CI shows I'm right (hopefully it does since we no
>> longer reboot, in the past finding these took a few runs until you had the
>> right test combination to trigger this stuff).
>>
>> Even if we don't yet have such a case I'd really prefer that list_iter
>> isn't nested between the acquire_ctx and modeset ww_mutex lockdep
>> contexts.
> Correction, this exact locking pattern already exists in
> drm_atomic_add_affected_connectors, changing to what I suggested would
> actually upset lockdep. Hence
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Thanks, pushed up to here, would be nice if 7/11 would get a r-b too. No IPS related changes there yet, just the preparations. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 10/11] drm/i915: Enable FIFO underrun reporting after initial fastset, v3.

[Ville Syrjälä]

On Mon, Nov 13, 2017 at 01:01:39PM +0100, Maarten Lankhorst wrote:
> Op 10-11-17 om 20:48 schreef Ville Syrjälä:
> > On Fri, Nov 10, 2017 at 12:35:02PM +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.
> >> Changes since v2:
> >> - Remove unneeded HAS_DDI, simplify GEN2 case.
> >>
> >> Testcase: debugfs_test.read_all_entries
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 1b75af773ef7..f0dc33fb3390 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12906,6 +12906,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);
> >> @@ -12913,6 +12914,16 @@ 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))
> >> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> >> +
> >> +		if (new_crtc_state->has_pch_encoder)
> >> +			intel_set_pch_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> > Still using the wrong transcoder for HSW/BDW.
> Would it be better with a call to intel_crtc_pch_transcoder?

That should do it (+ probably want to remove the crtc->config usage from
therein).

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

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

[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.
Changes since v2:
- Remove unneeded HAS_DDI, simplify GEN2 case.
Changes since v3:
- Use intel_crtc_pch_transcoder to determine pch transcoder for underruns. (Ville)
- Remove crtc->config dereference in intel_crtc_pch_transcoder. (Ville)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 03e274a11d95..e49ee9d3bd61 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1872,8 +1872,6 @@ enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
-	WARN_ON(!crtc->config->has_pch_encoder);
-
 	if (HAS_PCH_LPT(dev_priv))
 		return PIPE_A;
 	else
@@ -12908,6 +12906,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);
@@ -12915,6 +12914,20 @@ 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))
+			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
+
+		if (new_crtc_state->has_pch_encoder) {
+			enum pipe pch_transcoder =
+				intel_crtc_pch_transcoder(dev_priv, intel_crtc);
+
+			intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true);
+		}
+	}
 }
 
 /**
-- 
2.15.0

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

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

[Ville Syrjälä]

On Mon, Nov 13, 2017 at 03:40:43PM +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.
> Changes since v2:
> - Remove unneeded HAS_DDI, simplify GEN2 case.
> Changes since v3:
> - Use intel_crtc_pch_transcoder to determine pch transcoder for underruns. (Ville)
> - Remove crtc->config dereference in intel_crtc_pch_transcoder. (Ville)
> 
> Testcase: debugfs_test.read_all_entries
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 03e274a11d95..e49ee9d3bd61 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1872,8 +1872,6 @@ enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  
> -	WARN_ON(!crtc->config->has_pch_encoder);
> -
>  	if (HAS_PCH_LPT(dev_priv))
>  		return PIPE_A;
>  	else
> @@ -12908,6 +12906,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);
> @@ -12915,6 +12914,20 @@ 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))
> +			intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true);
> +
> +		if (new_crtc_state->has_pch_encoder) {
> +			enum pipe pch_transcoder =
> +				intel_crtc_pch_transcoder(dev_priv, intel_crtc);
> +
> +			intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true);
> +		}
> +	}
>  }
>  
>  /**
> -- 
> 2.15.0

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

✗ Fi.CI.BAT: failure for series starting with [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks (rev2)
URL   : https://patchwork.freedesktop.org/series/33595/
State : failure

== Summary ==

Series 33595 revision 2 was fully merged or fully failed: no git log

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

✗ Fi.CI.BAT: failure for series starting with [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/11] drm/i915: Update watermark state correctly in sanitize_watermarks (rev2)
URL   : https://patchwork.freedesktop.org/series/33595/
State : failure

== Summary ==

Series 33595 revision 2 was fully merged or fully failed: no git log

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

Re: [PATCH v3 07/11] drm/i915: Pass idle crtc_state to intel_dp_sink_crc

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 12:34:59PM +0100, Maarten Lankhorst wrote:
> IPS can only be enabled if the primary plane is visible, so
> first make sure sw state matches hw state by waiting for hw_done.
> 
> After this pass crtc_state to intel_dp_sink_crc() so that can be used,
> instead of using legacy pointers.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++--
>  drivers/gpu/drm/i915/intel_dp.c     | 23 +++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h    |  3 ++-
>  3 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7e8f40eb970d..31db026494e0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2747,6 +2747,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>  	for_each_intel_connector_iter(connector, &conn_iter) {
>  		struct drm_crtc *crtc;
>  		struct drm_connector_state *state;
> +		struct intel_crtc_state *crtc_state;
>  
>  		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
>  			continue;
> @@ -2765,12 +2766,24 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>  		if (ret)
>  			goto err;
>  
> -		if (!crtc->state->active)
> +		crtc_state = to_intel_crtc_state(crtc->state);

We're under modeset_lock_all() so we can do this safely.

> +		if (!crtc_state->base.active)
>  			continue;
>  
> +		/*
> +		 * We need to wait for all crtc updates to complete, to make
> +		 * sure any pending modesets and plane updates are completed.
> +		 */
> +		if (crtc_state->base.commit) {
> +			ret = wait_for_completion_interruptible(&crtc_state->base.commit->hw_done);

I guess it would be nice if we could actually keep grabbing crcs across
a longer period instead of just one frame. But we can't do that with the
current uapi for thos. Given that constraint this seems like the right
solution.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +			if (ret)
> +				goto err;
> +		}
> +
>  		intel_dp = enc_to_intel_dp(state->best_encoder);
>  
> -		ret = intel_dp_sink_crc(intel_dp, crc);
> +		ret = intel_dp_sink_crc(intel_dp, crtc_state, crc);
>  		if (ret)
>  			goto err;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cddd96b24878..abef0392abb9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3907,11 +3907,12 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>  					intel_dp->is_mst);
>  }
>  
> -static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> +static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *crtc_state, bool disable_wa)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	u8 buf;
>  	int ret = 0;
>  	int count = 0;
> @@ -3947,15 +3948,17 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  	}
>  
>   out:
> -	hsw_enable_ips(intel_crtc);
> +	if (disable_wa)
> +		hsw_enable_ips(intel_crtc);
>  	return ret;
>  }
>  
> -static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
> +				   struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	u8 buf;
>  	int ret;
>  
> @@ -3969,7 +3972,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  		return -EIO;
>  
>  	if (buf & DP_TEST_SINK_START) {
> -		ret = intel_dp_sink_crc_stop(intel_dp);
> +		ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false);
>  		if (ret)
>  			return ret;
>  	}
> @@ -3986,16 +3989,16 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  	return 0;
>  }
>  
> -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state, u8 *crc)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	u8 buf;
>  	int count, ret;
>  	int attempts = 6;
>  
> -	ret = intel_dp_sink_crc_start(intel_dp);
> +	ret = intel_dp_sink_crc_start(intel_dp, crtc_state);
>  	if (ret)
>  		return ret;
>  
> @@ -4023,7 +4026,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	}
>  
>  stop:
> -	intel_dp_sink_crc_stop(intel_dp);
> +	intel_dp_sink_crc_stop(intel_dp, crtc_state, true);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 00b488688042..6abe52161437 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1534,7 +1534,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_reset(struct drm_encoder *encoder);
>  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
> +int intel_dp_sink_crc(struct intel_dp *intel_dp,
> +		      struct intel_crtc_state *crtc_state, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config,
>  			     struct drm_connector_state *conn_state);
> -- 
> 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 v3 08/11] drm/i915: Pass crtc_state to ips toggle functions, v2

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 12:35:00PM +0100, Maarten Lankhorst wrote:
> Changes since v1:
> - Only pass crtc_state, not crtc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_color.c   |  4 ++--
>  drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_dp.c      |  6 +++---
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  4 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index b8315bca852b..aa66e952a95d 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_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_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 1b3f258038b2..4aa02e27985d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4832,11 +4832,11 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>  	}
>  }
>  
> -void hsw_enable_ips(struct intel_crtc *crtc)
> +void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc_state *crtc_state = crtc->config;
>  
>  	if (!crtc_state->ips_enabled ||
>  	    !(crtc_state->active_planes & BIT(PLANE_PRIMARY)))
> @@ -4873,12 +4873,13 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>  	}
>  }
>  
> -void hsw_disable_ips(struct intel_crtc *crtc)
> +void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	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);
> @@ -4926,7 +4927,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);
> @@ -4939,7 +4941,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(new_crtc_state);
>  
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> @@ -4958,7 +4960,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);
> @@ -4980,7 +4983,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(old_crtc_state);
>  }
>  
>  /* FIXME get rid of this and use pre_plane_update */
> @@ -4992,7 +4995,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
> @@ -5036,7 +5039,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);
>  	}
>  }
>  
> @@ -5065,7 +5068,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 abef0392abb9..8b57e4c064e1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3949,7 +3949,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
>  
>   out:
>  	if (disable_wa)
> -		hsw_enable_ips(intel_crtc);
> +		hsw_enable_ips(crtc_state);
>  	return ret;
>  }
>  
> @@ -3977,11 +3977,11 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
>  			return ret;
>  	}
>  
> -	hsw_disable_ips(intel_crtc);
> +	hsw_disable_ips(crtc_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(crtc_state);
>  		return -EIO;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6abe52161437..b5db6bbef7b9 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(const struct intel_crtc_state *crtc_state);
> +void hsw_disable_ips(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 v3 02/11] drm/i915: Remove bogus ips_enabled check.

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 01:57:06PM +0100, Daniel Vetter wrote:
> On Fri, Nov 10, 2017 at 12:34:54PM +0100, Maarten Lankhorst wrote:
> > The flag just tells us IPS can be enabled, if the primary plane
> > is not enabled it means IPS might not be. This never triggered
> > in CI because we don't have a haswell ULT there, but can be
> > reproduced easily with kms_atomic_transitions.plane-all-modeset-transition
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 17665ee06c9a..8d2e1111ef44 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11258,10 +11258,6 @@ 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);
> 
> Hm ... looking at this ips business I think we got it all wrong. For this
> patch I think you should also remove the hw state readout for ips_enabled
> in haswell_get_pipe_config, it's effectively dead code. With that:

IMO better to fix ips_enabled to account for the active planes.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > -
> >  	PIPE_CONF_CHECK_I(double_wide);
> >  
> >  	PIPE_CONF_CHECK_P(shared_dpll);
> > -- 
> > 2.15.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> 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 v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

[Ville Syrjälä]

On Fri, Nov 10, 2017 at 02:02:45PM +0100, Daniel Vetter wrote:
> On Fri, Nov 10, 2017 at 12:34:56PM +0100, Maarten Lankhorst wrote:
> > Some parameters use CHECK_BOOLL, but should really use
> > CHECK_BOOL_INCOMPLETE. We cannot currently check whether
> > the inherited infoframes and audio are set up correctly.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 425167da560b..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) { \
> > @@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  		ret = false; \
> >  	}
> >  
> 
> Maybe add a comment here like
> 
> /*
>  * Checks state where we only read out the enabling, but not the entire
>  * state itself (like full infoframes or ELD for audio). These states
>  * require a full modeset on bootup to fix up.

has_audio shouldn't need the ELD. If audio is enabled then it's enabled,
otherwise it's not.

So I guess if the BIOS doesn't enable audio, then what we really need is
a way to enable it without a full modeset. And I can't immediately
recall if we can do that or not.

I'm worried that ignoring the issue will just lead to piles of bugs
where people complain that their audio doesn't work.

>  */
> 
> With that:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > +#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), \
> > @@ -11228,10 +11242,10 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  
> >  	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
> >  	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
> > -	PIPE_CONF_CHECK_BOOL(has_infoframe);
> > +	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> >  	PIPE_CONF_CHECK_BOOL(ycbcr420);
> >  
> > -	PIPE_CONF_CHECK_BOOL(has_audio);
> > +	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
> >  
> >  	PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags,
> >  			      DRM_MODE_FLAG_INTERLACE);
> > @@ -11306,6 +11320,7 @@ 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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> 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 v3 08/11] drm/i915: Pass crtc_state to ips toggle functions, v2

[Maarten Lankhorst]

Op 13-11-17 om 18:18 schreef Ville Syrjälä:
> On Fri, Nov 10, 2017 at 12:35:00PM +0100, Maarten Lankhorst wrote:
>> Changes since v1:
>> - Only pass crtc_state, not crtc.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
Thanks, pushed this and previous patch.

Need to think about IPS some more, how to calculate it cleanly..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

[Daniel Vetter]

On Mon, Nov 13, 2017 at 07:24:22PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 10, 2017 at 02:02:45PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 10, 2017 at 12:34:56PM +0100, Maarten Lankhorst wrote:
> > > Some parameters use CHECK_BOOLL, but should really use
> > > CHECK_BOOL_INCOMPLETE. We cannot currently check whether
> > > the inherited infoframes and audio are set up correctly.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 425167da560b..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) { \
> > > @@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > >  		ret = false; \
> > >  	}
> > >  
> > 
> > Maybe add a comment here like
> > 
> > /*
> >  * Checks state where we only read out the enabling, but not the entire
> >  * state itself (like full infoframes or ELD for audio). These states
> >  * require a full modeset on bootup to fix up.
> 
> has_audio shouldn't need the ELD. If audio is enabled then it's enabled,
> otherwise it's not.
> 
> So I guess if the BIOS doesn't enable audio, then what we really need is
> a way to enable it without a full modeset. And I can't immediately
> recall if we can do that or not.

Afaiui the audio enable bit fires off the irq on the snd driver, which
then reads the eld to figure out how to set up the audio side.

So we indeed need the eld to be correct. We might also need more stuff
(maybe audio only reads eld when the irq fires, I didn't check).
 
> I'm worried that ignoring the issue will just lead to piles of bugs
> where people complain that their audio doesn't work.

I'm not blocking your patches, the r-b was included ... what do you mean?
I'm all for fixing this ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

[Ville Syrjälä]

On Mon, Nov 20, 2017 at 11:38:49AM +0100, Daniel Vetter wrote:
> On Mon, Nov 13, 2017 at 07:24:22PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 10, 2017 at 02:02:45PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 10, 2017 at 12:34:56PM +0100, Maarten Lankhorst wrote:
> > > > Some parameters use CHECK_BOOLL, but should really use
> > > > CHECK_BOOL_INCOMPLETE. We cannot currently check whether
> > > > the inherited infoframes and audio are set up correctly.
> > > > 
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
> > > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 425167da560b..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) { \
> > > > @@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > > >  		ret = false; \
> > > >  	}
> > > >  
> > > 
> > > Maybe add a comment here like
> > > 
> > > /*
> > >  * Checks state where we only read out the enabling, but not the entire
> > >  * state itself (like full infoframes or ELD for audio). These states
> > >  * require a full modeset on bootup to fix up.
> > 
> > has_audio shouldn't need the ELD. If audio is enabled then it's enabled,
> > otherwise it's not.
> > 
> > So I guess if the BIOS doesn't enable audio, then what we really need is
> > a way to enable it without a full modeset. And I can't immediately
> > recall if we can do that or not.
> 
> Afaiui the audio enable bit fires off the irq on the snd driver, which
> then reads the eld to figure out how to set up the audio side.

You're thinking about the ELD valid bit, which is totally different. And
I don't think we use that (on modern platforms at least) ever since we
added these eld notify hooks and whatnot. Well, we still frob the bit but
the audio driver no longer uses the corresponding interrupt, I think.

> 
> So we indeed need the eld to be correct. We might also need more stuff
> (maybe audio only reads eld when the irq fires, I didn't check).
>  
> > I'm worried that ignoring the issue will just lead to piles of bugs
> > where people complain that their audio doesn't work.
> 
> I'm not blocking your patches, the r-b was included ... what do you mean?
> I'm all for fixing this ...

I have no idea what you're saying here. What patches?

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

Re: [PATCH v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

[Daniel Vetter]

On Mon, Nov 20, 2017 at 12:53:28PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 20, 2017 at 11:38:49AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 13, 2017 at 07:24:22PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 10, 2017 at 02:02:45PM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 10, 2017 at 12:34:56PM +0100, Maarten Lankhorst wrote:
> > > > > Some parameters use CHECK_BOOLL, but should really use
> > > > > CHECK_BOOL_INCOMPLETE. We cannot currently check whether
> > > > > the inherited infoframes and audio are set up correctly.
> > > > > 
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
> > > > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 425167da560b..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) { \
> > > > > @@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > > > >  		ret = false; \
> > > > >  	}
> > > > >  
> > > > 
> > > > Maybe add a comment here like
> > > > 
> > > > /*
> > > >  * Checks state where we only read out the enabling, but not the entire
> > > >  * state itself (like full infoframes or ELD for audio). These states
> > > >  * require a full modeset on bootup to fix up.
> > > 
> > > has_audio shouldn't need the ELD. If audio is enabled then it's enabled,
> > > otherwise it's not.
> > > 
> > > So I guess if the BIOS doesn't enable audio, then what we really need is
> > > a way to enable it without a full modeset. And I can't immediately
> > > recall if we can do that or not.
> > 
> > Afaiui the audio enable bit fires off the irq on the snd driver, which
> > then reads the eld to figure out how to set up the audio side.
> 
> You're thinking about the ELD valid bit, which is totally different. And
> I don't think we use that (on modern platforms at least) ever since we
> added these eld notify hooks and whatnot. Well, we still frob the bit but
> the audio driver no longer uses the corresponding interrupt, I think.

I thought there was 2 interrupts, buy yeah you're probably right ... tbh I
just don't really know how exactly this works on the snd side.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx