[PATCH v3 00/22] drm/i915: Convert to atomic, part 2.

[PATCH v3 00/22] drm/i915: Convert to atomic, part 2.

[Maarten Lankhorst]

The goal of this patch series is to implement hardware readout using
atomic state, and restore sw state with a single call to intel_set_mode.

After that's done intel_crtc_control can be safely converted to
atomic modeset, because nothing relies on transitional state any
more.

This patch series slightly reduces some user of crtc->config and
crtc->active, but doesn't remove it completely.

Ander Conselvan de Oliveira (6):
  drm/i915: Make __intel_set_mode() take only atomic state as argument
  drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()
  drm/i915: Support modeset across multiple pipes
  drm/i915: Use global atomic state for staged pll config, v2.
  drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()
  drm/i915: Read hw state into an atomic state struct

Maarten Lankhorst (16):
  drm/i915: get rid of put_shared_dpll
  drm/i915: get rid of intel_crtc_disable and related code, v2
  drm/i915: use intel_crtc_control everywhere, v2.
  drm/i915: Use drm_atomic_helper_update_legacy_modeset_state, v2.
  drm/i915: Use crtc_state->active instead of crtc_state->enable
  drm/i915: Zap call to drm_plane_helper_disable.
  drm/i915: calculate primary visibility changes instead of calling from
    set_config
  drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.
  drm/i915: Swap planes on each crtc separately.
  drm/i915: Implement intel_crtc_control using atomic state, v3
  drm/i915: move swap state to the right place
  drm/i915: Use crtc->hwmode for vblanks, v2.
  drm/i915: Remove use of crtc->config from i915_debugfs.c
  drm/i915: Calculate haswell plane workaround, v3.
  drm/i915: Use atomic state for calculating DVO_2X_MODE on i830.
  drm/i915: use calculated state for vblank evasion

 drivers/gpu/drm/i915/i915_debugfs.c  |   50 +-
 drivers/gpu/drm/i915/i915_drv.h      |    2 -
 drivers/gpu/drm/i915/i915_irq.c      |   13 +-
 drivers/gpu/drm/i915/intel_atomic.c  |   96 +--
 drivers/gpu/drm/i915/intel_display.c | 1154 ++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |   32 +-
 6 files changed, 664 insertions(+), 683 deletions(-)

-- 
2.1.0

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

[PATCH v3 01/22] drm/i915: get rid of put_shared_dpll

[Maarten Lankhorst]

Now that the pll updates are staged the put_shared_dpll function
consists only of checks that are done in check_shared_dpll_state
after a modeset too.

The changes to pll->config are overwritten by
intel_shared_dpll_commit, so this entire function is a noop.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 34 +++-------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c97b4963e5c1..7c6adda424f3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4214,27 +4214,6 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
 	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
 }
 
-void intel_put_shared_dpll(struct intel_crtc *crtc)
-{
-	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
-
-	if (pll == NULL)
-		return;
-
-	if (!(pll->config.crtc_mask & (1 << crtc->pipe))) {
-		WARN(1, "bad %s crtc mask\n", pll->name);
-		return;
-	}
-
-	pll->config.crtc_mask &= ~(1 << crtc->pipe);
-	if (pll->config.crtc_mask == 0) {
-		WARN_ON(pll->on);
-		WARN_ON(pll->active);
-	}
-
-	crtc->config->shared_dpll = DPLL_ID_PRIVATE;
-}
-
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 						struct intel_crtc_state *crtc_state)
 {
@@ -5208,13 +5187,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 		intel_disable_shared_dpll(intel_crtc);
 }
 
-static void ironlake_crtc_off(struct drm_crtc *crtc)
-{
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	intel_put_shared_dpll(intel_crtc);
-}
-
-
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -14115,7 +14087,7 @@ static void intel_init_display(struct drm_device *dev)
 			haswell_crtc_compute_clock;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
-		dev_priv->display.off = ironlake_crtc_off;
+		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			skylake_update_primary_plane;
 	} else if (HAS_DDI(dev)) {
@@ -14126,7 +14098,7 @@ static void intel_init_display(struct drm_device *dev)
 			haswell_crtc_compute_clock;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
-		dev_priv->display.off = ironlake_crtc_off;
+		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
@@ -14137,7 +14109,7 @@ static void intel_init_display(struct drm_device *dev)
 			ironlake_crtc_compute_clock;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
-		dev_priv->display.off = ironlake_crtc_off;
+		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47bc729043c5..70d0cc8fe70e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1082,7 +1082,6 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false)
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 						struct intel_crtc_state *state);
-void intel_put_shared_dpll(struct intel_crtc *crtc);
 
 void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
 		      const struct dpll *dpll);
-- 
2.1.0

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

[PATCH v3 02/22] drm/i915: get rid of intel_crtc_disable and related code, v2

[Maarten Lankhorst]

Now that the dpll updates are (mostly) atomic, the .off() code is a noop,
and intel_crtc_disable does mostly the same as intel_modeset_update_state.

Move all logic for connectors_active and setting dpms to that function.

Changes since v1:
- Move drm_atomic_helper_swap_state up.
Changes since v2:
- Split out intel_put_shared_dpll removal.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/intel_display.c | 75 ++++++++----------------------------
 2 files changed, 15 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 731b5cedf11d..37e2312b547c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -574,7 +574,6 @@ struct drm_i915_display_funcs {
 				  struct intel_crtc_state *crtc_state);
 	void (*crtc_enable)(struct drm_crtc *crtc);
 	void (*crtc_disable)(struct drm_crtc *crtc);
-	void (*off)(struct drm_crtc *crtc);
 	void (*audio_codec_enable)(struct drm_connector *connector,
 				   struct intel_encoder *encoder,
 				   struct drm_display_mode *mode);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7c6adda424f3..cae36ec1c2ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6018,10 +6018,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static void i9xx_crtc_off(struct drm_crtc *crtc)
-{
-}
-
 /* Master function to enable/disable CRTC and corresponding power wells */
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
@@ -6071,34 +6067,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 	crtc->state->active = enable;
 }
 
-static void intel_crtc_disable(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_connector *connector;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	/* crtc should still be enabled when we disable it. */
-	WARN_ON(!crtc->state->enable);
-
-	intel_crtc_disable_planes(crtc);
-	dev_priv->display.crtc_disable(crtc);
-	dev_priv->display.off(crtc);
-
-	drm_plane_helper_disable(crtc->primary);
-
-	/* Update computed state. */
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (!connector->encoder || !connector->encoder->crtc)
-			continue;
-
-		if (connector->encoder->crtc != crtc)
-			continue;
-
-		connector->dpms = DRM_MODE_DPMS_OFF;
-		to_intel_encoder(connector->encoder)->connectors_active = false;
-	}
-}
-
 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
@@ -11626,26 +11594,22 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
-	int i;
 
 	intel_shared_dpll_commit(dev_priv);
+	drm_atomic_helper_swap_state(state->dev, state);
 
 	for_each_intel_encoder(dev, intel_encoder) {
 		if (!intel_encoder->base.crtc)
 			continue;
 
-		for_each_crtc_in_state(state, crtc, crtc_state, i)
-			if (crtc == intel_encoder->base.crtc)
-				break;
-
-		if (crtc != intel_encoder->base.crtc)
+		crtc = intel_encoder->base.crtc;
+		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+		if (!crtc_state || !needs_modeset(crtc->state))
 			continue;
 
-		if (crtc_state->enable && needs_modeset(crtc_state))
-			intel_encoder->connectors_active = false;
+		intel_encoder->connectors_active = false;
 	}
 
-	drm_atomic_helper_swap_state(state->dev, state);
 	intel_modeset_fixup_state(state);
 
 	/* Double check state. */
@@ -11657,14 +11621,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		if (!connector->encoder || !connector->encoder->crtc)
 			continue;
 
-		for_each_crtc_in_state(state, crtc, crtc_state, i)
-			if (crtc == connector->encoder->crtc)
-				break;
-
-		if (crtc != connector->encoder->crtc)
+		crtc = connector->encoder->crtc;
+		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+		if (!crtc_state || !needs_modeset(crtc->state))
 			continue;
 
-		if (crtc->state->enable && needs_modeset(crtc->state)) {
+		if (crtc->state->enable) {
 			struct drm_property *dpms_property =
 				dev->mode_config.dpms_property;
 
@@ -11675,9 +11637,9 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 
 			intel_encoder = to_intel_encoder(connector->encoder);
 			intel_encoder->connectors_active = true;
-		}
+		} else
+			connector->dpms = DRM_MODE_DPMS_OFF;
 	}
-
 }
 
 static bool intel_fuzzy_clock_check(int clock1, int clock2)
@@ -12351,12 +12313,10 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 		if (!needs_modeset(crtc_state))
 			continue;
 
-		if (!crtc_state->enable) {
-			intel_crtc_disable(crtc);
-		} else if (crtc->state->enable) {
-			intel_crtc_disable_planes(crtc);
-			dev_priv->display.crtc_disable(crtc);
-		}
+		intel_crtc_disable_planes(crtc);
+		dev_priv->display.crtc_disable(crtc);
+		if (!crtc_state->enable)
+			drm_plane_helper_disable(crtc->primary);
 	}
 
 	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
@@ -14087,7 +14047,6 @@ static void intel_init_display(struct drm_device *dev)
 			haswell_crtc_compute_clock;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
-		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			skylake_update_primary_plane;
 	} else if (HAS_DDI(dev)) {
@@ -14098,7 +14057,6 @@ static void intel_init_display(struct drm_device *dev)
 			haswell_crtc_compute_clock;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
-		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
@@ -14109,7 +14067,6 @@ static void intel_init_display(struct drm_device *dev)
 			ironlake_crtc_compute_clock;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
-		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
@@ -14119,7 +14076,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
-		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			i9xx_update_primary_plane;
 	} else {
@@ -14129,7 +14085,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
-		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			i9xx_update_primary_plane;
 	}
-- 
2.1.0

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

[PATCH v3 03/22] drm/i915: use intel_crtc_control everywhere, v2.

[Maarten Lankhorst]

Having a single path for everything makes it a lot easier to keep
crtc_state->active in sync with intel_crtc->active.

A crtc cannot be changed to active when not enabled, because it means
no mode is set and no connectors are connected.

This should also make intel_crtc->active match crtc_state->active.

Changes since v1:
- Reworded commit message, there's no intel_crtc_toggle.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 18 ++++++++--
 drivers/gpu/drm/i915/intel_display.c | 64 +++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 3 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a32b669bab89..8bf21e8abd93 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3606,12 +3606,18 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	 */
 	if (crtc->config->cpu_transcoder == TRANSCODER_EDP &&
 	    !crtc->config->pch_pfit.enabled) {
+		bool active = crtc->active;
+
+		if (active)
+			intel_crtc_control(&crtc->base, false);
+
 		crtc->config->pch_pfit.force_thru = true;
 
 		intel_display_power_get(dev_priv,
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
 
-		intel_crtc_reset(crtc);
+		if (active)
+			intel_crtc_control(&crtc->base, true);
 	}
 	drm_modeset_unlock_all(dev);
 }
@@ -3630,12 +3636,18 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	 * routing.
 	 */
 	if (crtc->config->pch_pfit.force_thru) {
-		crtc->config->pch_pfit.force_thru = false;
+		bool active = crtc->active;
 
-		intel_crtc_reset(crtc);
+		if (active)
+			intel_crtc_control(&crtc->base, false);
+
+		crtc->config->pch_pfit.force_thru = false;
 
 		intel_display_power_put(dev_priv,
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+
+		if (active)
+			intel_crtc_control(&crtc->base, true);
 	}
 	drm_modeset_unlock_all(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cae36ec1c2ef..2919da975ab2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3189,22 +3189,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
 	}
 }
 
-void intel_crtc_reset(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-
-	if (!crtc->active)
-		return;
-
-	intel_crtc_disable_planes(&crtc->base);
-	dev_priv->display.crtc_disable(&crtc->base);
-	dev_priv->display.crtc_enable(&crtc->base);
-	intel_crtc_enable_planes(&crtc->base);
-}
-
 void intel_prepare_reset(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *crtc;
 
 	/* no reset support for gen2 */
@@ -3216,18 +3202,12 @@ void intel_prepare_reset(struct drm_device *dev)
 		return;
 
 	drm_modeset_lock_all(dev);
-
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
-	}
+	for_each_intel_crtc(dev, crtc)
+		intel_crtc_control(&crtc->base, false);
 }
 
 void intel_finish_reset(struct drm_device *dev)
@@ -6027,26 +6007,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	enum intel_display_power_domain domain;
 	unsigned long domains;
 
+	if (enable == intel_crtc->active)
+		return;
+
+	if (enable && !crtc->state->enable)
+		return;
+
+	crtc->state->active = enable;
 	if (enable) {
-		if (!intel_crtc->active) {
-			domains = get_crtc_power_domains(crtc);
-			for_each_power_domain(domain, domains)
-				intel_display_power_get(dev_priv, domain);
-			intel_crtc->enabled_power_domains = domains;
+		domains = get_crtc_power_domains(crtc);
+		for_each_power_domain(domain, domains)
+			intel_display_power_get(dev_priv, domain);
+		intel_crtc->enabled_power_domains = domains;
 
-			dev_priv->display.crtc_enable(crtc);
-			intel_crtc_enable_planes(crtc);
-		}
+		dev_priv->display.crtc_enable(crtc);
+		intel_crtc_enable_planes(crtc);
 	} else {
-		if (intel_crtc->active) {
-			intel_crtc_disable_planes(crtc);
-			dev_priv->display.crtc_disable(crtc);
+		intel_crtc_disable_planes(crtc);
+		dev_priv->display.crtc_disable(crtc);
 
-			domains = intel_crtc->enabled_power_domains;
-			for_each_power_domain(domain, domains)
-				intel_display_power_put(dev_priv, domain);
-			intel_crtc->enabled_power_domains = 0;
-		}
+		domains = intel_crtc->enabled_power_domains;
+		for_each_power_domain(domain, domains)
+			intel_display_power_put(dev_priv, domain);
+		intel_crtc->enabled_power_domains = 0;
 	}
 }
 
@@ -6063,8 +6046,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 		enable |= intel_encoder->connectors_active;
 
 	intel_crtc_control(crtc, enable);
-
-	crtc->state->active = enable;
 }
 
 void intel_encoder_destroy(struct drm_encoder *encoder)
@@ -14556,8 +14537,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		plane = crtc->plane;
 		to_intel_plane_state(crtc->base.primary->state)->visible = true;
 		crtc->plane = !plane;
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
+		intel_crtc_control(&crtc->base, false);
 		crtc->plane = plane;
 
 		/* ... and break all links. */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 70d0cc8fe70e..d0d99669aa00 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -993,7 +993,6 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
-void intel_crtc_reset(struct intel_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
-- 
2.1.0

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

[PATCH v3 04/22] drm/i915: Use drm_atomic_helper_update_legacy_modeset_state, v2.

[Maarten Lankhorst]

Now that the helper is exported there's no need to duplicate
this code any more.

Changes since v1:
- move intel_modeset_update_staged_output_state call to the right place.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2919da975ab2..abf297bf1f3e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11067,43 +11067,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 	}
 }
 
-/* Fixup legacy state after an atomic state swap.
- */
-static void intel_modeset_fixup_state(struct drm_atomic_state *state)
-{
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-
-	for_each_intel_connector(state->dev, connector) {
-		connector->base.encoder = connector->base.state->best_encoder;
-		if (connector->base.encoder)
-			connector->base.encoder->crtc =
-				connector->base.state->crtc;
-	}
-
-	/* Update crtc of disabled encoders */
-	for_each_intel_encoder(state->dev, encoder) {
-		int num_connectors = 0;
-
-		for_each_intel_connector(state->dev, connector)
-			if (connector->base.encoder == &encoder->base)
-				num_connectors++;
-
-		if (num_connectors == 0)
-			encoder->base.crtc = NULL;
-	}
-
-	for_each_intel_crtc(state->dev, crtc) {
-		crtc->base.enabled = crtc->base.state->enable;
-		crtc->config = to_intel_crtc_state(crtc->base.state);
-	}
-
-	/* Copy the new configuration to the staged state, to keep the few
-	 * pieces of code that haven't been converted yet happy */
-	intel_modeset_update_staged_output_state(state->dev);
-}
-
 static void
 connected_sink_compute_bpp(struct intel_connector *connector,
 			   struct intel_crtc_state *pipe_config)
@@ -11591,11 +11554,14 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		intel_encoder->connectors_active = false;
 	}
 
-	intel_modeset_fixup_state(state);
+	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
+	intel_modeset_update_staged_output_state(state->dev);
 
 	/* Double check state. */
 	for_each_crtc(dev, crtc) {
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
+
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
 	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -12300,25 +12266,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			drm_plane_helper_disable(crtc->primary);
 	}
 
-	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
-	 * to set it here already despite that we pass it down the callchain.
-	 *
-	 * Note we'll need to fix this up when we start tracking multiple
-	 * pipes; here we assume a single modeset_pipe and only track the
-	 * single crtc and mode.
-	 */
-	if (pipe_config->base.enable && needs_modeset(&pipe_config->base)) {
-		modeset_crtc->mode = pipe_config->base.mode;
-
-		/*
-		 * Calculate and store various constants which
-		 * are later needed by vblank and swap-completion
-		 * timestamping. They are derived from true hwmode.
-		 */
-		drm_calc_timestamping_constants(modeset_crtc,
-						&pipe_config->base.adjusted_mode);
-	}
-
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
 	intel_modeset_update_state(state);
-- 
2.1.0

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

[PATCH v3 05/22] drm/i915: Make __intel_set_mode() take only atomic state as argument

[Maarten Lankhorst]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

