[PATCH 0/2] drm/i915: Split up modeset state checker.

[PATCH 0/2] drm/i915: Split up modeset state checker.

[Maarten Lankhorst]

Right now the state checker assumes that the global state is fixed,
with support for the atomic ioctl not all state will be locked any more,
so the state checker has to check only what's part of the state.

This is done by first splitting up the state checker in one part that
checks all disabled connectors/encoders and global state, and then
split again to run for each crtc separately.

Maarten Lankhorst (2):
  drm/i915: Make modeset state checker take crtc as argument.
  drm/i915: Move modeset state checker calls.

 drivers/gpu/drm/i915/intel_display.c | 310 +++++++++++++++++++----------------
 1 file changed, 173 insertions(+), 137 deletions(-)

-- 
2.1.0

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

[PATCH 1/2] drm/i915: Make modeset state checker take crtc as argument.

[Maarten Lankhorst]

This will make it easier to keep the crtc checker when atomic
commit is reworked to take a intel_flip_work.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 51f913fb199d..4148b262f2a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12747,48 +12747,43 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void check_wm_state(struct drm_device *dev)
+static void check_wm_state(struct drm_crtc *crtc,
+			   struct drm_crtc_state *new_state)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct skl_ddb_allocation hw_ddb, *sw_ddb;
-	struct intel_crtc *intel_crtc;
+	struct skl_ddb_entry *hw_entry, *sw_entry;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	const enum pipe pipe = intel_crtc->pipe;
 	int plane;
 
-	if (INTEL_INFO(dev)->gen < 9)
+	if (INTEL_INFO(dev)->gen < 9 || !new_state->active)
 		return;
 
 	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
 	sw_ddb = &dev_priv->wm.skl_hw.ddb;
 
-	for_each_intel_crtc(dev, intel_crtc) {
-		struct skl_ddb_entry *hw_entry, *sw_entry;
-		const enum pipe pipe = intel_crtc->pipe;
+	/* planes */
+	for_each_plane(dev_priv, pipe, plane) {
+		hw_entry = &hw_ddb.plane[pipe][plane];
+		sw_entry = &sw_ddb->plane[pipe][plane];
 
-		if (!intel_crtc->active)
+		if (skl_ddb_entry_equal(hw_entry, sw_entry))
 			continue;
 
-		/* planes */
-		for_each_plane(dev_priv, pipe, plane) {
-			hw_entry = &hw_ddb.plane[pipe][plane];
-			sw_entry = &sw_ddb->plane[pipe][plane];
-
-			if (skl_ddb_entry_equal(hw_entry, sw_entry))
-				continue;
-
-			DRM_ERROR("mismatch in DDB state pipe %c plane %d "
-				  "(expected (%u,%u), found (%u,%u))\n",
-				  pipe_name(pipe), plane + 1,
-				  sw_entry->start, sw_entry->end,
-				  hw_entry->start, hw_entry->end);
-		}
-
-		/* cursor */
-		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+		DRM_ERROR("mismatch in DDB state pipe %c plane %d "
+			  "(expected (%u,%u), found (%u,%u))\n",
+			  pipe_name(pipe), plane + 1,
+			  sw_entry->start, sw_entry->end,
+			  hw_entry->start, hw_entry->end);
+	}
 
-		if (skl_ddb_entry_equal(hw_entry, sw_entry))
-			continue;
+	/* cursor */
+	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
 
+	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
 		DRM_ERROR("mismatch in DDB state pipe %c cursor "
 			  "(expected (%u,%u), found (%u,%u))\n",
 			  pipe_name(pipe),
@@ -12798,19 +12793,17 @@ static void check_wm_state(struct drm_device *dev)
 }
 
 static void
-check_connector_state(struct drm_device *dev,
-		      struct drm_atomic_state *old_state)
+check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
 {
-	struct drm_connector_state *old_conn_state;
 	struct drm_connector *connector;
-	int i;
 
-	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+	drm_for_each_connector(connector, dev) {
 		struct drm_encoder *encoder = connector->encoder;
 		struct drm_connector_state *state = connector->state;
 
-		/* This also checks the encoder/connector hw state with the
-		 * ->get_hw_state callbacks. */
+		if (state->crtc != crtc)
+			continue;
+
 		intel_connector_check_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
@@ -12859,144 +12852,201 @@ check_encoder_state(struct drm_device *dev)
 }
 
 static void
-check_crtc_state(struct drm_device *dev, struct drm_atomic_state *old_state)
+check_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 = dev->dev_private;
 	struct intel_encoder *encoder;
-	struct drm_crtc_state *old_crtc_state;
-	struct drm_crtc *crtc;
-	int i;
-
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-		struct intel_crtc_state *pipe_config, *sw_config;
-		bool active;
-
-		if (!needs_modeset(crtc->state) &&
-		    !to_intel_crtc_state(crtc->state)->update_pipe)
-			continue;
+	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;
 
-		__drm_atomic_helper_crtc_destroy_state(crtc, 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;
+	old_state = old_crtc_state->state;
+	__drm_atomic_helper_crtc_destroy_state(crtc, 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]\n",
-			      crtc->base.id);
+	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
 
-		active = dev_priv->display.get_pipe_config(intel_crtc,
-							   pipe_config);
+	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
 
-		/* hw state is inconsistent with the pipe quirk */
-		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
-			active = crtc->state->active;
+	/* hw state is inconsistent with the pipe quirk */
+	if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
+	    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+		active = new_crtc_state->active;
 
-		I915_STATE_WARN(crtc->state->active != active,
-		     "crtc active state doesn't match with hw state "
-		     "(expected %i, found %i)\n", crtc->state->active, 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 != crtc->state->active,
-		     "transitional active state does not match atomic hw state "
-		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->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;
+	for_each_encoder_on_crtc(dev, crtc, encoder) {
+		enum pipe pipe;
 
-			active = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(active != crtc->state->active,
-				"[ENCODER:%i] active %i with crtc active %i\n",
-				encoder->base.base.id, active, crtc->state->active);
+		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));
+		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);
-		}
+		if (active)
+			encoder->get_config(encoder, pipe_config);
+	}
 
