[RFC,1/2] drm/i915: start moving state checker to intel_verify.c

Submitted by Jani Nikula on April 16, 2019, 10:36 a.m.

Details

Message ID 20190416103603.8266-1-jani.nikula@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula April 16, 2019, 10:36 a.m.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/Makefile        |   1 +
 drivers/gpu/drm/i915/intel_display.c | 474 +--------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  12 +
 drivers/gpu/drm/i915/intel_verify.c  | 464 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_verify.h  |  22 ++
 5 files changed, 510 insertions(+), 463 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_verify.c
 create mode 100644 drivers/gpu/drm/i915/intel_verify.h

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index fbcb0904..a000fad 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -166,6 +166,7 @@  i915-y += dvo_ch7017.o \
 	  intel_panel.o \
 	  intel_sdvo.o \
 	  intel_tv.o \
+	  intel_verify.o \
 	  vlv_dsi.o \
 	  vlv_dsi_pll.o \
 	  intel_vdsc.o
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3bd40a..31a931 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -69,6 +69,7 @@ 
 #include "intel_sdvo.h"
 #include "intel_sprite.h"
 #include "intel_tv.h"
+#include "intel_verify.h"
 
 /* Primary plane formats for gen <= 3 */
 static const u32 i8xx_primary_formats[] = {
@@ -1243,7 +1244,7 @@  void assert_pipe(struct drm_i915_private *dev_priv,
 			pipe_name(pipe), onoff(state), onoff(cur_state));
 }
 
-static void assert_plane(struct intel_plane *plane, bool state)
+void assert_plane(struct intel_plane *plane, bool state)
 {
 	enum pipe pipe;
 	bool cur_state;
@@ -6607,45 +6608,6 @@  void intel_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_encoder);
 }
 
-/* Cross check the actual hw state with our own modeset state tracking (and it's
- * internal consistency). */
-static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
-					 struct drm_connector_state *conn_state)
-{
-	struct intel_connector *connector = to_intel_connector(conn_state->connector);
-
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.base.id,
-		      connector->base.name);
-
-	if (connector->get_hw_state(connector)) {
-		struct intel_encoder *encoder = connector->encoder;
-
-		I915_STATE_WARN(!crtc_state,
-			 "connector enabled without attached crtc\n");
-
-		if (!crtc_state)
-			return;
-
-		I915_STATE_WARN(!crtc_state->active,
-		      "connector is active, but attached crtc isn't\n");
-
-		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
-			return;
-
-		I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
-			"atomic encoder doesn't match attached encoder\n");
-
-		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
-			"attached encoder crtc differs from connector crtc\n");
-	} else {
-		I915_STATE_WARN(crtc_state && crtc_state->active,
-			"attached crtc is active, but connector isn't\n");
-		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
-			"best encoder set without crtc!\n");
-	}
-}
-
 static int pipe_required_fdi_lanes(struct intel_crtc_state *crtc_state)
 {
 	if (crtc_state->base.enable && crtc_state->has_pch_encoder)
@@ -6879,7 +6841,7 @@  static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
 	return pixel_rate;
 }
 
-static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
+void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
 
@@ -11584,9 +11546,9 @@  static const char *output_formats(enum intel_output_format format)
 	return output_format_str[format];
 }
 
-static void intel_dump_pipe_config(struct intel_crtc *crtc,
-				   struct intel_crtc_state *pipe_config,
-				   const char *context)
+void intel_dump_pipe_config(struct intel_crtc *crtc,
+			    struct intel_crtc_state *pipe_config,
+			    const char *context)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -12076,7 +12038,7 @@  static bool fastboot_enabled(struct drm_i915_private *dev_priv)
 	return false;
 }
 
-static bool
+bool
 intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 			  struct intel_crtc_state *current_config,
 			  struct intel_crtc_state *pipe_config,
@@ -12389,8 +12351,8 @@  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
-					   const struct intel_crtc_state *pipe_config)
+void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
+				    const struct intel_crtc_state *pipe_config)
 {
 	if (pipe_config->has_pch_encoder) {
 		int fdi_dotclock = intel_dotclock_calculate(intel_fdi_link_freq(dev_priv, pipe_config),
@@ -12407,420 +12369,6 @@  static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void verify_wm_state(struct drm_crtc *crtc,
-			    struct drm_crtc_state *new_state)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
-	struct skl_hw_state {
-		struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
-		struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
-		struct skl_ddb_allocation ddb;
-		struct skl_pipe_wm wm;
-	} *hw;
-	struct skl_ddb_allocation *sw_ddb;
-	struct skl_pipe_wm *sw_wm;
-	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const enum pipe pipe = intel_crtc->pipe;
-	int plane, level, max_level = ilk_wm_max_level(dev_priv);
-
-	if (INTEL_GEN(dev_priv) < 9 || !new_state->active)
-		return;
-
-	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
-	if (!hw)
-		return;
-
-	skl_pipe_wm_get_hw_state(intel_crtc, &hw->wm);
-	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
-
-	skl_pipe_ddb_get_hw_state(intel_crtc, hw->ddb_y, hw->ddb_uv);
-
-	skl_ddb_get_hw_state(dev_priv, &hw->ddb);
-	sw_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	if (INTEL_GEN(dev_priv) >= 11 &&
-	    hw->ddb.enabled_slices != sw_ddb->enabled_slices)
-		DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
-			  sw_ddb->enabled_slices,
-			  hw->ddb.enabled_slices);
-
-	/* planes */
-	for_each_universal_plane(dev_priv, pipe, plane) {
-		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
-
-		hw_plane_wm = &hw->wm.planes[plane];
-		sw_plane_wm = &sw_wm->planes[plane];
-
-		/* Watermarks */
-		for (level = 0; level <= max_level; level++) {
-			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
-						&sw_plane_wm->wm[level]))
-				continue;
-
-			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
-				  pipe_name(pipe), plane + 1, level,
-				  sw_plane_wm->wm[level].plane_en,
-				  sw_plane_wm->wm[level].plane_res_b,
-				  sw_plane_wm->wm[level].plane_res_l,
-				  hw_plane_wm->wm[level].plane_en,
-				  hw_plane_wm->wm[level].plane_res_b,
-				  hw_plane_wm->wm[level].plane_res_l);
-		}
-
-		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
-					 &sw_plane_wm->trans_wm)) {
-			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
-				  pipe_name(pipe), plane + 1,
-				  sw_plane_wm->trans_wm.plane_en,
-				  sw_plane_wm->trans_wm.plane_res_b,
-				  sw_plane_wm->trans_wm.plane_res_l,
-				  hw_plane_wm->trans_wm.plane_en,
-				  hw_plane_wm->trans_wm.plane_res_b,
-				  hw_plane_wm->trans_wm.plane_res_l);
-		}
-
-		/* DDB */
-		hw_ddb_entry = &hw->ddb_y[plane];
-		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
-
-		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
-			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
-				  pipe_name(pipe), plane + 1,
-				  sw_ddb_entry->start, sw_ddb_entry->end,
-				  hw_ddb_entry->start, hw_ddb_entry->end);
-		}
-	}
-
-	/*
-	 * cursor
-	 * If the cursor plane isn't active, we may not have updated it's ddb
-	 * allocation. In that case since the ddb allocation will be updated
-	 * once the plane becomes visible, we can skip this check
-	 */
-	if (1) {
-		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
-
-		hw_plane_wm = &hw->wm.planes[PLANE_CURSOR];
-		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
-
-		/* Watermarks */
-		for (level = 0; level <= max_level; level++) {
-			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
-						&sw_plane_wm->wm[level]))
-				continue;
-
-			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
-				  pipe_name(pipe), level,
-				  sw_plane_wm->wm[level].plane_en,
-				  sw_plane_wm->wm[level].plane_res_b,
-				  sw_plane_wm->wm[level].plane_res_l,
-				  hw_plane_wm->wm[level].plane_en,
-				  hw_plane_wm->wm[level].plane_res_b,
-				  hw_plane_wm->wm[level].plane_res_l);
-		}
-
-		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
-					 &sw_plane_wm->trans_wm)) {
-			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
-				  pipe_name(pipe),
-				  sw_plane_wm->trans_wm.plane_en,
-				  sw_plane_wm->trans_wm.plane_res_b,
-				  sw_plane_wm->trans_wm.plane_res_l,
-				  hw_plane_wm->trans_wm.plane_en,
-				  hw_plane_wm->trans_wm.plane_res_b,
-				  hw_plane_wm->trans_wm.plane_res_l);
-		}
-
-		/* DDB */
-		hw_ddb_entry = &hw->ddb_y[PLANE_CURSOR];
-		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
-
-		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
-			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
-				  pipe_name(pipe),
-				  sw_ddb_entry->start, sw_ddb_entry->end,
-				  hw_ddb_entry->start, hw_ddb_entry->end);
-		}
-	}
-
-	kfree(hw);
-}
-
-static void
-verify_connector_state(struct drm_device *dev,
-		       struct drm_atomic_state *state,
-		       struct drm_crtc *crtc)
-{
-	struct drm_connector *connector;
-	struct drm_connector_state *new_conn_state;
-	int i;
-
-	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
-		struct drm_encoder *encoder = connector->encoder;
-		struct drm_crtc_state *crtc_state = NULL;
-
-		if (new_conn_state->crtc != crtc)
-			continue;
-
-		if (crtc)
-			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
-
-		intel_connector_verify_state(crtc_state, new_conn_state);
-
-		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
-		     "connector's atomic encoder doesn't match legacy encoder\n");
-	}
-}
-
-static void
-verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
-{
-	struct intel_encoder *encoder;
-	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state, *new_conn_state;
-	int i;
-
-	for_each_intel_encoder(dev, encoder) {
-		bool enabled = false, found = false;
-		enum pipe pipe;
-
-		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
-			      encoder->base.base.id,
-			      encoder->base.name);
-
-		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
-						   new_conn_state, i) {
-			if (old_conn_state->best_encoder == &encoder->base)
-				found = true;
-
-			if (new_conn_state->best_encoder != &encoder->base)
-				continue;
-			found = enabled = true;
-
-			I915_STATE_WARN(new_conn_state->crtc !=
-					encoder->base.crtc,
-			     "connector's crtc doesn't match encoder crtc\n");
-		}
-
-		if (!found)
-			continue;
-
-		I915_STATE_WARN(!!encoder->base.crtc != enabled,
-		     "encoder's enabled state mismatch "
-		     "(expected %i, found %i)\n",
-		     !!encoder->base.crtc, enabled);
-
-		if (!encoder->base.crtc) {
-			bool active;
-
-			active = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(active,
-			     "encoder detached but still enabled on pipe %c.\n",
-			     pipe_name(pipe));
-		}
-	}
-}
-
-static void
-verify_crtc_state(struct drm_crtc *crtc,
-		  struct drm_crtc_state *old_crtc_state,
-		  struct drm_crtc_state *new_crtc_state)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_encoder *encoder;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *pipe_config, *sw_config;
-	struct drm_atomic_state *old_state;
-	bool active;
-
-	old_state = old_crtc_state->state;
-	__drm_atomic_helper_crtc_destroy_state(old_crtc_state);
-	pipe_config = to_intel_crtc_state(old_crtc_state);
-	memset(pipe_config, 0, sizeof(*pipe_config));
-	pipe_config->base.crtc = crtc;
-	pipe_config->base.state = old_state;
-
-	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
-
-	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
-
-	/* we keep both pipes enabled on 830 */
-	if (IS_I830(dev_priv))
-		active = new_crtc_state->active;
-
-	I915_STATE_WARN(new_crtc_state->active != active,
-	     "crtc active state doesn't match with hw state "
-	     "(expected %i, found %i)\n", new_crtc_state->active, active);
-
-	I915_STATE_WARN(intel_crtc->active != new_crtc_state->active,
-	     "transitional active state does not match atomic hw state "
-	     "(expected %i, found %i)\n", new_crtc_state->active, intel_crtc->active);
-
-	for_each_encoder_on_crtc(dev, crtc, encoder) {
-		enum pipe pipe;
-
-		active = encoder->get_hw_state(encoder, &pipe);
-		I915_STATE_WARN(active != new_crtc_state->active,
-			"[ENCODER:%i] active %i with crtc active %i\n",
-			encoder->base.base.id, active, new_crtc_state->active);
-
-		I915_STATE_WARN(active && intel_crtc->pipe != pipe,
-				"Encoder connected to wrong pipe %c\n",
-				pipe_name(pipe));
-
-		if (active)
-			encoder->get_config(encoder, pipe_config);
-	}
-
-	intel_crtc_compute_pixel_rate(pipe_config);
-
-	if (!new_crtc_state->active)
-		return;
-
-	intel_pipe_config_sanity_check(dev_priv, pipe_config);
-
-	sw_config = to_intel_crtc_state(new_crtc_state);
-	if (!intel_pipe_config_compare(dev_priv, sw_config,
-				       pipe_config, false)) {
-		I915_STATE_WARN(1, "pipe state doesn't match!\n");
-		intel_dump_pipe_config(intel_crtc, pipe_config,
-				       "[hw state]");
-		intel_dump_pipe_config(intel_crtc, sw_config,
-				       "[sw state]");
-	}
-}
-
-static void
-intel_verify_planes(struct intel_atomic_state *state)
-{
-	struct intel_plane *plane;
-	const struct intel_plane_state *plane_state;
-	int i;
-
-	for_each_new_intel_plane_in_state(state, plane,
-					  plane_state, i)
-		assert_plane(plane, plane_state->slave ||
-			     plane_state->base.visible);
-}
-
-static void
-verify_single_dpll_state(struct drm_i915_private *dev_priv,
-			 struct intel_shared_dpll *pll,
-			 struct drm_crtc *crtc,
-			 struct drm_crtc_state *new_state)
-{
-	struct intel_dpll_hw_state dpll_hw_state;
-	unsigned int crtc_mask;
-	bool active;
-
-	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
-
-	DRM_DEBUG_KMS("%s\n", pll->info->name);
-
-	active = pll->info->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state);
-
-	if (!(pll->info->flags & INTEL_DPLL_ALWAYS_ON)) {
-		I915_STATE_WARN(!pll->on && pll->active_mask,
-		     "pll in active use but not on in sw tracking\n");
-		I915_STATE_WARN(pll->on && !pll->active_mask,
-		     "pll is on but not used by any active crtc\n");
-		I915_STATE_WARN(pll->on != active,
-		     "pll on state mismatch (expected %i, found %i)\n",
-		     pll->on, active);
-	}
-
-	if (!crtc) {
-		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
-				"more active pll users than references: %x vs %x\n",
-				pll->active_mask, pll->state.crtc_mask);
-
-		return;
-	}
-
-	crtc_mask = drm_crtc_mask(crtc);
-
-	if (new_state->active)
-		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
-				"pll active mismatch (expected pipe %c in active mask 0x%02x)\n",
-				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
-	else
-		I915_STATE_WARN(pll->active_mask & crtc_mask,
-				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
-				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
-
-	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
-			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
-			crtc_mask, pll->state.crtc_mask);
-
-	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
-					  &dpll_hw_state,
-					  sizeof(dpll_hw_state)),
-			"pll hw state mismatch\n");
-}
-
-static void
-verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
-			 struct drm_crtc_state *old_crtc_state,
-			 struct drm_crtc_state *new_crtc_state)
-{
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc_state *old_state = to_intel_crtc_state(old_crtc_state);
-	struct intel_crtc_state *new_state = to_intel_crtc_state(new_crtc_state);
-
-	if (new_state->shared_dpll)
-		verify_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
-
-	if (old_state->shared_dpll &&
-	    old_state->shared_dpll != new_state->shared_dpll) {
-		unsigned int crtc_mask = drm_crtc_mask(crtc);
-		struct intel_shared_dpll *pll = old_state->shared_dpll;
-
-		I915_STATE_WARN(pll->active_mask & crtc_mask,
-				"pll active mismatch (didn't expect pipe %c in active mask)\n",
-				pipe_name(drm_crtc_index(crtc)));
-		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
-				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
-				pipe_name(drm_crtc_index(crtc)));
-	}
-}
-
-static void
-intel_modeset_verify_crtc(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state,
-			  struct drm_crtc_state *old_state,
-			  struct drm_crtc_state *new_state)
-{
-	if (!needs_modeset(new_state) &&
-	    !to_intel_crtc_state(new_state)->update_pipe)
-		return;
-
-	verify_wm_state(crtc, new_state);
-	verify_connector_state(crtc->dev, state, crtc);
-	verify_crtc_state(crtc, old_state, new_state);
-	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
-}
-
-static void
-verify_disabled_dpll_state(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	int i;
-
-	for (i = 0; i < dev_priv->num_shared_dpll; i++)
-		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
-}
-
-static void
-intel_modeset_verify_disabled(struct drm_device *dev,
-			      struct drm_atomic_state *state)
-{
-	verify_encoder_state(dev, state);
-	verify_connector_state(dev, state, NULL);
-	verify_disabled_dpll_state(dev);
-}
-
 static void update_scanline_offset(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
@@ -13521,7 +13069,7 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		if (!intel_can_enable_sagv(state))
 			intel_disable_sagv(dev_priv);
 
-		intel_modeset_verify_disabled(dev, state);
+		intel_verify_modeset_disabled(dev, state);
 	}
 
 	/* Complete the events for pipes that have now been disabled */