With the use of drm_atomic_helper_update_legacy_modeset_state the
last user of modeset_crtc is removed from this function.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index abf297bf1f3e..2f8ec7fc7737 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12237,12 +12237,10 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
-static int __intel_set_mode(struct drm_crtc *modeset_crtc,
-			    struct intel_crtc_state *pipe_config)
+static int __intel_set_mode(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = modeset_crtc->dev;
+	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_atomic_state *state = pipe_config->base.state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int ret = 0;
@@ -12302,7 +12300,7 @@ static int intel_set_mode_with_config(struct drm_crtc *crtc,
 {
 	int ret;
 
-	ret = __intel_set_mode(crtc, pipe_config);
+	ret = __intel_set_mode(pipe_config->base.state);
 
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
-- 
2.1.0

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

[PATCH v3 06/22] drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()

[Maarten Lankhorst]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

A follow up patch will make intel_modeset_compute_config() deal with
multiple crtcs, so move crtc specific stuff into the lower level crtc
specific function.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f8ec7fc7737..f10e7ca7ad81 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -431,6 +431,12 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
 }
 
+static bool
+needs_modeset(struct drm_crtc_state *state)
+{
+	return state->mode_changed || state->active_changed;
+}
+
 /**
  * Returns whether any output on the specified pipe is of the specified type
  */
@@ -11415,6 +11421,15 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	/*
+	 * XXX: Add all connectors to make the crtc state match the encoders.
+	 */
+	if (!needs_modeset(&pipe_config->base)) {
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			return ret;
+	}
+
 	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->cpu_transcoder =
@@ -11506,6 +11521,18 @@ encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
+	/* Check if we need to force a modeset */
+	if (pipe_config->has_audio !=
+	    to_intel_crtc_state(crtc->state)->has_audio)
+		pipe_config->base.mode_changed = true;
+
+	/*
+	 * Note we have an issue here with infoframes: current code
+	 * only updates them on the full mode set path per hw
+	 * requirements.  So here we should be checking for any
+	 * required changes and forcing a mode set.
+	 */
+
 	return 0;
 fail:
 	return ret;
@@ -11523,12 +11550,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
 	return false;
 }
 
-static bool
-needs_modeset(struct drm_crtc_state *state)
-{
-	return state->mode_changed || state->active_changed;
-}
-
 static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
@@ -12117,10 +12138,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	struct intel_crtc_state *pipe_config;
 	int ret = 0;
 
-	ret = drm_atomic_add_affected_connectors(state, crtc);
-	if (ret)
-		return ERR_PTR(ret);
-
 	ret = drm_atomic_helper_check_modeset(state->dev, state);
 	if (ret)
 		return ERR_PTR(ret);
@@ -12142,19 +12159,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (ret)
 		return ERR_PTR(ret);
 
-	/* Check things that can only be changed through modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc(crtc)->config->has_audio)
-		pipe_config->base.mode_changed = true;
-
-	/*
-	 * Note we have an issue here with infoframes: current code
-	 * only updates them on the full mode set path per hw
-	 * requirements.  So here we should be checking for any
-	 * required changes and forcing a mode set.
-	 */
-
-	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
+	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
 
 	ret = drm_atomic_helper_check_planes(state->dev, state);
 	if (ret)
-- 
2.1.0

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

[PATCH v3 07/22] drm/i915: Use crtc_state->active instead of crtc_state->enable

[Maarten Lankhorst]

crtc_state->enable means a crtc is configured, but it may be turned
off for dpms. Until the commit "use intel_crtc_control everywhere"
crtc_state->active was not updated on crtc off, but now
crtc_state->active should be used for tracking whether a crtc is
scanning out or not.

A few commits from now dpms will be handled by calling
intel_set_mode with a different value for crtc_state->active,
which causes a crtc to turn on or off.

At this point crtc->active should mirror crtc_state->active,
so some paranoia from the crtc_disable functions can be removed.

intel_set_mode_setup_plls still checks for ->enable, because all
resources that are needed have to be calculated, else
dpms changes may not succeed.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 557af8877a2e..ca457317a8ac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 
-	if (!crtc->state->enable) {
+	if (!crtc->state->active) {
 		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
 		return -EBUSY;
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f10e7ca7ad81..9e1723e20377 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4646,7 +4646,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	bool reenable_ips = false;
 
 	/* The clocks have to be on to load the palette. */
-	if (!crtc->state->enable || !intel_crtc->active)
+	if (!crtc->state->active)
 		return;
 
 	if (HAS_GMCH_DISPLAY(dev_priv->dev)) {
@@ -4851,9 +4851,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -4957,9 +4955,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	if (intel_crtc_to_shared_dpll(intel_crtc))
@@ -5061,7 +5057,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	if (!intel_crtc->active)
+	if (WARN_ON(!intel_crtc->active))
 		return;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -5123,7 +5119,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
-	if (!intel_crtc->active)
+	if (WARN_ON(!intel_crtc->active))
 		return;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
@@ -5721,7 +5717,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* add all active pipes to the state */
 	for_each_crtc(state->dev, crtc) {
-		if (!crtc->state->enable)
+		if (!crtc->state->active)
 			continue;
 
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
@@ -5731,7 +5727,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* disable/enable all currently active pipes while we change cdclk */
 	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		if (crtc_state->enable)
+		if (crtc_state->active)
 			crtc_state->mode_changed = true;
 
 	return 0;
@@ -5819,9 +5815,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	bool is_dsi;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
@@ -5897,9 +5891,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	i9xx_set_pll_dividers(intel_crtc);
@@ -5959,7 +5951,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (!intel_crtc->active)
+	if (WARN_ON(!intel_crtc->active))
 		return;
 
 	/*
@@ -11594,7 +11586,7 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		if (!crtc_state || !needs_modeset(crtc->state))
 			continue;
 
-		if (crtc->state->enable) {
+		if (crtc->state->active) {
 			struct drm_property *dpms_property =
 				dev->mode_config.dpms_property;
 
@@ -12011,6 +12003,10 @@ check_crtc_state(struct drm_device *dev)
 		     "crtc active state doesn't match with hw state "
 		     "(expected %i, found %i)\n", crtc->active, active);
 
+		I915_STATE_WARN(crtc->active != crtc->base.state->active,
+		     "transitional active state does not match atomic hw state "
+		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
+
 		if (active &&
 		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
@@ -12152,6 +12148,10 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (IS_ERR(pipe_config))
 		return pipe_config;
 
+	if (!pipe_config->base.enable &&
+	    WARN_ON(pipe_config->base.active))
+		pipe_config->base.active = false;
+
 	if (!pipe_config->base.enable)
 		return pipe_config;
 
@@ -12260,7 +12260,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc_state))
+		if (!needs_modeset(crtc_state) || !crtc->state->active)
 			continue;
 
 		intel_crtc_disable_planes(crtc);
@@ -12282,7 +12282,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc->state) || !crtc->state->enable)
+		if (!needs_modeset(crtc->state) || !crtc->state->active)
 			continue;
 
 		update_scanline_offset(to_intel_crtc(crtc));
@@ -14525,7 +14525,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	 * have active connectors/encoders. */
 	intel_crtc_update_dpms(&crtc->base);
 
-	if (crtc->active != crtc->base.state->enable) {
+	if (crtc->active != crtc->base.state->active) {
 		struct intel_encoder *encoder;
 
 		/* This can happen either due to bugs in the get_hw_state
-- 
2.1.0

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

[PATCH v3 08/22] drm/i915: Zap call to drm_plane_helper_disable.

[Maarten Lankhorst]

The primary plane can still be configured when crtc is off,
furthermore this is also a noop now that affected planes are
added on modesets.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9e1723e20377..d738230bedf8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12265,8 +12265,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 		intel_crtc_disable_planes(crtc);
 		dev_priv->display.crtc_disable(crtc);
-		if (!crtc_state->enable)
-			drm_plane_helper_disable(crtc->primary);
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
-- 
2.1.0

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

[PATCH v3 09/22] drm/i915: calculate primary visibility changes instead of calling from set_config

[Maarten Lankhorst]

This should be much cleaner, with the same effects.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d738230bedf8..e592082846d1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12546,20 +12546,11 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	return 0;
 }
 
-static bool primary_plane_visible(struct drm_crtc *crtc)
-{
-	struct intel_plane_state *plane_state =
-		to_intel_plane_state(crtc->primary->state);
-
-	return plane_state->visible;
-}
-
 static int intel_crtc_set_config(struct drm_mode_set *set)
 {
 	struct drm_device *dev;
 	struct drm_atomic_state *state = NULL;
 	struct intel_crtc_state *pipe_config;
-	bool primary_plane_was_visible;
 	int ret;
 
 	BUG_ON(!set);
@@ -12598,38 +12589,8 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 
 	intel_update_pipe_size(to_intel_crtc(set->crtc));
 
-	primary_plane_was_visible = primary_plane_visible(set->crtc);
-
 	ret = intel_set_mode_with_config(set->crtc, pipe_config);
 
-	if (ret == 0 &&
-	    pipe_config->base.enable &&
-	    pipe_config->base.planes_changed &&
-	    !needs_modeset(&pipe_config->base)) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
-
-		/*
-		 * We need to make sure the primary plane is re-enabled if it
-		 * has previously been turned off.
-		 */
-		if (ret == 0 && !primary_plane_was_visible &&
-		    primary_plane_visible(set->crtc)) {
-			WARN_ON(!intel_crtc->active);
-			intel_post_enable_primary(set->crtc);
-		}
-
-		/*
-		 * In the fastboot case this may be our only check of the
-		 * state after boot.  It would be better to only do it on
-		 * the first update, but we don't have a nice way of doing that
-		 * (and really, set_config isn't used much for high freq page
-		 * flipping, so increasing its cost here shouldn't be a big
-		 * deal).
-		 */
-		if (i915.fastboot && ret == 0)
-			intel_modeset_check_state(set->crtc->dev);
-	}
-
 	if (ret) {
 		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
 			      set->crtc->base.id, ret);
@@ -12957,8 +12918,15 @@ intel_check_primary_plane(struct drm_plane *plane,
 			 */
 			if (IS_BROADWELL(dev))
 				intel_crtc->atomic.wait_vblank = true;
+
+			if (crtc_state && !needs_modeset(&crtc_state->base))
+				intel_crtc->atomic.post_enable_primary = true;
 		}
 
+		if (!state->visible && old_state->visible &&
+		    crtc_state && !needs_modeset(&crtc_state->base))
+			intel_crtc->atomic.pre_disable_primary = true;
+
 		intel_crtc->atomic.fb_bits |=
 			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 
-- 
2.1.0

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

[PATCH v3 10/22] drm/i915: Support modeset across multiple pipes

[maarten.lankhorst]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Compute new pipe_configs for all crtcs in the atomic state. The commit
part of the mode set (__intel_set_mode()) is already enabled to support
multiple pipes, the only thing missing was calculating a new pipe_config
for every crtc.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e592082846d1..652865e21bc5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -86,8 +86,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config);
 
-static int intel_set_mode(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state);
+static int intel_set_mode(struct drm_atomic_state *state);
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -9809,7 +9808,7 @@ retry:
 
 	drm_mode_copy(&crtc_state->base.mode, mode);
 
-	if (intel_set_mode(crtc, state)) {
+	if (intel_set_mode(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
@@ -9883,7 +9882,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (ret)
 			goto fail;
 
-		ret = intel_set_mode(crtc, state);
+		ret = intel_set_mode(state);
 		if (ret)
 			goto fail;
 
@@ -11393,9 +11392,10 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 
 static int
 intel_modeset_pipe_config(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state,
-			  struct intel_crtc_state *pipe_config)
+			  struct drm_atomic_state *state)
 {
+	struct drm_crtc_state *crtc_state;
+	struct intel_crtc_state *pipe_config;
 	struct intel_encoder *encoder;
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
@@ -11413,6 +11413,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+	if (WARN_ON(!crtc_state))
+		return -EINVAL;
+
+	pipe_config = to_intel_crtc_state(crtc_state);
+
 	/*
 	 * XXX: Add all connectors to make the crtc state match the encoders.
 	 */
@@ -12127,45 +12133,35 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
-static struct intel_crtc_state *
-intel_modeset_compute_config(struct drm_crtc *crtc,
-			     struct drm_atomic_state *state)
+static int
+intel_modeset_compute_config(struct drm_atomic_state *state)
 {
-	struct intel_crtc_state *pipe_config;
-	int ret = 0;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
 
 	ret = drm_atomic_helper_check_modeset(state->dev, state);
 	if (ret)
-		return ERR_PTR(ret);
-
-	/*
-	 * Note this needs changes when we start tracking multiple modes
-	 * and crtcs.  At that point we'll need to compute the whole config
-	 * (i.e. one pipe_config for each crtc) rather than just the one
-	 * for this crtc.
-	 */
-	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
-	if (IS_ERR(pipe_config))
-		return pipe_config;
-
-	if (!pipe_config->base.enable &&
-	    WARN_ON(pipe_config->base.active))
-		pipe_config->base.active = false;
+		return ret;
 
-	if (!pipe_config->base.enable)
-		return pipe_config;
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc_state->enable &&
+		    WARN_ON(crtc_state->active))
+			crtc_state->active = false;
 
-	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
-	if (ret)
-		return ERR_PTR(ret);
+		if (!crtc_state->enable)
+			continue;
 
-	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
+		ret = intel_modeset_pipe_config(crtc, state);
+		if (ret)
+			return ret;
 
-	ret = drm_atomic_helper_check_planes(state->dev, state);
-	if (ret)
-		return ERR_PTR(ret);
+		intel_dump_pipe_config(to_intel_crtc(crtc),
+				       to_intel_crtc_state(crtc_state),
+				       "[modeset]");
+	}
 
-	return pipe_config;
+	return drm_atomic_helper_check_planes(state->dev, state);
 }
 
 static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
@@ -12298,37 +12294,27 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	return 0;
 }
 
-static int intel_set_mode_with_config(struct drm_crtc *crtc,
-				      struct intel_crtc_state *pipe_config)
+static int intel_set_mode_checked(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
 	int ret;
 
-	ret = __intel_set_mode(pipe_config->base.state);
-
+	ret = __intel_set_mode(state);
 	if (ret == 0)
-		intel_modeset_check_state(crtc->dev);
+		intel_modeset_check_state(dev);
 
 	return ret;
 }
 
-static int intel_set_mode(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state)
+static int intel_set_mode(struct drm_atomic_state *state)
 {
-	struct intel_crtc_state *pipe_config;
-	int ret = 0;
-
-	pipe_config = intel_modeset_compute_config(crtc, state);
-	if (IS_ERR(pipe_config)) {
-		ret = PTR_ERR(pipe_config);
-		goto out;
-	}
+	int ret;
 
-	ret = intel_set_mode_with_config(crtc, pipe_config);
+	ret = intel_modeset_compute_config(state);
 	if (ret)
-		goto out;
+		return ret;
 
-out:
-	return ret;
+	return intel_set_mode_checked(state);
 }
 
 void intel_crtc_restore_mode(struct drm_crtc *crtc)
@@ -12400,7 +12386,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
 					crtc->primary->fb, crtc->x, crtc->y);
 
-	ret = intel_set_mode(crtc, state);
+	ret = intel_set_mode(state);
 	if (ret)
 		drm_atomic_state_free(state);
 }
@@ -12550,7 +12536,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 {
 	struct drm_device *dev;
 	struct drm_atomic_state *state = NULL;
-	struct intel_crtc_state *pipe_config;
 	int ret;
 
 	BUG_ON(!set);
@@ -12581,16 +12566,13 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	if (ret)
 		goto out;
 
-	pipe_config = intel_modeset_compute_config(set->crtc, state);
-	if (IS_ERR(pipe_config)) {
-		ret = PTR_ERR(pipe_config);
+	ret = intel_modeset_compute_config(state);
+	if (ret)
 		goto out;
-	}
 
 	intel_update_pipe_size(to_intel_crtc(set->crtc));
 
-	ret = intel_set_mode_with_config(set->crtc, pipe_config);
-
+	ret = intel_set_mode_checked(state);
 	if (ret) {
 		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
 			      set->crtc->base.id, ret);
-- 
2.1.0

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

[PATCH v3 11/22] drm/i915: Use global atomic state for staged pll config, v2.

[maarten.lankhorst]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Now that we can subclass drm_atomic_state we can also use it to keep
track of all the pll settings. atomic_state is a better place to hold
all shared state than keeping pll->new_config everywhere.

Changes since v1:
- Assert connection_mutex is held.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 -
 drivers/gpu/drm/i915/intel_atomic.c  |  51 ++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
 4 files changed, 97 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 37e2312b547c..62e15a3d2984 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
 
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
-	struct intel_shared_dpll_config *new_config;
 
 	int active; /* count of number of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 7ed8033aae60..6ab82dc56cea 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -421,3 +421,54 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 
 	return 0;
 }
+
+static void
+intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
+				  struct intel_shared_dpll_config *shared_dpll)
+{
+	enum intel_dpll_id i;
+
+	/* Copy shared dpll state */
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
+
+		shared_dpll[i] = pll->config;
+	}
+}
+
+struct intel_shared_dpll_config *
+intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(s);
+
+	WARN_ON(!drm_modeset_is_locked(&s->dev->mode_config.connection_mutex));
+
+	if (!state->dpll_set) {
+		state->dpll_set = true;
+
+		intel_atomic_duplicate_dpll_state(to_i915(s->dev),
+						  state->shared_dpll);
+	}
+
+	return state->shared_dpll;
+}
+
+struct drm_atomic_state *
+intel_atomic_state_alloc(struct drm_device *dev)
+{
+	struct intel_atomic_state *state = kzalloc(GFP_KERNEL, sizeof(*state));
+
+	if (!state || drm_atomic_state_init(dev, &state->base) < 0) {
+		kfree(state);
+		return NULL;
+	}
+
+	return &state->base;
+}
+
+void intel_atomic_state_clear(struct drm_atomic_state *s)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(s);
+	drm_atomic_state_default_clear(&state->base);
+	state->dpll_set = false;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 652865e21bc5..adb19c3c9172 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4204,8 +4204,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_shared_dpll *pll;
+	struct intel_shared_dpll_config *shared_dpll;
 	enum intel_dpll_id i;
 
+	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
+
 	if (HAS_PCH_IBX(dev_priv->dev)) {
 		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
 		i = (enum intel_dpll_id) crtc->pipe;
@@ -4214,7 +4217,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->new_config->crtc_mask);
+		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
 	}
@@ -4234,7 +4237,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		pll = &dev_priv->shared_dplls[i];
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			crtc->base.base.id, pll->name);
-		WARN_ON(pll->new_config->crtc_mask);
+		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
 	}
@@ -4243,15 +4246,15 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->new_config->crtc_mask == 0)
+		if (shared_dpll[i].crtc_mask == 0)
 			continue;
 
 		if (memcmp(&crtc_state->dpll_hw_state,
-			   &pll->new_config->hw_state,
-			   sizeof(pll->new_config->hw_state)) == 0) {
+			   &shared_dpll[i].hw_state,
+			   sizeof(crtc_state->dpll_hw_state)) == 0) {
 			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
 				      crtc->base.base.id, pll->name,
-				      pll->new_config->crtc_mask,
+				      shared_dpll[i].crtc_mask,
 				      pll->active);
 			goto found;
 		}
@@ -4260,7 +4263,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->new_config->crtc_mask == 0) {
+		if (shared_dpll[i].crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;
@@ -4270,83 +4273,33 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 	return NULL;
 
 found:
-	if (pll->new_config->crtc_mask == 0)
-		pll->new_config->hw_state = crtc_state->dpll_hw_state;
+	if (shared_dpll[i].crtc_mask == 0)
+		shared_dpll[i].hw_state =
+			crtc_state->dpll_hw_state;
 
 	crtc_state->shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->new_config->crtc_mask |= 1 << crtc->pipe;
+	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
 
-/**
- * intel_shared_dpll_start_config - start a new PLL staged config
- * @dev_priv: DRM device
- * @clear_pipes: mask of pipes that will have their PLLs freed
- *
- * Starts a new PLL staged config, copying the current config but
- * releasing the references of pipes specified in clear_pipes.
- */
-static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
-					  unsigned clear_pipes)
-{
-	struct intel_shared_dpll *pll;
-	enum intel_dpll_id i;
-
-	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
-		pll = &dev_priv->shared_dplls[i];
-
-		pll->new_config = kmemdup(&pll->config, sizeof pll->config,
-					  GFP_KERNEL);
-		if (!pll->new_config)
-			goto cleanup;
-
-		pll->new_config->crtc_mask &= ~clear_pipes;
-	}
-
-	return 0;
-
-cleanup:
-	while (--i >= 0) {
-		pll = &dev_priv->shared_dplls[i];
-		kfree(pll->new_config);
-		pll->new_config = NULL;
-	}
-
-	return -ENOMEM;
-}
-
-static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
+static void intel_shared_dpll_commit(struct drm_atomic_state *state)
 {
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_shared_dpll_config *shared_dpll;
 	struct intel_shared_dpll *pll;
 	enum intel_dpll_id i;
 
-	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
-		pll = &dev_priv->shared_dplls[i];
-
-		WARN_ON(pll->new_config == &pll->config);
-
-		pll->config = *pll->new_config;
-		kfree(pll->new_config);
-		pll->new_config = NULL;
-	}
-}
-
-static void intel_shared_dpll_abort_config(struct drm_i915_private *dev_priv)
-{
-	struct intel_shared_dpll *pll;
-	enum intel_dpll_id i;
+	if (!to_intel_atomic_state(state)->dpll_set)
+		return;
 
+	shared_dpll = to_intel_atomic_state(state)->shared_dpll;
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-
-		WARN_ON(pll->new_config == &pll->config);
-
-		kfree(pll->new_config);
-		pll->new_config = NULL;
+		pll->config = shared_dpll[i];
 	}
 }
 
@@ -11552,13 +11505,12 @@ static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
 
-	intel_shared_dpll_commit(dev_priv);
+	intel_shared_dpll_commit(state);
 	drm_atomic_helper_swap_state(state->dev, state);
 
 	for_each_intel_encoder(dev, intel_encoder) {
@@ -12189,9 +12141,13 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 		}
 	}
 
-	ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
-	if (ret)
-		goto done;
+	if (clear_pipes) {
+		struct intel_shared_dpll_config *shared_dpll =
+			intel_atomic_get_shared_dpll_state(state);
+
+		for (i = 0; i < dev_priv->num_shared_dpll; i++)
+			shared_dpll[i].crtc_mask &= ~clear_pipes;
+	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state) || !crtc_state->enable)
@@ -12202,13 +12158,10 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 
 		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
 							   intel_crtc_state);
-		if (ret) {
-			intel_shared_dpll_abort_config(dev_priv);
-			goto done;
-		}
+		if (ret)
+			return ret;
 	}
 
-done:
 	return ret;
 }
 
@@ -13900,6 +13853,8 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
 	.output_poll_changed = intel_fbdev_output_poll_changed,
 	.atomic_check = intel_atomic_check,
 	.atomic_commit = intel_atomic_commit,
+	.atomic_state_alloc = intel_atomic_state_alloc,
+	.atomic_state_clear = intel_atomic_state_clear,
 };
 
 /* Set up chip specific display functions */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d0d99669aa00..7012c67de8d5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -241,6 +241,13 @@ typedef struct dpll {
 	int	p;
 } intel_clock_t;
 
+struct intel_atomic_state {
+	struct drm_atomic_state base;
+
+	bool dpll_set;
+	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
+};
+
 struct intel_plane_state {
 	struct drm_plane_state base;
 	struct drm_rect src;
@@ -627,6 +634,7 @@ struct cxsr_latency {
 	unsigned long cursor_hpll_disable;
 };
 
+#define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
 #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
 #define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, base)
 #define to_intel_connector(x) container_of(x, struct intel_connector, base)
@@ -1402,6 +1410,11 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
 			       struct drm_crtc_state *state);
+struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
+void intel_atomic_state_clear(struct drm_atomic_state *);
+struct intel_shared_dpll_config *
+intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
+
 static inline struct intel_crtc_state *
 intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 			    struct intel_crtc *crtc)
-- 
2.1.0

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

[PATCH v3 12/22] drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