-		if (!crtc->state->active)
-			continue;
+	if (!new_crtc_state->active)
+		return;
 
-		intel_pipe_config_sanity_check(dev_priv, pipe_config);
+	intel_pipe_config_sanity_check(dev_priv, pipe_config);
 
-		sw_config = to_intel_crtc_state(crtc->state);
-		if (!intel_pipe_config_compare(dev, 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]");
-		}
+	sw_config = to_intel_crtc_state(crtc->state);
+	if (!intel_pipe_config_compare(dev, 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
-check_shared_dpll_state(struct drm_device *dev)
+check_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 drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc;
 	struct intel_dpll_hw_state dpll_hw_state;
-	int i;
+	unsigned crtc_mask;
+	bool active;
 
-	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
-		struct intel_shared_dpll *pll =
-			intel_get_shared_dpll_by_id(dev_priv, i);
-		unsigned enabled_crtcs = 0, active_crtcs = 0;
-		bool active;
+	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
 
-		memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
+	DRM_DEBUG_KMS("%s\n", pll->name);
 
-		DRM_DEBUG_KMS("%s\n", pll->name);
+	active = pll->funcs.get_hw_state(dev_priv, pll, &dpll_hw_state);
 
-		active = pll->funcs.get_hw_state(dev_priv, pll, &dpll_hw_state);
+	if (!(pll->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->config.crtc_mask,
-		     "more active pll users than references: %x vs %x\n",
-		     pll->active_mask, pll->config.crtc_mask);
+				"more active pll users than references: %x vs %x\n",
+				pll->active_mask, pll->config.crtc_mask);
 
-		if (!(pll->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);
-		}
+		return;
+	}
 
-		for_each_intel_crtc(dev, crtc) {
-			if (crtc->base.state->enable && crtc->config->shared_dpll == pll)
-				enabled_crtcs |= 1 << drm_crtc_index(&crtc->base);
-			if (crtc->base.state->active && crtc->config->shared_dpll == pll)
-				active_crtcs |= 1 << drm_crtc_index(&crtc->base);
-		}
+	crtc_mask = 1 << drm_crtc_index(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->config.crtc_mask & crtc_mask),
+			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
+			crtc_mask, pll->config.crtc_mask);
 
-		I915_STATE_WARN(pll->active_mask != active_crtcs,
-		     "pll active crtcs mismatch (expected %x, found %x)\n",
-		     pll->active_mask, active_crtcs);
-		I915_STATE_WARN(pll->config.crtc_mask != enabled_crtcs,
-		     "pll enabled crtcs mismatch (expected %x, found %x)\n",
-		     pll->config.crtc_mask, enabled_crtcs);
+	I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state,
+					  &dpll_hw_state,
+					  sizeof(dpll_hw_state)),
+			"pll hw state mismatch\n");
+}
 
-		I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state,
-				       sizeof(dpll_hw_state)),
-		     "pll hw state mismatch\n");
+static void
+check_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 = dev->dev_private;
+	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)
+		check_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 crtc_mask = 1 << drm_crtc_index(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->config.crtc_mask & crtc_mask,
+				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
+				pipe_name(drm_crtc_index(crtc)));
 	}
 }
 
 static void
+intel_modeset_check_crtc(struct drm_crtc *crtc,
+			 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;
+
+	check_wm_state(crtc, new_state);
+	check_connector_state(crtc->dev, crtc);
+	check_crtc_state(crtc, old_state, new_state);
+	check_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
+}
+
+static void
+check_disabled_dpll_state(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int i;
+
+	for (i = 0; i < dev_priv->num_shared_dpll; i++)
+		check_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
+}
+
+static void
+intel_modeset_check_disabled(struct drm_device *dev,
+			     struct drm_atomic_state *old_state)
+{
+	check_encoder_state(dev);
+	check_connector_state(dev, NULL);
+	check_disabled_dpll_state(dev);
+}
+
+static void
 intel_modeset_check_state(struct drm_device *dev,
 			  struct drm_atomic_state *old_state)
 {
-	check_wm_state(dev);
-	check_connector_state(dev, old_state);
-	check_encoder_state(dev);
-	check_crtc_state(dev, old_state);
-	check_shared_dpll_state(dev);
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i)
+		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
+
+	intel_modeset_check_disabled(dev, old_state);
 }
 
 static void update_scanline_offset(struct intel_crtc *crtc)
-- 
2.1.0

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

[PATCH 2/2] drm/i915: Move modeset state checker calls.

[Maarten Lankhorst]

The modeset state checker no longer has full access to the hardware,
instead it should only check affected crtc's.

Looking for disabled stuff can be checked immediately after all crtc
disables have completed, while each enabled crtc can be checked right
after being enabled.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4148b262f2a7..b6a75aa32e5a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13027,28 +13027,13 @@ check_disabled_dpll_state(struct drm_device *dev)
 }
 
 static void