@@ -13589,7 +13137,7 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 
-		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
+		intel_verify_modeset_crtc(crtc, state, old_crtc_state, new_crtc_state);
 	}
 
 	if (intel_state->modeset)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a38b9c..77767c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1668,6 +1668,17 @@  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv);
 enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 					     enum pipe pipe);
+void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
+				    const struct intel_crtc_state *pipe_config);
+bool
+intel_pipe_config_compare(struct drm_i915_private *dev_priv,
+			  struct intel_crtc_state *current_config,
+			  struct intel_crtc_state *pipe_config,
+			  bool adjust);
+void intel_dump_pipe_config(struct intel_crtc *crtc,
+			    struct intel_crtc_state *pipe_config,
+			    const char *context);
+void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state);
 static inline bool
 intel_crtc_has_type(const struct intel_crtc_state *crtc_state,
 		    enum intel_output_type type)
@@ -1760,6 +1771,7 @@  void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
 void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state);
 #define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
 #define assert_pipe_disabled(d, p) assert_pipe(d, p, false)
+void assert_plane(struct intel_plane *plane, bool state);
 void intel_prepare_reset(struct drm_i915_private *dev_priv);
 void intel_finish_reset(struct drm_i915_private *dev_priv);
 void hsw_enable_pc8(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_verify.c b/drivers/gpu/drm/i915/intel_verify.c
new file mode 100644
index 000000..4c8990
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_verify.c
@@ -0,0 +1,464 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <drm/drm_atomic_state_helper.h>
+
+#include "i915_drv.h"
+#include "intel_drv.h"
+#include "intel_pm.h"
+#include "intel_verify.h"
+
+static void verify_wm_state(struct drm_crtc *crtc,
+			    struct drm_crtc_state *new_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct skl_hw_state {
+		struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
+		struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
+		struct skl_ddb_allocation ddb;
+		struct skl_pipe_wm wm;
+	} *hw;
+	struct skl_ddb_allocation *sw_ddb;
+	struct skl_pipe_wm *sw_wm;
+	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	const enum pipe pipe = intel_crtc->pipe;
+	int plane, level, max_level = ilk_wm_max_level(dev_priv);
+
+	if (INTEL_GEN(dev_priv) < 9 || !new_state->active)
+		return;
+
+	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+	if (!hw)
+		return;
+
+	skl_pipe_wm_get_hw_state(intel_crtc, &hw->wm);
+	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
+
+	skl_pipe_ddb_get_hw_state(intel_crtc, hw->ddb_y, hw->ddb_uv);
+
+	skl_ddb_get_hw_state(dev_priv, &hw->ddb);
+	sw_ddb = &dev_priv->wm.skl_hw.ddb;
+
+	if (INTEL_GEN(dev_priv) >= 11 &&
+	    hw->ddb.enabled_slices != sw_ddb->enabled_slices)
+		DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
+			  sw_ddb->enabled_slices,
+			  hw->ddb.enabled_slices);
+
+	/* planes */
+	for_each_universal_plane(dev_priv, pipe, plane) {
+		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
+
+		hw_plane_wm = &hw->wm.planes[plane];
+		sw_plane_wm = &sw_wm->planes[plane];
+
+		/* Watermarks */
+		for (level = 0; level <= max_level; level++) {
+			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
+						&sw_plane_wm->wm[level]))
+				continue;
+
+			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+				  pipe_name(pipe), plane + 1, level,
+				  sw_plane_wm->wm[level].plane_en,
+				  sw_plane_wm->wm[level].plane_res_b,
+				  sw_plane_wm->wm[level].plane_res_l,
+				  hw_plane_wm->wm[level].plane_en,
+				  hw_plane_wm->wm[level].plane_res_b,
+				  hw_plane_wm->wm[level].plane_res_l);
+		}
+
+		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
+					 &sw_plane_wm->trans_wm)) {
+			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+				  pipe_name(pipe), plane + 1,
+				  sw_plane_wm->trans_wm.plane_en,
+				  sw_plane_wm->trans_wm.plane_res_b,
+				  sw_plane_wm->trans_wm.plane_res_l,
+				  hw_plane_wm->trans_wm.plane_en,
+				  hw_plane_wm->trans_wm.plane_res_b,
+				  hw_plane_wm->trans_wm.plane_res_l);
+		}
+
+		/* DDB */
+		hw_ddb_entry = &hw->ddb_y[plane];
+		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
+
+		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe), plane + 1,
+				  sw_ddb_entry->start, sw_ddb_entry->end,
+				  hw_ddb_entry->start, hw_ddb_entry->end);
+		}
+	}
+
+	/*
+	 * cursor
+	 * If the cursor plane isn't active, we may not have updated it's ddb
+	 * allocation. In that case since the ddb allocation will be updated
+	 * once the plane becomes visible, we can skip this check
+	 */
+	if (1) {
+		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
+
+		hw_plane_wm = &hw->wm.planes[PLANE_CURSOR];
+		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
+
+		/* Watermarks */
+		for (level = 0; level <= max_level; level++) {
+			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
+						&sw_plane_wm->wm[level]))
+				continue;
+
+			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+				  pipe_name(pipe), level,
+				  sw_plane_wm->wm[level].plane_en,
+				  sw_plane_wm->wm[level].plane_res_b,
+				  sw_plane_wm->wm[level].plane_res_l,
+				  hw_plane_wm->wm[level].plane_en,
+				  hw_plane_wm->wm[level].plane_res_b,
+				  hw_plane_wm->wm[level].plane_res_l);
+		}
+
+		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
+					 &sw_plane_wm->trans_wm)) {
+			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+				  pipe_name(pipe),
+				  sw_plane_wm->trans_wm.plane_en,
+				  sw_plane_wm->trans_wm.plane_res_b,
+				  sw_plane_wm->trans_wm.plane_res_l,
+				  hw_plane_wm->trans_wm.plane_en,
+				  hw_plane_wm->trans_wm.plane_res_b,
+				  hw_plane_wm->trans_wm.plane_res_l);
+		}
+
+		/* DDB */
+		hw_ddb_entry = &hw->ddb_y[PLANE_CURSOR];
+		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
+
+		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe),
+				  sw_ddb_entry->start, sw_ddb_entry->end,
+				  hw_ddb_entry->start, hw_ddb_entry->end);
+		}
+	}
+
+	kfree(hw);
+}
+
+/* Cross check the actual hw state with our own modeset state tracking (and it's
+ * internal consistency). */
+static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
+					 struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.base.id,
+		      connector->base.name);
+
+	if (connector->get_hw_state(connector)) {
+		struct intel_encoder *encoder = connector->encoder;
+
+		I915_STATE_WARN(!crtc_state,
+			 "connector enabled without attached crtc\n");
+
+		if (!crtc_state)
+			return;
+
+		I915_STATE_WARN(!crtc_state->active,
+		      "connector is active, but attached crtc isn't\n");
+
+		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
+			return;
+
+		I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
+			"atomic encoder doesn't match attached encoder\n");
+
+		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
+			"attached encoder crtc differs from connector crtc\n");
+	} else {
+		I915_STATE_WARN(crtc_state && crtc_state->active,
+			"attached crtc is active, but connector isn't\n");
+		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
+			"best encoder set without crtc!\n");
+	}
+}
+
+static void
+verify_connector_state(struct drm_device *dev,
+		       struct drm_atomic_state *state,
+		       struct drm_crtc *crtc)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+		struct drm_encoder *encoder = connector->encoder;
+		struct drm_crtc_state *crtc_state = NULL;
+
+		if (new_conn_state->crtc != crtc)
+			continue;
+
+		if (crtc)
+			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
+
+		intel_connector_verify_state(crtc_state, new_conn_state);
+
+		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
+		     "connector's atomic encoder doesn't match legacy encoder\n");
+	}
+}
+
+static void
+verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	struct intel_encoder *encoder;
+	struct drm_connector *connector;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
+	int i;
+
+	for_each_intel_encoder(dev, encoder) {
+		bool enabled = false, found = false;
+		enum pipe pipe;
+
+		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
+			      encoder->base.base.id,
+			      encoder->base.name);
+
+		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
+						   new_conn_state, i) {
+			if (old_conn_state->best_encoder == &encoder->base)
+				found = true;
+
+			if (new_conn_state->best_encoder != &encoder->base)
+				continue;
+			found = enabled = true;
+
+			I915_STATE_WARN(new_conn_state->crtc !=
+					encoder->base.crtc,
+			     "connector's crtc doesn't match encoder crtc\n");
+		}
+
+		if (!found)
+			continue;
+
+		I915_STATE_WARN(!!encoder->base.crtc != enabled,
+		     "encoder's enabled state mismatch "
+		     "(expected %i, found %i)\n",
+		     !!encoder->base.crtc, enabled);
+
+		if (!encoder->base.crtc) {
+			bool active;
+
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active,
+			     "encoder detached but still enabled on pipe %c.\n",
+			     pipe_name(pipe));
+		}
+	}
+}
+
+static void
+verify_crtc_state(struct drm_crtc *crtc,
+		  struct drm_crtc_state *old_crtc_state,
+		  struct drm_crtc_state *new_crtc_state)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_encoder *encoder;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *pipe_config, *sw_config;
+	struct drm_atomic_state *old_state;
+	bool active;
+
+	old_state = old_crtc_state->state;
+	__drm_atomic_helper_crtc_destroy_state(old_crtc_state);
+	pipe_config = to_intel_crtc_state(old_crtc_state);
+	memset(pipe_config, 0, sizeof(*pipe_config));
+	pipe_config->base.crtc = crtc;
+	pipe_config->base.state = old_state;
+
+	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
+
+	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
+
+	/* we keep both pipes enabled on 830 */
+	if (IS_I830(dev_priv))
+		active = new_crtc_state->active;
+
+	I915_STATE_WARN(new_crtc_state->active != active,
+	     "crtc active state doesn't match with hw state "
+	     "(expected %i, found %i)\n", new_crtc_state->active, active);
+
+	I915_STATE_WARN(intel_crtc->active != new_crtc_state->active,
+	     "transitional active state does not match atomic hw state "
+	     "(expected %i, found %i)\n", new_crtc_state->active, intel_crtc->active);
+
+	for_each_encoder_on_crtc(dev, crtc, encoder) {
+		enum pipe pipe;
+
+		active = encoder->get_hw_state(encoder, &pipe);
+		I915_STATE_WARN(active != new_crtc_state->active,
+			"[ENCODER:%i] active %i with crtc active %i\n",
+			encoder->base.base.id, active, new_crtc_state->active);
+
+		I915_STATE_WARN(active && intel_crtc->pipe != pipe,
+				"Encoder connected to wrong pipe %c\n",
+				pipe_name(pipe));
+
+		if (active)
+			encoder->get_config(encoder, pipe_config);
+	}
+
+	intel_crtc_compute_pixel_rate(pipe_config);
+
+	if (!new_crtc_state->active)
+		return;
+
+	intel_pipe_config_sanity_check(dev_priv, pipe_config);
+
+	sw_config = to_intel_crtc_state(new_crtc_state);
+	if (!intel_pipe_config_compare(dev_priv, sw_config,
+				       pipe_config, false)) {
+		I915_STATE_WARN(1, "pipe state doesn't match!\n");
+		intel_dump_pipe_config(intel_crtc, pipe_config,
+				       "[hw state]");
+		intel_dump_pipe_config(intel_crtc, sw_config,
+				       "[sw state]");
+	}
+}
+
+void
+intel_verify_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane;
+	const struct intel_plane_state *plane_state;
+	int i;
+
+	for_each_new_intel_plane_in_state(state, plane,
+					  plane_state, i)
+		assert_plane(plane, plane_state->slave ||
+			     plane_state->base.visible);
+}
+
+static void
+verify_single_dpll_state(struct drm_i915_private *dev_priv,
+			 struct intel_shared_dpll *pll,
+			 struct drm_crtc *crtc,
+			 struct drm_crtc_state *new_state)
+{
+	struct intel_dpll_hw_state dpll_hw_state;
+	unsigned int crtc_mask;
+	bool active;
+
+	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
+
+	DRM_DEBUG_KMS("%s\n", pll->info->name);
+
+	active = pll->info->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state);
+
+	if (!(pll->info->flags & INTEL_DPLL_ALWAYS_ON)) {
+		I915_STATE_WARN(!pll->on && pll->active_mask,
+		     "pll in active use but not on in sw tracking\n");
+		I915_STATE_WARN(pll->on && !pll->active_mask,
+		     "pll is on but not used by any active crtc\n");
+		I915_STATE_WARN(pll->on != active,
+		     "pll on state mismatch (expected %i, found %i)\n",
+		     pll->on, active);
+	}
+
+	if (!crtc) {
+		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
+				"more active pll users than references: %x vs %x\n",
+				pll->active_mask, pll->state.crtc_mask);
+
+		return;
+	}
+
+	crtc_mask = drm_crtc_mask(crtc);
+
+	if (new_state->active)
+		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
+				"pll active mismatch (expected pipe %c in active mask 0x%02x)\n",
+				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
+	else
+		I915_STATE_WARN(pll->active_mask & crtc_mask,
+				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
+				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
+
+	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
+			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
+			crtc_mask, pll->state.crtc_mask);
+
+	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
+					  &dpll_hw_state,
+					  sizeof(dpll_hw_state)),
+			"pll hw state mismatch\n");
+}
+
+static void
+verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
+			 struct drm_crtc_state *old_crtc_state,
+			 struct drm_crtc_state *new_crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc_state *old_state = to_intel_crtc_state(old_crtc_state);
+	struct intel_crtc_state *new_state = to_intel_crtc_state(new_crtc_state);
+
+	if (new_state->shared_dpll)
+		verify_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
+
+	if (old_state->shared_dpll &&
+	    old_state->shared_dpll != new_state->shared_dpll) {
+		unsigned int crtc_mask = drm_crtc_mask(crtc);
+		struct intel_shared_dpll *pll = old_state->shared_dpll;
+
+		I915_STATE_WARN(pll->active_mask & crtc_mask,
+				"pll active mismatch (didn't expect pipe %c in active mask)\n",
+				pipe_name(drm_crtc_index(crtc)));
+		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
+				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
+				pipe_name(drm_crtc_index(crtc)));
+	}
+}
+
+void
+intel_verify_modeset_crtc(struct drm_crtc *crtc,
+			  struct drm_atomic_state *state,
+			  struct drm_crtc_state *old_state,
+			  struct drm_crtc_state *new_state)
+{
+	if (!drm_atomic_crtc_needs_modeset(new_state) &&
+	    !to_intel_crtc_state(new_state)->update_pipe)
+		return;
+
+	verify_wm_state(crtc, new_state);
+	verify_connector_state(crtc->dev, state, crtc);
+	verify_crtc_state(crtc, old_state, new_state);
+	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
+}
+
+static void
+verify_disabled_dpll_state(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int i;
+
+	for (i = 0; i < dev_priv->num_shared_dpll; i++)
+		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
+}
+
+void
+intel_verify_modeset_disabled(struct drm_device *dev,
+			      struct drm_atomic_state *state)
+{
+	verify_encoder_state(dev, state);
+	verify_connector_state(dev, state, NULL);
+	verify_disabled_dpll_state(dev);
+}
diff --git a/drivers/gpu/drm/i915/intel_verify.h b/drivers/gpu/drm/i915/intel_verify.h
new file mode 100644
index 000000..4b751ea
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_verify.h
@@ -0,0 +1,22 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __INTEL_VERIFY_H__
+#define __INTEL_VERIFY_H__
+
+struct drm_atomic_state;
+struct drm_crtc;
+struct drm_crtc_state;
+struct drm_device;
+
+void intel_verify_modeset_crtc(struct drm_crtc *crtc,
+			       struct drm_atomic_state *state,
+			       struct drm_crtc_state *old_state,
+			       struct drm_crtc_state *new_state);
+void intel_verify_modeset_disabled(struct drm_device *dev,
+				   struct drm_atomic_state *state);
+void intel_verify_planes(struct intel_atomic_state *state);
+
+#endif /* __INTEL_VERIFY_H__ */