And update crtc->config to point to the new state. There is no point
in swapping only part of the state when the rest of the state
should be untouched.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 6ab82dc56cea..e93aade0f2dc 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -129,6 +129,8 @@ int intel_atomic_commit(struct drm_device *dev,
 			struct drm_atomic_state *state,
 			bool async)
 {
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
 	int ret;
 	int i;
 
@@ -142,48 +144,26 @@ int intel_atomic_commit(struct drm_device *dev,
 		return ret;
 
 	/* Point of no return */
+	drm_atomic_helper_swap_state(dev, state);
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+
+		if (INTEL_INFO(dev)->gen >= 9)
+			skl_detach_scalers(to_intel_crtc(crtc));
+	}
 
 	/*
 	 * FIXME:  The proper sequence here will eventually be:
 	 *
-	 * drm_atomic_helper_swap_state(dev, state)
 	 * drm_atomic_helper_commit_modeset_disables(dev, state);
 	 * drm_atomic_helper_commit_planes(dev, state);
 	 * drm_atomic_helper_commit_modeset_enables(dev, state);
-	 * drm_atomic_helper_wait_for_vblanks(dev, state);
-	 * drm_atomic_helper_cleanup_planes(dev, state);
-	 * drm_atomic_state_free(state);
 	 *
-	 * once we have full atomic modeset.  For now, just manually update
-	 * plane states to avoid clobbering good states with dummy states
-	 * while nuclear pageflipping.
+	 * once we have full atomic modeset.
 	 */
-	for (i = 0; i < dev->mode_config.num_total_plane; i++) {
-		struct drm_plane *plane = state->planes[i];
-
-		if (!plane)
-			continue;
-
-		plane->state->state = state;
-		swap(state->plane_states[i], plane->state);
-		plane->state->state = NULL;
-	}
-
-	/* swap crtc_scaler_state */
-	for (i = 0; i < dev->mode_config.num_crtc; i++) {
-		struct drm_crtc *crtc = state->crtcs[i];
-		if (!crtc) {
-			continue;
-		}
-
-		to_intel_crtc(crtc)->config->scaler_state =
-			to_intel_crtc_state(state->crtc_states[i])->scaler_state;
-
-		if (INTEL_INFO(dev)->gen >= 9)
-			skl_detach_scalers(to_intel_crtc(crtc));
-	}
-
 	drm_atomic_helper_commit_planes(dev, state);
+
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
 	drm_atomic_state_free(state);
-- 
2.1.0

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

[PATCH v3 13/22] drm/i915: Swap planes on each crtc separately.

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Repeated calls to begin_crtc_commit can cause warnings like this:
[  169.127746] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:616
[  169.127835] in_atomic(): 0, irqs_disabled(): 1, pid: 1947, name: kms_flip
[  169.127840] 3 locks held by kms_flip/1947:
[  169.127843]  #0:  (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffff814774bc>] __drm_modeset_lock_all+0x9c/0x130
[  169.127860]  #1:  (crtc_ww_class_acquire){+.+.+.}, at: [<ffffffff814774cd>] __drm_modeset_lock_all+0xad/0x130
[  169.127870]  #2:  (crtc_ww_class_mutex){+.+.+.}, at: [<ffffffff81477178>] drm_modeset_lock+0x38/0x110
[  169.127879] irq event stamp: 665690
[  169.127882] hardirqs last  enabled at (665689): [<ffffffff817ffdb5>] _raw_spin_unlock_irqrestore+0x55/0x70
[  169.127889] hardirqs last disabled at (665690): [<ffffffffc0197a23>] intel_pipe_update_start+0x113/0x5c0 [i915]
[  169.127936] softirqs last  enabled at (665470): [<ffffffff8108a766>] __do_softirq+0x236/0x650
[  169.127942] softirqs last disabled at (665465): [<ffffffff8108ae75>] irq_exit+0xc5/0xd0
[  169.127951] CPU: 1 PID: 1947 Comm: kms_flip Not tainted 4.1.0-rc4-patser+ #4039
[  169.127954] Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
[  169.127957]  ffff8800c49036f0 ffff8800cde5fa28 ffffffff817f6907 0000000080000001
[  169.127964]  0000000000000000 ffff8800cde5fa58 ffffffff810aebed 0000000000000046
[  169.127970]  ffffffff81c5d518 0000000000000268 0000000000000000 ffff8800cde5fa88
[  169.127981] Call Trace:
[  169.127992]  [<ffffffff817f6907>] dump_stack+0x4f/0x7b
[  169.128001]  [<ffffffff810aebed>] ___might_sleep+0x16d/0x270
[  169.128008]  [<ffffffff810aed38>] __might_sleep+0x48/0x90
[  169.128017]  [<ffffffff817fc359>] mutex_lock_nested+0x29/0x410
[  169.128073]  [<ffffffffc01635f0>] ? vgpu_write64+0x220/0x220 [i915]
[  169.128138]  [<ffffffffc017fddf>] ? ironlake_update_primary_plane+0x2ff/0x410 [i915]
[  169.128198]  [<ffffffffc0190e75>] intel_frontbuffer_flush+0x25/0x70 [i915]
[  169.128253]  [<ffffffffc01831ac>] intel_finish_crtc_commit+0x4c/0x180 [i915]
[  169.128279]  [<ffffffffc00784ac>] drm_atomic_helper_commit_planes+0x12c/0x240 [drm_kms_helper]
[  169.128338]  [<ffffffffc0184264>] __intel_set_mode+0x684/0x830 [i915]
[  169.128378]  [<ffffffffc018a84a>] intel_crtc_set_config+0x49a/0x620 [i915]
[  169.128385]  [<ffffffff817fdd39>] ? mutex_unlock+0x9/0x10
[  169.128391]  [<ffffffff81467b69>] drm_mode_set_config_internal+0x69/0x120
[  169.128398]  [<ffffffff8119b547>] ? might_fault+0x57/0xb0
[  169.128403]  [<ffffffff8146bf93>] drm_mode_setcrtc+0x253/0x620
[  169.128409]  [<ffffffff8145c600>] drm_ioctl+0x1a0/0x6a0
[  169.128415]  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
[  169.128424]  [<ffffffff811e9ab8>] do_vfs_ioctl+0x2f8/0x530
[  169.128429]  [<ffffffff810d0fcd>] ? trace_hardirqs_on+0xd/0x10
[  169.128435]  [<ffffffff812e7676>] ? selinux_file_ioctl+0x56/0x100
[  169.128439]  [<ffffffff811e9d71>] SyS_ioctl+0x81/0xa0
[  169.128445]  [<ffffffff81800697>] system_call_fastpath+0x12/0x6f

Solve it by using the newly introduced drm_atomic_helper_commit_planes_on_crtc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 13 +++----------
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index e93aade0f2dc..83078763ba14 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -151,18 +151,11 @@ int intel_atomic_commit(struct drm_device *dev,
 
 		if (INTEL_INFO(dev)->gen >= 9)
 			skl_detach_scalers(to_intel_crtc(crtc));
+
+		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 	}
 
-	/*
-	 * FIXME:  The proper sequence here will eventually be:
-	 *
-	 * drm_atomic_helper_commit_modeset_disables(dev, state);
-	 * drm_atomic_helper_commit_planes(dev, state);
-	 * drm_atomic_helper_commit_modeset_enables(dev, state);
-	 *
-	 * once we have full atomic modeset.
-	 */
-	drm_atomic_helper_commit_planes(dev, state);
+	/* FIXME: This function should eventually call __intel_set_mode when needed */
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index adb19c3c9172..81d5358efdde 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12225,10 +12225,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	modeset_update_crtc_power_domains(state);
 
-	drm_atomic_helper_commit_planes(dev, state);
-
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+
 		if (!needs_modeset(crtc->state) || !crtc->state->active)
 			continue;
 
@@ -14572,6 +14572,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		struct intel_plane_state *plane_state;
 
 		memset(crtc->config, 0, sizeof(*crtc->config));
+		crtc->config->base.crtc = &crtc->base;
 
 		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
-- 
2.1.0

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

[PATCH v3 14/22] drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()

[maarten.lankhorst]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

It makes more sense there, since these are computation steps that can
fail.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81d5358efdde..e7aa8610cbdc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
-static int
-intel_modeset_compute_config(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int ret, i;
-
-	ret = drm_atomic_helper_check_modeset(state->dev, state);
-	if (ret)
-		return ret;
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc_state->enable &&
-		    WARN_ON(crtc_state->active))
-			crtc_state->active = false;
-
-		if (!crtc_state->enable)
-			continue;
-
-		ret = intel_modeset_pipe_config(crtc, state);
-		if (ret)
-			return ret;
-
-		intel_dump_pipe_config(to_intel_crtc(crtc),
-				       to_intel_crtc_state(crtc_state),
-				       "[modeset]");
-	}
-
-	return drm_atomic_helper_check_planes(state->dev, state);
-}
-
 static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
+static int
+intel_modeset_compute_config(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	ret = drm_atomic_helper_check_modeset(state->dev, state);
+	if (ret)
+		return ret;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc_state->enable &&
+		    WARN_ON(crtc_state->active))
+			crtc_state->active = false;
+
+		if (!crtc_state->enable)
+			continue;
+
+		ret = intel_modeset_pipe_config(crtc, state);
+		if (ret)
+			return ret;
+
+		intel_dump_pipe_config(to_intel_crtc(crtc),
+				       to_intel_crtc_state(crtc_state),
+				       "[modeset]");
+	}
+
+	ret = drm_atomic_helper_check_planes(state->dev, state);
+	if (ret)
+		return ret;
+
+	return __intel_set_mode_checks(state);
+}
+
 static int __intel_set_mode(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -12200,10 +12204,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	int ret = 0;
 	int i;
 
-	ret = __intel_set_mode_checks(state);
-	if (ret < 0)
-		return ret;
-
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
 		return ret;
-- 
2.1.0

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

[PATCH v3 15/22] drm/i915: Read hw state into an atomic state struct

[maarten.lankhorst]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

To make this work we load the new hardware state into the
atomic_state, then swap it with the sw state.

This lets us change the force restore path in setup_hw_state()
to use a single call to intel_mode_set() to restore all the
previous state.

As a nice bonus this kills off encoder->new_encoder,
connector->new_enabled and crtc->new_enabled. They were used only
to restore the state after a modeset.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 241 insertions(+), 152 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 83078763ba14..17bf9e80c557 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 	return 0;
 }
 
-static void
+void
 intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
 				  struct intel_shared_dpll_config *shared_dpll)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e7aa8610cbdc..a42e7d7bf86b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9639,7 +9639,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
-		goto fail_unlock;
+		goto fail;
 
 	/*
 	 * Algorithm gets a little messy:
@@ -9657,10 +9657,10 @@ retry:
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
-			goto fail_unlock;
+			goto fail;
 		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 		if (ret)
-			goto fail_unlock;
+			goto fail;
 
 		old->dpms_mode = connector->dpms;
 		old->load_detect_temp = false;
@@ -9679,9 +9679,6 @@ retry:
 			continue;
 		if (possible_crtc->state->enable)
 			continue;
-		/* This can occur when applying the pipe A quirk on resume. */
-		if (to_intel_crtc(possible_crtc)->new_enabled)
-			continue;
 
 		crtc = possible_crtc;
 		break;
@@ -9692,20 +9689,17 @@ retry:
 	 */
 	if (!crtc) {
 		DRM_DEBUG_KMS("no pipe available for load-detect\n");
-		goto fail_unlock;
+		goto fail;
 	}
 
 	ret = drm_modeset_lock(&crtc->mutex, ctx);
 	if (ret)
-		goto fail_unlock;
+		goto fail;
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
-		goto fail_unlock;
-	intel_encoder->new_crtc = to_intel_crtc(crtc);
-	to_intel_connector(connector)->new_encoder = intel_encoder;
+		goto fail;
 
 	intel_crtc = to_intel_crtc(crtc);
-	intel_crtc->new_enabled = true;
 	old->dpms_mode = connector->dpms;
 	old->load_detect_temp = true;
 	old->release_fb = NULL;
@@ -9773,9 +9767,7 @@ retry:
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 	return true;
 
- fail:
-	intel_crtc->new_enabled = crtc->state->enable;
-fail_unlock:
+fail:
 	drm_atomic_state_free(state);
 	state = NULL;
 
@@ -9821,10 +9813,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (IS_ERR(crtc_state))
 			goto fail;
 
-		to_intel_connector(connector)->new_encoder = NULL;
-		intel_encoder->new_crtc = NULL;
-		intel_crtc->new_enabled = false;
-
 		connector_state->best_encoder = NULL;
 		connector_state->crtc = NULL;
 
@@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.atomic_flush = intel_finish_crtc_commit,
 };
 
-/**
- * intel_modeset_update_staged_output_state
- *
- * Updates the staged output configuration state, e.g. after we've read out the
- * current hw state.
- */
-static void intel_modeset_update_staged_output_state(struct drm_device *dev)
-{
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-
-	for_each_intel_connector(dev, connector) {
-		connector->new_encoder =
-			to_intel_encoder(connector->base.encoder);
-	}
-
-	for_each_intel_encoder(dev, encoder) {
-		encoder->new_crtc =
-			to_intel_crtc(encoder->base.crtc);
-	}
-
-	for_each_intel_crtc(dev, crtc) {
-		crtc->new_enabled = crtc->base.state->enable;
-	}
-}
-
 /* Transitional helper to copy current connector/encoder state to
  * connector->state. This is needed so that code that is partially
  * converted to atomic does the right thing.
@@ -11526,7 +11487,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	}
 
 	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-	intel_modeset_update_staged_output_state(state->dev);
 
 	/* Double check state. */
 	for_each_crtc(dev, crtc) {
@@ -11832,11 +11792,14 @@ check_connector_state(struct drm_device *dev)
 	struct intel_connector *connector;
 
 	for_each_intel_connector(dev, connector) {
+		struct drm_encoder *encoder = connector->base.encoder;
+		struct drm_connector_state *state = connector->base.state;
+
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
 		intel_connector_check_state(connector);
 
-		I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
+		I915_STATE_WARN(state->best_encoder != encoder,
 		     "connector's staged encoder doesn't match current encoder\n");
 	}
 }
@@ -11856,8 +11819,6 @@ check_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
-		     "encoder's stage crtc doesn't match current crtc\n");
 		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
 		     "encoder's active_connectors set, but no crtc\n");
 
@@ -11867,6 +11828,9 @@ check_encoder_state(struct drm_device *dev)
 			enabled = true;
 			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
 				active = true;
+
+			I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc,
+			     "encoder's stage crtc doesn't match current crtc\n");
 		}
 		/*
 		 * for MST connectors if we unplug the connector is gone
@@ -12296,11 +12260,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 	 * need to copy the staged config to the atomic state, otherwise the
 	 * mode set will just reapply the state the HW is already in. */
 	for_each_intel_encoder(dev, encoder) {
-		if (&encoder->new_crtc->base != crtc)
+		if (encoder->base.crtc != crtc)
 			continue;
 
 		for_each_intel_connector(dev, connector) {
-			if (connector->new_encoder != encoder)
+			if (connector->base.state->best_encoder != &encoder->base)
 				continue;
 
 			connector_state = drm_atomic_get_connector_state(state, &connector->base);
@@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			}
 
 			connector_state->crtc = crtc;
-			connector_state->best_encoder = &encoder->base;
 		}
 	}
 
 	for_each_intel_crtc(dev, intel_crtc) {
-		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
-			continue;
-
 		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 		if (IS_ERR(crtc_state)) {
 			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
@@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			continue;
 		}
 
-		crtc_state->base.active = crtc_state->base.enable =
-			intel_crtc->new_enabled;
-
 		if (&intel_crtc->base == crtc)
 			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
 	}
@@ -14552,99 +14509,224 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
-	if (!crtc->active)
+	if (!crtc->base.enabled)
 		return false;
 
 	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
 }
 
-static void intel_modeset_readout_hw_state(struct drm_device *dev)
+static int readout_hw_crtc_state(struct drm_atomic_state *state,
+				 struct intel_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	int i;
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_crtc_state *crtc_state;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *drm_plane_state;
+	struct intel_plane_state *plane_state;
+	int ret;
 
-	for_each_intel_crtc(dev, crtc) {
-		struct drm_plane *primary = crtc->base.primary;
-		struct intel_plane_state *plane_state;
+	crtc_state = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
 
-		memset(crtc->config, 0, sizeof(*crtc->config));
-		crtc->config->base.crtc = &crtc->base;
+	ret = drm_atomic_add_affected_planes(state, &crtc->base);
+	if (ret)
+		return ret;
 
-		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
+	memset(crtc_state, 0, sizeof(*crtc_state));
+	crtc_state->base.crtc = &crtc->base;
+	crtc_state->base.state = state;
 
-		crtc->active = dev_priv->display.get_pipe_config(crtc,
-								 crtc->config);
+	crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
-		crtc->base.state->enable = crtc->active;
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
+	crtc_state->base.enable = crtc_state->base.active =
+	crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state);
 
-		plane_state = to_intel_plane_state(primary->state);
-		plane_state->visible = primary_get_hw_state(crtc);
+	/* update transitional state */
+	crtc->active = crtc_state->base.active;
+	crtc->config = crtc_state;
 
-		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
-			      crtc->base.base.id,
-			      crtc->active ? "enabled" : "disabled");
-	}
+	drm_plane_state = drm_atomic_get_plane_state(state, primary);
+	if (IS_ERR(drm_plane_state))
+		return PTR_ERR(drm_plane_state);
+
+	plane_state = to_intel_plane_state(drm_plane_state);
+	plane_state->visible = primary_get_hw_state(crtc);
+
+	if (plane_state->visible)
+		crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
+	else
+		crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
+
+	DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
+		      crtc->base.base.id,
+		      crtc_state->base.active ? "enabled" : "disabled");
+
+	return 0;
+}
+
+static int readout_hw_pll_state(struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_shared_dpll_config *shared_dpll;
+	struct intel_crtc *crtc;
+	struct intel_crtc_state *crtc_state;
+	int i;
 
+	shared_dpll = intel_atomic_get_shared_dpll_state(state);
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
 		pll->on = pll->get_hw_state(dev_priv, pll,
-					    &pll->config.hw_state);
+					    &shared_dpll[i].hw_state);
+
 		pll->active = 0;
-		pll->config.crtc_mask = 0;
-		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
+		shared_dpll[i].crtc_mask = 0;
+
+		for_each_intel_crtc(state->dev, crtc) {
+			crtc_state = intel_atomic_get_crtc_state(state, crtc);
+			if (IS_ERR(crtc_state))
+				return PTR_ERR(crtc_state);
+
+			if (crtc_state->base.active &&
+			    crtc_state->shared_dpll == i) {
 				pll->active++;
-				pll->config.crtc_mask |= 1 << crtc->pipe;
+				shared_dpll[i].crtc_mask |=
+					1 << crtc->pipe;
 			}
 		}
 
 		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
-			      pll->name, pll->config.crtc_mask, pll->on);
+			      pll->name, shared_dpll[i].crtc_mask,
+			      pll->on);
 
-		if (pll->config.crtc_mask)
+		if (shared_dpll[i].crtc_mask)
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
-	for_each_intel_encoder(dev, encoder) {
-		pipe = 0;
+	return 0;
+}
 
-		if (encoder->get_hw_state(encoder, &pipe)) {
-			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-			encoder->base.crtc = &crtc->base;
-			encoder->get_config(encoder, crtc->config);
-		} else {
-			encoder->base.crtc = NULL;
-		}
+static struct drm_connector_state *
+get_connector_state_for_encoder(struct drm_atomic_state *state,
+				struct intel_encoder *encoder)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
 
-		encoder->connectors_active = false;
-		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
-			      encoder->base.base.id,
-			      encoder->base.name,
-			      encoder->base.crtc ? "enabled" : "disabled",
-			      pipe_name(pipe));
-	}
+	for_each_connector_in_state(state, connector, connector_state, i)
+		if (connector_state->best_encoder == &encoder->base)
+			return connector_state;
+
+	return NULL;
+}
+
+static int readout_hw_connector_encoder_state(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_crtc *crtc;
+	struct drm_crtc_state *drm_crtc_state;
+	struct intel_crtc_state *crtc_state;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct drm_connector_state *connector_state;
+	enum pipe pipe;
 
 	for_each_intel_connector(dev, connector) {
+		connector_state =
+			drm_atomic_get_connector_state(state, &connector->base);
+		if (IS_ERR(connector_state))
+			return PTR_ERR(connector_state);
+
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
-			connector->encoder->connectors_active = true;
 			connector->base.encoder = &connector->encoder->base;
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
 			connector->base.encoder = NULL;
 		}
+
+		/* We'll update the crtc field when reading encoder state */
+		connector_state->crtc = NULL;
+
+		connector_state->best_encoder = connector->base.encoder;
+
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] hw state readout: %s\n",
 			      connector->base.base.id,
 			      connector->base.name,
 			      connector->base.encoder ? "enabled" : "disabled");
 	}
+
+	for_each_intel_encoder(dev, encoder) {
+		pipe = 0;
+
+		connector_state =
+			get_connector_state_for_encoder(state, encoder);
+
+		encoder->connectors_active = !!connector_state;
+
+		if (encoder->get_hw_state(encoder, &pipe)) {
+			encoder->base.crtc =
+				dev_priv->pipe_to_crtc_mapping[pipe];
+			crtc = to_intel_crtc(encoder->base.crtc);
+
+			drm_crtc_state =
+				state->crtc_states[drm_crtc_index(&crtc->base)];
+			crtc_state = to_intel_crtc_state(drm_crtc_state);
+
+			encoder->get_config(encoder, crtc_state);
+
+			if (connector_state)
+				connector_state->crtc = &crtc->base;
+		} else {
+			encoder->base.crtc = NULL;
+		}
+
+		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
+			      encoder->base.base.id,
+			      encoder->base.name,
+			      encoder->base.crtc ? "enabled" : "disabled",
+			      pipe_name(pipe));
+	}
+
+	return 0;
+}
+
+static struct drm_atomic_state *
+intel_modeset_readout_hw_state(struct drm_device *dev)
+{
+	struct intel_crtc *crtc;
+	int ret = 0;
+
+	struct drm_atomic_state *state;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	for_each_intel_crtc(dev, crtc) {
+		ret = readout_hw_crtc_state(state, crtc);
+		if (ret)
+			goto err_free;
+	}
+
+	ret = readout_hw_pll_state(state);
+	if (ret)
+		goto err_free;
+
+	ret = readout_hw_connector_encoder_state(state);
+	if (ret)
+		goto err_free;
+
+	return state;
+
+err_free:
+	drm_atomic_state_free(state);
+	return ERR_PTR(ret);
 }
 
 /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
@@ -14653,37 +14735,57 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 				  bool force_restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct intel_encoder *encoder;
+	struct drm_atomic_state *state;
+	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
 	int i;
 