-intel_modeset_check_disabled(struct drm_device *dev,
-			     struct drm_atomic_state *old_state)
+intel_modeset_check_disabled(struct drm_device *dev)
 {
 	check_encoder_state(dev);
 	check_connector_state(dev, NULL);
 	check_disabled_dpll_state(dev);
 }
 
-static void
-intel_modeset_check_state(struct drm_device *dev,
-			  struct drm_atomic_state *old_state)
-{
-	struct drm_crtc_state *old_crtc_state;
-	struct drm_crtc *crtc;
-	int i;
-
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i)
-		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
-
-	intel_modeset_check_disabled(dev, old_state);
-}
-
 static void update_scanline_offset(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -13616,6 +13601,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (dev_priv->display.modeset_commit_cdclk &&
 		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
 			dev_priv->display.modeset_commit_cdclk(state);
+
+		intel_modeset_check_disabled(dev);
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -13682,6 +13669,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
+
+		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
 	if (intel_state->modeset)
@@ -13691,9 +13680,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);
 
-	if (hw_check)
-		intel_modeset_check_state(dev, state);
-
 	drm_atomic_state_free(state);
 
 	/* As one of the primary mmio accessors, KMS has a high likelihood
-- 
2.1.0

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

Re: [PATCH 0/2] drm/i915: Split up modeset state checker.

[Matt Roper]

On Wed, Mar 23, 2016 at 02:58:05PM +0100, Maarten Lankhorst wrote:
> Right now the state checker assumes that the global state is fixed,
> with support for the atomic ioctl not all state will be locked any more,
> so the state checker has to check only what's part of the state.
> 
> This is done by first splitting up the state checker in one part that
> checks all disabled connectors/encoders and global state, and then
> split again to run for each crtc separately.

While you're touching these functions, maybe it would be a good time to
rename them as well?  The 'check' in their names can mislead people into
thinking that they're somehow related to the atomic 'check' phase, even
though they're actually something we do after the commit is complete.

Renaming these with  s/check/confirm/ or s/check/verify/ might make it a
little bit easier to understand what their purpose is and avoid
confusion.


Matt

> 
> Maarten Lankhorst (2):
>   drm/i915: Make modeset state checker take crtc as argument.
>   drm/i915: Move modeset state checker calls.
> 
>  drivers/gpu/drm/i915/intel_display.c | 310 +++++++++++++++++++----------------
>  1 file changed, 173 insertions(+), 137 deletions(-)
> 
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/2] drm/i915: Split up modeset state checker.

[Maarten Lankhorst]

Op 25-03-16 om 17:59 schreef Matt Roper:
> On Wed, Mar 23, 2016 at 02:58:05PM +0100, Maarten Lankhorst wrote:
>> Right now the state checker assumes that the global state is fixed,
>> with support for the atomic ioctl not all state will be locked any more,
>> so the state checker has to check only what's part of the state.
>>
>> This is done by first splitting up the state checker in one part that
>> checks all disabled connectors/encoders and global state, and then
>> split again to run for each crtc separately.
> While you're touching these functions, maybe it would be a good time to
> rename them as well?  The 'check' in their names can mislead people into
> thinking that they're somehow related to the atomic 'check' phase, even
> though they're actually something we do after the commit is complete.
>
> Renaming these with  s/check/confirm/ or s/check/verify/ might make it a
> little bit easier to understand what their purpose is and avoid
> confusion.
I'm ok with s/check/verify/ -- anything wrong with the patches itself?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/2] drm/i915: Rename hw state checker to hw state verifier.

[Maarten Lankhorst]

Check functions are used by atomic to see if the new state will
be allowed. There's also a hw state checker which checks afterwards
that the committed state is correct. Rename it to hw state verifier
to reduce some confusion.

Suggested-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9e63720428fc..8b423cdd5295 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6283,7 +6283,7 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
 
 /* Cross check the actual hw state with our own modeset state tracking (and it's
  * internal consistency). */
-static void intel_connector_check_state(struct intel_connector *connector)
+static void intel_connector_verify_state(struct intel_connector *connector)
 {
 	struct drm_crtc *crtc = connector->base.state->crtc;
 
@@ -12752,8 +12752,8 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void check_wm_state(struct drm_crtc *crtc,
-			   struct drm_crtc_state *new_state)
+static void verify_wm_state(struct drm_crtc *crtc,
+			    struct drm_crtc_state *new_state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -12798,7 +12798,7 @@ static void check_wm_state(struct drm_crtc *crtc,
 }
 
 static void
-check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
+verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
 {
 	struct drm_connector *connector;
 
@@ -12809,7 +12809,7 @@ check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
 		if (state->crtc != crtc)
 			continue;
 
-		intel_connector_check_state(to_intel_connector(connector));
+		intel_connector_verify_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
 		     "connector's atomic encoder doesn't match legacy encoder\n");
@@ -12817,7 +12817,7 @@ check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
 }
 
 static void
-check_encoder_state(struct drm_device *dev)
+verify_encoder_state(struct drm_device *dev)
 {
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
@@ -12857,9 +12857,9 @@ check_encoder_state(struct drm_device *dev)
 }
 
 static void
-check_crtc_state(struct drm_crtc *crtc,
-		 struct drm_crtc_state *old_crtc_state,
-		 struct drm_crtc_state *new_crtc_state)
+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 = dev->dev_private;
@@ -12926,10 +12926,10 @@ check_crtc_state(struct drm_crtc *crtc,
 }
 
 static void
-check_single_dpll_state(struct drm_i915_private *dev_priv,
-			struct intel_shared_dpll *pll,
-			struct drm_crtc *crtc,
-			struct drm_crtc_state *new_state)
+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 crtc_mask;
@@ -12981,16 +12981,16 @@ check_single_dpll_state(struct drm_i915_private *dev_priv,
 }
 
 static void
-check_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)
+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 = dev->dev_private;
 	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)