Comments

On Tue, Apr 16, 2019 at 01:36:02PM +0300, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile        |   1 +
>  drivers/gpu/drm/i915/intel_display.c | 474 +--------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  12 +
>  drivers/gpu/drm/i915/intel_verify.c  | 464 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_verify.h  |  22 ++
>  5 files changed, 510 insertions(+), 463 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_verify.c
>  create mode 100644 drivers/gpu/drm/i915/intel_verify.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index fbcb0904..a000fad 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -166,6 +166,7 @@ i915-y += dvo_ch7017.o \
>  	  intel_panel.o \
>  	  intel_sdvo.o \
>  	  intel_tv.o \
> +	  intel_verify.o \
>  	  vlv_dsi.o \
>  	  vlv_dsi_pll.o \
>  	  intel_vdsc.o
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3bd40a..31a931 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -69,6 +69,7 @@
>  #include "intel_sdvo.h"
>  #include "intel_sprite.h"
>  #include "intel_tv.h"
> +#include "intel_verify.h"
>  
>  /* Primary plane formats for gen <= 3 */
>  static const u32 i8xx_primary_formats[] = {
> @@ -1243,7 +1244,7 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  			pipe_name(pipe), onoff(state), onoff(cur_state));
>  }
>  
> -static void assert_plane(struct intel_plane *plane, bool state)
> +void assert_plane(struct intel_plane *plane, bool state)
>  {
>  	enum pipe pipe;
>  	bool cur_state;
> @@ -6607,45 +6608,6 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
>  	kfree(intel_encoder);
>  }
>  
> -/* Cross check the actual hw state with our own modeset state tracking (and it's
> - * internal consistency). */
> -static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
> -					 struct drm_connector_state *conn_state)
> -{
> -	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> -
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.base.id,
> -		      connector->base.name);
> -
> -	if (connector->get_hw_state(connector)) {
> -		struct intel_encoder *encoder = connector->encoder;
> -
> -		I915_STATE_WARN(!crtc_state,
> -			 "connector enabled without attached crtc\n");
> -
> -		if (!crtc_state)
> -			return;
> -
> -		I915_STATE_WARN(!crtc_state->active,
> -		      "connector is active, but attached crtc isn't\n");
> -
> -		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> -			return;
> -
> -		I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> -			"atomic encoder doesn't match attached encoder\n");
> -
> -		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> -			"attached encoder crtc differs from connector crtc\n");
> -	} else {
> -		I915_STATE_WARN(crtc_state && crtc_state->active,
> -			"attached crtc is active, but connector isn't\n");
> -		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
> -			"best encoder set without crtc!\n");
> -	}
> -}
> -
>  static int pipe_required_fdi_lanes(struct intel_crtc_state *crtc_state)
>  {
>  	if (crtc_state->base.enable && crtc_state->has_pch_encoder)
> @@ -6879,7 +6841,7 @@ static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
>  	return pixel_rate;
>  }
>  
> -static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> +void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  
> @@ -11584,9 +11546,9 @@ static const char *output_formats(enum intel_output_format format)
>  	return output_format_str[format];
>  }
>  
> -static void intel_dump_pipe_config(struct intel_crtc *crtc,
> -				   struct intel_crtc_state *pipe_config,
> -				   const char *context)
> +void intel_dump_pipe_config(struct intel_crtc *crtc,
> +			    struct intel_crtc_state *pipe_config,
> +			    const char *context)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -12076,7 +12038,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
>  	return false;
>  }
>  
> -static bool
> +bool
>  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  			  struct intel_crtc_state *current_config,
>  			  struct intel_crtc_state *pipe_config,
> @@ -12389,8 +12351,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> -static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> -					   const struct intel_crtc_state *pipe_config)
> +void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> +				    const struct intel_crtc_state *pipe_config)
>  {
>  	if (pipe_config->has_pch_encoder) {
>  		int fdi_dotclock = intel_dotclock_calculate(intel_fdi_link_freq(dev_priv, pipe_config),
> @@ -12407,420 +12369,6 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static void verify_wm_state(struct drm_crtc *crtc,
> -			    struct drm_crtc_state *new_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> -	struct skl_hw_state {
> -		struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
> -		struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
> -		struct skl_ddb_allocation ddb;
> -		struct skl_pipe_wm wm;
> -	} *hw;
> -	struct skl_ddb_allocation *sw_ddb;
> -	struct skl_pipe_wm *sw_wm;
> -	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	const enum pipe pipe = intel_crtc->pipe;
> -	int plane, level, max_level = ilk_wm_max_level(dev_priv);
> -
> -	if (INTEL_GEN(dev_priv) < 9 || !new_state->active)
> -		return;
> -
> -	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> -	if (!hw)
> -		return;
> -
> -	skl_pipe_wm_get_hw_state(intel_crtc, &hw->wm);
> -	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
> -
> -	skl_pipe_ddb_get_hw_state(intel_crtc, hw->ddb_y, hw->ddb_uv);
> -
> -	skl_ddb_get_hw_state(dev_priv, &hw->ddb);
> -	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> -
> -	if (INTEL_GEN(dev_priv) >= 11 &&
> -	    hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> -		DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> -			  sw_ddb->enabled_slices,
> -			  hw->ddb.enabled_slices);
> -
> -	/* planes */
> -	for_each_universal_plane(dev_priv, pipe, plane) {
> -		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> -
> -		hw_plane_wm = &hw->wm.planes[plane];
> -		sw_plane_wm = &sw_wm->planes[plane];
> -
> -		/* Watermarks */
> -		for (level = 0; level <= max_level; level++) {
> -			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> -						&sw_plane_wm->wm[level]))
> -				continue;
> -
> -			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe), plane + 1, level,
> -				  sw_plane_wm->wm[level].plane_en,
> -				  sw_plane_wm->wm[level].plane_res_b,
> -				  sw_plane_wm->wm[level].plane_res_l,
> -				  hw_plane_wm->wm[level].plane_en,
> -				  hw_plane_wm->wm[level].plane_res_b,
> -				  hw_plane_wm->wm[level].plane_res_l);
> -		}
> -
> -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> -					 &sw_plane_wm->trans_wm)) {
> -			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe), plane + 1,
> -				  sw_plane_wm->trans_wm.plane_en,
> -				  sw_plane_wm->trans_wm.plane_res_b,
> -				  sw_plane_wm->trans_wm.plane_res_l,
> -				  hw_plane_wm->trans_wm.plane_en,
> -				  hw_plane_wm->trans_wm.plane_res_b,
> -				  hw_plane_wm->trans_wm.plane_res_l);
> -		}
> -
> -		/* DDB */
> -		hw_ddb_entry = &hw->ddb_y[plane];
> -		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
> -
> -		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> -			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
> -				  pipe_name(pipe), plane + 1,
> -				  sw_ddb_entry->start, sw_ddb_entry->end,
> -				  hw_ddb_entry->start, hw_ddb_entry->end);
> -		}
> -	}
> -
> -	/*
> -	 * cursor
> -	 * If the cursor plane isn't active, we may not have updated it's ddb
> -	 * allocation. In that case since the ddb allocation will be updated
> -	 * once the plane becomes visible, we can skip this check
> -	 */
> -	if (1) {
> -		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> -
> -		hw_plane_wm = &hw->wm.planes[PLANE_CURSOR];
> -		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> -
> -		/* Watermarks */
> -		for (level = 0; level <= max_level; level++) {
> -			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> -						&sw_plane_wm->wm[level]))
> -				continue;
> -
> -			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe), level,
> -				  sw_plane_wm->wm[level].plane_en,
> -				  sw_plane_wm->wm[level].plane_res_b,
> -				  sw_plane_wm->wm[level].plane_res_l,
> -				  hw_plane_wm->wm[level].plane_en,
> -				  hw_plane_wm->wm[level].plane_res_b,
> -				  hw_plane_wm->wm[level].plane_res_l);
> -		}
> -
> -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> -					 &sw_plane_wm->trans_wm)) {
> -			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe),
> -				  sw_plane_wm->trans_wm.plane_en,
> -				  sw_plane_wm->trans_wm.plane_res_b,
> -				  sw_plane_wm->trans_wm.plane_res_l,
> -				  hw_plane_wm->trans_wm.plane_en,
> -				  hw_plane_wm->trans_wm.plane_res_b,
> -				  hw_plane_wm->trans_wm.plane_res_l);
> -		}
> -
> -		/* DDB */
> -		hw_ddb_entry = &hw->ddb_y[PLANE_CURSOR];
> -		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
> -
> -		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> -			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
> -				  pipe_name(pipe),
> -				  sw_ddb_entry->start, sw_ddb_entry->end,
> -				  hw_ddb_entry->start, hw_ddb_entry->end);
> -		}
> -	}
> -
> -	kfree(hw);
> -}
> -
> -static void
> -verify_connector_state(struct drm_device *dev,
> -		       struct drm_atomic_state *state,
> -		       struct drm_crtc *crtc)
> -{
> -	struct drm_connector *connector;
> -	struct drm_connector_state *new_conn_state;
> -	int i;
> -
> -	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> -		struct drm_encoder *encoder = connector->encoder;
> -		struct drm_crtc_state *crtc_state = NULL;
> -
> -		if (new_conn_state->crtc != crtc)
> -			continue;
> -
> -		if (crtc)
> -			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> -
> -		intel_connector_verify_state(crtc_state, new_conn_state);
> -
> -		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
> -		     "connector's atomic encoder doesn't match legacy encoder\n");
> -	}
> -}
> -
> -static void
> -verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
> -{
> -	struct intel_encoder *encoder;
> -	struct drm_connector *connector;
> -	struct drm_connector_state *old_conn_state, *new_conn_state;
> -	int i;
> -
> -	for_each_intel_encoder(dev, encoder) {
> -		bool enabled = false, found = false;
> -		enum pipe pipe;
> -
> -		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> -			      encoder->base.base.id,
> -			      encoder->base.name);
> -
> -		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> -						   new_conn_state, i) {
> -			if (old_conn_state->best_encoder == &encoder->base)
> -				found = true;
> -
> -			if (new_conn_state->best_encoder != &encoder->base)
> -				continue;
> -			found = enabled = true;
> -
> -			I915_STATE_WARN(new_conn_state->crtc !=
> -					encoder->base.crtc,
> -			     "connector's crtc doesn't match encoder crtc\n");
> -		}
> -
> -		if (!found)
> -			continue;
> -
> -		I915_STATE_WARN(!!encoder->base.crtc != enabled,
> -		     "encoder's enabled state mismatch "
> -		     "(expected %i, found %i)\n",
> -		     !!encoder->base.crtc, enabled);
> -
> -		if (!encoder->base.crtc) {
> -			bool active;
> -
> -			active = encoder->get_hw_state(encoder, &pipe);
> -			I915_STATE_WARN(active,
> -			     "encoder detached but still enabled on pipe %c.\n",
> -			     pipe_name(pipe));
> -		}
> -	}
> -}
> -
> -static void
> -verify_crtc_state(struct drm_crtc *crtc,
> -		  struct drm_crtc_state *old_crtc_state,
> -		  struct drm_crtc_state *new_crtc_state)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_encoder *encoder;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *pipe_config, *sw_config;
> -	struct drm_atomic_state *old_state;
> -	bool active;
> -
> -	old_state = old_crtc_state->state;
> -	__drm_atomic_helper_crtc_destroy_state(old_crtc_state);
> -	pipe_config = to_intel_crtc_state(old_crtc_state);
> -	memset(pipe_config, 0, sizeof(*pipe_config));
> -	pipe_config->base.crtc = crtc;
> -	pipe_config->base.state = old_state;
> -
> -	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
> -
> -	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
> -
> -	/* we keep both pipes enabled on 830 */
> -	if (IS_I830(dev_priv))
> -		active = new_crtc_state->active;
> -
> -	I915_STATE_WARN(new_crtc_state->active != active,
> -	     "crtc active state doesn't match with hw state "
> -	     "(expected %i, found %i)\n", new_crtc_state->active, active);
> -
> -	I915_STATE_WARN(intel_crtc->active != new_crtc_state->active,
> -	     "transitional active state does not match atomic hw state "
> -	     "(expected %i, found %i)\n", new_crtc_state->active, intel_crtc->active);
> -
> -	for_each_encoder_on_crtc(dev, crtc, encoder) {
> -		enum pipe pipe;
> -
> -		active = encoder->get_hw_state(encoder, &pipe);
> -		I915_STATE_WARN(active != new_crtc_state->active,
> -			"[ENCODER:%i] active %i with crtc active %i\n",
> -			encoder->base.base.id, active, new_crtc_state->active);
> -
> -		I915_STATE_WARN(active && intel_crtc->pipe != pipe,
> -				"Encoder connected to wrong pipe %c\n",
> -				pipe_name(pipe));
> -
> -		if (active)
> -			encoder->get_config(encoder, pipe_config);
> -	}
> -
> -	intel_crtc_compute_pixel_rate(pipe_config);
> -
> -	if (!new_crtc_state->active)
> -		return;
> -
> -	intel_pipe_config_sanity_check(dev_priv, pipe_config);
> -
> -	sw_config = to_intel_crtc_state(new_crtc_state);
> -	if (!intel_pipe_config_compare(dev_priv, sw_config,
> -				       pipe_config, false)) {
> -		I915_STATE_WARN(1, "pipe state doesn't match!\n");
> -		intel_dump_pipe_config(intel_crtc, pipe_config,
> -				       "[hw state]");
> -		intel_dump_pipe_config(intel_crtc, sw_config,
> -				       "[sw state]");
> -	}
> -}
> -
> -static void
> -intel_verify_planes(struct intel_atomic_state *state)
> -{
> -	struct intel_plane *plane;
> -	const struct intel_plane_state *plane_state;
> -	int i;
> -
> -	for_each_new_intel_plane_in_state(state, plane,
> -					  plane_state, i)
> -		assert_plane(plane, plane_state->slave ||
> -			     plane_state->base.visible);
> -}
> -
> -static void
> -verify_single_dpll_state(struct drm_i915_private *dev_priv,
> -			 struct intel_shared_dpll *pll,
> -			 struct drm_crtc *crtc,
> -			 struct drm_crtc_state *new_state)
> -{
> -	struct intel_dpll_hw_state dpll_hw_state;
> -	unsigned int crtc_mask;
> -	bool active;
> -
> -	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
> -
> -	DRM_DEBUG_KMS("%s\n", pll->info->name);
> -
> -	active = pll->info->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state);
> -
> -	if (!(pll->info->flags & INTEL_DPLL_ALWAYS_ON)) {
> -		I915_STATE_WARN(!pll->on && pll->active_mask,
> -		     "pll in active use but not on in sw tracking\n");
> -		I915_STATE_WARN(pll->on && !pll->active_mask,
> -		     "pll is on but not used by any active crtc\n");
> -		I915_STATE_WARN(pll->on != active,
> -		     "pll on state mismatch (expected %i, found %i)\n",
> -		     pll->on, active);
> -	}
> -
> -	if (!crtc) {
> -		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
> -				"more active pll users than references: %x vs %x\n",
> -				pll->active_mask, pll->state.crtc_mask);
> -
> -		return;
> -	}
> -
> -	crtc_mask = drm_crtc_mask(crtc);
> -
> -	if (new_state->active)
> -		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
> -				"pll active mismatch (expected pipe %c in active mask 0x%02x)\n",
> -				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> -	else
> -		I915_STATE_WARN(pll->active_mask & crtc_mask,
> -				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
> -				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> -
> -	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
> -			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
> -			crtc_mask, pll->state.crtc_mask);
> -
> -	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
> -					  &dpll_hw_state,
> -					  sizeof(dpll_hw_state)),
> -			"pll hw state mismatch\n");
> -}
> -
> -static void
> -verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
> -			 struct drm_crtc_state *old_crtc_state,
> -			 struct drm_crtc_state *new_crtc_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc_state *old_state = to_intel_crtc_state(old_crtc_state);
> -	struct intel_crtc_state *new_state = to_intel_crtc_state(new_crtc_state);
> -
> -	if (new_state->shared_dpll)
> -		verify_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
> -
> -	if (old_state->shared_dpll &&
> -	    old_state->shared_dpll != new_state->shared_dpll) {
> -		unsigned int crtc_mask = drm_crtc_mask(crtc);
> -		struct intel_shared_dpll *pll = old_state->shared_dpll;
> -
> -		I915_STATE_WARN(pll->active_mask & crtc_mask,
> -				"pll active mismatch (didn't expect pipe %c in active mask)\n",
> -				pipe_name(drm_crtc_index(crtc)));
> -		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
> -				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
> -				pipe_name(drm_crtc_index(crtc)));
> -	}
> -}
> -
> -static void
> -intel_modeset_verify_crtc(struct drm_crtc *crtc,
> -			  struct drm_atomic_state *state,
> -			  struct drm_crtc_state *old_state,
> -			  struct drm_crtc_state *new_state)
> -{
> -	if (!needs_modeset(new_state) &&
> -	    !to_intel_crtc_state(new_state)->update_pipe)
> -		return;
> -
> -	verify_wm_state(crtc, new_state);
> -	verify_connector_state(crtc->dev, state, crtc);
> -	verify_crtc_state(crtc, old_state, new_state);
> -	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> -}
> -
> -static void
> -verify_disabled_dpll_state(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int i;
> -
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> -		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> -}
> -
> -static void
> -intel_modeset_verify_disabled(struct drm_device *dev,
> -			      struct drm_atomic_state *state)
> -{
> -	verify_encoder_state(dev, state);
> -	verify_connector_state(dev, state, NULL);
> -	verify_disabled_dpll_state(dev);
> -}
> -
>  static void update_scanline_offset(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -13521,7 +13069,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		if (!intel_can_enable_sagv(state))
>  			intel_disable_sagv(dev_priv);
>  
> -		intel_modeset_verify_disabled(dev, state);
> +		intel_verify_modeset_disabled(dev, state);
>  	}