-	intel_modeset_readout_hw_state(dev);
-
-	/*
-	 * Now that we have the config, copy it to each CRTC struct
-	 * Note that this could go away if we move to using crtc_config
-	 * checking everywhere.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (crtc->active && i915.fastboot) {
-			intel_mode_from_pipe_config(&crtc->base.mode,
-						    crtc->config);
-			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
-				      crtc->base.base.id);
-			drm_mode_debug_printmodeline(&crtc->base.mode);
-		}
+	state = intel_modeset_readout_hw_state(dev);
+	if (IS_ERR(state)) {
+		DRM_ERROR("Failed to read out hw state\n");
+		return;
 	}
 
+	drm_atomic_helper_swap_state(dev, state);
+
+	/* swap sw/hw dpll state */
+	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
+	intel_shared_dpll_commit(state);
+	memcpy(to_intel_atomic_state(state)->shared_dpll,
+	       shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
+
 	/* HW state is read out, now we need to sanitize this mess. */
 	for_each_intel_encoder(dev, encoder) {
 		intel_sanitize_encoder(encoder);
 	}
 
-	for_each_pipe(dev_priv, pipe) {
-		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-		intel_sanitize_crtc(crtc);
-		intel_dump_pipe_config(crtc, crtc->config,
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		/* prevent unnneeded restores with force_restore */
+		crtc_state->active_changed =
+		crtc_state->mode_changed =
+		crtc_state->planes_changed = false;
+
+		if (crtc->enabled) {
+			intel_mode_from_pipe_config(&crtc->state->mode,
+				to_intel_crtc_state(crtc->state));
+
+			drm_mode_copy(&crtc->mode, &crtc->state->mode);
+			drm_mode_copy(&crtc->hwmode,
+				      &crtc->state->adjusted_mode);
+		}
+
+		intel_sanitize_crtc(intel_crtc);
+
+		/*
+		 * sanitize_crtc may have forced an update of crtc->state,
+		 * so reload in intel_dump_pipe_config
+		 */
+		intel_dump_pipe_config(intel_crtc,
+				       to_intel_crtc_state(crtc->state),
 				       "[setup_hw_state]");
 	}
 
@@ -14707,20 +14809,17 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		ilk_wm_get_hw_state(dev);
 
 	if (force_restore) {
-		i915_redisable_vga(dev);
+		int ret;
 
-		/*
-		 * We need to use raw interfaces for restoring state to avoid
-		 * checking (bogus) intermediate states.
-		 */
-		for_each_pipe(dev_priv, pipe) {
-			struct drm_crtc *crtc =
-				dev_priv->pipe_to_crtc_mapping[pipe];
+		i915_redisable_vga(dev);
 
-			intel_crtc_restore_mode(crtc);
+		ret = intel_set_mode(state);
+		if (ret) {
+			DRM_ERROR("Failed to restore previous mode\n");
+			drm_atomic_state_free(state);
 		}
 	} else {
-		intel_modeset_update_staged_output_state(dev);
+		drm_atomic_state_free(state);
 	}
 
 	intel_modeset_check_state(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7012c67de8d5..65f5f3d41b5a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -130,11 +130,6 @@ struct intel_fbdev {
 
 struct intel_encoder {
 	struct drm_encoder base;
-	/*
-	 * The new crtc this encoder will be driven from. Only differs from
-	 * base->crtc while a modeset is in progress.
-	 */
-	struct intel_crtc *new_crtc;
 
 	enum intel_output_type type;
 	unsigned int cloneable;
@@ -195,12 +190,6 @@ struct intel_connector {
 	 */
 	struct intel_encoder *encoder;
 
-	/*
-	 * The new encoder this connector will be driven. Only differs from
-	 * encoder while a modeset is in progress.
-	 */
-	struct intel_encoder *new_encoder;
-
 	/* Reads out the current hw, returning true if the connector is enabled
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
@@ -534,7 +523,6 @@ struct intel_crtc {
 
 	struct intel_initial_plane_config plane_config;
 	struct intel_crtc_state *config;
-	bool new_enabled;
 
 	/* reset counter value when the last flip was submitted */
 	unsigned int reset_counter;
@@ -1414,6 +1402,8 @@ struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
 void intel_atomic_state_clear(struct drm_atomic_state *);
 struct intel_shared_dpll_config *
 intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
+void intel_atomic_duplicate_dpll_state(struct drm_i915_private *,
+				       struct intel_shared_dpll_config *);
 
 static inline struct intel_crtc_state *
 intel_atomic_get_crtc_state(struct drm_atomic_state *state,
-- 
2.1.0

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

[PATCH v3 16/22] drm/i915: Implement intel_crtc_control using atomic state, v3

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Assume the callers lock everything with drm_modeset_lock_all.

This change had to be done after converting suspend/resume to
use atomic_state so the atomic state is preserved, otherwise
all transitional state is erased.

Now all callers of .crtc_enable and .crtc_disable go through
atomic modeset! :-D

Changes since v1:
- Only check for crtc_state->active in valleyview_modeset_global_pipes.
- Only check for crtc_state->active in modeset_update_crtc_power_domains.
Changes since v2:
- Rework on top of the changed patch order.
Changes since v3:
- Rename intel_crtc_toggle in description to *_control

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a42e7d7bf86b..e819f1738458 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5952,10 +5952,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum intel_display_power_domain domain;
-	unsigned long domains;
+	struct intel_crtc_state *pipe_config;
+	struct drm_plane_state *plane_state;
+	struct drm_atomic_state *state;
+	int ret;
 
 	if (enable == intel_crtc->active)
 		return;
@@ -5963,24 +5966,40 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	if (enable && !crtc->state->enable)
 		return;
 
-	crtc->state->active = enable;
-	if (enable) {
-		domains = get_crtc_power_domains(crtc);
-		for_each_power_domain(domain, domains)
-			intel_display_power_get(dev_priv, domain);
-		intel_crtc->enabled_power_domains = domains;
+	/* this function should be called with drm_modeset_lock_all for now */
+	if (WARN_ON(!ctx))
+		return;
+	lockdep_assert_held(&ctx->ww_ctx);
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
-	} else {
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return;
 
-		domains = intel_crtc->enabled_power_domains;
-		for_each_power_domain(domain, domains)
-			intel_display_power_put(dev_priv, domain);
-		intel_crtc->enabled_power_domains = 0;
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
+
+	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto err;
+	}
+	pipe_config->base.active = enable;
+
+	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
+	if (IS_ERR(plane_state)) {
+		ret = PTR_ERR(plane_state);
+		goto err;
 	}
+
+	ret = intel_set_mode(state);
+	if (!ret)
+		return;
+
+	DRM_ERROR("Failed to toggle crtc!\n");
+
+err:
+	DRM_ERROR("Updating crtc active failed with %i\n", ret);
+	drm_atomic_state_free(state);
 }
 
 /**
-- 
2.1.0

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

[PATCH v3 17/22] drm/i915: move swap state to the right place

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This is a preparation for passing crtc state to the helpers.
When converting all users of crtc->config to use the old or
new state it's easier to find regressions when swap_state is
done first.

If crtc->config is swapped at the same place as swap_state
bugs will never be found.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e819f1738458..12286b4ccd03 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11491,7 +11491,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	struct drm_connector *connector;
 
 	intel_shared_dpll_commit(state);
-	drm_atomic_helper_swap_state(state->dev, state);
 
 	for_each_intel_encoder(dev, intel_encoder) {
 		if (!intel_encoder->base.crtc)
@@ -12191,8 +12190,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	if (ret)
 		return ret;
 
+	drm_atomic_helper_swap_state(dev, state);
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc_state) || !crtc->state->active)
+		if (!needs_modeset(crtc->state) || !crtc_state->active)
 			continue;
 
 		intel_crtc_disable_planes(crtc);
-- 
2.1.0

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

[PATCH v3 18/22] drm/i915: Use crtc->hwmode for vblanks, v2.

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

intel_crtc->config will be removed eventually, so use crtc->hwmode.
drm_atomic_helper_update_legacy_modeset_state updates hwmode,
but crtc->active will eventually be gone too. Set dotclock to zero
to indicate the crtc is inactive.

Changes since v1:
- With the hwmode update in drm*update_legacy_modeset_state removed,
  intel_modeset_update_state has to assign it instead.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ca457317a8ac..9359ea2399f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-	const struct drm_display_mode *mode =
-		&intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 
 	htotal = mode->crtc_htotal;
 	hsync_start = mode->crtc_hsync_start;
@@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &crtc->base.hwmode;
 	enum pipe pipe = crtc->pipe;
 	int position, vtotal;
 
@@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 	int position;
 	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
 	bool in_vbl = true;
 	int ret = 0;
 	unsigned long irqflags;
 
-	if (!intel_crtc->active) {
+	if (WARN_ON(!mode->crtc_clock)) {
 		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
 				 "pipe %c\n", pipe_name(pipe));
 		return 0;
@@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 
-	if (!crtc->state->active) {
+	if (!crtc->hwmode.crtc_clock) {
 		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
 		return -EBUSY;
 	}
@@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
 						     vblank_time, flags,
 						     crtc,
-						     &to_intel_crtc(crtc)->config->base.adjusted_mode);
+						     &crtc->hwmode);
 }
 
 static bool intel_hpd_irq_event(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 12286b4ccd03..8acc27845fb5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11511,6 +11511,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
 
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+
+		/* Update hwmode for vblank functions */
+		if (crtc->state->active)
+			crtc->hwmode = crtc->state->adjusted_mode;
+		else
+			crtc->hwmode.crtc_clock = 0;
 	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-- 
2.1.0

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

[PATCH v3 19/22] drm/i915: Remove use of crtc->config from i915_debugfs.c

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

crtc->config is updated to always contain to the active crtc_state
and only differs from crtc_state during crtc_disable. It will
eventually be removed, so start with some low hanging fruit.

For crtc->active the situation is the same; it will be removed
eventually. Instead use crtc->state->active.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8bf21e8abd93..79a86d3e8f13 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2755,13 +2755,16 @@ static int i915_display_info(struct seq_file *m, void *unused)
 	seq_printf(m, "---------\n");
 	for_each_intel_crtc(dev, crtc) {
 		bool active;
+		struct intel_crtc_state *pipe_config;
 		int x, y;
 
+		pipe_config = to_intel_crtc_state(crtc->base.state);
+
 		seq_printf(m, "CRTC %d: pipe: %c, active=%s (size=%dx%d)\n",
 			   crtc->base.base.id, pipe_name(crtc->pipe),
-			   yesno(crtc->active), crtc->config->pipe_src_w,
-			   crtc->config->pipe_src_h);
-		if (crtc->active) {
+			   yesno(pipe_config->base.active),
+			   pipe_config->pipe_src_w, pipe_config->pipe_src_h);
+		if (pipe_config->base.active) {
 			intel_crtc_info(m, crtc);
 
 			active = cursor_position(dev, crtc->pipe, &x, &y);
@@ -3002,7 +3005,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
 
 	seq_puts(m, "\n\n");
 
-	if (intel_crtc->config->has_drrs) {
+	if (to_intel_crtc_state(intel_crtc->base.state)->has_drrs) {
 		struct intel_panel *panel;
 
 		mutex_lock(&drrs->mutex);
@@ -3054,7 +3057,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
 	for_each_intel_crtc(dev, intel_crtc) {
 		drm_modeset_lock(&intel_crtc->base.mutex, NULL);
 
-		if (intel_crtc->active) {
+		if (intel_crtc->base.state->active) {
 			active_crtc_cnt++;
 			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
 
@@ -3596,22 +3599,27 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
+	struct intel_crtc_state *pipe_config;
 
 	drm_modeset_lock_all(dev);
+	pipe_config = to_intel_crtc_state(crtc->base.state);
+
 	/*
 	 * If we use the eDP transcoder we need to make sure that we don't
 	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
 	 * relevant on hsw with pipe A when using the always-on power well
 	 * routing.
 	 */
-	if (crtc->config->cpu_transcoder == TRANSCODER_EDP &&
-	    !crtc->config->pch_pfit.enabled) {
-		bool active = crtc->active;
+	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+	    !pipe_config->pch_pfit.enabled) {
+		bool active = pipe_config->base.active;
 
-		if (active)
+		if (active) {
 			intel_crtc_control(&crtc->base, false);
+			pipe_config = to_intel_crtc_state(crtc->base.state);
+		}
 
-		crtc->config->pch_pfit.force_thru = true;
+		pipe_config->pch_pfit.force_thru = true;
 
 		intel_display_power_get(dev_priv,
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
@@ -3627,6 +3635,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
+	struct intel_crtc_state *pipe_config;
 
 	drm_modeset_lock_all(dev);
 	/*
@@ -3635,13 +3644,16 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	 * relevant on hsw with pipe A when using the always-on power well
 	 * routing.
 	 */
-	if (crtc->config->pch_pfit.force_thru) {
-		bool active = crtc->active;
+	pipe_config = to_intel_crtc_state(crtc->base.state);
+	if (pipe_config->pch_pfit.force_thru) {
+		bool active = pipe_config->base.active;
 
-		if (active)
+		if (active) {
 			intel_crtc_control(&crtc->base, false);
+			pipe_config = to_intel_crtc_state(crtc->base.state);
+		}
 
-		crtc->config->pch_pfit.force_thru = false;
+		pipe_config->pch_pfit.force_thru = false;
 
 		intel_display_power_put(dev_priv,
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
@@ -3763,7 +3775,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 				 pipe_name(pipe));
 
 		drm_modeset_lock(&crtc->base.mutex, NULL);
-		if (crtc->active)
+		if (crtc->base.state->active)
 			intel_wait_for_vblank(dev, pipe);
 		drm_modeset_unlock(&crtc->base.mutex);
 
-- 
2.1.0

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

[PATCH v3 20/22] drm/i915: Calculate haswell plane workaround, v3.

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This needs to be a global check because at the time of crtc checking
not all modesets have to be calculated yet. Use intel_crtc->atomic
because there's no reason to keep it in state.

Changes since v1:
 - Use intel_crtc->atomic as a place to put hsw_workaround_pipe.
 - Make sure quirk only applies to haswell.
 - Use first loop to iterate over newly enabled crtc's only.
   This increases readability.
Changes since v2:
 - Move hsw_workaround_pipe back to crtc_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 107 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 2 files changed, 79 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8acc27845fb5..2c7581c6803d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4870,42 +4870,15 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
 	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
 }
 
-/*
- * This implements the workaround described in the "notes" section of the mode
- * set sequence documentation. When going from no pipes or single pipe to
- * multiple pipes, and planes are enabled after the pipe, we need to wait at
- * least 2 vblanks on the first pipe before enabling planes on the second pipe.
- */
-static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
-
-	/* We want to get the other_active_crtc only if there's only 1 other
-	 * active crtc. */
-	for_each_intel_crtc(dev, crtc_it) {
-		if (!crtc_it->active || crtc_it == crtc)
-			continue;
-
-		if (other_active_crtc)
-			return;
-
-		other_active_crtc = crtc_it;
-	}
-	if (!other_active_crtc)
-		return;
-
-	intel_wait_for_vblank(dev, other_active_crtc->pipe);
-	intel_wait_for_vblank(dev, other_active_crtc->pipe);
-}
-
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
-	int pipe = intel_crtc->pipe;
+	int pipe = intel_crtc->pipe, hsw_workaround_pipe;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->state);
 
 	if (WARN_ON(intel_crtc->active))
 		return;
@@ -4982,7 +4955,11 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
-	haswell_mode_set_planes_workaround(intel_crtc);
+	hsw_workaround_pipe = pipe_config->hsw_workaround_pipe;
+	if (IS_HASWELL(dev) && hsw_workaround_pipe != INVALID_PIPE) {
+		intel_wait_for_vblank(dev, hsw_workaround_pipe);
+		intel_wait_for_vblank(dev, hsw_workaround_pipe);
+	}
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -12122,6 +12099,71 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 	return ret;
 }
 
+/*
+ * This implements the workaround described in the "notes" section of the mode
+ * set sequence documentation. When going from no pipes or single pipe to
+ * multiple pipes, and planes are enabled after the pipe, we need to wait at
+ * least 2 vblanks on the first pipe before enabling planes on the second pipe.
+ */
+static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct intel_crtc *intel_crtc;
+	struct drm_crtc *crtc;
+	struct intel_crtc_state *first_crtc_state = NULL;
+	struct intel_crtc_state *other_crtc_state = NULL;
+	enum pipe first_pipe = INVALID_PIPE, enabled_pipe = INVALID_PIPE;
+	int i;
+
+	/* look at all crtc's that are going to be enabled in during modeset */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!crtc_state->active || !needs_modeset(crtc_state))
+			continue;
+
+		if (first_crtc_state) {
+			other_crtc_state = to_intel_crtc_state(crtc_state);
+			break;
+		} else {
+			first_crtc_state = to_intel_crtc_state(crtc_state);
+			first_pipe = intel_crtc->pipe;
+		}
+	}
+
+	/* No workaround needed? */
+	if (!first_crtc_state)
+		return 0;
+
+	/* w/a possibly needed, check how many crtc's are already enabled. */
+	for_each_intel_crtc(state->dev, intel_crtc) {
+		struct intel_crtc_state *pipe_config;
+
+		pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(pipe_config))
+			return PTR_ERR(pipe_config);
+
+		pipe_config->hsw_workaround_pipe = INVALID_PIPE;
+
+		if (!pipe_config->base.active ||
+		    needs_modeset(&pipe_config->base))
+			continue;
+
+		/* 2 or more enabled crtcs means no need for w/a */
+		if (enabled_pipe != INVALID_PIPE)
+			return 0;
+
+		enabled_pipe = intel_crtc->pipe;
+	}
+
+	if (enabled_pipe != INVALID_PIPE)
+		first_crtc_state->hsw_workaround_pipe = enabled_pipe;
+	else if (other_crtc_state)
+		other_crtc_state->hsw_workaround_pipe = first_pipe;
+
+	return 0;
+}
+
 /* Code that should eventually be part of atomic_check() */
 static int __intel_set_mode_checks(struct drm_atomic_state *state)
 {
@@ -12145,6 +12187,9 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	if (ret)
 		return ret;
 
+	if (IS_HASWELL(dev))
+		haswell_mode_set_planes_workaround(state);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 65f5f3d41b5a..bce52c5f4fe4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -443,6 +443,9 @@ struct intel_crtc_state {
 	int pbn;
 
 	struct intel_crtc_scaler_state scaler_state;
+
+	/* w/a for waiting 2 vblanks during crtc enable */
+	enum pipe hsw_workaround_pipe;
 };
 
 struct intel_pipe_wm {
-- 
2.1.0

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

[PATCH v3 21/22] drm/i915: Use atomic state for calculating DVO_2X_MODE on i830.

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This is a small behavioral change because it leaves DVO_2X_MODE
set between crtc_disable and crtc_enable. This is probably harmless
though and if not should be fixed by calculating 2x mode before
enable/disable pll.

This is needed because intel_crtc->active will be removed eventually.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2c7581c6803d..008b011308de 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1698,7 +1698,7 @@ static int intel_num_dvo_pipes(struct drm_device *dev)
 	int count = 0;
 
 	for_each_intel_crtc(dev, crtc)
-		count += crtc->active &&
+		count += crtc->base.state->active &&
 			intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO);
 
 	return count;
@@ -1779,7 +1779,7 @@ static void i9xx_disable_pll(struct intel_crtc *crtc)
 	/* Disable DVO 2x clock on both PLLs if necessary */
 	if (IS_I830(dev) &&
 	    intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO) &&
-	    intel_num_dvo_pipes(dev) == 1) {
+	    !intel_num_dvo_pipes(dev)) {
 		I915_WRITE(DPLL(PIPE_B),
 			   I915_READ(DPLL(PIPE_B)) & ~DPLL_DVO_2X_MODE);
 		I915_WRITE(DPLL(PIPE_A),
-- 
2.1.0

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

[PATCH v3 22/22] drm/i915: use calculated state for vblank evasion

[maarten.lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

crtc->active will be gone eventually, and this check should be just as good.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 008b011308de..d050a59ce9f9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12953,6 +12953,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
 	struct intel_plane *intel_plane;
 	struct drm_plane *p;
 	unsigned fb_bits = 0;
@@ -12996,7 +12997,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	intel_runtime_pm_get(dev_priv);
 
 	/* Perform vblank evasion around commit operation */
-	if (intel_crtc->active)
+	if (crtc_state->active && !needs_modeset(crtc_state))
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
-- 
2.1.0

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

[PATCH v3.5 02.5/22] drm/i915: add intel_display_suspend

[Maarten Lankhorst]

This is a function used to disable all crtc's. This makes it clearer
to distinguish between when mode needs to be preserved and when
it can be trashed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Oops, I was trashing all state during suspend and on gpu reset.
I will send an amended intel_crtc_control patch too with the
suspend and prepare_reset parts taken out.

 drivers/gpu/drm/i915/i915_drv.c      |  4 +---
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5cc57f2ec192..d1a090a9f653 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
 static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
 	int error;
 
@@ -631,8 +630,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	 * for _thaw. Also, power gate the CRTC power wells.
 	 */
 	drm_modeset_lock_all(dev);
-	for_each_crtc(dev, crtc)
-		intel_crtc_control(crtc, false);
+	intel_display_suspend(dev);
 	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cae36ec1c2ef..8d50c4ca561f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3204,9 +3204,6 @@ void intel_crtc_reset(struct intel_crtc *crtc)
 
 void intel_prepare_reset(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *crtc;
-
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev))
 		return;
@@ -3221,13 +3218,7 @@ void intel_prepare_reset(struct drm_device *dev)
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
-	}
+	intel_display_suspend(dev);
 }
 
 void intel_finish_reset(struct drm_device *dev)
@@ -6018,6 +6009,24 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+/*
+ * turn all crtc's off, but do not adjust state
+ * This has to be paired with a call to intel_modeset_setup_hw_state.
+ */
+void intel_display_suspend(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc *crtc;
+
+	for_each_crtc(dev, crtc) {
+		if (!to_intel_crtc(crtc)->active)
+			continue;
+
+		intel_crtc_disable_planes(crtc);
+		dev_priv->display.crtc_disable(crtc);
+	}
+}
+
 /* Master function to enable/disable CRTC and corresponding power wells */
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 70d0cc8fe70e..c24d670529e3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -992,6 +992,7 @@ int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
+void intel_display_suspend(struct drm_device *dev);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_reset(struct intel_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
-- 
2.1.0


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

Re: [PATCH v3.5 03/22] drm/i915: use intel_crtc_control everywhere, v3.

[Maarten Lankhorst]

Having a single path for everything makes it a lot easier to keep
crtc_state->active in sync with intel_crtc->active.

A crtc cannot be changed to active when not enabled, because it means
no mode is set and no connectors are connected.

This should also make intel_crtc->active match crtc_state->active.

Changes since v1:
- Reworded commit message, there's no intel_crtc_toggle.
Changes since v2:
- Change some callers of intel_crtc_control to intel_display_suspend.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 18 ++++++++++--
 drivers/gpu/drm/i915/intel_display.c | 54 +++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 3 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a32b669bab89..8bf21e8abd93 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3606,12 +3606,18 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	 */
 	if (crtc->config->cpu_transcoder == TRANSCODER_EDP &&
 	    !crtc->config->pch_pfit.enabled) {
+		bool active = crtc->active;
+
+		if (active)
+			intel_crtc_control(&crtc->base, false);
+
 		crtc->config->pch_pfit.force_thru = true;
 
 		intel_display_power_get(dev_priv,
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
 
-		intel_crtc_reset(crtc);
+		if (active)
+			intel_crtc_control(&crtc->base, true);
 	}
 	drm_modeset_unlock_all(dev);
 }
@@ -3630,12 +3636,18 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 	 * routing.
 	 */
 	if (crtc->config->pch_pfit.force_thru) {
-		crtc->config->pch_pfit.force_thru = false;
+		bool active = crtc->active;
 
-		intel_crtc_reset(crtc);
+		if (active)
+			intel_crtc_control(&crtc->base, false);
+
+		crtc->config->pch_pfit.force_thru = false;
 
 		intel_display_power_put(dev_priv,
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+
+		if (active)
+			intel_crtc_control(&crtc->base, true);
 	}
 	drm_modeset_unlock_all(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d50c4ca561f..7d386561531f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3189,19 +3189,6 @@ static void intel_update_primary_planes(struct drm_device *dev)
 	}
 }
 
-void intel_crtc_reset(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-
-	if (!crtc->active)
-		return;
-
-	intel_crtc_disable_planes(&crtc->base);
-	dev_priv->display.crtc_disable(&crtc->base);
-	dev_priv->display.crtc_enable(&crtc->base);
-	intel_crtc_enable_planes(&crtc->base);
-}
-
 void intel_prepare_reset(struct drm_device *dev)
 {
 	/* no reset support for gen2 */
@@ -3213,7 +3200,6 @@ void intel_prepare_reset(struct drm_device *dev)
 		return;
 
 	drm_modeset_lock_all(dev);
-
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
@@ -6036,26 +6022,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	enum intel_display_power_domain domain;
 	unsigned long domains;
 
+	if (enable == intel_crtc->active)
+		return;
+
+	if (enable && !crtc->state->enable)
+		return;
+
+	crtc->state->active = enable;
 	if (enable) {
-		if (!intel_crtc->active) {
-			domains = get_crtc_power_domains(crtc);
-			for_each_power_domain(domain, domains)
-				intel_display_power_get(dev_priv, domain);
-			intel_crtc->enabled_power_domains = domains;
+		domains = get_crtc_power_domains(crtc);
+		for_each_power_domain(domain, domains)
+			intel_display_power_get(dev_priv, domain);
+		intel_crtc->enabled_power_domains = domains;
 
-			dev_priv->display.crtc_enable(crtc);
-			intel_crtc_enable_planes(crtc);
-		}
+		dev_priv->display.crtc_enable(crtc);
+		intel_crtc_enable_planes(crtc);
 	} else {
-		if (intel_crtc->active) {
-			intel_crtc_disable_planes(crtc);
-			dev_priv->display.crtc_disable(crtc);
+		intel_crtc_disable_planes(crtc);
+		dev_priv->display.crtc_disable(crtc);
 
-			domains = intel_crtc->enabled_power_domains;
-			for_each_power_domain(domain, domains)
-				intel_display_power_put(dev_priv, domain);
-			intel_crtc->enabled_power_domains = 0;
-		}
+		domains = intel_crtc->enabled_power_domains;
+		for_each_power_domain(domain, domains)
+			intel_display_power_put(dev_priv, domain);
+		intel_crtc->enabled_power_domains = 0;
 	}
 }
 
@@ -6072,8 +6061,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 		enable |= intel_encoder->connectors_active;
 
 	intel_crtc_control(crtc, enable);
-
-	crtc->state->active = enable;
 }
 
 void intel_encoder_destroy(struct drm_encoder *encoder)
@@ -14565,8 +14552,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		plane = crtc->plane;
 		to_intel_plane_state(crtc->base.primary->state)->visible = true;
 		crtc->plane = !plane;
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
+		intel_crtc_control(&crtc->base, false);
 		crtc->plane = plane;
 
 		/* ... and break all links. */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c24d670529e3..14685abf1b3d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -994,7 +994,6 @@ void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_display_suspend(struct drm_device *dev);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
-void intel_crtc_reset(struct intel_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
-- 
2.1.0

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

[PATCH v3.5 16.5/22] drm/i915: Make intel_display_suspend atomic.

[Maarten Lankhorst]

Calculate all state using a normal transition, but afterwards fudge
crtc->state->active back to its old value. This should still allow
state restore in setup_hw_state to work properly.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8b1d91136d25..2dc59147d890 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5951,16 +5951,55 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
  */
 void intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+	struct drm_atomic_state *state;
 	struct drm_crtc *crtc;
+	unsigned crtc_mask = 0;
+	int ret = 0;
+
+	if (WARN_ON(!ctx))
+		return;
+
+	lockdep_assert_held(&ctx->ww_ctx);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return;
+
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
 
 	for_each_crtc(dev, crtc) {
-		if (!to_intel_crtc(crtc)->active)
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_crtc_state(state, crtc);
+
+		ret = PTR_ERR_OR_ZERO(crtc_state);
+		if (ret)
+			goto free;
+
+		if (!crtc_state->active)
 			continue;
 
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+		crtc_state->active = false;
+		crtc_mask |= 1 << drm_crtc_index(crtc);
 	}
+
+	if (crtc_mask) {
+		ret = intel_set_mode(state);
+
+		if (!ret) {
+			for_each_crtc(dev, crtc)
+				if (crtc_mask & (1 << drm_crtc_index(crtc)))
+					crtc->state->active = true;
+
+			return;
+		}
+	}
+
+free:
+	if (ret)
+		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+	drm_atomic_state_free(state);
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
-- 
2.1.0


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

Re: [PATCH v3.5 02.5/22] drm/i915: add intel_display_suspend

[Matt Roper]

On Thu, May 21, 2015 at 02:33:31PM +0200, Maarten Lankhorst wrote:
> This is a function used to disable all crtc's. This makes it clearer
> to distinguish between when mode needs to be preserved and when
> it can be trashed.

To clarify, when you talk about mode being preserved or trashed here,
you're talking about the hardware's idea of the mode, not the driver's
software state, right?  I.e., because when we shut down a power well the
registers vanish and whatever was programmed in them is lost?

See my comments farther down.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Oops, I was trashing all state during suspend and on gpu reset.
> I will send an amended intel_crtc_control patch too with the
> suspend and prepare_reset parts taken out.
> 
>  drivers/gpu/drm/i915/i915_drv.c      |  4 +---
>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5cc57f2ec192..d1a090a9f653 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
>  static int i915_drm_suspend(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_crtc *crtc;
>  	pci_power_t opregion_target_state;
>  	int error;
>  
> @@ -631,8 +630,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	 * for _thaw. Also, power gate the CRTC power wells.
>  	 */
>  	drm_modeset_lock_all(dev);
> -	for_each_crtc(dev, crtc)
> -		intel_crtc_control(crtc, false);
> +	intel_display_suspend(dev);

I'm not terribly familiar with the power well details, but it looks like
part of the motivation of commit

        commit b04c5bd6fda54703e56f29569e4bca489d6c5a5c
        Author: Borun Fu <borun.fu@intel.com>
        Date:   Sat Jul 12 10:02:27 2014 +0530

            drm/i915: Power gating display wells during i915_pm_suspend

which added intel_crtc_control() was to ensure the power wells were
gated at this point; by replacing the intel_crtc_control() with
intel_display_suspend() here, you're removing that power well
programming...is that intentional (and is it going to cause the display
to stay in D0 state)?

If it is intentional, the comment above this block is out of date now.
Since this patch (and the following one) seem to change the semantics of
when we're touching power wells at various points in the code, maybe you
can elaborate a little bit on that in the commit message of one or both
commits.


Matt

>  	drm_modeset_unlock_all(dev);
>  
>  	intel_dp_mst_suspend(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cae36ec1c2ef..8d50c4ca561f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3204,9 +3204,6 @@ void intel_crtc_reset(struct intel_crtc *crtc)
>  
>  void intel_prepare_reset(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc *crtc;
> -
>  	/* no reset support for gen2 */
>  	if (IS_GEN2(dev))
>  		return;
> @@ -3221,13 +3218,7 @@ void intel_prepare_reset(struct drm_device *dev)
>  	 * Disabling the crtcs gracefully seems nicer. Also the
>  	 * g33 docs say we should at least disable all the planes.
>  	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> -
> -		intel_crtc_disable_planes(&crtc->base);
> -		dev_priv->display.crtc_disable(&crtc->base);
> -	}
> +	intel_display_suspend(dev);
>  }
>  
>  void intel_finish_reset(struct drm_device *dev)
> @@ -6018,6 +6009,24 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> +/*
> + * turn all crtc's off, but do not adjust state
> + * This has to be paired with a call to intel_modeset_setup_hw_state.
> + */
> +void intel_display_suspend(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_crtc *crtc;
> +
> +	for_each_crtc(dev, crtc) {
> +		if (!to_intel_crtc(crtc)->active)
> +			continue;
> +
> +		intel_crtc_disable_planes(crtc);
> +		dev_priv->display.crtc_disable(crtc);
> +	}
> +}
> +
>  /* Master function to enable/disable CRTC and corresponding power wells */
>  void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 70d0cc8fe70e..c24d670529e3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -992,6 +992,7 @@ int intel_pch_rawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
> +void intel_display_suspend(struct drm_device *dev);
>  void intel_crtc_control(struct drm_crtc *crtc, bool enable);
>  void intel_crtc_reset(struct intel_crtc *crtc);
>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.5 02.5/22] drm/i915: add intel_display_suspend

[Maarten Lankhorst]

Op 23-05-15 om 01:03 schreef Matt Roper:
> On Thu, May 21, 2015 at 02:33:31PM +0200, Maarten Lankhorst wrote:
>> This is a function used to disable all crtc's. This makes it clearer
>> to distinguish between when mode needs to be preserved and when
>> it can be trashed.
> To clarify, when you talk about mode being preserved or trashed here,
> you're talking about the hardware's idea of the mode, not the driver's
> software state, right?  I.e., because when we shut down a power well the
> registers vanish and whatever was programmed in them is lost?
>
> See my comments farther down.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> Oops, I was trashing all state during suspend and on gpu reset.
>> I will send an amended intel_crtc_control patch too with the
>> suspend and prepare_reset parts taken out.
>>
>>  drivers/gpu/drm/i915/i915_drv.c      |  4 +---
>>  drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  3 files changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 5cc57f2ec192..d1a090a9f653 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
>>  static int i915_drm_suspend(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct drm_crtc *crtc;
>>  	pci_power_t opregion_target_state;
>>  	int error;
>>  
>> @@ -631,8 +630,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>  	 * for _thaw. Also, power gate the CRTC power wells.
>>  	 */
>>  	drm_modeset_lock_all(dev);
>> -	for_each_crtc(dev, crtc)
>> -		intel_crtc_control(crtc, false);
>> +	intel_display_suspend(dev);
> I'm not terribly familiar with the power well details, but it looks like
> part of the motivation of commit
>
>         commit b04c5bd6fda54703e56f29569e4bca489d6c5a5c
>         Author: Borun Fu <borun.fu@intel.com>
>         Date:   Sat Jul 12 10:02:27 2014 +0530
>
>             drm/i915: Power gating display wells during i915_pm_suspend
>
> which added intel_crtc_control() was to ensure the power wells were
> gated at this point; by replacing the intel_crtc_control() with
> intel_display_suspend() here, you're removing that power well
> programming...is that intentional (and is it going to cause the display
> to stay in D0 state)?
>
> If it is intentional, the comment above this block is out of date now.
> Since this patch (and the following one) seem to change the semantics of
> when we're touching power wells at various points in the code, maybe you
> can elaborate a little bit on that in the commit message of one or both
> commits.
>
You're right, I'm not touching power wells here. For the hang case that doesn't matter but for pm_suspend it probably does.

The followup patch that converts intel_display_suspend to atomic modeset should restore the old behavior.
I'll respin with some changes that I'll undo when converting intel_display_suspend to atomic modeset.

One thing that also seems to unintentionally change behavior is intel_display_set_init_power being unset by modeset_update_crtc_power_domains.
I'll fix that in the followup patch that converts this function to atomic.

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

[PATCH v3.6 02.5/22] drm/i915: add intel_display_suspend, v2.

[Maarten Lankhorst]

This is a function used to disable all crtc's. This makes it clearer
to distinguish between when mode needs to be preserved and when
it can be trashed.

Changes since v1:
- Copy power changes from intel_crtc_control.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5cc57f2ec192..d1a090a9f653 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
 static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
 	int error;
 
@@ -631,8 +630,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	 * for _thaw. Also, power gate the CRTC power wells.
 	 */
 	drm_modeset_lock_all(dev);
-	for_each_crtc(dev, crtc)
-		intel_crtc_control(crtc, false);
+	intel_display_suspend(dev);
 	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cae36ec1c2ef..68e008c15f43 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3204,9 +3204,6 @@ void intel_crtc_reset(struct intel_crtc *crtc)
 
 void intel_prepare_reset(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *crtc;
-
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev))
 		return;
@@ -3221,13 +3218,7 @@ void intel_prepare_reset(struct drm_device *dev)
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
-	}
+	intel_display_suspend(dev);
 }
 
 void intel_finish_reset(struct drm_device *dev)
@@ -6018,6 +6009,33 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+/*
+ * turn all crtc's off, but do not adjust state
+ * This has to be paired with a call to intel_modeset_setup_hw_state.
+ */
+void intel_display_suspend(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc *crtc;
+
+	for_each_crtc(dev, crtc) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		enum intel_display_power_domain domain;
+		unsigned long domains;
+
+		if (!intel_crtc->active)
+			continue;
+
+		intel_crtc_disable_planes(crtc);
+		dev_priv->display.crtc_disable(crtc);
+
+		domains = intel_crtc->enabled_power_domains;
+		for_each_power_domain(domain, domains)
+			intel_display_power_put(dev_priv, domain);
+		intel_crtc->enabled_power_domains = 0;
+	}
+}
+
 /* Master function to enable/disable CRTC and corresponding power wells */
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 70d0cc8fe70e..c24d670529e3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -992,6 +992,7 @@ int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
+void intel_display_suspend(struct drm_device *dev);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_reset(struct intel_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
-- 
2.1.0


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

[PATCH v3.6 16.5/22] drm/i915: Make intel_display_suspend atomic, v2.

[Maarten Lankhorst]

Calculate all state using a normal transition, but afterwards fudge
crtc->state->active back to its old value. This should still allow
state restore in setup_hw_state to work properly.

Calling intel_set_mode will cause intel_display_set_init_power to be
called, make sure init_power gets set again afterwards.

Changes since v1:
- Fix to compile with v2 of the patch that adds intel_display_suspend.
- Add intel_display_set_init_power.
- Set return value to int to allow error checking.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  3 ++
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d1a090a9f653..69fc3c7057df 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -633,6 +633,9 @@ static int i915_drm_suspend(struct drm_device *dev)
 	intel_display_suspend(dev);
 	drm_modeset_unlock_all(dev);
 
+	/* suspending displays will unsets init power */
+	intel_display_set_init_power(dev_priv, true);
+
 	intel_dp_mst_suspend(dev);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 32bab28432f7..392d6d68cb14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5949,27 +5949,58 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
  * turn all crtc's off, but do not adjust state
  * This has to be paired with a call to intel_modeset_setup_hw_state.
  */
-void intel_display_suspend(struct drm_device *dev)
+int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+	struct drm_atomic_state *state;
 	struct drm_crtc *crtc;
+	unsigned crtc_mask = 0;
+	int ret = 0;
+
+	if (WARN_ON(!ctx))
+		return 0;
+
+	lockdep_assert_held(&ctx->ww_ctx);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return -ENOMEM;
+
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
 
 	for_each_crtc(dev, crtc) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-		enum intel_display_power_domain domain;
-		unsigned long domains;
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_crtc_state(state, crtc);
 
-		if (!intel_crtc->active)
+		ret = PTR_ERR_OR_ZERO(crtc_state);
+		if (ret)
+			goto free;
+
+		if (!crtc_state->active)
 			continue;
 
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+		crtc_state->active = false;
+		crtc_mask |= 1 << drm_crtc_index(crtc);
+	}
 
-		domains = intel_crtc->enabled_power_domains;
-		for_each_power_domain(domain, domains)
-			intel_display_power_put(dev_priv, domain);
-		intel_crtc->enabled_power_domains = 0;
+	if (crtc_mask) {
+		ret = intel_set_mode(state);
+
+		if (!ret) {
+			for_each_crtc(dev, crtc)
+				if (crtc_mask & (1 << drm_crtc_index(crtc)))
+					crtc->state->active = true;
+
+			return ret;
+		}
 	}
+
+free:
+	if (ret)
+		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+	drm_atomic_state_free(state);
+	return ret;
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c113f187090f..665e249ae8bf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -988,7 +988,7 @@ int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
-void intel_display_suspend(struct drm_device *dev);
+int intel_display_suspend(struct drm_device *dev);
 int intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
-- 
2.1.0


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

[PATCH v3.6 16/22] drm/i915: Implement intel_crtc_control using atomic state, v4.

[Maarten Lankhorst]

Assume the callers lock everything with drm_modeset_lock_all.

This change had to be done after converting suspend/resume to
use atomic_state so the atomic state is preserved, otherwise
all transitional state is erased.

Now all callers of .crtc_enable and .crtc_disable go through
atomic modeset! :-D

Changes since v1:
- Only check for crtc_state->active in valleyview_modeset_global_pipes.
- Only check for crtc_state->active in modeset_update_crtc_power_domains.
Changes since v2:
- Rework on top of the changed patch order.
Changes since v3:
- Rename intel_crtc_toggle in description to *_control
- Change return value to int.
- Do not add plane state, should be done implicitly already.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9680eb85bd0..32bab28432f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev)
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
-void intel_crtc_control(struct drm_crtc *crtc, bool enable)
+int intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum intel_display_power_domain domain;
-	unsigned long domains;
+	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret;
 
 	if (enable == intel_crtc->active)
-		return;
+		return 0;
 
 	if (enable && !crtc->state->enable)
-		return;
+		return 0;
 
-	crtc->state->active = enable;
-	if (enable) {
-		domains = get_crtc_power_domains(crtc);
-		for_each_power_domain(domain, domains)
-			intel_display_power_get(dev_priv, domain);
-		intel_crtc->enabled_power_domains = domains;
+	/* this function should be called with drm_modeset_lock_all for now */
+	if (WARN_ON(!ctx))
+		return -EIO;
+	lockdep_assert_held(&ctx->ww_ctx);
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
-	} else {
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return -ENOMEM;
 
-		domains = intel_crtc->enabled_power_domains;
-		for_each_power_domain(domain, domains)
-			intel_display_power_put(dev_priv, domain);
-		intel_crtc->enabled_power_domains = 0;
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
+
+	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto err;
 	}
+	pipe_config->base.active = enable;
+
+	ret = intel_set_mode(state);
+	if (!ret)
+		return ret;
+
+err:
+	DRM_ERROR("Updating crtc active failed with %i\n", ret);
+	drm_atomic_state_free(state);
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 60a29ff65f1f..c113f187090f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_display_suspend(struct drm_device *dev);
-void intel_crtc_control(struct drm_crtc *crtc, bool enable);
+int intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
-- 
2.1.0


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

[PATCH v3.6 20/22] drm/i915: Calculate haswell plane workaround, v4.

[Maarten Lankhorst]

This needs to be a global check because at the time of crtc checking
not all modesets have to be calculated yet. Use intel_crtc->atomic
because there's no reason to keep it in state.

Changes since v1:
 - Use intel_crtc->atomic as a place to put hsw_workaround_pipe.
 - Make sure quirk only applies to haswell.
 - Use first loop to iterate over newly enabled crtc's only.
   This increases readability.
Changes since v2:
 - Move hsw_workaround_pipe back to crtc_state.
Changes since v3:
 - Return errors from haswell_mode_set_planes_workaround.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 2 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index de13c1c14c93..f6733a777590 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4867,42 +4867,15 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
 	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
 }
 
-/*
- * This implements the workaround described in the "notes" section of the mode
- * set sequence documentation. When going from no pipes or single pipe to
- * multiple pipes, and planes are enabled after the pipe, we need to wait at
- * least 2 vblanks on the first pipe before enabling planes on the second pipe.
- */
-static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
-
-	/* We want to get the other_active_crtc only if there's only 1 other
-	 * active crtc. */
-	for_each_intel_crtc(dev, crtc_it) {
-		if (!crtc_it->active || crtc_it == crtc)
-			continue;
-
-		if (other_active_crtc)
-			return;
-
-		other_active_crtc = crtc_it;
-	}
-	if (!other_active_crtc)
-		return;
-
-	intel_wait_for_vblank(dev, other_active_crtc->pipe);
-	intel_wait_for_vblank(dev, other_active_crtc->pipe);
-}
-
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
-	int pipe = intel_crtc->pipe;
+	int pipe = intel_crtc->pipe, hsw_workaround_pipe;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->state);
 
 	if (WARN_ON(intel_crtc->active))
 		return;
@@ -4979,7 +4952,11 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
-	haswell_mode_set_planes_workaround(intel_crtc);
+	hsw_workaround_pipe = pipe_config->hsw_workaround_pipe;
+	if (IS_HASWELL(dev) && hsw_workaround_pipe != INVALID_PIPE) {
+		intel_wait_for_vblank(dev, hsw_workaround_pipe);
+		intel_wait_for_vblank(dev, hsw_workaround_pipe);
+	}
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -12169,6 +12146,71 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 	return ret;
 }
 
+/*
+ * This implements the workaround described in the "notes" section of the mode
+ * set sequence documentation. When going from no pipes or single pipe to
+ * multiple pipes, and planes are enabled after the pipe, we need to wait at
+ * least 2 vblanks on the first pipe before enabling planes on the second pipe.
+ */
+static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct intel_crtc *intel_crtc;
+	struct drm_crtc *crtc;
+	struct intel_crtc_state *first_crtc_state = NULL;
+	struct intel_crtc_state *other_crtc_state = NULL;
+	enum pipe first_pipe = INVALID_PIPE, enabled_pipe = INVALID_PIPE;
+	int i;
+
+	/* look at all crtc's that are going to be enabled in during modeset */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!crtc_state->active || !needs_modeset(crtc_state))
+			continue;
+
+		if (first_crtc_state) {
+			other_crtc_state = to_intel_crtc_state(crtc_state);
+			break;
+		} else {
+			first_crtc_state = to_intel_crtc_state(crtc_state);
+			first_pipe = intel_crtc->pipe;
+		}
+	}
+
+	/* No workaround needed? */
+	if (!first_crtc_state)
+		return 0;
+
+	/* w/a possibly needed, check how many crtc's are already enabled. */
+	for_each_intel_crtc(state->dev, intel_crtc) {
+		struct intel_crtc_state *pipe_config;
+
+		pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(pipe_config))
+			return PTR_ERR(pipe_config);
+
+		pipe_config->hsw_workaround_pipe = INVALID_PIPE;
+
+		if (!pipe_config->base.active ||
+		    needs_modeset(&pipe_config->base))
+			continue;
+
+		/* 2 or more enabled crtcs means no need for w/a */
+		if (enabled_pipe != INVALID_PIPE)
+			return 0;
+
+		enabled_pipe = intel_crtc->pipe;
+	}
+
+	if (enabled_pipe != INVALID_PIPE)
+		first_crtc_state->hsw_workaround_pipe = enabled_pipe;
+	else if (other_crtc_state)
+		other_crtc_state->hsw_workaround_pipe = first_pipe;
+
+	return 0;
+}
+
 /* Code that should eventually be part of atomic_check() */
 static int __intel_set_mode_checks(struct drm_atomic_state *state)
 {
@@ -12192,7 +12234,10 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	if (ret)
 		return ret;
 
-	return 0;
+	if (IS_HASWELL(dev))
+		ret = haswell_mode_set_planes_workaround(state);
+
+	return ret;
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 665e249ae8bf..d5f38e3d732b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -443,6 +443,9 @@ struct intel_crtc_state {
 	int pbn;
 
 	struct intel_crtc_scaler_state scaler_state;
+
+	/* w/a for waiting 2 vblanks during crtc enable */
+	enum pipe hsw_workaround_pipe;
 };
 
 struct intel_pipe_wm {
-- 
2.1.0


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

Re: [PATCH v3 15/22] drm/i915: Read hw state into an atomic state struct

[Maarten Lankhorst]

Hey,

Op 20-05-15 om 18:04 schreef maarten.lankhorst@linux.intel.com:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> To make this work we load the new hardware state into the
> atomic_state, then swap it with the sw state.
>
> This lets us change the force restore path in setup_hw_state()
> to use a single call to intel_mode_set() to restore all the
> previous state.
>
> As a nice bonus this kills off encoder->new_encoder,
> connector->new_enabled and crtc->new_enabled. They were used only
> to restore the state after a modeset.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |  14 +-
>  3 files changed, 241 insertions(+), 152 deletions(-)
>
Small fix up. This patch may force the modeset code to no longer do a modeset on boot, causing a black screen
on Daniel Stone's machine. Fixup below disables all crtc's for the !i915.fastboot case.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 13b09efeb2a0..b70e3890e2bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14138,7 +14138,7 @@ void intel_modeset_init(struct drm_device *dev)
 	intel_fbc_disable(dev);
 
 	drm_modeset_lock_all(dev);
-	intel_modeset_setup_hw_state(dev, false);
+	intel_modeset_setup_hw_state(dev, !i915.fastboot);
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {

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

Re: [PATCH v3 15/22] drm/i915: Read hw state into an atomic state struct

[Daniel Vetter]

On Wed, May 27, 2015 at 07:30:01AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 20-05-15 om 18:04 schreef maarten.lankhorst@linux.intel.com:
> > From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >
> > To make this work we load the new hardware state into the
> > atomic_state, then swap it with the sw state.
> >
> > This lets us change the force restore path in setup_hw_state()
> > to use a single call to intel_mode_set() to restore all the
> > previous state.
> >
> > As a nice bonus this kills off encoder->new_encoder,
> > connector->new_enabled and crtc->new_enabled. They were used only
> > to restore the state after a modeset.
> >
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  14 +-
> >  3 files changed, 241 insertions(+), 152 deletions(-)
> >
> Small fix up. This patch may force the modeset code to no longer do a modeset on boot, causing a black screen
> on Daniel Stone's machine. Fixup below disables all crtc's for the !i915.fastboot case.

Why does this happen? It could be that we're throwing a state difference
under the rug, which wouldn't be great.
-Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 13b09efeb2a0..b70e3890e2bf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14138,7 +14138,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	intel_fbc_disable(dev);
>  
>  	drm_modeset_lock_all(dev);
> -	intel_modeset_setup_hw_state(dev, false);
> +	intel_modeset_setup_hw_state(dev, !i915.fastboot);
>  	drm_modeset_unlock_all(dev);
>  
>  	for_each_intel_crtc(dev, crtc) {
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 08/22] drm/i915: Zap call to drm_plane_helper_disable.

[Matt Roper]

On Wed, May 20, 2015 at 03:38:13PM +0200, Maarten Lankhorst wrote:
> The primary plane can still be configured when crtc is off,
> furthermore this is also a noop now that affected planes are
> added on modesets.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This was added in patch #2.  Can we squash this change up, or was there
a reason we needed it for the intermediate patches?

Actually, I'm not sure this is really quite a noop yet.
prepare_plane_fb is never called on the primary plane as far as I can
see, so I think our frontbuffer tracking and such might get confused.
In the regular plane update codepath, we have to handle this with a
special case by doing:

        intel_crtc->atomic.disabled_planes |= (1 << drm_plane_index(plane));

in the intel_plane_atomic_check function.  But from what I can see, we
bypass that in this codepath.  igt/kms_universal_plane shows that we do
run into frontbuffer tracking warnings if we don't put an equivalent
update to the 'disabled_planes' flag here.


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9e1723e20377..d738230bedf8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12265,8 +12265,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  
>  		intel_crtc_disable_planes(crtc);
>  		dev_priv->display.crtc_disable(crtc);
> -		if (!crtc_state->enable)
> -			drm_plane_helper_disable(crtc->primary);
>  	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 08/22] drm/i915: Zap call to drm_plane_helper_disable.

[Maarten Lankhorst]

Op 28-05-15 om 03:37 schreef Matt Roper:
> On Wed, May 20, 2015 at 03:38:13PM +0200, Maarten Lankhorst wrote:
>> The primary plane can still be configured when crtc is off,
>> furthermore this is also a noop now that affected planes are
>> added on modesets.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> This was added in patch #2.  Can we squash this change up, or was there
> a reason we needed it for the intermediate patches?
>
> Actually, I'm not sure this is really quite a noop yet.
> prepare_plane_fb is never called on the primary plane as far as I can
> see, so I think our frontbuffer tracking and such might get confused.
> In the regular plane update codepath, we have to handle this with a
> special case by doing:
>
>         intel_crtc->atomic.disabled_planes |= (1 << drm_plane_index(plane));
>
> in the intel_plane_atomic_check function.  But from what I can see, we
> bypass that in this codepath.  igt/kms_universal_plane shows that we do
> run into frontbuffer tracking warnings if we don't put an equivalent
> update to the 'disabled_planes' flag here.

Well if this is the case we're really messing up on adding planes.. If 
we're fine with leaving the warning for now I'm fixing it up in series 
3.

Do the below patches fix it?

commit 05fac5b2c29a85492c677e42ec68e94f9011daa2
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue May 26 13:00:08 2015 +0200

    drm/i915: update plane state during init
    
    Atomic planes updates rely on having a accurate plane_mask.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 558fe51f151e..692cf9092f6a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2645,9 +2645,9 @@ valid_fb:
 		dev_priv->preserve_bios_swizzle = true;
 
 	primary->fb = fb;
-	primary->state->crtc = &intel_crtc->base;
-	primary->crtc = &intel_crtc->base;
+	primary->crtc = primary->state->crtc = &intel_crtc->base;
 	update_state_fb(primary);
+	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
 	obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 }
 
@@ -14959,7 +14959,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
 				  to_intel_crtc(c)->pipe);
 			drm_framebuffer_unreference(c->primary->fb);
 			c->primary->fb = NULL;