-		check_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
+		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) {
@@ -13007,7 +13007,7 @@ check_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
 }
 
 static void
-intel_modeset_check_crtc(struct drm_crtc *crtc,
+intel_modeset_verify_crtc(struct drm_crtc *crtc,
 			 struct drm_crtc_state *old_state,
 			 struct drm_crtc_state *new_state)
 {
@@ -13015,28 +13015,28 @@ intel_modeset_check_crtc(struct drm_crtc *crtc,
 	    !to_intel_crtc_state(new_state)->update_pipe)
 		return;
 
-	check_wm_state(crtc, new_state);
-	check_connector_state(crtc->dev, crtc);
-	check_crtc_state(crtc, old_state, new_state);
-	check_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
+	verify_wm_state(crtc, new_state);
+	verify_connector_state(crtc->dev, crtc);
+	verify_crtc_state(crtc, old_state, new_state);
+	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
 }
 
 static void
-check_disabled_dpll_state(struct drm_device *dev)
+verify_disabled_dpll_state(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 
 	for (i = 0; i < dev_priv->num_shared_dpll; i++)
-		check_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
+		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
 }
 
 static void
-intel_modeset_check_disabled(struct drm_device *dev)
+intel_modeset_verify_disabled(struct drm_device *dev)
 {
-	check_encoder_state(dev);
-	check_connector_state(dev, NULL);
-	check_disabled_dpll_state(dev);
+	verify_encoder_state(dev);
+	verify_connector_state(dev, NULL);
+	verify_disabled_dpll_state(dev);
 }
 
 static void update_scanline_offset(struct intel_crtc *crtc)
@@ -13607,7 +13607,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
 			dev_priv->display.modeset_commit_cdclk(state);
 
-		intel_modeset_check_disabled(dev);
+		intel_modeset_verify_disabled(dev);
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -13675,7 +13675,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 
-		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
+		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
 	if (intel_state->modeset)
-- 
2.1.0


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

Re: [PATCH 3/2] drm/i915: Rename hw state checker to hw state verifier.

[Daniel Vetter]

On Wed, Mar 30, 2016 at 10:00:05AM +0200, Maarten Lankhorst wrote:
> Check functions are used by atomic to see if the new state will
> be allowed. There's also a hw state checker which checks afterwards
> that the committed state is correct. Rename it to hw state verifier
> to reduce some confusion.
> 
> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Great idea!

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9e63720428fc..8b423cdd5295 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6283,7 +6283,7 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
>  
>  /* Cross check the actual hw state with our own modeset state tracking (and it's
>   * internal consistency). */
> -static void intel_connector_check_state(struct intel_connector *connector)
> +static void intel_connector_verify_state(struct intel_connector *connector)
>  {
>  	struct drm_crtc *crtc = connector->base.state->crtc;
>  
> @@ -12752,8 +12752,8 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static void check_wm_state(struct drm_crtc *crtc,
> -			   struct drm_crtc_state *new_state)
> +static void verify_wm_state(struct drm_crtc *crtc,
> +			    struct drm_crtc_state *new_state)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -12798,7 +12798,7 @@ static void check_wm_state(struct drm_crtc *crtc,
>  }
>  
>  static void
> -check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
> +verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  {
>  	struct drm_connector *connector;
>  
> @@ -12809,7 +12809,7 @@ check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  		if (state->crtc != crtc)
>  			continue;
>  
> -		intel_connector_check_state(to_intel_connector(connector));
> +		intel_connector_verify_state(to_intel_connector(connector));
>  
>  		I915_STATE_WARN(state->best_encoder != encoder,
>  		     "connector's atomic encoder doesn't match legacy encoder\n");
> @@ -12817,7 +12817,7 @@ check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  }
>  
>  static void
> -check_encoder_state(struct drm_device *dev)
> +verify_encoder_state(struct drm_device *dev)
>  {
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
> @@ -12857,9 +12857,9 @@ check_encoder_state(struct drm_device *dev)
>  }
>  
>  static void
> -check_crtc_state(struct drm_crtc *crtc,
> -		 struct drm_crtc_state *old_crtc_state,
> -		 struct drm_crtc_state *new_crtc_state)
> +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 = dev->dev_private;
> @@ -12926,10 +12926,10 @@ check_crtc_state(struct drm_crtc *crtc,
>  }
>  
>  static void
> -check_single_dpll_state(struct drm_i915_private *dev_priv,
> -			struct intel_shared_dpll *pll,
> -			struct drm_crtc *crtc,
> -			struct drm_crtc_state *new_state)
> +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 crtc_mask;
> @@ -12981,16 +12981,16 @@ check_single_dpll_state(struct drm_i915_private *dev_priv,
>  }
>  
>  static void
> -check_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)
> +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 = dev->dev_private;
>  	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)
> -		check_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
> +		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) {
> @@ -13007,7 +13007,7 @@ check_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
>  }
>  
>  static void
> -intel_modeset_check_crtc(struct drm_crtc *crtc,
> +intel_modeset_verify_crtc(struct drm_crtc *crtc,
>  			 struct drm_crtc_state *old_state,
>  			 struct drm_crtc_state *new_state)
>  {
> @@ -13015,28 +13015,28 @@ intel_modeset_check_crtc(struct drm_crtc *crtc,
>  	    !to_intel_crtc_state(new_state)->update_pipe)
>  		return;
>  
> -	check_wm_state(crtc, new_state);
> -	check_connector_state(crtc->dev, crtc);
> -	check_crtc_state(crtc, old_state, new_state);
> -	check_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> +	verify_wm_state(crtc, new_state);
> +	verify_connector_state(crtc->dev, crtc);
> +	verify_crtc_state(crtc, old_state, new_state);
> +	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
>  }
>  
>  static void
> -check_disabled_dpll_state(struct drm_device *dev)
> +verify_disabled_dpll_state(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i;
>  
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> -		check_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> +		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
>  }
>  
>  static void
> -intel_modeset_check_disabled(struct drm_device *dev)
> +intel_modeset_verify_disabled(struct drm_device *dev)
>  {
> -	check_encoder_state(dev);
> -	check_connector_state(dev, NULL);
> -	check_disabled_dpll_state(dev);
> +	verify_encoder_state(dev);
> +	verify_connector_state(dev, NULL);
> +	verify_disabled_dpll_state(dev);
>  }
>  
>  static void update_scanline_offset(struct intel_crtc *crtc)
> @@ -13607,7 +13607,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> -		intel_modeset_check_disabled(dev);
> +		intel_modeset_verify_disabled(dev);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -13675,7 +13675,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv, put_domains[i]);
>  
> -		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
> +		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
>  	if (intel_state->modeset)
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm/i915: Make modeset state checker take crtc as argument.