I think we should take this opportunity to slightly clean up the interface
here, and align a bit with
drm_atomic_helper_commit_modeset_enables/disables from helpers. So
rename this function here to 

intel_verify_modeset_disables(dev, state)

so the naming is a bit clearer - it took me a while what exactly _disabled
here means: Is the verifier disabled, or the modeset is disabled or what
exactly. I think the idea of "modeset_disables" as a phase of an atomic
commit is better established.
>  
>  	/* Complete the events for pipes that have now been disabled */
> @@ -13589,7 +13137,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv, put_domains[i]);
>  
> -		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
> +		intel_verify_modeset_crtc(crtc, state, old_crtc_state, new_crtc_state);
>  	}

Same here, create a new

intel_verify_modeset_enables(dev, state)

which combines the crtc and plan checks (the additional loop wont matter),
and neatly aligns with the idea of a "modeset_enables" phase from atomic
helpers.

With that I think we'll have some really tidy interface into
intel_verify.c, and your patch here has my Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel

>  
>  	if (intel_state->modeset)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a38b9c..77767c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1668,6 +1668,17 @@ int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>  				      struct drm_file *file_priv);
>  enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
>  					     enum pipe pipe);
> +void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> +				    const struct intel_crtc_state *pipe_config);
> +bool
> +intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> +			  struct intel_crtc_state *current_config,
> +			  struct intel_crtc_state *pipe_config,
> +			  bool adjust);
> +void intel_dump_pipe_config(struct intel_crtc *crtc,
> +			    struct intel_crtc_state *pipe_config,
> +			    const char *context);
> +void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state);
>  static inline bool
>  intel_crtc_has_type(const struct intel_crtc_state *crtc_state,
>  		    enum intel_output_type type)
> @@ -1760,6 +1771,7 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
>  void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state);
>  #define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
>  #define assert_pipe_disabled(d, p) assert_pipe(d, p, false)
> +void assert_plane(struct intel_plane *plane, bool state);
>  void intel_prepare_reset(struct drm_i915_private *dev_priv);
>  void intel_finish_reset(struct drm_i915_private *dev_priv);
>  void hsw_enable_pc8(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_verify.c b/drivers/gpu/drm/i915/intel_verify.c
> new file mode 100644
> index 000000..4c8990
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_verify.c
> @@ -0,0 +1,464 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <drm/drm_atomic_state_helper.h>
> +
> +#include "i915_drv.h"
> +#include "intel_drv.h"
> +#include "intel_pm.h"
> +#include "intel_verify.h"
> +
> +static void verify_wm_state(struct drm_crtc *crtc,
> +			    struct drm_crtc_state *new_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct skl_hw_state {
> +		struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
> +		struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
> +		struct skl_ddb_allocation ddb;
> +		struct skl_pipe_wm wm;
> +	} *hw;
> +	struct skl_ddb_allocation *sw_ddb;
> +	struct skl_pipe_wm *sw_wm;
> +	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	const enum pipe pipe = intel_crtc->pipe;
> +	int plane, level, max_level = ilk_wm_max_level(dev_priv);
> +
> +	if (INTEL_GEN(dev_priv) < 9 || !new_state->active)
> +		return;
> +
> +	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> +	if (!hw)
> +		return;
> +
> +	skl_pipe_wm_get_hw_state(intel_crtc, &hw->wm);
> +	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
> +
> +	skl_pipe_ddb_get_hw_state(intel_crtc, hw->ddb_y, hw->ddb_uv);
> +
> +	skl_ddb_get_hw_state(dev_priv, &hw->ddb);
> +	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 &&
> +	    hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> +		DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> +			  sw_ddb->enabled_slices,
> +			  hw->ddb.enabled_slices);
> +
> +	/* planes */
> +	for_each_universal_plane(dev_priv, pipe, plane) {
> +		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> +
> +		hw_plane_wm = &hw->wm.planes[plane];
> +		sw_plane_wm = &sw_wm->planes[plane];
> +
> +		/* Watermarks */
> +		for (level = 0; level <= max_level; level++) {
> +			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> +						&sw_plane_wm->wm[level]))
> +				continue;
> +
> +			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> +				  pipe_name(pipe), plane + 1, level,
> +				  sw_plane_wm->wm[level].plane_en,
> +				  sw_plane_wm->wm[level].plane_res_b,
> +				  sw_plane_wm->wm[level].plane_res_l,
> +				  hw_plane_wm->wm[level].plane_en,
> +				  hw_plane_wm->wm[level].plane_res_b,
> +				  hw_plane_wm->wm[level].plane_res_l);
> +		}
> +
> +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> +					 &sw_plane_wm->trans_wm)) {
> +			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> +				  pipe_name(pipe), plane + 1,
> +				  sw_plane_wm->trans_wm.plane_en,
> +				  sw_plane_wm->trans_wm.plane_res_b,
> +				  sw_plane_wm->trans_wm.plane_res_l,
> +				  hw_plane_wm->trans_wm.plane_en,
> +				  hw_plane_wm->trans_wm.plane_res_b,
> +				  hw_plane_wm->trans_wm.plane_res_l);
> +		}
> +
> +		/* DDB */
> +		hw_ddb_entry = &hw->ddb_y[plane];
> +		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
> +
> +		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> +			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
> +				  pipe_name(pipe), plane + 1,
> +				  sw_ddb_entry->start, sw_ddb_entry->end,
> +				  hw_ddb_entry->start, hw_ddb_entry->end);
> +		}
> +	}
> +
> +	/*
> +	 * cursor
> +	 * If the cursor plane isn't active, we may not have updated it's ddb
> +	 * allocation. In that case since the ddb allocation will be updated
> +	 * once the plane becomes visible, we can skip this check
> +	 */
> +	if (1) {
> +		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> +
> +		hw_plane_wm = &hw->wm.planes[PLANE_CURSOR];
> +		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> +
> +		/* Watermarks */
> +		for (level = 0; level <= max_level; level++) {
> +			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> +						&sw_plane_wm->wm[level]))
> +				continue;
> +
> +			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> +				  pipe_name(pipe), level,
> +				  sw_plane_wm->wm[level].plane_en,
> +				  sw_plane_wm->wm[level].plane_res_b,
> +				  sw_plane_wm->wm[level].plane_res_l,
> +				  hw_plane_wm->wm[level].plane_en,
> +				  hw_plane_wm->wm[level].plane_res_b,
> +				  hw_plane_wm->wm[level].plane_res_l);
> +		}
> +
> +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> +					 &sw_plane_wm->trans_wm)) {
> +			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> +				  pipe_name(pipe),
> +				  sw_plane_wm->trans_wm.plane_en,
> +				  sw_plane_wm->trans_wm.plane_res_b,
> +				  sw_plane_wm->trans_wm.plane_res_l,
> +				  hw_plane_wm->trans_wm.plane_en,
> +				  hw_plane_wm->trans_wm.plane_res_b,
> +				  hw_plane_wm->trans_wm.plane_res_l);
> +		}
> +
> +		/* DDB */
> +		hw_ddb_entry = &hw->ddb_y[PLANE_CURSOR];
> +		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
> +
> +		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> +			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
> +				  pipe_name(pipe),
> +				  sw_ddb_entry->start, sw_ddb_entry->end,
> +				  hw_ddb_entry->start, hw_ddb_entry->end);
> +		}
> +	}
> +
> +	kfree(hw);
> +}
> +
> +/* Cross check the actual hw state with our own modeset state tracking (and it's
> + * internal consistency). */
> +static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
> +					 struct drm_connector_state *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.base.id,
> +		      connector->base.name);
> +
> +	if (connector->get_hw_state(connector)) {
> +		struct intel_encoder *encoder = connector->encoder;
> +
> +		I915_STATE_WARN(!crtc_state,
> +			 "connector enabled without attached crtc\n");
> +
> +		if (!crtc_state)
> +			return;
> +
> +		I915_STATE_WARN(!crtc_state->active,
> +		      "connector is active, but attached crtc isn't\n");
> +
> +		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> +			return;
> +
> +		I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> +			"atomic encoder doesn't match attached encoder\n");
> +
> +		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> +			"attached encoder crtc differs from connector crtc\n");
> +	} else {
> +		I915_STATE_WARN(crtc_state && crtc_state->active,
> +			"attached crtc is active, but connector isn't\n");
> +		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
> +			"best encoder set without crtc!\n");
> +	}
> +}
> +
> +static void
> +verify_connector_state(struct drm_device *dev,
> +		       struct drm_atomic_state *state,
> +		       struct drm_crtc *crtc)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct drm_encoder *encoder = connector->encoder;
> +		struct drm_crtc_state *crtc_state = NULL;
> +
> +		if (new_conn_state->crtc != crtc)
> +			continue;
> +
> +		if (crtc)
> +			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> +
> +		intel_connector_verify_state(crtc_state, new_conn_state);
> +
> +		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
> +		     "connector's atomic encoder doesn't match legacy encoder\n");
> +	}
> +}
> +
> +static void
> +verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	struct intel_encoder *encoder;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	int i;
> +
> +	for_each_intel_encoder(dev, encoder) {
> +		bool enabled = false, found = false;
> +		enum pipe pipe;
> +
> +		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> +			      encoder->base.base.id,
> +			      encoder->base.name);
> +
> +		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> +						   new_conn_state, i) {
> +			if (old_conn_state->best_encoder == &encoder->base)
> +				found = true;
> +
> +			if (new_conn_state->best_encoder != &encoder->base)
> +				continue;
> +			found = enabled = true;
> +
> +			I915_STATE_WARN(new_conn_state->crtc !=
> +					encoder->base.crtc,
> +			     "connector's crtc doesn't match encoder crtc\n");
> +		}
> +
> +		if (!found)
> +			continue;
> +
> +		I915_STATE_WARN(!!encoder->base.crtc != enabled,
> +		     "encoder's enabled state mismatch "
> +		     "(expected %i, found %i)\n",
> +		     !!encoder->base.crtc, enabled);
> +
> +		if (!encoder->base.crtc) {
> +			bool active;
> +
> +			active = encoder->get_hw_state(encoder, &pipe);
> +			I915_STATE_WARN(active,
> +			     "encoder detached but still enabled on pipe %c.\n",
> +			     pipe_name(pipe));
> +		}
> +	}
> +}
> +
> +static void
> +verify_crtc_state(struct drm_crtc *crtc,
> +		  struct drm_crtc_state *old_crtc_state,
> +		  struct drm_crtc_state *new_crtc_state)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *pipe_config, *sw_config;
> +	struct drm_atomic_state *old_state;
> +	bool active;
> +
> +	old_state = old_crtc_state->state;
> +	__drm_atomic_helper_crtc_destroy_state(old_crtc_state);
> +	pipe_config = to_intel_crtc_state(old_crtc_state);
> +	memset(pipe_config, 0, sizeof(*pipe_config));
> +	pipe_config->base.crtc = crtc;
> +	pipe_config->base.state = old_state;
> +
> +	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
> +
> +	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
> +
> +	/* we keep both pipes enabled on 830 */
> +	if (IS_I830(dev_priv))
> +		active = new_crtc_state->active;
> +
> +	I915_STATE_WARN(new_crtc_state->active != active,
> +	     "crtc active state doesn't match with hw state "
> +	     "(expected %i, found %i)\n", new_crtc_state->active, active);
> +
> +	I915_STATE_WARN(intel_crtc->active != new_crtc_state->active,
> +	     "transitional active state does not match atomic hw state "
> +	     "(expected %i, found %i)\n", new_crtc_state->active, intel_crtc->active);
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder) {
> +		enum pipe pipe;
> +
> +		active = encoder->get_hw_state(encoder, &pipe);
> +		I915_STATE_WARN(active != new_crtc_state->active,
> +			"[ENCODER:%i] active %i with crtc active %i\n",
> +			encoder->base.base.id, active, new_crtc_state->active);
> +
> +		I915_STATE_WARN(active && intel_crtc->pipe != pipe,
> +				"Encoder connected to wrong pipe %c\n",
> +				pipe_name(pipe));
> +
> +		if (active)
> +			encoder->get_config(encoder, pipe_config);
> +	}
> +
> +	intel_crtc_compute_pixel_rate(pipe_config);
> +
> +	if (!new_crtc_state->active)
> +		return;
> +
> +	intel_pipe_config_sanity_check(dev_priv, pipe_config);
> +
> +	sw_config = to_intel_crtc_state(new_crtc_state);
> +	if (!intel_pipe_config_compare(dev_priv, sw_config,
> +				       pipe_config, false)) {
> +		I915_STATE_WARN(1, "pipe state doesn't match!\n");
> +		intel_dump_pipe_config(intel_crtc, pipe_config,
> +				       "[hw state]");
> +		intel_dump_pipe_config(intel_crtc, sw_config,
> +				       "[sw state]");
> +	}
> +}
> +
> +void
> +intel_verify_planes(struct intel_atomic_state *state)
> +{
> +	struct intel_plane *plane;
> +	const struct intel_plane_state *plane_state;
> +	int i;
> +
> +	for_each_new_intel_plane_in_state(state, plane,
> +					  plane_state, i)
> +		assert_plane(plane, plane_state->slave ||
> +			     plane_state->base.visible);
> +}
> +
> +static void
> +verify_single_dpll_state(struct drm_i915_private *dev_priv,
> +			 struct intel_shared_dpll *pll,
> +			 struct drm_crtc *crtc,
> +			 struct drm_crtc_state *new_state)
> +{
> +	struct intel_dpll_hw_state dpll_hw_state;
> +	unsigned int crtc_mask;
> +	bool active;
> +
> +	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
> +
> +	DRM_DEBUG_KMS("%s\n", pll->info->name);
> +
> +	active = pll->info->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state);
> +
> +	if (!(pll->info->flags & INTEL_DPLL_ALWAYS_ON)) {
> +		I915_STATE_WARN(!pll->on && pll->active_mask,
> +		     "pll in active use but not on in sw tracking\n");
> +		I915_STATE_WARN(pll->on && !pll->active_mask,
> +		     "pll is on but not used by any active crtc\n");
> +		I915_STATE_WARN(pll->on != active,
> +		     "pll on state mismatch (expected %i, found %i)\n",
> +		     pll->on, active);
> +	}
> +
> +	if (!crtc) {
> +		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
> +				"more active pll users than references: %x vs %x\n",
> +				pll->active_mask, pll->state.crtc_mask);
> +
> +		return;
> +	}
> +
> +	crtc_mask = drm_crtc_mask(crtc);
> +
> +	if (new_state->active)
> +		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
> +				"pll active mismatch (expected pipe %c in active mask 0x%02x)\n",
> +				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> +	else
> +		I915_STATE_WARN(pll->active_mask & crtc_mask,
> +				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
> +				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> +
> +	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
> +			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
> +			crtc_mask, pll->state.crtc_mask);
> +
> +	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
> +					  &dpll_hw_state,
> +					  sizeof(dpll_hw_state)),
> +			"pll hw state mismatch\n");
> +}
> +
> +static void
> +verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
> +			 struct drm_crtc_state *old_crtc_state,
> +			 struct drm_crtc_state *new_crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc_state *old_state = to_intel_crtc_state(old_crtc_state);
> +	struct intel_crtc_state *new_state = to_intel_crtc_state(new_crtc_state);
> +
> +	if (new_state->shared_dpll)
> +		verify_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
> +
> +	if (old_state->shared_dpll &&
> +	    old_state->shared_dpll != new_state->shared_dpll) {
> +		unsigned int crtc_mask = drm_crtc_mask(crtc);
> +		struct intel_shared_dpll *pll = old_state->shared_dpll;
> +
> +		I915_STATE_WARN(pll->active_mask & crtc_mask,
> +				"pll active mismatch (didn't expect pipe %c in active mask)\n",
> +				pipe_name(drm_crtc_index(crtc)));
> +		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
> +				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
> +				pipe_name(drm_crtc_index(crtc)));
> +	}
> +}
> +
> +void
> +intel_verify_modeset_crtc(struct drm_crtc *crtc,
> +			  struct drm_atomic_state *state,
> +			  struct drm_crtc_state *old_state,
> +			  struct drm_crtc_state *new_state)
> +{
> +	if (!drm_atomic_crtc_needs_modeset(new_state) &&
> +	    !to_intel_crtc_state(new_state)->update_pipe)
> +		return;
> +
> +	verify_wm_state(crtc, new_state);
> +	verify_connector_state(crtc->dev, state, crtc);
> +	verify_crtc_state(crtc, old_state, new_state);
> +	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> +}
> +
> +static void
> +verify_disabled_dpll_state(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int i;
> +
> +	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> +		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> +}
> +
> +void
> +intel_verify_modeset_disabled(struct drm_device *dev,
> +			      struct drm_atomic_state *state)
> +{
> +	verify_encoder_state(dev, state);
> +	verify_connector_state(dev, state, NULL);
> +	verify_disabled_dpll_state(dev);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_verify.h b/drivers/gpu/drm/i915/intel_verify.h
> new file mode 100644
> index 000000..4b751ea
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_verify.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __INTEL_VERIFY_H__
> +#define __INTEL_VERIFY_H__
> +
> +struct drm_atomic_state;
> +struct drm_crtc;
> +struct drm_crtc_state;
> +struct drm_device;
> +
> +void intel_verify_modeset_crtc(struct drm_crtc *crtc,
> +			       struct drm_atomic_state *state,
> +			       struct drm_crtc_state *old_state,
> +			       struct drm_crtc_state *new_state);
> +void intel_verify_modeset_disabled(struct drm_device *dev,
> +				   struct drm_atomic_state *state);
> +void intel_verify_planes(struct intel_atomic_state *state);
> +
> +#endif /* __INTEL_VERIFY_H__ */
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 15, 2019 at 11:33:31AM +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2019 at 01:36:02PM +0300, Jani Nikula wrote:
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/Makefile        |   1 +
> >  drivers/gpu/drm/i915/intel_display.c | 474 +--------------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  12 +
> >  drivers/gpu/drm/i915/intel_verify.c  | 464 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_verify.h  |  22 ++
> >  5 files changed, 510 insertions(+), 463 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/intel_verify.c
> >  create mode 100644 drivers/gpu/drm/i915/intel_verify.h
> > 
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index fbcb0904..a000fad 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -166,6 +166,7 @@ i915-y += dvo_ch7017.o \
> >  	  intel_panel.o \
> >  	  intel_sdvo.o \
> >  	  intel_tv.o \
> > +	  intel_verify.o \
> >  	  vlv_dsi.o \
> >  	  vlv_dsi_pll.o \
> >  	  intel_vdsc.o
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3bd40a..31a931 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -69,6 +69,7 @@
> >  #include "intel_sdvo.h"
> >  #include "intel_sprite.h"
> >  #include "intel_tv.h"
> > +#include "intel_verify.h"
> >  
> >  /* Primary plane formats for gen <= 3 */
> >  static const u32 i8xx_primary_formats[] = {
> > @@ -1243,7 +1244,7 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >  			pipe_name(pipe), onoff(state), onoff(cur_state));
> >  }
> >  
> > -static void assert_plane(struct intel_plane *plane, bool state)
> > +void assert_plane(struct intel_plane *plane, bool state)
> >  {
> >  	enum pipe pipe;
> >  	bool cur_state;
> > @@ -6607,45 +6608,6 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
> >  	kfree(intel_encoder);
> >  }
> >  
> > -/* Cross check the actual hw state with our own modeset state tracking (and it's
> > - * internal consistency). */
> > -static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
> > -					 struct drm_connector_state *conn_state)
> > -{
> > -	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> > -
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > -		      connector->base.base.id,
> > -		      connector->base.name);
> > -
> > -	if (connector->get_hw_state(connector)) {
> > -		struct intel_encoder *encoder = connector->encoder;
> > -
> > -		I915_STATE_WARN(!crtc_state,
> > -			 "connector enabled without attached crtc\n");
> > -
> > -		if (!crtc_state)
> > -			return;
> > -
> > -		I915_STATE_WARN(!crtc_state->active,
> > -		      "connector is active, but attached crtc isn't\n");
> > -
> > -		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> > -			return;
> > -
> > -		I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> > -			"atomic encoder doesn't match attached encoder\n");
> > -
> > -		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> > -			"attached encoder crtc differs from connector crtc\n");
> > -	} else {
> > -		I915_STATE_WARN(crtc_state && crtc_state->active,
> > -			"attached crtc is active, but connector isn't\n");
> > -		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
> > -			"best encoder set without crtc!\n");
> > -	}
> > -}
> > -
> >  static int pipe_required_fdi_lanes(struct intel_crtc_state *crtc_state)
> >  {
> >  	if (crtc_state->base.enable && crtc_state->has_pch_encoder)
> > @@ -6879,7 +6841,7 @@ static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
> >  	return pixel_rate;
> >  }
> >  
> > -static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> > +void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >  
> > @@ -11584,9 +11546,9 @@ static const char *output_formats(enum intel_output_format format)
> >  	return output_format_str[format];
> >  }
> >  
> > -static void intel_dump_pipe_config(struct intel_crtc *crtc,
> > -				   struct intel_crtc_state *pipe_config,
> > -				   const char *context)
> > +void intel_dump_pipe_config(struct intel_crtc *crtc,
> > +			    struct intel_crtc_state *pipe_config,
> > +			    const char *context)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -12076,7 +12038,7 @@ static bool fastboot_enabled(struct drm_i915_private *dev_priv)
> >  	return false;
> >  }
> >  
> > -static bool
> > +bool
> >  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  			  struct intel_crtc_state *current_config,
> >  			  struct intel_crtc_state *pipe_config,
> > @@ -12389,8 +12351,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  	return ret;
> >  }
> >  
> > -static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> > -					   const struct intel_crtc_state *pipe_config)
> > +void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> > +				    const struct intel_crtc_state *pipe_config)
> >  {
> >  	if (pipe_config->has_pch_encoder) {
> >  		int fdi_dotclock = intel_dotclock_calculate(intel_fdi_link_freq(dev_priv, pipe_config),
> > @@ -12407,420 +12369,6 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > -static void verify_wm_state(struct drm_crtc *crtc,
> > -			    struct drm_crtc_state *new_state)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > -	struct skl_hw_state {
> > -		struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
> > -		struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
> > -		struct skl_ddb_allocation ddb;
> > -		struct skl_pipe_wm wm;
> > -	} *hw;
> > -	struct skl_ddb_allocation *sw_ddb;
> > -	struct skl_pipe_wm *sw_wm;
> > -	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	const enum pipe pipe = intel_crtc->pipe;
> > -	int plane, level, max_level = ilk_wm_max_level(dev_priv);
> > -
> > -	if (INTEL_GEN(dev_priv) < 9 || !new_state->active)
> > -		return;
> > -
> > -	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> > -	if (!hw)
> > -		return;
> > -
> > -	skl_pipe_wm_get_hw_state(intel_crtc, &hw->wm);
> > -	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
> > -
> > -	skl_pipe_ddb_get_hw_state(intel_crtc, hw->ddb_y, hw->ddb_uv);
> > -
> > -	skl_ddb_get_hw_state(dev_priv, &hw->ddb);
> > -	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> > -
> > -	if (INTEL_GEN(dev_priv) >= 11 &&
> > -	    hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> > -		DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> > -			  sw_ddb->enabled_slices,
> > -			  hw->ddb.enabled_slices);
> > -
> > -	/* planes */
> > -	for_each_universal_plane(dev_priv, pipe, plane) {
> > -		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> > -
> > -		hw_plane_wm = &hw->wm.planes[plane];
> > -		sw_plane_wm = &sw_wm->planes[plane];
> > -
> > -		/* Watermarks */
> > -		for (level = 0; level <= max_level; level++) {
> > -			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> > -						&sw_plane_wm->wm[level]))
> > -				continue;
> > -
> > -			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > -				  pipe_name(pipe), plane + 1, level,
> > -				  sw_plane_wm->wm[level].plane_en,
> > -				  sw_plane_wm->wm[level].plane_res_b,
> > -				  sw_plane_wm->wm[level].plane_res_l,
> > -				  hw_plane_wm->wm[level].plane_en,
> > -				  hw_plane_wm->wm[level].plane_res_b,
> > -				  hw_plane_wm->wm[level].plane_res_l);
> > -		}
> > -
> > -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > -					 &sw_plane_wm->trans_wm)) {
> > -			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > -				  pipe_name(pipe), plane + 1,
> > -				  sw_plane_wm->trans_wm.plane_en,
> > -				  sw_plane_wm->trans_wm.plane_res_b,
> > -				  sw_plane_wm->trans_wm.plane_res_l,
> > -				  hw_plane_wm->trans_wm.plane_en,
> > -				  hw_plane_wm->trans_wm.plane_res_b,
> > -				  hw_plane_wm->trans_wm.plane_res_l);
> > -		}
> > -
> > -		/* DDB */
> > -		hw_ddb_entry = &hw->ddb_y[plane];
> > -		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
> > -
> > -		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> > -			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
> > -				  pipe_name(pipe), plane + 1,
> > -				  sw_ddb_entry->start, sw_ddb_entry->end,
> > -				  hw_ddb_entry->start, hw_ddb_entry->end);
> > -		}
> > -	}
> > -
> > -	/*
> > -	 * cursor
> > -	 * If the cursor plane isn't active, we may not have updated it's ddb
> > -	 * allocation. In that case since the ddb allocation will be updated
> > -	 * once the plane becomes visible, we can skip this check
> > -	 */
> > -	if (1) {
> > -		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> > -
> > -		hw_plane_wm = &hw->wm.planes[PLANE_CURSOR];
> > -		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> > -
> > -		/* Watermarks */
> > -		for (level = 0; level <= max_level; level++) {
> > -			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> > -						&sw_plane_wm->wm[level]))
> > -				continue;
> > -
> > -			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > -				  pipe_name(pipe), level,
> > -				  sw_plane_wm->wm[level].plane_en,
> > -				  sw_plane_wm->wm[level].plane_res_b,
> > -				  sw_plane_wm->wm[level].plane_res_l,
> > -				  hw_plane_wm->wm[level].plane_en,
> > -				  hw_plane_wm->wm[level].plane_res_b,
> > -				  hw_plane_wm->wm[level].plane_res_l);
> > -		}
> > -
> > -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > -					 &sw_plane_wm->trans_wm)) {
> > -			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > -				  pipe_name(pipe),
> > -				  sw_plane_wm->trans_wm.plane_en,
> > -				  sw_plane_wm->trans_wm.plane_res_b,
> > -				  sw_plane_wm->trans_wm.plane_res_l,
> > -				  hw_plane_wm->trans_wm.plane_en,
> > -				  hw_plane_wm->trans_wm.plane_res_b,
> > -				  hw_plane_wm->trans_wm.plane_res_l);
> > -		}
> > -
> > -		/* DDB */
> > -		hw_ddb_entry = &hw->ddb_y[PLANE_CURSOR];
> > -		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
> > -
> > -		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> > -			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
> > -				  pipe_name(pipe),
> > -				  sw_ddb_entry->start, sw_ddb_entry->end,
> > -				  hw_ddb_entry->start, hw_ddb_entry->end);
> > -		}
> > -	}
> > -
> > -	kfree(hw);
> > -}
> > -
> > -static void
> > -verify_connector_state(struct drm_device *dev,
> > -		       struct drm_atomic_state *state,
> > -		       struct drm_crtc *crtc)
> > -{
> > -	struct drm_connector *connector;
> > -	struct drm_connector_state *new_conn_state;
> > -	int i;
> > -
> > -	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> > -		struct drm_encoder *encoder = connector->encoder;
> > -		struct drm_crtc_state *crtc_state = NULL;
> > -
> > -		if (new_conn_state->crtc != crtc)
> > -			continue;
> > -
> > -		if (crtc)
> > -			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> > -
> > -		intel_connector_verify_state(crtc_state, new_conn_state);
> > -
> > -		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
> > -		     "connector's atomic encoder doesn't match legacy encoder\n");
> > -	}
> > -}
> > -
> > -static void
> > -verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
> > -{
> > -	struct intel_encoder *encoder;
> > -	struct drm_connector *connector;
> > -	struct drm_connector_state *old_conn_state, *new_conn_state;
> > -	int i;
> > -
> > -	for_each_intel_encoder(dev, encoder) {
> > -		bool enabled = false, found = false;
> > -		enum pipe pipe;
> > -
> > -		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> > -			      encoder->base.base.id,
> > -			      encoder->base.name);
> > -
> > -		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> > -						   new_conn_state, i) {
> > -			if (old_conn_state->best_encoder == &encoder->base)
> > -				found = true;
> > -
> > -			if (new_conn_state->best_encoder != &encoder->base)
> > -				continue;
> > -			found = enabled = true;
> > -
> > -			I915_STATE_WARN(new_conn_state->crtc !=
> > -					encoder->base.crtc,
> > -			     "connector's crtc doesn't match encoder crtc\n");
> > -		}
> > -
> > -		if (!found)
> > -			continue;
> > -
> > -		I915_STATE_WARN(!!encoder->base.crtc != enabled,
> > -		     "encoder's enabled state mismatch "
> > -		     "(expected %i, found %i)\n",
> > -		     !!encoder->base.crtc, enabled);
> > -
> > -		if (!encoder->base.crtc) {
> > -			bool active;
> > -
> > -			active = encoder->get_hw_state(encoder, &pipe);
> > -			I915_STATE_WARN(active,
> > -			     "encoder detached but still enabled on pipe %c.\n",
> > -			     pipe_name(pipe));
> > -		}
> > -	}
> > -}
> > -
> > -static void
> > -verify_crtc_state(struct drm_crtc *crtc,
> > -		  struct drm_crtc_state *old_crtc_state,
> > -		  struct drm_crtc_state *new_crtc_state)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_encoder *encoder;
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_crtc_state *pipe_config, *sw_config;
> > -	struct drm_atomic_state *old_state;
> > -	bool active;
> > -
> > -	old_state = old_crtc_state->state;
> > -	__drm_atomic_helper_crtc_destroy_state(old_crtc_state);
> > -	pipe_config = to_intel_crtc_state(old_crtc_state);
> > -	memset(pipe_config, 0, sizeof(*pipe_config));
> > -	pipe_config->base.crtc = crtc;
> > -	pipe_config->base.state = old_state;
> > -
> > -	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
> > -
> > -	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
> > -
> > -	/* we keep both pipes enabled on 830 */
> > -	if (IS_I830(dev_priv))
> > -		active = new_crtc_state->active;
> > -
> > -	I915_STATE_WARN(new_crtc_state->active != active,
> > -	     "crtc active state doesn't match with hw state "
> > -	     "(expected %i, found %i)\n", new_crtc_state->active, active);
> > -
> > -	I915_STATE_WARN(intel_crtc->active != new_crtc_state->active,
> > -	     "transitional active state does not match atomic hw state "
> > -	     "(expected %i, found %i)\n", new_crtc_state->active, intel_crtc->active);
> > -
> > -	for_each_encoder_on_crtc(dev, crtc, encoder) {
> > -		enum pipe pipe;
> > -
> > -		active = encoder->get_hw_state(encoder, &pipe);
> > -		I915_STATE_WARN(active != new_crtc_state->active,
> > -			"[ENCODER:%i] active %i with crtc active %i\n",
> > -			encoder->base.base.id, active, new_crtc_state->active);
> > -
> > -		I915_STATE_WARN(active && intel_crtc->pipe != pipe,
> > -				"Encoder connected to wrong pipe %c\n",
> > -				pipe_name(pipe));
> > -
> > -		if (active)
> > -			encoder->get_config(encoder, pipe_config);
> > -	}
> > -
> > -	intel_crtc_compute_pixel_rate(pipe_config);
> > -
> > -	if (!new_crtc_state->active)
> > -		return;
> > -
> > -	intel_pipe_config_sanity_check(dev_priv, pipe_config);
> > -
> > -	sw_config = to_intel_crtc_state(new_crtc_state);
> > -	if (!intel_pipe_config_compare(dev_priv, sw_config,
> > -				       pipe_config, false)) {
> > -		I915_STATE_WARN(1, "pipe state doesn't match!\n");
> > -		intel_dump_pipe_config(intel_crtc, pipe_config,
> > -				       "[hw state]");
> > -		intel_dump_pipe_config(intel_crtc, sw_config,
> > -				       "[sw state]");
> > -	}
> > -}
> > -
> > -static void
> > -intel_verify_planes(struct intel_atomic_state *state)
> > -{
> > -	struct intel_plane *plane;
> > -	const struct intel_plane_state *plane_state;
> > -	int i;
> > -
> > -	for_each_new_intel_plane_in_state(state, plane,
> > -					  plane_state, i)
> > -		assert_plane(plane, plane_state->slave ||
> > -			     plane_state->base.visible);
> > -}
> > -
> > -static void
> > -verify_single_dpll_state(struct drm_i915_private *dev_priv,
> > -			 struct intel_shared_dpll *pll,
> > -			 struct drm_crtc *crtc,
> > -			 struct drm_crtc_state *new_state)
> > -{
> > -	struct intel_dpll_hw_state dpll_hw_state;
> > -	unsigned int crtc_mask;
> > -	bool active;
> > -
> > -	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
> > -
> > -	DRM_DEBUG_KMS("%s\n", pll->info->name);
> > -
> > -	active = pll->info->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state);
> > -
> > -	if (!(pll->info->flags & INTEL_DPLL_ALWAYS_ON)) {
> > -		I915_STATE_WARN(!pll->on && pll->active_mask,
> > -		     "pll in active use but not on in sw tracking\n");
> > -		I915_STATE_WARN(pll->on && !pll->active_mask,
> > -		     "pll is on but not used by any active crtc\n");
> > -		I915_STATE_WARN(pll->on != active,
> > -		     "pll on state mismatch (expected %i, found %i)\n",
> > -		     pll->on, active);
> > -	}
> > -
> > -	if (!crtc) {
> > -		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
> > -				"more active pll users than references: %x vs %x\n",
> > -				pll->active_mask, pll->state.crtc_mask);
> > -
> > -		return;
> > -	}
> > -
> > -	crtc_mask = drm_crtc_mask(crtc);
> > -
> > -	if (new_state->active)
> > -		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
> > -				"pll active mismatch (expected pipe %c in active mask 0x%02x)\n",
> > -				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> > -	else
> > -		I915_STATE_WARN(pll->active_mask & crtc_mask,
> > -				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
> > -				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> > -
> > -	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
> > -			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
> > -			crtc_mask, pll->state.crtc_mask);
> > -
> > -	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
> > -					  &dpll_hw_state,
> > -					  sizeof(dpll_hw_state)),
> > -			"pll hw state mismatch\n");
> > -}
> > -
> > -static void
> > -verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
> > -			 struct drm_crtc_state *old_crtc_state,
> > -			 struct drm_crtc_state *new_crtc_state)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_crtc_state *old_state = to_intel_crtc_state(old_crtc_state);
> > -	struct intel_crtc_state *new_state = to_intel_crtc_state(new_crtc_state);
> > -
> > -	if (new_state->shared_dpll)
> > -		verify_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
> > -
> > -	if (old_state->shared_dpll &&
> > -	    old_state->shared_dpll != new_state->shared_dpll) {
> > -		unsigned int crtc_mask = drm_crtc_mask(crtc);
> > -		struct intel_shared_dpll *pll = old_state->shared_dpll;
> > -
> > -		I915_STATE_WARN(pll->active_mask & crtc_mask,
> > -				"pll active mismatch (didn't expect pipe %c in active mask)\n",
> > -				pipe_name(drm_crtc_index(crtc)));
> > -		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
> > -				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
> > -				pipe_name(drm_crtc_index(crtc)));
> > -	}
> > -}
> > -
> > -static void
> > -intel_modeset_verify_crtc(struct drm_crtc *crtc,
> > -			  struct drm_atomic_state *state,
> > -			  struct drm_crtc_state *old_state,
> > -			  struct drm_crtc_state *new_state)
> > -{
> > -	if (!needs_modeset(new_state) &&
> > -	    !to_intel_crtc_state(new_state)->update_pipe)
> > -		return;
> > -
> > -	verify_wm_state(crtc, new_state);
> > -	verify_connector_state(crtc->dev, state, crtc);
> > -	verify_crtc_state(crtc, old_state, new_state);
> > -	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> > -}
> > -
> > -static void
> > -verify_disabled_dpll_state(struct drm_device *dev)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	int i;
> > -
> > -	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> > -		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> > -}
> > -
> > -static void
> > -intel_modeset_verify_disabled(struct drm_device *dev,
> > -			      struct drm_atomic_state *state)
> > -{
> > -	verify_encoder_state(dev, state);
> > -	verify_connector_state(dev, state, NULL);
> > -	verify_disabled_dpll_state(dev);
> > -}
> > -
> >  static void update_scanline_offset(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > @@ -13521,7 +13069,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  		if (!intel_can_enable_sagv(state))
> >  			intel_disable_sagv(dev_priv);
> >  
> > -		intel_modeset_verify_disabled(dev, state);
> > +		intel_verify_modeset_disabled(dev, state);
> >  	}
> 
> I think we should take this opportunity to slightly clean up the interface
> here, and align a bit with
> drm_atomic_helper_commit_modeset_enables/disables from helpers. So
> rename this function here to 
> 
> intel_verify_modeset_disables(dev, state)
> 
> so the naming is a bit clearer - it took me a while what exactly _disabled
> here means: Is the verifier disabled, or the modeset is disabled or what
> exactly. I think the idea of "modeset_disables" as a phase of an atomic
> commit is better established.
> >  
> >  	/* Complete the events for pipes that have now been disabled */
> > @@ -13589,7 +13137,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  		if (put_domains[i])
> >  			modeset_put_power_domains(dev_priv, put_domains[i]);
> >  
> > -		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
> > +		intel_verify_modeset_crtc(crtc, state, old_crtc_state, new_crtc_state);
> >  	}
> 
> Same here, create a new
> 
> intel_verify_modeset_enables(dev, state)
> 
> which combines the crtc and plan checks (the additional loop wont matter),
> and neatly aligns with the idea of a "modeset_enables" phase from atomic
> helpers.
> 
> With that I think we'll have some really tidy interface into
> intel_verify.c, and your patch here has my Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Bonus if you can throw some kerneldoc for these on top, explaining the
link with core helper concepts for the actual functions, plus a short
intro on wtf this is. Because the idea behind our state verifier seems
rather elusive to newbies. But that would be a 2nd patch I think.