+			c->primary->crtc = c->primary->state->crtc = NULL;
 			update_state_fb(c->primary);
+			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
 		}
 	}
 


commit b2caffde98d9f7335e774d1859b881b0df8a109a
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Mon May 25 10:25:45 2015 +0200

    drm/i915: Make sure all planes and connectors are added on modeset.
    
    Add missing calls to drm_atomic_add_affected_*. This is needed
    to convert to atomic planes. When converting to atomic all planes
    are needed on modeset. For good measure make sure all connectors
    are added too.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5c1e74d1fb1b..558fe51f151e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5628,7 +5628,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int max_pixclk = intel_mode_max_pixclk(state->dev, state);
-	int cdclk, i;
+	int cdclk, ret = 0;
 
 	if (max_pixclk < 0)
 		return max_pixclk;
@@ -5643,20 +5643,25 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* add all active pipes to the state */
 	for_each_crtc(state->dev, crtc) {
-		if (!crtc->state->active)
-			continue;
-
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
-	}
 
-	/* disable/enable all currently active pipes while we change cdclk */
-	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		if (crtc_state->active)
-			crtc_state->mode_changed = true;
+		if (!crtc_state->active || needs_modeset(crtc_state))
+			continue;
 
-	return 0;
+		crtc_state->mode_changed = true;
+
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			break;
+
+		ret = drm_atomic_add_affected_planes(state, crtc);
+		if (ret)
+			break;
+	}
+
+	return ret;
 }
 
 static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