[Matt Roper]

On Wed, Mar 23, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote:
> This will make it easier to keep the crtc checker when atomic
> commit is reworked to take a intel_flip_work.

Looks good overall.  You might want to expand the commit message
slightly since 'intel_flip_work' isn't something that exists in today's
driver.  I assume intel_flip_work will refer to the work structure to
track asynchronous commits?

Even though this patch helps ensure that we don't check CRTC's that
weren't updated as part of this transaction (and that could have had
racing transactions happening), it seems to me that the check_disabled
part of the tests will remain unsafe, right?  As far as I know, we don't
have any locks that are guaranteed to cover the structures you're
testing in that case.  It seems like if we want to make that part of the
test safe, we'd only be able to do so if we waited for the system to
quiesce.

Anyway, this is definitely a move in the right direction, so

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 316 ++++++++++++++++++++---------------
>  1 file changed, 183 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 51f913fb199d..4148b262f2a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12747,48 +12747,43 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static void check_wm_state(struct drm_device *dev)
> +static void check_wm_state(struct drm_crtc *crtc,
> +			   struct drm_crtc_state *new_state)
>  {
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct skl_ddb_allocation hw_ddb, *sw_ddb;
> -	struct intel_crtc *intel_crtc;
> +	struct skl_ddb_entry *hw_entry, *sw_entry;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	const enum pipe pipe = intel_crtc->pipe;
>  	int plane;
>  
> -	if (INTEL_INFO(dev)->gen < 9)
> +	if (INTEL_INFO(dev)->gen < 9 || !new_state->active)
>  		return;
>  
>  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
>  
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		struct skl_ddb_entry *hw_entry, *sw_entry;
> -		const enum pipe pipe = intel_crtc->pipe;
> +	/* planes */
> +	for_each_plane(dev_priv, pipe, plane) {
> +		hw_entry = &hw_ddb.plane[pipe][plane];
> +		sw_entry = &sw_ddb->plane[pipe][plane];
>  
> -		if (!intel_crtc->active)
> +		if (skl_ddb_entry_equal(hw_entry, sw_entry))
>  			continue;
>  
> -		/* planes */
> -		for_each_plane(dev_priv, pipe, plane) {
> -			hw_entry = &hw_ddb.plane[pipe][plane];
> -			sw_entry = &sw_ddb->plane[pipe][plane];
> -
> -			if (skl_ddb_entry_equal(hw_entry, sw_entry))
> -				continue;
> -
> -			DRM_ERROR("mismatch in DDB state pipe %c plane %d "
> -				  "(expected (%u,%u), found (%u,%u))\n",
> -				  pipe_name(pipe), plane + 1,
> -				  sw_entry->start, sw_entry->end,
> -				  hw_entry->start, hw_entry->end);
> -		}
> -
> -		/* cursor */
> -		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> -		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> +		DRM_ERROR("mismatch in DDB state pipe %c plane %d "
> +			  "(expected (%u,%u), found (%u,%u))\n",
> +			  pipe_name(pipe), plane + 1,
> +			  sw_entry->start, sw_entry->end,
> +			  hw_entry->start, hw_entry->end);
> +	}
>  
> -		if (skl_ddb_entry_equal(hw_entry, sw_entry))
> -			continue;
> +	/* cursor */
> +	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> +	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
>  
> +	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
>  		DRM_ERROR("mismatch in DDB state pipe %c cursor "
>  			  "(expected (%u,%u), found (%u,%u))\n",
>  			  pipe_name(pipe),
> @@ -12798,19 +12793,17 @@ static void check_wm_state(struct drm_device *dev)
>  }
>  
>  static void
> -check_connector_state(struct drm_device *dev,
> -		      struct drm_atomic_state *old_state)
> +check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  {
> -	struct drm_connector_state *old_conn_state;
>  	struct drm_connector *connector;
> -	int i;
>  
> -	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> +	drm_for_each_connector(connector, dev) {
>  		struct drm_encoder *encoder = connector->encoder;
>  		struct drm_connector_state *state = connector->state;
>  
> -		/* This also checks the encoder/connector hw state with the
> -		 * ->get_hw_state callbacks. */
> +		if (state->crtc != crtc)
> +			continue;
> +
>  		intel_connector_check_state(to_intel_connector(connector));
>  
>  		I915_STATE_WARN(state->best_encoder != encoder,
> @@ -12859,144 +12852,201 @@ check_encoder_state(struct drm_device *dev)
>  }
>  
>  static void
> -check_crtc_state(struct drm_device *dev, struct drm_atomic_state *old_state)
> +check_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 = dev->dev_private;
>  	struct intel_encoder *encoder;
> -	struct drm_crtc_state *old_crtc_state;
> -	struct drm_crtc *crtc;
> -	int i;
> -
> -	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -		struct intel_crtc_state *pipe_config, *sw_config;
> -		bool active;
> -
> -		if (!needs_modeset(crtc->state) &&
> -		    !to_intel_crtc_state(crtc->state)->update_pipe)
> -			continue;
> +	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;
>  
> -		__drm_atomic_helper_crtc_destroy_state(crtc, 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;
> +	old_state = old_crtc_state->state;
> +	__drm_atomic_helper_crtc_destroy_state(crtc, 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]\n",
> -			      crtc->base.id);
> +	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
>  
> -		active = dev_priv->display.get_pipe_config(intel_crtc,
> -							   pipe_config);
> +	active = dev_priv->display.get_pipe_config(intel_crtc, pipe_config);
>  
> -		/* hw state is inconsistent with the pipe quirk */
> -		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> -		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> -			active = crtc->state->active;
> +	/* hw state is inconsistent with the pipe quirk */
> +	if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> +	    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> +		active = new_crtc_state->active;
>  
> -		I915_STATE_WARN(crtc->state->active != active,
> -		     "crtc active state doesn't match with hw state "
> -		     "(expected %i, found %i)\n", crtc->state->active, 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 != crtc->state->active,
> -		     "transitional active state does not match atomic hw state "
> -		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->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;
> +	for_each_encoder_on_crtc(dev, crtc, encoder) {
> +		enum pipe pipe;
>  
> -			active = encoder->get_hw_state(encoder, &pipe);
> -			I915_STATE_WARN(active != crtc->state->active,
> -				"[ENCODER:%i] active %i with crtc active %i\n",
> -				encoder->base.base.id, active, crtc->state->active);
> +		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));
> +		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);
> -		}
> +		if (active)
> +			encoder->get_config(encoder, pipe_config);
> +	}
>  
> -		if (!crtc->state->active)
> -			continue;
> +	if (!new_crtc_state->active)
> +		return;
>  
> -		intel_pipe_config_sanity_check(dev_priv, pipe_config);
> +	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>  
> -		sw_config = to_intel_crtc_state(crtc->state);
> -		if (!intel_pipe_config_compare(dev, 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]");
> -		}
> +	sw_config = to_intel_crtc_state(crtc->state);
> +	if (!intel_pipe_config_compare(dev, 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
> -check_shared_dpll_state(struct drm_device *dev)
> +check_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 drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc;
>  	struct intel_dpll_hw_state dpll_hw_state;
> -	int i;
> +	unsigned crtc_mask;
> +	bool active;
>  
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll =
> -			intel_get_shared_dpll_by_id(dev_priv, i);
> -		unsigned enabled_crtcs = 0, active_crtcs = 0;
> -		bool active;
> +	memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
>  
> -		memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));
> +	DRM_DEBUG_KMS("%s\n", pll->name);
>  
> -		DRM_DEBUG_KMS("%s\n", pll->name);
> +	active = pll->funcs.get_hw_state(dev_priv, pll, &dpll_hw_state);
>  
> -		active = pll->funcs.get_hw_state(dev_priv, pll, &dpll_hw_state);
> +	if (!(pll->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->config.crtc_mask,
> -		     "more active pll users than references: %x vs %x\n",
> -		     pll->active_mask, pll->config.crtc_mask);
> +				"more active pll users than references: %x vs %x\n",
> +				pll->active_mask, pll->config.crtc_mask);
>  
> -		if (!(pll->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);
> -		}
> +		return;
> +	}
>  
> -		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->base.state->enable && crtc->config->shared_dpll == pll)
> -				enabled_crtcs |= 1 << drm_crtc_index(&crtc->base);
> -			if (crtc->base.state->active && crtc->config->shared_dpll == pll)
> -				active_crtcs |= 1 << drm_crtc_index(&crtc->base);
> -		}
> +	crtc_mask = 1 << drm_crtc_index(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->config.crtc_mask & crtc_mask),
> +			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
> +			crtc_mask, pll->config.crtc_mask);
>  
> -		I915_STATE_WARN(pll->active_mask != active_crtcs,
> -		     "pll active crtcs mismatch (expected %x, found %x)\n",
> -		     pll->active_mask, active_crtcs);
> -		I915_STATE_WARN(pll->config.crtc_mask != enabled_crtcs,
> -		     "pll enabled crtcs mismatch (expected %x, found %x)\n",
> -		     pll->config.crtc_mask, enabled_crtcs);
> +	I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state,
> +					  &dpll_hw_state,
> +					  sizeof(dpll_hw_state)),
> +			"pll hw state mismatch\n");
> +}
>  
> -		I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state,
> -				       sizeof(dpll_hw_state)),
> -		     "pll hw state mismatch\n");
> +static void
> +check_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 = dev->dev_private;
> +	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)
> +		check_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 crtc_mask = 1 << drm_crtc_index(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->config.crtc_mask & crtc_mask,
> +				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
> +				pipe_name(drm_crtc_index(crtc)));
>  	}
>  }
>  
>  static void
> +intel_modeset_check_crtc(struct drm_crtc *crtc,
> +			 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;
> +
> +	check_wm_state(crtc, new_state);
> +	check_connector_state(crtc->dev, crtc);
> +	check_crtc_state(crtc, old_state, new_state);
> +	check_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> +}
> +
> +static void
> +check_disabled_dpll_state(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int i;
> +
> +	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> +		check_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> +}
> +
> +static void
> +intel_modeset_check_disabled(struct drm_device *dev,
> +			     struct drm_atomic_state *old_state)
> +{
> +	check_encoder_state(dev);
> +	check_connector_state(dev, NULL);
> +	check_disabled_dpll_state(dev);
> +}
> +
> +static void
>  intel_modeset_check_state(struct drm_device *dev,
>  			  struct drm_atomic_state *old_state)
>  {
> -	check_wm_state(dev);
> -	check_connector_state(dev, old_state);
> -	check_encoder_state(dev);
> -	check_crtc_state(dev, old_state);
> -	check_shared_dpll_state(dev);
> +	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i)
> +		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
> +
> +	intel_modeset_check_disabled(dev, old_state);
>  }
>  
>  static void update_scanline_offset(struct intel_crtc *crtc)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Move modeset state checker calls.