I can also help with the docs if you want.
-Daniel

> 
> Cheers, Daniel
> 
> >  
> >  	if (intel_state->modeset)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a38b9c..77767c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1668,6 +1668,17 @@ int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> >  				      struct drm_file *file_priv);
> >  enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
> >  					     enum pipe pipe);
> > +void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
> > +				    const struct intel_crtc_state *pipe_config);
> > +bool
> > +intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > +			  struct intel_crtc_state *current_config,
> > +			  struct intel_crtc_state *pipe_config,
> > +			  bool adjust);
> > +void intel_dump_pipe_config(struct intel_crtc *crtc,
> > +			    struct intel_crtc_state *pipe_config,
> > +			    const char *context);
> > +void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state);
> >  static inline bool
> >  intel_crtc_has_type(const struct intel_crtc_state *crtc_state,
> >  		    enum intel_output_type type)
> > @@ -1760,6 +1771,7 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
> >  void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state);
> >  #define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
> >  #define assert_pipe_disabled(d, p) assert_pipe(d, p, false)
> > +void assert_plane(struct intel_plane *plane, bool state);
> >  void intel_prepare_reset(struct drm_i915_private *dev_priv);
> >  void intel_finish_reset(struct drm_i915_private *dev_priv);
> >  void hsw_enable_pc8(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_verify.c b/drivers/gpu/drm/i915/intel_verify.c
> > new file mode 100644
> > index 000000..4c8990
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_verify.c
> > @@ -0,0 +1,464 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + */
> > +
> > +#include <drm/drm_atomic_state_helper.h>
> > +
> > +#include "i915_drv.h"
> > +#include "intel_drv.h"
> > +#include "intel_pm.h"
> > +#include "intel_verify.h"
> > +
> > +static void verify_wm_state(struct drm_crtc *crtc,
> > +			    struct drm_crtc_state *new_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	struct skl_hw_state {
> > +		struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
> > +		struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
> > +		struct skl_ddb_allocation ddb;
> > +		struct skl_pipe_wm wm;
> > +	} *hw;
> > +	struct skl_ddb_allocation *sw_ddb;
> > +	struct skl_pipe_wm *sw_wm;
> > +	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	const enum pipe pipe = intel_crtc->pipe;
> > +	int plane, level, max_level = ilk_wm_max_level(dev_priv);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9 || !new_state->active)
> > +		return;
> > +
> > +	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> > +	if (!hw)
> > +		return;
> > +
> > +	skl_pipe_wm_get_hw_state(intel_crtc, &hw->wm);
> > +	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
> > +
> > +	skl_pipe_ddb_get_hw_state(intel_crtc, hw->ddb_y, hw->ddb_uv);
> > +
> > +	skl_ddb_get_hw_state(dev_priv, &hw->ddb);
> > +	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> > +
> > +	if (INTEL_GEN(dev_priv) >= 11 &&
> > +	    hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> > +		DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> > +			  sw_ddb->enabled_slices,
> > +			  hw->ddb.enabled_slices);
> > +
> > +	/* planes */
> > +	for_each_universal_plane(dev_priv, pipe, plane) {
> > +		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> > +
> > +		hw_plane_wm = &hw->wm.planes[plane];
> > +		sw_plane_wm = &sw_wm->planes[plane];
> > +
> > +		/* Watermarks */
> > +		for (level = 0; level <= max_level; level++) {
> > +			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> > +						&sw_plane_wm->wm[level]))
> > +				continue;
> > +
> > +			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > +				  pipe_name(pipe), plane + 1, level,
> > +				  sw_plane_wm->wm[level].plane_en,
> > +				  sw_plane_wm->wm[level].plane_res_b,
> > +				  sw_plane_wm->wm[level].plane_res_l,
> > +				  hw_plane_wm->wm[level].plane_en,
> > +				  hw_plane_wm->wm[level].plane_res_b,
> > +				  hw_plane_wm->wm[level].plane_res_l);
> > +		}
> > +
> > +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > +					 &sw_plane_wm->trans_wm)) {
> > +			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > +				  pipe_name(pipe), plane + 1,
> > +				  sw_plane_wm->trans_wm.plane_en,
> > +				  sw_plane_wm->trans_wm.plane_res_b,
> > +				  sw_plane_wm->trans_wm.plane_res_l,
> > +				  hw_plane_wm->trans_wm.plane_en,
> > +				  hw_plane_wm->trans_wm.plane_res_b,
> > +				  hw_plane_wm->trans_wm.plane_res_l);
> > +		}
> > +
> > +		/* DDB */
> > +		hw_ddb_entry = &hw->ddb_y[plane];
> > +		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
> > +
> > +		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> > +			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
> > +				  pipe_name(pipe), plane + 1,
> > +				  sw_ddb_entry->start, sw_ddb_entry->end,
> > +				  hw_ddb_entry->start, hw_ddb_entry->end);
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * cursor
> > +	 * If the cursor plane isn't active, we may not have updated it's ddb
> > +	 * allocation. In that case since the ddb allocation will be updated
> > +	 * once the plane becomes visible, we can skip this check
> > +	 */
> > +	if (1) {
> > +		struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> > +
> > +		hw_plane_wm = &hw->wm.planes[PLANE_CURSOR];
> > +		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> > +
> > +		/* Watermarks */
> > +		for (level = 0; level <= max_level; level++) {
> > +			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> > +						&sw_plane_wm->wm[level]))
> > +				continue;
> > +
> > +			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > +				  pipe_name(pipe), level,
> > +				  sw_plane_wm->wm[level].plane_en,
> > +				  sw_plane_wm->wm[level].plane_res_b,
> > +				  sw_plane_wm->wm[level].plane_res_l,
> > +				  hw_plane_wm->wm[level].plane_en,
> > +				  hw_plane_wm->wm[level].plane_res_b,
> > +				  hw_plane_wm->wm[level].plane_res_l);
> > +		}
> > +
> > +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > +					 &sw_plane_wm->trans_wm)) {
> > +			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > +				  pipe_name(pipe),
> > +				  sw_plane_wm->trans_wm.plane_en,
> > +				  sw_plane_wm->trans_wm.plane_res_b,
> > +				  sw_plane_wm->trans_wm.plane_res_l,
> > +				  hw_plane_wm->trans_wm.plane_en,
> > +				  hw_plane_wm->trans_wm.plane_res_b,
> > +				  hw_plane_wm->trans_wm.plane_res_l);
> > +		}
> > +
> > +		/* DDB */
> > +		hw_ddb_entry = &hw->ddb_y[PLANE_CURSOR];
> > +		sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
> > +
> > +		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> > +			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
> > +				  pipe_name(pipe),
> > +				  sw_ddb_entry->start, sw_ddb_entry->end,
> > +				  hw_ddb_entry->start, hw_ddb_entry->end);
> > +		}
> > +	}
> > +
> > +	kfree(hw);
> > +}
> > +
> > +/* Cross check the actual hw state with our own modeset state tracking (and it's
> > + * internal consistency). */
> > +static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
> > +					 struct drm_connector_state *conn_state)
> > +{
> > +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> > +
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		      connector->base.base.id,
> > +		      connector->base.name);
> > +
> > +	if (connector->get_hw_state(connector)) {
> > +		struct intel_encoder *encoder = connector->encoder;
> > +
> > +		I915_STATE_WARN(!crtc_state,
> > +			 "connector enabled without attached crtc\n");
> > +
> > +		if (!crtc_state)
> > +			return;
> > +
> > +		I915_STATE_WARN(!crtc_state->active,
> > +		      "connector is active, but attached crtc isn't\n");
> > +
> > +		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> > +			return;
> > +
> > +		I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> > +			"atomic encoder doesn't match attached encoder\n");
> > +
> > +		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> > +			"attached encoder crtc differs from connector crtc\n");
> > +	} else {
> > +		I915_STATE_WARN(crtc_state && crtc_state->active,
> > +			"attached crtc is active, but connector isn't\n");
> > +		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
> > +			"best encoder set without crtc!\n");
> > +	}
> > +}
> > +
> > +static void
> > +verify_connector_state(struct drm_device *dev,
> > +		       struct drm_atomic_state *state,
> > +		       struct drm_crtc *crtc)
> > +{
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *new_conn_state;
> > +	int i;
> > +
> > +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> > +		struct drm_encoder *encoder = connector->encoder;
> > +		struct drm_crtc_state *crtc_state = NULL;
> > +
> > +		if (new_conn_state->crtc != crtc)
> > +			continue;
> > +
> > +		if (crtc)
> > +			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> > +
> > +		intel_connector_verify_state(crtc_state, new_conn_state);
> > +
> > +		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
> > +		     "connector's atomic encoder doesn't match legacy encoder\n");
> > +	}
> > +}
> > +
> > +static void
> > +verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
> > +{
> > +	struct intel_encoder *encoder;
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > +	int i;
> > +
> > +	for_each_intel_encoder(dev, encoder) {
> > +		bool enabled = false, found = false;
> > +		enum pipe pipe;
> > +
> > +		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> > +			      encoder->base.base.id,
> > +			      encoder->base.name);
> > +
> > +		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> > +						   new_conn_state, i) {
> > +			if (old_conn_state->best_encoder == &encoder->base)
> > +				found = true;
> > +
> > +			if (new_conn_state->best_encoder != &encoder->base)
> > +				continue;
> > +			found = enabled = true;
> > +
> > +			I915_STATE_WARN(new_conn_state->crtc !=
> > +					encoder->base.crtc,
> > +			     "connector's crtc doesn't match encoder crtc\n");
> > +		}
> > +
> > +		if (!found)
> > +			continue;
> > +
> > +		I915_STATE_WARN(!!encoder->base.crtc != enabled,
> > +		     "encoder's enabled state mismatch "
> > +		     "(expected %i, found %i)\n",
> > +		     !!encoder->base.crtc, enabled);
> > +
> > +		if (!encoder->base.crtc) {
> > +			bool active;
> > +
> > +			active = encoder->get_hw_state(encoder, &pipe);
> > +			I915_STATE_WARN(active,
> > +			     "encoder detached but still enabled on pipe %c.\n",
> > +			     pipe_name(pipe));
> > +		}
> > +	}
> > +}
> > +
> > +static void
> > +verify_crtc_state(struct drm_crtc *crtc,
> > +		  struct drm_crtc_state *old_crtc_state,
> > +		  struct drm_crtc_state *new_crtc_state)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_encoder *encoder;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_crtc_state *pipe_config, *sw_config;
> > +	struct drm_atomic_state *old_state;
> > +	bool active;
> > +
> > +	old_state = old_crtc_state->state;
> > +	__drm_atomic_helper_crtc_destroy_state(old_crtc_state);
> > +	pipe_config = to_intel_crtc_state(old_crtc_state);
> > +	memset(pipe_config, 0, sizeof(*pipe_config));
> > +	pipe_config->base.crtc = crtc;
> > +	pipe_config->base.state = old_state;
> > +
> > +	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
> > +
> > +	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
> > +
> > +	/* we keep both pipes enabled on 830 */
> > +	if (IS_I830(dev_priv))
> > +		active = new_crtc_state->active;
> > +
> > +	I915_STATE_WARN(new_crtc_state->active != active,
> > +	     "crtc active state doesn't match with hw state "
> > +	     "(expected %i, found %i)\n", new_crtc_state->active, active);
> > +
> > +	I915_STATE_WARN(intel_crtc->active != new_crtc_state->active,
> > +	     "transitional active state does not match atomic hw state "
> > +	     "(expected %i, found %i)\n", new_crtc_state->active, intel_crtc->active);
> > +
> > +	for_each_encoder_on_crtc(dev, crtc, encoder) {
> > +		enum pipe pipe;
> > +
> > +		active = encoder->get_hw_state(encoder, &pipe);
> > +		I915_STATE_WARN(active != new_crtc_state->active,
> > +			"[ENCODER:%i] active %i with crtc active %i\n",
> > +			encoder->base.base.id, active, new_crtc_state->active);
> > +
> > +		I915_STATE_WARN(active && intel_crtc->pipe != pipe,
> > +				"Encoder connected to wrong pipe %c\n",
> > +				pipe_name(pipe));
> > +
> > +		if (active)
> > +			encoder->get_config(encoder, pipe_config);
> > +	}
> > +
> > +	intel_crtc_compute_pixel_rate(pipe_config);
> > +
> > +	if (!new_crtc_state->active)
> > +		return;
> > +
> > +	intel_pipe_config_sanity_check(dev_priv, pipe_config);
> > +
> > +	sw_config = to_intel_crtc_state(new_crtc_state);
> > +	if (!intel_pipe_config_compare(dev_priv, sw_config,
> > +				       pipe_config, false)) {
> > +		I915_STATE_WARN(1, "pipe state doesn't match!\n");
> > +		intel_dump_pipe_config(intel_crtc, pipe_config,
> > +				       "[hw state]");
> > +		intel_dump_pipe_config(intel_crtc, sw_config,
> > +				       "[sw state]");
> > +	}
> > +}
> > +
> > +void
> > +intel_verify_planes(struct intel_atomic_state *state)
> > +{
> > +	struct intel_plane *plane;
> > +	const struct intel_plane_state *plane_state;
> > +	int i;
> > +
> > +	for_each_new_intel_plane_in_state(state, plane,
> > +					  plane_state, i)
> > +		assert_plane(plane, plane_state->slave ||
> > +			     plane_state->base.visible);
> > +}
> > +
> > +static void
> > +verify_single_dpll_state(struct drm_i915_private *dev_priv,
> > +			 struct intel_shared_dpll *pll,
> > +			 struct drm_crtc *crtc,
> > +			 struct drm_crtc_state *new_state)
> > +{
> > +	struct intel_dpll_hw_state dpll_hw_state;
> > +	unsigned int crtc_mask;
> > +	bool active;
> > +
> > +	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
> > +
> > +	DRM_DEBUG_KMS("%s\n", pll->info->name);
> > +
> > +	active = pll->info->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state);
> > +
> > +	if (!(pll->info->flags & INTEL_DPLL_ALWAYS_ON)) {
> > +		I915_STATE_WARN(!pll->on && pll->active_mask,
> > +		     "pll in active use but not on in sw tracking\n");
> > +		I915_STATE_WARN(pll->on && !pll->active_mask,
> > +		     "pll is on but not used by any active crtc\n");
> > +		I915_STATE_WARN(pll->on != active,
> > +		     "pll on state mismatch (expected %i, found %i)\n",
> > +		     pll->on, active);
> > +	}
> > +
> > +	if (!crtc) {
> > +		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
> > +				"more active pll users than references: %x vs %x\n",
> > +				pll->active_mask, pll->state.crtc_mask);
> > +
> > +		return;
> > +	}
> > +
> > +	crtc_mask = drm_crtc_mask(crtc);
> > +
> > +	if (new_state->active)
> > +		I915_STATE_WARN(!(pll->active_mask & crtc_mask),
> > +				"pll active mismatch (expected pipe %c in active mask 0x%02x)\n",
> > +				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> > +	else
> > +		I915_STATE_WARN(pll->active_mask & crtc_mask,
> > +				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
> > +				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
> > +
> > +	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
> > +			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
> > +			crtc_mask, pll->state.crtc_mask);
> > +
> > +	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
> > +					  &dpll_hw_state,
> > +					  sizeof(dpll_hw_state)),
> > +			"pll hw state mismatch\n");
> > +}
> > +
> > +static void
> > +verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
> > +			 struct drm_crtc_state *old_crtc_state,
> > +			 struct drm_crtc_state *new_crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_crtc_state *old_state = to_intel_crtc_state(old_crtc_state);
> > +	struct intel_crtc_state *new_state = to_intel_crtc_state(new_crtc_state);
> > +
> > +	if (new_state->shared_dpll)
> > +		verify_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
> > +
> > +	if (old_state->shared_dpll &&
> > +	    old_state->shared_dpll != new_state->shared_dpll) {
> > +		unsigned int crtc_mask = drm_crtc_mask(crtc);
> > +		struct intel_shared_dpll *pll = old_state->shared_dpll;
> > +
> > +		I915_STATE_WARN(pll->active_mask & crtc_mask,
> > +				"pll active mismatch (didn't expect pipe %c in active mask)\n",
> > +				pipe_name(drm_crtc_index(crtc)));
> > +		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
> > +				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
> > +				pipe_name(drm_crtc_index(crtc)));
> > +	}
> > +}
> > +
> > +void
> > +intel_verify_modeset_crtc(struct drm_crtc *crtc,
> > +			  struct drm_atomic_state *state,
> > +			  struct drm_crtc_state *old_state,
> > +			  struct drm_crtc_state *new_state)
> > +{
> > +	if (!drm_atomic_crtc_needs_modeset(new_state) &&
> > +	    !to_intel_crtc_state(new_state)->update_pipe)
> > +		return;
> > +
> > +	verify_wm_state(crtc, new_state);
> > +	verify_connector_state(crtc->dev, state, crtc);
> > +	verify_crtc_state(crtc, old_state, new_state);
> > +	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> > +}
> > +
> > +static void
> > +verify_disabled_dpll_state(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	int i;
> > +
> > +	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> > +		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> > +}
> > +
> > +void
> > +intel_verify_modeset_disabled(struct drm_device *dev,
> > +			      struct drm_atomic_state *state)
> > +{
> > +	verify_encoder_state(dev, state);
> > +	verify_connector_state(dev, state, NULL);
> > +	verify_disabled_dpll_state(dev);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_verify.h b/drivers/gpu/drm/i915/intel_verify.h
> > new file mode 100644
> > index 000000..4b751ea
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_verify.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + */
> > +
> > +#ifndef __INTEL_VERIFY_H__
> > +#define __INTEL_VERIFY_H__
> > +
> > +struct drm_atomic_state;
> > +struct drm_crtc;
> > +struct drm_crtc_state;
> > +struct drm_device;
> > +
> > +void intel_verify_modeset_crtc(struct drm_crtc *crtc,
> > +			       struct drm_atomic_state *state,
> > +			       struct drm_crtc_state *old_state,
> > +			       struct drm_crtc_state *new_state);
> > +void intel_verify_modeset_disabled(struct drm_device *dev,
> > +				   struct drm_atomic_state *state);
> > +void intel_verify_planes(struct intel_atomic_state *state);
> > +
> > +#endif /* __INTEL_VERIFY_H__ */
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > 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