@@ -11483,8 +11488,10 @@ encoder_retry:
 
 	/* Check if we need to force a modeset */
 	if (pipe_config->has_audio !=
-	    to_intel_crtc_state(crtc->state)->has_audio)
+	    to_intel_crtc_state(crtc->state)->has_audio) {
 		pipe_config->base.mode_changed = true;
+		ret = drm_atomic_add_affected_planes(state, crtc);
+	}
 
 	/*
 	 * Note we have an issue here with infoframes: current code
@@ -11492,8 +11499,6 @@ encoder_retry:
 	 * requirements.  So here we should be checking for any
 	 * required changes and forcing a mode set.
 	 */
-
-	return 0;
 fail:
 	return ret;
 }

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

Re: [PATCH v3 14/22] drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()

[Matt Roper]

On Wed, May 20, 2015 at 06:04:26PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> It makes more sense there, since these are computation steps that can
> fail.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I've noticed that a few of the patches in this series were originally
written by Ander, but seem to be missing his s-o-b line.  I think you
generally want to just append your line after his in that case.

One other cosmetic note farther down.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 81d5358efdde..e7aa8610cbdc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  		crtc->scanline_offset = 1;
>  }
>  
> -static int
> -intel_modeset_compute_config(struct drm_atomic_state *state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	int ret, i;
> -
> -	ret = drm_atomic_helper_check_modeset(state->dev, state);
> -	if (ret)
> -		return ret;
> -
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!crtc_state->enable &&
> -		    WARN_ON(crtc_state->active))
> -			crtc_state->active = false;
> -
> -		if (!crtc_state->enable)
> -			continue;
> -
> -		ret = intel_modeset_pipe_config(crtc, state);
> -		if (ret)
> -			return ret;
> -
> -		intel_dump_pipe_config(to_intel_crtc(crtc),
> -				       to_intel_crtc_state(crtc_state),
> -				       "[modeset]");
> -	}
> -
> -	return drm_atomic_helper_check_planes(state->dev, state);
> -}
> -
>  static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static int
> +intel_modeset_compute_config(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int ret, i;
> +
> +	ret = drm_atomic_helper_check_modeset(state->dev, state);
> +	if (ret)
> +		return ret;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (!crtc_state->enable &&
> +		    WARN_ON(crtc_state->active))
> +			crtc_state->active = false;
> +
> +		if (!crtc_state->enable)
> +			continue;
> +
> +		ret = intel_modeset_pipe_config(crtc, state);
> +		if (ret)
> +			return ret;
> +
> +		intel_dump_pipe_config(to_intel_crtc(crtc),
> +				       to_intel_crtc_state(crtc_state),
> +				       "[modeset]");
> +	}
> +
> +	ret = drm_atomic_helper_check_planes(state->dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return __intel_set_mode_checks(state);

Just a cosmetic note, but maybe we should rename this function now?
It's not called from __intel_set_mode anymore and it isn't really
'checks' (but rather setup that we intend to be done during the check
phase), so the whole name seems a bit misleading now.


Matt

> +}
> +
>  static int __intel_set_mode(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -12200,10 +12204,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  	int ret = 0;
>  	int i;
>  
> -	ret = __intel_set_mode_checks(state);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
>  	if (ret)
>  		return ret;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 15/22] drm/i915: Read hw state into an atomic state struct

[Matt Roper]