[Matt Roper]

On Wed, Mar 23, 2016 at 02:58:07PM +0100, Maarten Lankhorst wrote:
> The modeset state checker no longer has full access to the hardware,
> instead it should only check affected crtc's.
> 
> Looking for disabled stuff can be checked immediately after all crtc
> disables have completed, while each enabled crtc can be checked right
> after being enabled.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4148b262f2a7..b6a75aa32e5a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13027,28 +13027,13 @@ check_disabled_dpll_state(struct drm_device *dev)
>  }
>  
>  static void
> -intel_modeset_check_disabled(struct drm_device *dev,
> -			     struct drm_atomic_state *old_state)
> +intel_modeset_check_disabled(struct drm_device *dev)
>  {
>  	check_encoder_state(dev);
>  	check_connector_state(dev, NULL);
>  	check_disabled_dpll_state(dev);
>  }
>  
> -static void
> -intel_modeset_check_state(struct drm_device *dev,
> -			  struct drm_atomic_state *old_state)
> -{
> -	struct drm_crtc_state *old_crtc_state;
> -	struct drm_crtc *crtc;
> -	int i;
> -
> -	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i)
> -		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
> -
> -	intel_modeset_check_disabled(dev, old_state);
> -}
> -
>  static void update_scanline_offset(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -13616,6 +13601,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		if (dev_priv->display.modeset_commit_cdclk &&
>  		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
>  			dev_priv->display.modeset_commit_cdclk(state);
> +
> +		intel_modeset_check_disabled(dev);

As noted on patch #1, since we're checking all disabled state (and not
just the things that are newly-disabled by this transaction), we may not
hold full locks over everything that's disabled here and might be racing
with other updates.  But we're no worse off than we were before.

As a minor behavior change, it looks like we're also no longer calling
this when we're doing non-modeset pipe updates, but that should be fine.


Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -13682,6 +13669,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv, put_domains[i]);
> +
> +		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
>  	if (intel_state->modeset)
> @@ -13691,9 +13680,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	if (hw_check)
> -		intel_modeset_check_state(dev, state);
> -
>  	drm_atomic_state_free(state);
>  
>  	/* As one of the primary mmio accessors, KMS has a high likelihood
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/2] drm/i915: Rename hw state checker to hw state verifier.

[Matt Roper]

On Wed, Mar 30, 2016 at 10:00:05AM +0200, Maarten Lankhorst wrote:
> Check functions are used by atomic to see if the new state will
> be allowed. There's also a hw state checker which checks afterwards
> that the committed state is correct. Rename it to hw state verifier
> to reduce some confusion.
> 
> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9e63720428fc..8b423cdd5295 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6283,7 +6283,7 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
>  
>  /* Cross check the actual hw state with our own modeset state tracking (and it's
>   * internal consistency). */
> -static void intel_connector_check_state(struct intel_connector *connector)
> +static void intel_connector_verify_state(struct intel_connector *connector)
>  {
>  	struct drm_crtc *crtc = connector->base.state->crtc;
>  
> @@ -12752,8 +12752,8 @@ static void intel_pipe_config_sanity_check(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static void check_wm_state(struct drm_crtc *crtc,
> -			   struct drm_crtc_state *new_state)
> +static void verify_wm_state(struct drm_crtc *crtc,
> +			    struct drm_crtc_state *new_state)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -12798,7 +12798,7 @@ static void check_wm_state(struct drm_crtc *crtc,
>  }
>  
>  static void
> -check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
> +verify_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  {
>  	struct drm_connector *connector;
>  
> @@ -12809,7 +12809,7 @@ check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  		if (state->crtc != crtc)
>  			continue;
>  
> -		intel_connector_check_state(to_intel_connector(connector));
> +		intel_connector_verify_state(to_intel_connector(connector));
>  
>  		I915_STATE_WARN(state->best_encoder != encoder,
>  		     "connector's atomic encoder doesn't match legacy encoder\n");
> @@ -12817,7 +12817,7 @@ check_connector_state(struct drm_device *dev, struct drm_crtc *crtc)
>  }
>  
>  static void
> -check_encoder_state(struct drm_device *dev)
> +verify_encoder_state(struct drm_device *dev)
>  {
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
> @@ -12857,9 +12857,9 @@ check_encoder_state(struct drm_device *dev)
>  }
>  
>  static void
> -check_crtc_state(struct drm_crtc *crtc,
> -		 struct drm_crtc_state *old_crtc_state,
> -		 struct drm_crtc_state *new_crtc_state)
> +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 = dev->dev_private;
> @@ -12926,10 +12926,10 @@ check_crtc_state(struct drm_crtc *crtc,
>  }
>  
>  static void
> -check_single_dpll_state(struct drm_i915_private *dev_priv,
> -			struct intel_shared_dpll *pll,
> -			struct drm_crtc *crtc,
> -			struct drm_crtc_state *new_state)
> +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 crtc_mask;
> @@ -12981,16 +12981,16 @@ check_single_dpll_state(struct drm_i915_private *dev_priv,
>  }
>  
>  static void
> -check_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)
> +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 = dev->dev_private;
>  	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)
> -		check_single_dpll_state(dev_priv, new_state->shared_dpll, crtc, new_crtc_state);
> +		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) {
> @@ -13007,7 +13007,7 @@ check_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
>  }
>  
>  static void
> -intel_modeset_check_crtc(struct drm_crtc *crtc,
> +intel_modeset_verify_crtc(struct drm_crtc *crtc,
>  			 struct drm_crtc_state *old_state,
>  			 struct drm_crtc_state *new_state)
>  {
> @@ -13015,28 +13015,28 @@ intel_modeset_check_crtc(struct drm_crtc *crtc,
>  	    !to_intel_crtc_state(new_state)->update_pipe)
>  		return;
>  
> -	check_wm_state(crtc, new_state);
> -	check_connector_state(crtc->dev, crtc);
> -	check_crtc_state(crtc, old_state, new_state);
> -	check_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
> +	verify_wm_state(crtc, new_state);
> +	verify_connector_state(crtc->dev, crtc);
> +	verify_crtc_state(crtc, old_state, new_state);
> +	verify_shared_dpll_state(crtc->dev, crtc, old_state, new_state);
>  }
>  
>  static void
> -check_disabled_dpll_state(struct drm_device *dev)
> +verify_disabled_dpll_state(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i;
>  
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++)
> -		check_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
> +		verify_single_dpll_state(dev_priv, &dev_priv->shared_dplls[i], NULL, NULL);
>  }
>  
>  static void
> -intel_modeset_check_disabled(struct drm_device *dev)
> +intel_modeset_verify_disabled(struct drm_device *dev)
>  {
> -	check_encoder_state(dev);
> -	check_connector_state(dev, NULL);
> -	check_disabled_dpll_state(dev);
> +	verify_encoder_state(dev);
> +	verify_connector_state(dev, NULL);
> +	verify_disabled_dpll_state(dev);
>  }
>  
>  static void update_scanline_offset(struct intel_crtc *crtc)
> @@ -13607,7 +13607,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> -		intel_modeset_check_disabled(dev);
> +		intel_modeset_verify_disabled(dev);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -13675,7 +13675,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv, put_domains[i]);
>  
> -		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
> +		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
>  	if (intel_state->modeset)
> -- 
> 2.1.0
> 
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Move modeset state checker calls.