On Wed, May 20, 2015 at 06:04:27PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> To make this work we load the new hardware state into the
> atomic_state, then swap it with the sw state.
> 
> This lets us change the force restore path in setup_hw_state()
> to use a single call to intel_mode_set() to restore all the
> previous state.
> 
> As a nice bonus this kills off encoder->new_encoder,
> connector->new_enabled and crtc->new_enabled. They were used only
> to restore the state after a modeset.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |  14 +-
>  3 files changed, 241 insertions(+), 152 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 83078763ba14..17bf9e80c557 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static void
> +void
>  intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
>  				  struct intel_shared_dpll_config *shared_dpll)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e7aa8610cbdc..a42e7d7bf86b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9639,7 +9639,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  retry:
>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
>  	if (ret)
> -		goto fail_unlock;
> +		goto fail;
>  
>  	/*
>  	 * Algorithm gets a little messy:
> @@ -9657,10 +9657,10 @@ retry:
>  
>  		ret = drm_modeset_lock(&crtc->mutex, ctx);
>  		if (ret)
> -			goto fail_unlock;
> +			goto fail;
>  		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>  		if (ret)
> -			goto fail_unlock;
> +			goto fail;
>  
>  		old->dpms_mode = connector->dpms;
>  		old->load_detect_temp = false;
> @@ -9679,9 +9679,6 @@ retry:
>  			continue;
>  		if (possible_crtc->state->enable)
>  			continue;
> -		/* This can occur when applying the pipe A quirk on resume. */
> -		if (to_intel_crtc(possible_crtc)->new_enabled)
> -			continue;
>  
>  		crtc = possible_crtc;
>  		break;
> @@ -9692,20 +9689,17 @@ retry:
>  	 */
>  	if (!crtc) {
>  		DRM_DEBUG_KMS("no pipe available for load-detect\n");
> -		goto fail_unlock;
> +		goto fail;
>  	}
>  
>  	ret = drm_modeset_lock(&crtc->mutex, ctx);
>  	if (ret)
> -		goto fail_unlock;
> +		goto fail;
>  	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>  	if (ret)
> -		goto fail_unlock;
> -	intel_encoder->new_crtc = to_intel_crtc(crtc);
> -	to_intel_connector(connector)->new_encoder = intel_encoder;
> +		goto fail;
>  
>  	intel_crtc = to_intel_crtc(crtc);
> -	intel_crtc->new_enabled = true;
>  	old->dpms_mode = connector->dpms;
>  	old->load_detect_temp = true;
>  	old->release_fb = NULL;
> @@ -9773,9 +9767,7 @@ retry:
>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
>  	return true;
>  
> - fail:
> -	intel_crtc->new_enabled = crtc->state->enable;
> -fail_unlock:
> +fail:
>  	drm_atomic_state_free(state);
>  	state = NULL;
>  
> @@ -9821,10 +9813,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  		if (IS_ERR(crtc_state))
>  			goto fail;
>  
> -		to_intel_connector(connector)->new_encoder = NULL;
> -		intel_encoder->new_crtc = NULL;
> -		intel_crtc->new_enabled = false;
> -
>  		connector_state->best_encoder = NULL;
>  		connector_state->crtc = NULL;
>  
> @@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.atomic_flush = intel_finish_crtc_commit,
>  };
>  
> -/**
> - * intel_modeset_update_staged_output_state
> - *
> - * Updates the staged output configuration state, e.g. after we've read out the
> - * current hw state.
> - */
> -static void intel_modeset_update_staged_output_state(struct drm_device *dev)
> -{
> -	struct intel_crtc *crtc;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> -
> -	for_each_intel_connector(dev, connector) {
> -		connector->new_encoder =
> -			to_intel_encoder(connector->base.encoder);
> -	}
> -
> -	for_each_intel_encoder(dev, encoder) {
> -		encoder->new_crtc =
> -			to_intel_crtc(encoder->base.crtc);
> -	}
> -
> -	for_each_intel_crtc(dev, crtc) {
> -		crtc->new_enabled = crtc->base.state->enable;
> -	}
> -}
> -
>  /* Transitional helper to copy current connector/encoder state to
>   * connector->state. This is needed so that code that is partially
>   * converted to atomic does the right thing.
> @@ -11526,7 +11487,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  	}
>  
>  	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> -	intel_modeset_update_staged_output_state(state->dev);
>  
>  	/* Double check state. */
>  	for_each_crtc(dev, crtc) {
> @@ -11832,11 +11792,14 @@ check_connector_state(struct drm_device *dev)
>  	struct intel_connector *connector;
>  
>  	for_each_intel_connector(dev, connector) {
> +		struct drm_encoder *encoder = connector->base.encoder;
> +		struct drm_connector_state *state = connector->base.state;
> +
>  		/* This also checks the encoder/connector hw state with the
>  		 * ->get_hw_state callbacks. */
>  		intel_connector_check_state(connector);
>  
> -		I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
> +		I915_STATE_WARN(state->best_encoder != encoder,
>  		     "connector's staged encoder doesn't match current encoder\n");
>  	}
>  }
> @@ -11856,8 +11819,6 @@ check_encoder_state(struct drm_device *dev)
>  			      encoder->base.base.id,
>  			      encoder->base.name);
>  
> -		I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
> -		     "encoder's stage crtc doesn't match current crtc\n");
>  		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
>  		     "encoder's active_connectors set, but no crtc\n");
>  
> @@ -11867,6 +11828,9 @@ check_encoder_state(struct drm_device *dev)
>  			enabled = true;
>  			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
>  				active = true;
> +
> +			I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc,
> +			     "encoder's stage crtc doesn't match current crtc\n");
>  		}
>  		/*
>  		 * for MST connectors if we unplug the connector is gone
> @@ -12296,11 +12260,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  	 * need to copy the staged config to the atomic state, otherwise the
>  	 * mode set will just reapply the state the HW is already in. */
>  	for_each_intel_encoder(dev, encoder) {
> -		if (&encoder->new_crtc->base != crtc)
> +		if (encoder->base.crtc != crtc)
>  			continue;
>  
>  		for_each_intel_connector(dev, connector) {
> -			if (connector->new_encoder != encoder)
> +			if (connector->base.state->best_encoder != &encoder->base)
>  				continue;
>  
>  			connector_state = drm_atomic_get_connector_state(state, &connector->base);
> @@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  			}
>  
>  			connector_state->crtc = crtc;
> -			connector_state->best_encoder = &encoder->base;
>  		}
>  	}
>  
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
> -			continue;
> -
>  		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>  		if (IS_ERR(crtc_state)) {
>  			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
> @@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  			continue;
>  		}
>  
> -		crtc_state->base.active = crtc_state->base.enable =
> -			intel_crtc->new_enabled;
> -
>  		if (&intel_crtc->base == crtc)
>  			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
>  	}
> @@ -14552,99 +14509,224 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  
> -	if (!crtc->active)
> +	if (!crtc->base.enabled)
>  		return false;
>  
>  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>  }
>  
> -static void intel_modeset_readout_hw_state(struct drm_device *dev)
> +static int readout_hw_crtc_state(struct drm_atomic_state *state,
> +				 struct intel_crtc *crtc)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum pipe pipe;
> -	struct intel_crtc *crtc;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> -	int i;
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_crtc_state *crtc_state;
> +	struct drm_plane *primary = crtc->base.primary;
> +	struct drm_plane_state *drm_plane_state;
> +	struct intel_plane_state *plane_state;
> +	int ret;
>  
> -	for_each_intel_crtc(dev, crtc) {
> -		struct drm_plane *primary = crtc->base.primary;
> -		struct intel_plane_state *plane_state;
> +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
>  
> -		memset(crtc->config, 0, sizeof(*crtc->config));
> -		crtc->config->base.crtc = &crtc->base;
> +	ret = drm_atomic_add_affected_planes(state, &crtc->base);
> +	if (ret)
> +		return ret;

Do we actually try to handle non-primary planes in this function?  If
I'm following this properly, at bootup we won't add any sprite or cursor
planes here, even though it's possible that a graphics firmware has
turned some of them on.  It seems like our sw and hw states will still
be a bit inconsistent in that case.


Matt

>  
> -		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
> +	memset(crtc_state, 0, sizeof(*crtc_state));
> +	crtc_state->base.crtc = &crtc->base;
> +	crtc_state->base.state = state;
>  
> -		crtc->active = dev_priv->display.get_pipe_config(crtc,
> -								 crtc->config);
> +	crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> -		crtc->base.state->enable = crtc->active;
> -		crtc->base.state->active = crtc->active;
> -		crtc->base.enabled = crtc->active;
> +	crtc_state->base.enable = crtc_state->base.active =
> +	crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state);
>  
> -		plane_state = to_intel_plane_state(primary->state);
> -		plane_state->visible = primary_get_hw_state(crtc);
> +	/* update transitional state */
> +	crtc->active = crtc_state->base.active;
> +	crtc->config = crtc_state;
>  
> -		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
> -			      crtc->base.base.id,
> -			      crtc->active ? "enabled" : "disabled");
> -	}
> +	drm_plane_state = drm_atomic_get_plane_state(state, primary);
> +	if (IS_ERR(drm_plane_state))
> +		return PTR_ERR(drm_plane_state);
> +
> +	plane_state = to_intel_plane_state(drm_plane_state);
> +	plane_state->visible = primary_get_hw_state(crtc);
> +
> +	if (plane_state->visible)
> +		crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
> +	else
> +		crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
> +
> +	DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
> +		      crtc->base.base.id,
> +		      crtc_state->base.active ? "enabled" : "disabled");
> +
> +	return 0;
> +}
> +
> +static int readout_hw_pll_state(struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_shared_dpll_config *shared_dpll;
> +	struct intel_crtc *crtc;
> +	struct intel_crtc_state *crtc_state;
> +	int i;
>  
> +	shared_dpll = intel_atomic_get_shared_dpll_state(state);
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>  
>  		pll->on = pll->get_hw_state(dev_priv, pll,
> -					    &pll->config.hw_state);
> +					    &shared_dpll[i].hw_state);
> +
>  		pll->active = 0;
> -		pll->config.crtc_mask = 0;
> -		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
> +		shared_dpll[i].crtc_mask = 0;
> +
> +		for_each_intel_crtc(state->dev, crtc) {
> +			crtc_state = intel_atomic_get_crtc_state(state, crtc);
> +			if (IS_ERR(crtc_state))
> +				return PTR_ERR(crtc_state);
> +
> +			if (crtc_state->base.active &&
> +			    crtc_state->shared_dpll == i) {
>  				pll->active++;
> -				pll->config.crtc_mask |= 1 << crtc->pipe;
> +				shared_dpll[i].crtc_mask |=
> +					1 << crtc->pipe;
>  			}
>  		}
>  
>  		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
> -			      pll->name, pll->config.crtc_mask, pll->on);
> +			      pll->name, shared_dpll[i].crtc_mask,
> +			      pll->on);
>  
> -		if (pll->config.crtc_mask)
> +		if (shared_dpll[i].crtc_mask)
>  			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	}
>  
> -	for_each_intel_encoder(dev, encoder) {
> -		pipe = 0;
> +	return 0;
> +}
>  
> -		if (encoder->get_hw_state(encoder, &pipe)) {
> -			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -			encoder->base.crtc = &crtc->base;
> -			encoder->get_config(encoder, crtc->config);
> -		} else {
> -			encoder->base.crtc = NULL;
> -		}
> +static struct drm_connector_state *
> +get_connector_state_for_encoder(struct drm_atomic_state *state,
> +				struct intel_encoder *encoder)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *connector_state;
> +	int i;
>  
> -		encoder->connectors_active = false;
> -		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
> -			      encoder->base.base.id,
> -			      encoder->base.name,
> -			      encoder->base.crtc ? "enabled" : "disabled",
> -			      pipe_name(pipe));
> -	}
> +	for_each_connector_in_state(state, connector, connector_state, i)
> +		if (connector_state->best_encoder == &encoder->base)
> +			return connector_state;
> +
> +	return NULL;
> +}
> +
> +static int readout_hw_connector_encoder_state(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_crtc *crtc;
> +	struct drm_crtc_state *drm_crtc_state;
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_encoder *encoder;
> +	struct intel_connector *connector;
> +	struct drm_connector_state *connector_state;
> +	enum pipe pipe;
>  
>  	for_each_intel_connector(dev, connector) {
> +		connector_state =
> +			drm_atomic_get_connector_state(state, &connector->base);
> +		if (IS_ERR(connector_state))
> +			return PTR_ERR(connector_state);
> +
>  		if (connector->get_hw_state(connector)) {
>  			connector->base.dpms = DRM_MODE_DPMS_ON;
> -			connector->encoder->connectors_active = true;
>  			connector->base.encoder = &connector->encoder->base;
>  		} else {
>  			connector->base.dpms = DRM_MODE_DPMS_OFF;
>  			connector->base.encoder = NULL;
>  		}
> +
> +		/* We'll update the crtc field when reading encoder state */
> +		connector_state->crtc = NULL;
> +
> +		connector_state->best_encoder = connector->base.encoder;
> +
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] hw state readout: %s\n",
>  			      connector->base.base.id,
>  			      connector->base.name,
>  			      connector->base.encoder ? "enabled" : "disabled");
>  	}
> +
> +	for_each_intel_encoder(dev, encoder) {
> +		pipe = 0;
> +
> +		connector_state =
> +			get_connector_state_for_encoder(state, encoder);
> +
> +		encoder->connectors_active = !!connector_state;
> +
> +		if (encoder->get_hw_state(encoder, &pipe)) {
> +			encoder->base.crtc =
> +				dev_priv->pipe_to_crtc_mapping[pipe];
> +			crtc = to_intel_crtc(encoder->base.crtc);
> +
> +			drm_crtc_state =
> +				state->crtc_states[drm_crtc_index(&crtc->base)];
> +			crtc_state = to_intel_crtc_state(drm_crtc_state);
> +
> +			encoder->get_config(encoder, crtc_state);
> +
> +			if (connector_state)
> +				connector_state->crtc = &crtc->base;
> +		} else {
> +			encoder->base.crtc = NULL;
> +		}
> +
> +		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
> +			      encoder->base.base.id,
> +			      encoder->base.name,
> +			      encoder->base.crtc ? "enabled" : "disabled",
> +			      pipe_name(pipe));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct drm_atomic_state *
> +intel_modeset_readout_hw_state(struct drm_device *dev)
> +{
> +	struct intel_crtc *crtc;
> +	int ret = 0;
> +
> +	struct drm_atomic_state *state;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		ret = readout_hw_crtc_state(state, crtc);
> +		if (ret)
> +			goto err_free;
> +	}
> +
> +	ret = readout_hw_pll_state(state);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = readout_hw_connector_encoder_state(state);
> +	if (ret)
> +		goto err_free;
> +
> +	return state;
> +
> +err_free:
> +	drm_atomic_state_free(state);
> +	return ERR_PTR(ret);
>  }
>  
>  /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
> @@ -14653,37 +14735,57 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  				  bool force_restore)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum pipe pipe;
> -	struct intel_crtc *crtc;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
>  	struct intel_encoder *encoder;
> +	struct drm_atomic_state *state;
> +	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
>  	int i;
>  
> -	intel_modeset_readout_hw_state(dev);
> -
> -	/*
> -	 * Now that we have the config, copy it to each CRTC struct
> -	 * Note that this could go away if we move to using crtc_config
> -	 * checking everywhere.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (crtc->active && i915.fastboot) {
> -			intel_mode_from_pipe_config(&crtc->base.mode,
> -						    crtc->config);
> -			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> -				      crtc->base.base.id);
> -			drm_mode_debug_printmodeline(&crtc->base.mode);
> -		}
> +	state = intel_modeset_readout_hw_state(dev);
> +	if (IS_ERR(state)) {
> +		DRM_ERROR("Failed to read out hw state\n");
> +		return;
>  	}
>  
> +	drm_atomic_helper_swap_state(dev, state);
> +
> +	/* swap sw/hw dpll state */
> +	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
> +	intel_shared_dpll_commit(state);
> +	memcpy(to_intel_atomic_state(state)->shared_dpll,
> +	       shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
> +
>  	/* HW state is read out, now we need to sanitize this mess. */
>  	for_each_intel_encoder(dev, encoder) {
>  		intel_sanitize_encoder(encoder);
>  	}
>  
> -	for_each_pipe(dev_priv, pipe) {
> -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -		intel_sanitize_crtc(crtc);
> -		intel_dump_pipe_config(crtc, crtc->config,
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +		/* prevent unnneeded restores with force_restore */
> +		crtc_state->active_changed =
> +		crtc_state->mode_changed =
> +		crtc_state->planes_changed = false;
> +
> +		if (crtc->enabled) {
> +			intel_mode_from_pipe_config(&crtc->state->mode,
> +				to_intel_crtc_state(crtc->state));
> +
> +			drm_mode_copy(&crtc->mode, &crtc->state->mode);
> +			drm_mode_copy(&crtc->hwmode,
> +				      &crtc->state->adjusted_mode);
> +		}
> +
> +		intel_sanitize_crtc(intel_crtc);
> +
> +		/*
> +		 * sanitize_crtc may have forced an update of crtc->state,
> +		 * so reload in intel_dump_pipe_config
> +		 */
> +		intel_dump_pipe_config(intel_crtc,
> +				       to_intel_crtc_state(crtc->state),
>  				       "[setup_hw_state]");
>  	}
>  
> @@ -14707,20 +14809,17 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		ilk_wm_get_hw_state(dev);
>  
>  	if (force_restore) {
> -		i915_redisable_vga(dev);
> +		int ret;
>  
> -		/*
> -		 * We need to use raw interfaces for restoring state to avoid
> -		 * checking (bogus) intermediate states.
> -		 */
> -		for_each_pipe(dev_priv, pipe) {
> -			struct drm_crtc *crtc =
> -				dev_priv->pipe_to_crtc_mapping[pipe];
> +		i915_redisable_vga(dev);
>  
> -			intel_crtc_restore_mode(crtc);
> +		ret = intel_set_mode(state);
> +		if (ret) {
> +			DRM_ERROR("Failed to restore previous mode\n");
> +			drm_atomic_state_free(state);
>  		}
>  	} else {
> -		intel_modeset_update_staged_output_state(dev);
> +		drm_atomic_state_free(state);
>  	}
>  
>  	intel_modeset_check_state(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7012c67de8d5..65f5f3d41b5a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -130,11 +130,6 @@ struct intel_fbdev {
>  
>  struct intel_encoder {
>  	struct drm_encoder base;
> -	/*
> -	 * The new crtc this encoder will be driven from. Only differs from
> -	 * base->crtc while a modeset is in progress.
> -	 */
> -	struct intel_crtc *new_crtc;
>  
>  	enum intel_output_type type;
>  	unsigned int cloneable;
> @@ -195,12 +190,6 @@ struct intel_connector {
>  	 */
>  	struct intel_encoder *encoder;
>  
> -	/*
> -	 * The new encoder this connector will be driven. Only differs from
> -	 * encoder while a modeset is in progress.
> -	 */
> -	struct intel_encoder *new_encoder;
> -
>  	/* Reads out the current hw, returning true if the connector is enabled
>  	 * and active (i.e. dpms ON state). */
>  	bool (*get_hw_state)(struct intel_connector *);
> @@ -534,7 +523,6 @@ struct intel_crtc {
>  
>  	struct intel_initial_plane_config plane_config;
>  	struct intel_crtc_state *config;
> -	bool new_enabled;
>  
>  	/* reset counter value when the last flip was submitted */
>  	unsigned int reset_counter;
> @@ -1414,6 +1402,8 @@ struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
>  void intel_atomic_state_clear(struct drm_atomic_state *);
>  struct intel_shared_dpll_config *
>  intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
> +void intel_atomic_duplicate_dpll_state(struct drm_i915_private *,
> +				       struct intel_shared_dpll_config *);
>  
>  static inline struct intel_crtc_state *
>  intel_atomic_get_crtc_state(struct drm_atomic_state *state,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 13/22] drm/i915: Swap planes on each crtc separately.

[Matt Roper]

On Wed, May 20, 2015 at 06:04:25PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Repeated calls to begin_crtc_commit can cause warnings like this:
> [  169.127746] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:616
> [  169.127835] in_atomic(): 0, irqs_disabled(): 1, pid: 1947, name: kms_flip
> [  169.127840] 3 locks held by kms_flip/1947:
> [  169.127843]  #0:  (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffff814774bc>] __drm_modeset_lock_all+0x9c/0x130
> [  169.127860]  #1:  (crtc_ww_class_acquire){+.+.+.}, at: [<ffffffff814774cd>] __drm_modeset_lock_all+0xad/0x130
> [  169.127870]  #2:  (crtc_ww_class_mutex){+.+.+.}, at: [<ffffffff81477178>] drm_modeset_lock+0x38/0x110
> [  169.127879] irq event stamp: 665690
> [  169.127882] hardirqs last  enabled at (665689): [<ffffffff817ffdb5>] _raw_spin_unlock_irqrestore+0x55/0x70
> [  169.127889] hardirqs last disabled at (665690): [<ffffffffc0197a23>] intel_pipe_update_start+0x113/0x5c0 [i915]
> [  169.127936] softirqs last  enabled at (665470): [<ffffffff8108a766>] __do_softirq+0x236/0x650
> [  169.127942] softirqs last disabled at (665465): [<ffffffff8108ae75>] irq_exit+0xc5/0xd0
> [  169.127951] CPU: 1 PID: 1947 Comm: kms_flip Not tainted 4.1.0-rc4-patser+ #4039
> [  169.127954] Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
> [  169.127957]  ffff8800c49036f0 ffff8800cde5fa28 ffffffff817f6907 0000000080000001
> [  169.127964]  0000000000000000 ffff8800cde5fa58 ffffffff810aebed 0000000000000046
> [  169.127970]  ffffffff81c5d518 0000000000000268 0000000000000000 ffff8800cde5fa88
> [  169.127981] Call Trace:
> [  169.127992]  [<ffffffff817f6907>] dump_stack+0x4f/0x7b
> [  169.128001]  [<ffffffff810aebed>] ___might_sleep+0x16d/0x270
> [  169.128008]  [<ffffffff810aed38>] __might_sleep+0x48/0x90
> [  169.128017]  [<ffffffff817fc359>] mutex_lock_nested+0x29/0x410
> [  169.128073]  [<ffffffffc01635f0>] ? vgpu_write64+0x220/0x220 [i915]
> [  169.128138]  [<ffffffffc017fddf>] ? ironlake_update_primary_plane+0x2ff/0x410 [i915]
> [  169.128198]  [<ffffffffc0190e75>] intel_frontbuffer_flush+0x25/0x70 [i915]
> [  169.128253]  [<ffffffffc01831ac>] intel_finish_crtc_commit+0x4c/0x180 [i915]
> [  169.128279]  [<ffffffffc00784ac>] drm_atomic_helper_commit_planes+0x12c/0x240 [drm_kms_helper]
> [  169.128338]  [<ffffffffc0184264>] __intel_set_mode+0x684/0x830 [i915]
> [  169.128378]  [<ffffffffc018a84a>] intel_crtc_set_config+0x49a/0x620 [i915]
> [  169.128385]  [<ffffffff817fdd39>] ? mutex_unlock+0x9/0x10
> [  169.128391]  [<ffffffff81467b69>] drm_mode_set_config_internal+0x69/0x120
> [  169.128398]  [<ffffffff8119b547>] ? might_fault+0x57/0xb0
> [  169.128403]  [<ffffffff8146bf93>] drm_mode_setcrtc+0x253/0x620
> [  169.128409]  [<ffffffff8145c600>] drm_ioctl+0x1a0/0x6a0
> [  169.128415]  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> [  169.128424]  [<ffffffff811e9ab8>] do_vfs_ioctl+0x2f8/0x530
> [  169.128429]  [<ffffffff810d0fcd>] ? trace_hardirqs_on+0xd/0x10
> [  169.128435]  [<ffffffff812e7676>] ? selinux_file_ioctl+0x56/0x100
> [  169.128439]  [<ffffffff811e9d71>] SyS_ioctl+0x81/0xa0
> [  169.128445]  [<ffffffff81800697>] system_call_fastpath+0x12/0x6f
> 
> Solve it by using the newly introduced drm_atomic_helper_commit_planes_on_crtc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This commit looks good, but I think you might want to elaborate on the
commit message a bit for people not super familiar with the atomic work.
The problem here was that the drm_atomic_helper_commit_planes() helper
we were using was basically designed to do

    begin_crtc_commit(crtc #1)
    begin_crtc_commit(crtc #2)
    ...
    commit all planes
    finish_crtc_commit(crtc #1)
    finish_crtc_commit(crtc #2)

The problem here is that since our hardware relies on vblank evasion,
our CRTC 'begin' function waits until we're out of the danger zone in
which register writes might wind up straddling the vblank, then disables
interrupts; our 'finish' function re-enables interrupts after the
registers have been written.  The expectation is that the operations between
'begin' and 'end' must be performed without sleeping (since interrupts
are disabled) and should happen as quickly as possible.  By clumping all
of the 'begin' calls together, we introducing a couple problems:
 * Subsequent 'begin' invocations might sleep (which is illegal)
 * The first 'begin' ensured that we were far enough from the vblank that
   we could write our registers safely and ensure they all fell within
   the same frame.  Adding extra delay waiting for subsequent CRTC's
   wasn't accounted for and could put us back into the 'danger zone' for
   CRTC #1.

This commit solves the problem by using a new helper that allows an
order of operations like:

   for each crtc {
        begin_crtc_commit(crtc)  // sleep (maybe), then disable interrupts
        commit planes for this specific CRTC
        end_crtc_commit(crtc)    // reenable interrupts
   }

so that sleeps will only be performed while interrupts are enabled and
we can be sure that registers for a CRTC will be written immediately
once we know we're in the safe zone.


The only other thing I noted is that the CRTC backpointer update in
intel_modeset_readout_hw_state() seems somewhat unrelated that might
have fit better in one of the earlier patches (but is necessary at this
point since otherwise the helper will dereference NULL).  You might want
to make a quick note about that.

Matt

> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 13 +++----------
>  drivers/gpu/drm/i915/intel_display.c |  5 +++--
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index e93aade0f2dc..83078763ba14 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -151,18 +151,11 @@ int intel_atomic_commit(struct drm_device *dev,
>  
>  		if (INTEL_INFO(dev)->gen >= 9)
>  			skl_detach_scalers(to_intel_crtc(crtc));
> +
> +		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>  	}
>  
> -	/*
> -	 * FIXME:  The proper sequence here will eventually be:
> -	 *
> -	 * drm_atomic_helper_commit_modeset_disables(dev, state);
> -	 * drm_atomic_helper_commit_planes(dev, state);
> -	 * drm_atomic_helper_commit_modeset_enables(dev, state);
> -	 *
> -	 * once we have full atomic modeset.
> -	 */
> -	drm_atomic_helper_commit_planes(dev, state);
> +	/* FIXME: This function should eventually call __intel_set_mode when needed */
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index adb19c3c9172..81d5358efdde 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12225,10 +12225,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  
>  	modeset_update_crtc_power_domains(state);
>  
> -	drm_atomic_helper_commit_planes(dev, state);
> -
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> +
>  		if (!needs_modeset(crtc->state) || !crtc->state->active)
>  			continue;
>  
> @@ -14572,6 +14572,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		struct intel_plane_state *plane_state;
>  
>  		memset(crtc->config, 0, sizeof(*crtc->config));
> +		crtc->config->base.crtc = &crtc->base;
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.6 16/22] drm/i915: Implement intel_crtc_control using atomic state, v4.

[Matt Roper]

On Tue, May 26, 2015 at 10:35:22AM +0200, Maarten Lankhorst wrote:
> Assume the callers lock everything with drm_modeset_lock_all.
> 
> This change had to be done after converting suspend/resume to
> use atomic_state so the atomic state is preserved, otherwise
> all transitional state is erased.
> 
> Now all callers of .crtc_enable and .crtc_disable go through
> atomic modeset! :-D
> 
> Changes since v1:
> - Only check for crtc_state->active in valleyview_modeset_global_pipes.
> - Only check for crtc_state->active in modeset_update_crtc_power_domains.
> Changes since v2:
> - Rework on top of the changed patch order.
> Changes since v3:
> - Rename intel_crtc_toggle in description to *_control
> - Change return value to int.
> - Do not add plane state, should be done implicitly already.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9680eb85bd0..32bab28432f7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev)
>  }
>  
>  /* Master function to enable/disable CRTC and corresponding power wells */
> -void intel_crtc_control(struct drm_crtc *crtc, bool enable)
> +int intel_crtc_control(struct drm_crtc *crtc, bool enable)

We change the return value to 'int' here, but we never check the error
code anywhere.  Maybe we should hold this off for another patch and then
also propagate the return value back up the call chain?  It seems like
at least some of the connector-specific DPMS functions should recognize
the failure of intel_crtc_update_dpms() before trying to continue with
other operations.


>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum intel_display_power_domain domain;
> -	unsigned long domains;
> +	struct intel_crtc_state *pipe_config;
> +	struct drm_atomic_state *state;
> +	int ret;
>  
>  	if (enable == intel_crtc->active)
> -		return;
> +		return 0;
>  
>  	if (enable && !crtc->state->enable)
> -		return;
> +		return 0;
>  
> -	crtc->state->active = enable;
> -	if (enable) {
> -		domains = get_crtc_power_domains(crtc);
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_get(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = domains;
> +	/* this function should be called with drm_modeset_lock_all for now */
> +	if (WARN_ON(!ctx))
> +		return -EIO;

EIO seems like an unusual choice here (but I'm not really sure what the
best option would be).


Matt

> +	lockdep_assert_held(&ctx->ww_ctx);
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> -	} else {
> -		intel_crtc_disable_planes(crtc);
> -		dev_priv->display.crtc_disable(crtc);
> +	state = drm_atomic_state_alloc(dev);
> +	if (WARN_ON(!state))
> +		return -ENOMEM;
>  
> -		domains = intel_crtc->enabled_power_domains;
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_put(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = 0;
> +	state->acquire_ctx = ctx;
> +	state->allow_modeset = true;
> +
> +	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto err;
>  	}
> +	pipe_config->base.active = enable;
> +
> +	ret = intel_set_mode(state);
> +	if (!ret)
> +		return ret;
> +
> +err:
> +	DRM_ERROR("Updating crtc active failed with %i\n", ret);
> +	drm_atomic_state_free(state);
> +	return ret;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 60a29ff65f1f..c113f187090f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>  void intel_display_suspend(struct drm_device *dev);
> -void intel_crtc_control(struct drm_crtc *crtc, bool enable);
> +int intel_crtc_control(struct drm_crtc *crtc, bool enable);
>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3.6 20/22] drm/i915: Calculate haswell plane workaround, v4.

[Matt Roper]

On Tue, May 26, 2015 at 10:36:14AM +0200, Maarten Lankhorst wrote:
> This needs to be a global check because at the time of crtc checking
> not all modesets have to be calculated yet. Use intel_crtc->atomic
> because there's no reason to keep it in state.

The note about intel_crtc->atomic is out of date as of v3.  Otherwise, I
think this looks okay.

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


Matt

> 
> Changes since v1:
>  - Use intel_crtc->atomic as a place to put hsw_workaround_pipe.
>  - Make sure quirk only applies to haswell.
>  - Use first loop to iterate over newly enabled crtc's only.
>    This increases readability.
> Changes since v2:
>  - Move hsw_workaround_pipe back to crtc_state.
> Changes since v3:
>  - Return errors from haswell_mode_set_planes_workaround.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  2 files changed, 80 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index de13c1c14c93..f6733a777590 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4867,42 +4867,15 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
>  	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>  }
>  
> -/*
> - * This implements the workaround described in the "notes" section of the mode
> - * set sequence documentation. When going from no pipes or single pipe to
> - * multiple pipes, and planes are enabled after the pipe, we need to wait at
> - * least 2 vblanks on the first pipe before enabling planes on the second pipe.
> - */
> -static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->base.dev;
> -	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
> -
> -	/* We want to get the other_active_crtc only if there's only 1 other
> -	 * active crtc. */
> -	for_each_intel_crtc(dev, crtc_it) {
> -		if (!crtc_it->active || crtc_it == crtc)
> -			continue;
> -
> -		if (other_active_crtc)
> -			return;
> -
> -		other_active_crtc = crtc_it;
> -	}
> -	if (!other_active_crtc)
> -		return;
> -
> -	intel_wait_for_vblank(dev, other_active_crtc->pipe);
> -	intel_wait_for_vblank(dev, other_active_crtc->pipe);
> -}
> -
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
> -	int pipe = intel_crtc->pipe;
> +	int pipe = intel_crtc->pipe, hsw_workaround_pipe;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->state);
>  
>  	if (WARN_ON(intel_crtc->active))
>  		return;
> @@ -4979,7 +4952,11 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
> -	haswell_mode_set_planes_workaround(intel_crtc);
> +	hsw_workaround_pipe = pipe_config->hsw_workaround_pipe;
> +	if (IS_HASWELL(dev) && hsw_workaround_pipe != INVALID_PIPE) {
> +		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> +		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> +	}
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -12169,6 +12146,71 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  	return ret;
>  }
>  
> +/*
> + * This implements the workaround described in the "notes" section of the mode
> + * set sequence documentation. When going from no pipes or single pipe to
> + * multiple pipes, and planes are enabled after the pipe, we need to wait at
> + * least 2 vblanks on the first pipe before enabling planes on the second pipe.
> + */
> +static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc_state *first_crtc_state = NULL;
> +	struct intel_crtc_state *other_crtc_state = NULL;
> +	enum pipe first_pipe = INVALID_PIPE, enabled_pipe = INVALID_PIPE;
> +	int i;
> +
> +	/* look at all crtc's that are going to be enabled in during modeset */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!crtc_state->active || !needs_modeset(crtc_state))
> +			continue;
> +
> +		if (first_crtc_state) {
> +			other_crtc_state = to_intel_crtc_state(crtc_state);
> +			break;
> +		} else {
> +			first_crtc_state = to_intel_crtc_state(crtc_state);
> +			first_pipe = intel_crtc->pipe;
> +		}
> +	}
> +
> +	/* No workaround needed? */
> +	if (!first_crtc_state)
> +		return 0;
> +
> +	/* w/a possibly needed, check how many crtc's are already enabled. */
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		struct intel_crtc_state *pipe_config;
> +
> +		pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> +		if (IS_ERR(pipe_config))
> +			return PTR_ERR(pipe_config);
> +
> +		pipe_config->hsw_workaround_pipe = INVALID_PIPE;
> +
> +		if (!pipe_config->base.active ||
> +		    needs_modeset(&pipe_config->base))
> +			continue;
> +
> +		/* 2 or more enabled crtcs means no need for w/a */
> +		if (enabled_pipe != INVALID_PIPE)
> +			return 0;
> +
> +		enabled_pipe = intel_crtc->pipe;
> +	}
> +
> +	if (enabled_pipe != INVALID_PIPE)
> +		first_crtc_state->hsw_workaround_pipe = enabled_pipe;
> +	else if (other_crtc_state)
> +		other_crtc_state->hsw_workaround_pipe = first_pipe;
> +
> +	return 0;
> +}
> +
>  /* Code that should eventually be part of atomic_check() */
>  static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  {
> @@ -12192,7 +12234,10 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> -	return 0;
> +	if (IS_HASWELL(dev))
> +		ret = haswell_mode_set_planes_workaround(state);
> +
> +	return ret;
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 665e249ae8bf..d5f38e3d732b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -443,6 +443,9 @@ struct intel_crtc_state {
>  	int pbn;
>  
>  	struct intel_crtc_scaler_state scaler_state;
> +
> +	/* w/a for waiting 2 vblanks during crtc enable */
> +	enum pipe hsw_workaround_pipe;
>  };
>  
>  struct intel_pipe_wm {
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 00/22] drm/i915: Convert to atomic, part 2.

[Matt Roper]

On Wed, May 20, 2015 at 03:38:05PM +0200, Maarten Lankhorst wrote:
> The goal of this patch series is to implement hardware readout using
> atomic state, and restore sw state with a single call to intel_set_mode.
> 
> After that's done intel_crtc_control can be safely converted to
> atomic modeset, because nothing relies on transitional state any
> more.
> 
> This patch series slightly reduces some user of crtc->config and
> crtc->active, but doesn't remove it completely.

I've reviewed all the patches here and responded to a few of the
messages; the ones I didn't explicitly respond to looked good and can be
considered to have my r-b tag.

Recap of the patches that I did reply directly to:
 * #8 causes a frontbuffer tracking warning as I mentioned, but that
   warning does go away a on a subsequent patch (I forget exactly which
   one now).  It would be nice to eliminate the warning here for bisect
   purposes.
 * #12 was giving me 'vblank not available on crtc #' warnings, but
   Maarten already identified a fix for that in a different
   email thread; with that fix applied, this patch has my r-b too.
 * #13 just needs some more details in the commit message.
 * #14 is just a cosmetic suggestion; feel free to ignore it if you
   disagree.
 * #15 is just a clarification question; I think we're still behaving
   the same way the original code did, so nothing that would block us
   merging this as-is.
 * #16 might warrant a follow-up patch to actually handle error
   return codes.
 * #20 is just a note about an outdated detail of the commit message.

Anything I didn't explicitly respond to is

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

The comments on #13-16 and #20 are pretty minor and non-blocking, so if
you disagree with the suggestions or if we're in a rush to get these
merged, you can still consider them to have my r-b tag.


Matt

> 
> Ander Conselvan de Oliveira (6):
>   drm/i915: Make __intel_set_mode() take only atomic state as argument
>   drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()
>   drm/i915: Support modeset across multiple pipes
>   drm/i915: Use global atomic state for staged pll config, v2.
>   drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()
>   drm/i915: Read hw state into an atomic state struct
> 
> Maarten Lankhorst (16):
>   drm/i915: get rid of put_shared_dpll
>   drm/i915: get rid of intel_crtc_disable and related code, v2
>   drm/i915: use intel_crtc_control everywhere, v2.
>   drm/i915: Use drm_atomic_helper_update_legacy_modeset_state, v2.
>   drm/i915: Use crtc_state->active instead of crtc_state->enable
>   drm/i915: Zap call to drm_plane_helper_disable.
>   drm/i915: calculate primary visibility changes instead of calling from
>     set_config
>   drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.
>   drm/i915: Swap planes on each crtc separately.
>   drm/i915: Implement intel_crtc_control using atomic state, v3
>   drm/i915: move swap state to the right place
>   drm/i915: Use crtc->hwmode for vblanks, v2.
>   drm/i915: Remove use of crtc->config from i915_debugfs.c
>   drm/i915: Calculate haswell plane workaround, v3.
>   drm/i915: Use atomic state for calculating DVO_2X_MODE on i830.
>   drm/i915: use calculated state for vblank evasion
> 
>  drivers/gpu/drm/i915/i915_debugfs.c  |   50 +-
>  drivers/gpu/drm/i915/i915_drv.h      |    2 -
>  drivers/gpu/drm/i915/i915_irq.c      |   13 +-
>  drivers/gpu/drm/i915/intel_atomic.c  |   96 +--
>  drivers/gpu/drm/i915/intel_display.c | 1154 ++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   32 +-
>  6 files changed, 664 insertions(+), 683 deletions(-)
> 
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 14/22] drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()

[Maarten Lankhorst]

Op 29-05-15 om 02:55 schreef Matt Roper:
> On Wed, May 20, 2015 at 06:04:26PM +0200, maarten.lankhorst@linux.intel.com wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> It makes more sense there, since these are computation steps that can
>> fail.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> I've noticed that a few of the patches in this series were originally
> written by Ander, but seem to be missing his s-o-b line.  I think you
> generally want to just append your line after his in that case.
>
> One other cosmetic note farther down.
>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------
>>  1 file changed, 35 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 81d5358efdde..e7aa8610cbdc 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>>  		crtc->scanline_offset = 1;
>>  }
>>  
>> -static int
>> -intel_modeset_compute_config(struct drm_atomic_state *state)
>> -{
>> -	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> -	int ret, i;
>> -
>> -	ret = drm_atomic_helper_check_modeset(state->dev, state);
>> -	if (ret)
>> -		return ret;
>> -
>> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (!crtc_state->enable &&
>> -		    WARN_ON(crtc_state->active))
>> -			crtc_state->active = false;
>> -
>> -		if (!crtc_state->enable)
>> -			continue;
>> -
>> -		ret = intel_modeset_pipe_config(crtc, state);
>> -		if (ret)
>> -			return ret;
>> -
>> -		intel_dump_pipe_config(to_intel_crtc(crtc),
>> -				       to_intel_crtc_state(crtc_state),
>> -				       "[modeset]");
>> -	}
>> -
>> -	return drm_atomic_helper_check_planes(state->dev, state);
>> -}
>> -
>>  static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>>  {
>>  	struct drm_device *dev = state->dev;
>> @@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>>  	return 0;
>>  }
>>  
>> +static int
>> +intel_modeset_compute_config(struct drm_atomic_state *state)
>> +{
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +	int ret, i;
>> +
>> +	ret = drm_atomic_helper_check_modeset(state->dev, state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		if (!crtc_state->enable &&
>> +		    WARN_ON(crtc_state->active))
>> +			crtc_state->active = false;
>> +
>> +		if (!crtc_state->enable)
>> +			continue;
>> +
>> +		ret = intel_modeset_pipe_config(crtc, state);
>> +		if (ret)
>> +			return ret;
>> +
>> +		intel_dump_pipe_config(to_intel_crtc(crtc),
>> +				       to_intel_crtc_state(crtc_state),
>> +				       "[modeset]");
>> +	}
>> +
>> +	ret = drm_atomic_helper_check_planes(state->dev, state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return __intel_set_mode_checks(state);
> Just a cosmetic note, but maybe we should rename this function now?
> It's not called from __intel_set_mode anymore and it isn't really
> 'checks' (but rather setup that we intend to be done during the check
> phase), so the whole name seems a bit misleading now.
>
Later on intel_modeset_compute_config gets renamed to intel_atomic_check,
and I conditionally run intel_set_mode_checks depending on whether a modeset
is requested or not. I guess __intel_set_mode_checks could be renamed to intel_modeset_checks.

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

Re: [PATCH v3 15/22] drm/i915: Read hw state into an atomic state struct

[Maarten Lankhorst]

Op 29-05-15 om 02:55 schreef Matt Roper:
> On Wed, May 20, 2015 at 06:04:27PM +0200, maarten.lankhorst@linux.intel.com wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> To make this work we load the new hardware state into the
>> atomic_state, then swap it with the sw state.
>>
>> This lets us change the force restore path in setup_hw_state()
>> to use a single call to intel_mode_set() to restore all the
>> previous state.
>>
>> As a nice bonus this kills off encoder->new_encoder,
>> connector->new_enabled and crtc->new_enabled. They were used only
>> to restore the state after a modeset.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>>  drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  14 +-
>>  3 files changed, 241 insertions(+), 152 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 83078763ba14..17bf9e80c557 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>>  	return 0;
>>  }
>>  
>> -static void
>> +void
>>  intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
>>  				  struct intel_shared_dpll_config *shared_dpll)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e7aa8610cbdc..a42e7d7bf86b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9639,7 +9639,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>>  retry:
>>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
>>  	if (ret)
>> -		goto fail_unlock;
>> +		goto fail;
>>  
>>  	/*
>>  	 * Algorithm gets a little messy:
>> @@ -9657,10 +9657,10 @@ retry:
>>  
>>  		ret = drm_modeset_lock(&crtc->mutex, ctx);
>>  		if (ret)
>> -			goto fail_unlock;
>> +			goto fail;
>>  		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>>  		if (ret)
>> -			goto fail_unlock;
>> +			goto fail;
>>  
>>  		old->dpms_mode = connector->dpms;
>>  		old->load_detect_temp = false;
>> @@ -9679,9 +9679,6 @@ retry:
>>  			continue;
>>  		if (possible_crtc->state->enable)
>>  			continue;
>> -		/* This can occur when applying the pipe A quirk on resume. */
>> -		if (to_intel_crtc(possible_crtc)->new_enabled)
>> -			continue;
>>  
>>  		crtc = possible_crtc;
>>  		break;
>> @@ -9692,20 +9689,17 @@ retry:
>>  	 */
>>  	if (!crtc) {
>>  		DRM_DEBUG_KMS("no pipe available for load-detect\n");
>> -		goto fail_unlock;
>> +		goto fail;
>>  	}
>>  
>>  	ret = drm_modeset_lock(&crtc->mutex, ctx);
>>  	if (ret)
>> -		goto fail_unlock;
>> +		goto fail;
>>  	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>>  	if (ret)
>> -		goto fail_unlock;
>> -	intel_encoder->new_crtc = to_intel_crtc(crtc);
>> -	to_intel_connector(connector)->new_encoder = intel_encoder;
>> +		goto fail;
>>  
>>  	intel_crtc = to_intel_crtc(crtc);
>> -	intel_crtc->new_enabled = true;
>>  	old->dpms_mode = connector->dpms;
>>  	old->load_detect_temp = true;
>>  	old->release_fb = NULL;
>> @@ -9773,9 +9767,7 @@ retry:
>>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
>>  	return true;
>>  
>> - fail:
>> -	intel_crtc->new_enabled = crtc->state->enable;
>> -fail_unlock:
>> +fail:
>>  	drm_atomic_state_free(state);
>>  	state = NULL;
>>  
>> @@ -9821,10 +9813,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>>  		if (IS_ERR(crtc_state))
>>  			goto fail;
>>  
>> -		to_intel_connector(connector)->new_encoder = NULL;
>> -		intel_encoder->new_crtc = NULL;
>> -		intel_crtc->new_enabled = false;
>> -
>>  		connector_state->best_encoder = NULL;
>>  		connector_state->crtc = NULL;
>>  
>> @@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
>>  	.atomic_flush = intel_finish_crtc_commit,
>>  };
>>  
>> -/**
>> - * intel_modeset_update_staged_output_state
>> - *
>> - * Updates the staged output configuration state, e.g. after we've read out the
>> - * current hw state.
>> - */
>> -static void intel_modeset_update_staged_output_state(struct drm_device *dev)
>> -{
>> -	struct intel_crtc *crtc;
>> -	struct intel_encoder *encoder;
>> -	struct intel_connector *connector;
>> -
>> -	for_each_intel_connector(dev, connector) {
>> -		connector->new_encoder =
>> -			to_intel_encoder(connector->base.encoder);
>> -	}
>> -
>> -	for_each_intel_encoder(dev, encoder) {
>> -		encoder->new_crtc =
>> -			to_intel_crtc(encoder->base.crtc);
>> -	}
>> -
>> -	for_each_intel_crtc(dev, crtc) {
>> -		crtc->new_enabled = crtc->base.state->enable;
>> -	}
>> -}
>> -
>>  /* Transitional helper to copy current connector/encoder state to
>>   * connector->state. This is needed so that code that is partially
>>   * converted to atomic does the right thing.
>> @@ -11526,7 +11487,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>>  	}
>>  
>>  	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>> -	intel_modeset_update_staged_output_state(state->dev);
>>  
>>  	/* Double check state. */
>>  	for_each_crtc(dev, crtc) {
>> @@ -11832,11 +11792,14 @@ check_connector_state(struct drm_device *dev)
>>  	struct intel_connector *connector;
>>  
>>  	for_each_intel_connector(dev, connector) {
>> +		struct drm_encoder *encoder = connector->base.encoder;
>> +		struct drm_connector_state *state = connector->base.state;
>> +
>>  		/* This also checks the encoder/connector hw state with the
>>  		 * ->get_hw_state callbacks. */
>>  		intel_connector_check_state(connector);
>>  
>> -		I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
>> +		I915_STATE_WARN(state->best_encoder != encoder,
>>  		     "connector's staged encoder doesn't match current encoder\n");
>>  	}
>>  }
>> @@ -11856,8 +11819,6 @@ check_encoder_state(struct drm_device *dev)
>>  			      encoder->base.base.id,
>>  			      encoder->base.name);
>>  
>> -		I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
>> -		     "encoder's stage crtc doesn't match current crtc\n");
>>  		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
>>  		     "encoder's active_connectors set, but no crtc\n");
>>  
>> @@ -11867,6 +11828,9 @@ check_encoder_state(struct drm_device *dev)
>>  			enabled = true;
>>  			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
>>  				active = true;
>> +
>> +			I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc,
>> +			     "encoder's stage crtc doesn't match current crtc\n");
>>  		}
>>  		/*
>>  		 * for MST connectors if we unplug the connector is gone
>> @@ -12296,11 +12260,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>>  	 * need to copy the staged config to the atomic state, otherwise the
>>  	 * mode set will just reapply the state the HW is already in. */
>>  	for_each_intel_encoder(dev, encoder) {
>> -		if (&encoder->new_crtc->base != crtc)
>> +		if (encoder->base.crtc != crtc)
>>  			continue;
>>  
>>  		for_each_intel_connector(dev, connector) {
>> -			if (connector->new_encoder != encoder)
>> +			if (connector->base.state->best_encoder != &encoder->base)
>>  				continue;
>>  
>>  			connector_state = drm_atomic_get_connector_state(state, &connector->base);
>> @@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>>  			}
>>  
>>  			connector_state->crtc = crtc;
>> -			connector_state->best_encoder = &encoder->base;
>>  		}
>>  	}
>>  
>>  	for_each_intel_crtc(dev, intel_crtc) {
>> -		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
>> -			continue;
>> -
>>  		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>>  		if (IS_ERR(crtc_state)) {
>>  			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
>> @@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>>  			continue;
>>  		}
>>  
>> -		crtc_state->base.active = crtc_state->base.enable =
>> -			intel_crtc->new_enabled;
>> -
>>  		if (&intel_crtc->base == crtc)
>>  			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
>>  	}
>> @@ -14552,99 +14509,224 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>>  {
>>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>>  
>> -	if (!crtc->active)
>> +	if (!crtc->base.enabled)
>>  		return false;
>>  
>>  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>>  }
>>  
>> -static void intel_modeset_readout_hw_state(struct drm_device *dev)
>> +static int readout_hw_crtc_state(struct drm_atomic_state *state,
>> +				 struct intel_crtc *crtc)
>>  {
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	enum pipe pipe;
>> -	struct intel_crtc *crtc;
>> -	struct intel_encoder *encoder;
>> -	struct intel_connector *connector;
>> -	int i;
>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>> +	struct intel_crtc_state *crtc_state;
>> +	struct drm_plane *primary = crtc->base.primary;
>> +	struct drm_plane_state *drm_plane_state;
>> +	struct intel_plane_state *plane_state;
>> +	int ret;
>>  
>> -	for_each_intel_crtc(dev, crtc) {
>> -		struct drm_plane *primary = crtc->base.primary;
>> -		struct intel_plane_state *plane_state;
>> +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
>> +	if (IS_ERR(crtc_state))
>> +		return PTR_ERR(crtc_state);
>>  
>> -		memset(crtc->config, 0, sizeof(*crtc->config));
>> -		crtc->config->base.crtc = &crtc->base;
>> +	ret = drm_atomic_add_affected_planes(state, &crtc->base);
>> +	if (ret)
>> +		return ret;
> Do we actually try to handle non-primary planes in this function?  If
> I'm following this properly, at bootup we won't add any sprite or cursor
> planes here, even though it's possible that a graphics firmware has
> turned some of them on.  It seems like our sw and hw states will still
> be a bit inconsistent in that case.
I'm afraid we don't, but probably should. Adding all possible planes might be better here,
even if we don't track the previous hw state it will cause at least everything to be disabled,
another way of making hw and sw state match up. :-)

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

Re: [PATCH v3.6 16/22] drm/i915: Implement intel_crtc_control using atomic state, v4.

[Maarten Lankhorst]

Op 29-05-15 om 02:57 schreef Matt Roper:
> On Tue, May 26, 2015 at 10:35:22AM +0200, Maarten Lankhorst wrote:
>> Assume the callers lock everything with drm_modeset_lock_all.
>>
>> This change had to be done after converting suspend/resume to
>> use atomic_state so the atomic state is preserved, otherwise
>> all transitional state is erased.
>>
>> Now all callers of .crtc_enable and .crtc_disable go through
>> atomic modeset! :-D
>>
>> Changes since v1:
>> - Only check for crtc_state->active in valleyview_modeset_global_pipes.
>> - Only check for crtc_state->active in modeset_update_crtc_power_domains.
>> Changes since v2:
>> - Rework on top of the changed patch order.
>> Changes since v3:
>> - Rename intel_crtc_toggle in description to *_control
>> - Change return value to int.
>> - Do not add plane state, should be done implicitly already.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>  2 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e9680eb85bd0..32bab28432f7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev)
>>  }
>>  
>>  /* Master function to enable/disable CRTC and corresponding power wells */
>> -void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>> +int intel_crtc_control(struct drm_crtc *crtc, bool enable)
> We change the return value to 'int' here, but we never check the error
> code anywhere.  Maybe we should hold this off for another patch and then
> also propagate the return value back up the call chain?  It seems like
> at least some of the connector-specific DPMS functions should recognize
> the failure of intel_crtc_update_dpms() before trying to continue with
> other operations.
Yeah, but I wasn't sure which ones, and handling is a lot harder. So I wanted to
mark this as 'int' to warn future users that this function may fail so they can check.
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	enum intel_display_power_domain domain;
>> -	unsigned long domains;
>> +	struct intel_crtc_state *pipe_config;
>> +	struct drm_atomic_state *state;
>> +	int ret;
>>  
>>  	if (enable == intel_crtc->active)
>> -		return;
>> +		return 0;
>>  
>>  	if (enable && !crtc->state->enable)
>> -		return;
>> +		return 0;
>>  
>> -	crtc->state->active = enable;
>> -	if (enable) {
>> -		domains = get_crtc_power_domains(crtc);
>> -		for_each_power_domain(domain, domains)
>> -			intel_display_power_get(dev_priv, domain);
>> -		intel_crtc->enabled_power_domains = domains;
>> +	/* this function should be called with drm_modeset_lock_all for now */
>> +	if (WARN_ON(!ctx))
>> +		return -EIO;
> EIO seems like an unusual choice here (but I'm not really sure what the
> best option would be).
No idea, I just wanted to make sure that it was recognized as programming error.
-EINVAL Is used everywhere in the atomic path, -ENODEV didn't seem appropriate.
>> +	lockdep_assert_held(&ctx->ww_ctx);
>>  
>> -		dev_priv->display.crtc_enable(crtc);
>> -		intel_crtc_enable_planes(crtc);
>> -	} else {
>> -		intel_crtc_disable_planes(crtc);
>> -		dev_priv->display.crtc_disable(crtc);
>> +	state = drm_atomic_state_alloc(dev);
>> +	if (WARN_ON(!state))
>> +		return -ENOMEM;
>>  
>> -		domains = intel_crtc->enabled_power_domains;
>> -		for_each_power_domain(domain, domains)
>> -			intel_display_power_put(dev_priv, domain);
>> -		intel_crtc->enabled_power_domains = 0;
>> +	state->acquire_ctx = ctx;
>> +	state->allow_modeset = true;
>> +
>> +	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
>> +	if (IS_ERR(pipe_config)) {
>> +		ret = PTR_ERR(pipe_config);
>> +		goto err;
>>  	}
>> +	pipe_config->base.active = enable;
>> +
>> +	ret = intel_set_mode(state);
>> +	if (!ret)
>> +		return ret;
>> +
>> +err:
>> +	DRM_ERROR("Updating crtc active failed with %i\n", ret);
>> +	drm_atomic_state_free(state);
>> +	return ret;
>>  }
>>  
>>  /**
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 60a29ff65f1f..c113f187090f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev);
>>  void intel_mark_idle(struct drm_device *dev);
>>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>>  void intel_display_suspend(struct drm_device *dev);
>> -void intel_crtc_control(struct drm_crtc *crtc, bool enable);
>> +int intel_crtc_control(struct drm_crtc *crtc, bool enable);
>>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>>  void intel_encoder_destroy(struct drm_encoder *encoder);
>>  int intel_connector_init(struct intel_connector *);
>> -- 
>> 2.1.0
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3 00/22] drm/i915: Convert to atomic, part 2.

[Ander Conselvan De Oliveira]

On Wed, 2015-05-20 at 15:38 +0200, Maarten Lankhorst wrote:
> The goal of this patch series is to implement hardware readout using
> atomic state, and restore sw state with a single call to intel_set_mode.
> 
> After that's done intel_crtc_control can be safely converted to
> atomic modeset, because nothing relies on transitional state any
> more.
> 
> This patch series slightly reduces some user of crtc->config and
> crtc->active, but doesn't remove it completely.
> 
> Ander Conselvan de Oliveira (6):
>   drm/i915: Make __intel_set_mode() take only atomic state as argument
>   drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()
>   drm/i915: Support modeset across multiple pipes
>   drm/i915: Use global atomic state for staged pll config, v2.
>   drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()
>   drm/i915: Read hw state into an atomic state struct

For the above, feel free to add

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>


Ander


> Maarten Lankhorst (16):
>   drm/i915: get rid of put_shared_dpll
>   drm/i915: get rid of intel_crtc_disable and related code, v2
>   drm/i915: use intel_crtc_control everywhere, v2.
>   drm/i915: Use drm_atomic_helper_update_legacy_modeset_state, v2.
>   drm/i915: Use crtc_state->active instead of crtc_state->enable
>   drm/i915: Zap call to drm_plane_helper_disable.
>   drm/i915: calculate primary visibility changes instead of calling from
>     set_config
>   drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.
>   drm/i915: Swap planes on each crtc separately.
>   drm/i915: Implement intel_crtc_control using atomic state, v3
>   drm/i915: move swap state to the right place
>   drm/i915: Use crtc->hwmode for vblanks, v2.
>   drm/i915: Remove use of crtc->config from i915_debugfs.c
>   drm/i915: Calculate haswell plane workaround, v3.
>   drm/i915: Use atomic state for calculating DVO_2X_MODE on i830.
>   drm/i915: use calculated state for vblank evasion
> 
>  drivers/gpu/drm/i915/i915_debugfs.c  |   50 +-
>  drivers/gpu/drm/i915/i915_drv.h      |    2 -
>  drivers/gpu/drm/i915/i915_irq.c      |   13 +-
>  drivers/gpu/drm/i915/intel_atomic.c  |   96 +--
>  drivers/gpu/drm/i915/intel_display.c | 1154 ++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   32 +-
>  6 files changed, 664 insertions(+), 683 deletions(-)
> 


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