[Maarten Lankhorst]

Op 08-04-16 om 01:23 schreef Matt Roper:
> On Wed, Mar 23, 2016 at 02:58:07PM +0100, Maarten Lankhorst wrote:
>> The modeset state checker no longer has full access to the hardware,
>> instead it should only check affected crtc's.
>>
>> Looking for disabled stuff can be checked immediately after all crtc
>> disables have completed, while each enabled crtc can be checked right
>> after being enabled.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 24 +++++-------------------
>>  1 file changed, 5 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4148b262f2a7..b6a75aa32e5a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13027,28 +13027,13 @@ check_disabled_dpll_state(struct drm_device *dev)
>>  }
>>  
>>  static void
>> -intel_modeset_check_disabled(struct drm_device *dev,
>> -			     struct drm_atomic_state *old_state)
>> +intel_modeset_check_disabled(struct drm_device *dev)
>>  {
>>  	check_encoder_state(dev);
>>  	check_connector_state(dev, NULL);
>>  	check_disabled_dpll_state(dev);
>>  }
>>  
>> -static void
>> -intel_modeset_check_state(struct drm_device *dev,
>> -			  struct drm_atomic_state *old_state)
>> -{
>> -	struct drm_crtc_state *old_crtc_state;
>> -	struct drm_crtc *crtc;
>> -	int i;
>> -
>> -	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i)
>> -		intel_modeset_check_crtc(crtc, old_crtc_state, crtc->state);
>> -
>> -	intel_modeset_check_disabled(dev, old_state);
>> -}
>> -
>>  static void update_scanline_offset(struct intel_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->base.dev;
>> @@ -13616,6 +13601,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  		if (dev_priv->display.modeset_commit_cdclk &&
>>  		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
>>  			dev_priv->display.modeset_commit_cdclk(state);
>> +
>> +		intel_modeset_check_disabled(dev);
> As noted on patch #1, since we're checking all disabled state (and not
> just the things that are newly-disabled by this transaction), we may not
> hold full locks over everything that's disabled here and might be racing
> with other updates.  But we're no worse off than we were before.
>
> As a minor behavior change, it looks like we're also no longer calling
> this when we're doing non-modeset pipe updates, but that should be fine.
>
This is true, but enabling a pipe requires connection_mutex, which is always held if we do a modeset anyway..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx