[PATCH v4 00/27] drm/i915: Convert to atomic, part 2.

[PATCH v4 00/27] 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 uses of crtc->config and
crtc->active, but doesn't remove it completely.

Changes since previous versions are noted in each commit, but roughly:
 - Add intel_display_suspend helper function.
 - Move 'Zap call to drm_plane_helper_disable' to some place later
   where it doesn't introduce warnings.
 - Add some more patches to silence some warnings:
   + drm/i915: Make sure all planes and connectors are added on modeset.
   + drm/i915: update plane state during init
   + drm/i915: do not wait for vblank when crtc is off
 - Make sure plane_mask is kept closer to reality, so plane disabling
   will work correctly later on.

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(), v2.
  drm/i915: Read hw state into an atomic state struct, v2.

Maarten Lankhorst (21):
  drm/i915: get rid of put_shared_dpll
  drm/i915: get rid of intel_crtc_disable and related code, v3
  drm/i915: add intel_display_suspend, v2
  drm/i915: use intel_crtc_control everywhere, v3.
  drm/i915: Use drm_atomic_helper_update_legacy_modeset_state, v2.
  drm/i915: Use crtc_state->active instead of crtc_state->enable
  drm/i915: Make sure all planes and connectors are added on modeset.
  drm/i915: update plane state during init
  drm/i915: do not wait for vblank when crtc is off
  drm/i915: calculate primary visibility changes instead of calling from set_config
  drm/i915: Zap call to drm_plane_helper_disable, v2.
  drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.
  drm/i915: Swap planes on each crtc separately, v2.
  drm/i915: Implement intel_crtc_control using atomic state, v4
  drm/i915: Make intel_display_suspend atomic, v2.
  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, v5.
  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.c      |    7 +-
 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 | 1248 +++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h     |   35 +-
 7 files changed, 744 insertions(+), 707 deletions(-)

-- 
2.1.0

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

[PATCH v4 01/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 16e159db5025..a5de28defcfd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4199,27 +4199,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)
 {
@@ -5206,13 +5185,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;
@@ -14512,7 +14484,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)) {
@@ -14523,7 +14495,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)) {
@@ -14534,7 +14506,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 2afb31a46275..48857f20e494 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1083,7 +1083,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 v4 02/27] drm/i915: get rid of intel_crtc_disable and related code, v3

[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.
Changes since v3:
- Rebase on top of latest drm-intel.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/intel_display.c | 93 +++++++++---------------------------
 2 files changed, 23 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 60aa9626f91f..862bb9d4b08d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -631,7 +631,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 a5de28defcfd..547ea588fa27 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6224,10 +6224,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)
 {
@@ -6277,34 +6273,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);
@@ -12020,26 +11988,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)
-				continue;
-
-			if (crtc_state->enable && needs_modeset(crtc_state))
-				intel_encoder->connectors_active = false;
+		crtc = intel_encoder->base.crtc;
+		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+		if (!crtc_state || !needs_modeset(crtc->state))
+			continue;
 
-			break;
-		}
+		intel_encoder->connectors_active = false;
 	}
 
-	drm_atomic_helper_swap_state(state->dev, state);
 	intel_modeset_fixup_state(state);
 
 	/* Double check state. */
@@ -12051,27 +12015,23 @@ 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)
-				continue;
-
-			if (crtc->state->enable && needs_modeset(crtc->state)) {
-				struct drm_property *dpms_property =
-					dev->mode_config.dpms_property;
+		crtc = connector->encoder->crtc;
+		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+		if (!crtc_state || !needs_modeset(crtc->state))
+			continue;
 
-				connector->dpms = DRM_MODE_DPMS_ON;
-				drm_object_property_set_value(&connector->base,
-								 dpms_property,
-								 DRM_MODE_DPMS_ON);
+		if (crtc->state->enable) {
+			struct drm_property *dpms_property =
+				dev->mode_config.dpms_property;
 
-				intel_encoder = to_intel_encoder(connector->encoder);
-				intel_encoder->connectors_active = true;
-			}
+			connector->dpms = DRM_MODE_DPMS_ON;
+			drm_object_property_set_value(&connector->base, dpms_property, DRM_MODE_DPMS_ON);
 
-			break;
-		}
+			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)
@@ -12745,12 +12705,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
@@ -14484,7 +14442,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)) {
@@ -14495,7 +14452,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)) {
@@ -14506,7 +14462,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)) {
@@ -14516,7 +14471,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 {
@@ -14526,7 +14480,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 v4 03/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 a051a0241883..78ef0bb53c36 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -601,7 +601,6 @@ static int bxt_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;
 
@@ -632,8 +631,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 547ea588fa27..ac394e795d10 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3190,9 +3190,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;
@@ -3207,13 +3204,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)
@@ -6224,6 +6215,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 48857f20e494..396354706f9e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -993,6 +993,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 v4 04/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 3e17210c3277..9a6159dd1972 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3628,12 +3628,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);
 }
@@ -3652,12 +3658,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 ac394e795d10..a41938165d04 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3175,19 +3175,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 */
@@ -3199,7 +3186,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.
@@ -6251,26 +6237,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;
 	}
 }
 
@@ -6287,8 +6276,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)
@@ -14983,8 +14970,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 396354706f9e..b9082fd16792 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -995,7 +995,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 v4 05/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 a41938165d04..8e4f93411fc1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11485,43 +11485,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)
@@ -12009,11 +11972,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) {
@@ -12716,25 +12682,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 v4 06/27] 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: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 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 8e4f93411fc1..b01f4b08aca5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12653,12 +12653,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;
@@ -12718,7 +12716,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 v4 07/27] 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: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 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 b01f4b08aca5..b9f071ead7ed 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
  */
@@ -11833,6 +11839,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 =
@@ -11924,6 +11939,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;
@@ -11941,12 +11968,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)
 {
@@ -12533,10 +12554,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);
@@ -12558,19 +12575,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 v4 08/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 eb52a039d628..dadd586f0527 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 b9f071ead7ed..5645bfb08776 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4629,7 +4629,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)) {
@@ -4845,9 +4845,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)
@@ -4951,9 +4949,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))
@@ -5055,7 +5051,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)
@@ -5118,7 +5114,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) {
@@ -5924,7 +5920,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);
@@ -5934,7 +5930,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;
@@ -6022,9 +6018,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);
@@ -6100,9 +6094,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);
@@ -6162,7 +6154,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;
 
 	/*
@@ -12012,7 +12004,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;
 
@@ -12427,6 +12419,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");
@@ -12568,6 +12564,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;
 
@@ -12676,7 +12676,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);
@@ -12698,7 +12698,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));
@@ -14958,7 +14958,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 v4 09/27] drm/i915: Make sure all planes and connectors are added on modeset.

[Maarten Lankhorst]

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>
---
 drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5645bfb08776..f93f9f3f137f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5905,7 +5905,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;
@@ -5920,20 +5920,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)
@@ -11933,8 +11938,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
@@ -11942,8 +11949,6 @@ encoder_retry:
 	 * requirements.  So here we should be checking for any
 	 * required changes and forcing a mode set.
 	 */
-
-	return 0;
 fail:
 	return ret;
 }
-- 
2.1.0

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

[PATCH v4 10/27] drm/i915: update plane state during init

[Maarten Lankhorst]

Atomic planes updates rely on having a accurate plane_mask.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f93f9f3f137f..3b36e1f24c4e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2632,9 +2632,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);
 }
 
@@ -15306,7 +15306,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));
 		}
 	}
 
-- 
2.1.0

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

[PATCH v4 11/27] drm/i915: do not wait for vblank when crtc is off

[Maarten Lankhorst]

This can happen when turning off a sprite plane. Because the crtc state
is not yet always swapped correctly and transitional helpers are used
the crtc state cannot be relied on.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b36e1f24c4e..93ac7c29f210 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13510,7 +13510,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_runtime_pm_put(dev_priv);
 
-	if (intel_crtc->atomic.wait_vblank)
+	if (intel_crtc->atomic.wait_vblank && intel_crtc->active)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
-- 
2.1.0

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

[PATCH v4 12/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 93ac7c29f210..c79ff10db43e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12969,20 +12969,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);
@@ -13021,38 +13012,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);
@@ -13383,8 +13344,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 v4 13/27] 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: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 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 c79ff10db43e..4fbff2987d2e 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,
@@ -10230,7 +10229,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);
@@ -10304,7 +10303,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;
 
@@ -11816,9 +11815,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;
@@ -11836,6 +11836,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.
 	 */
@@ -12548,45 +12554,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)
@@ -12721,37 +12717,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)
@@ -12823,7 +12809,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);
 }
@@ -12973,7 +12959,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);
@@ -13004,16 +12989,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 v4 14/27] drm/i915: Zap call to drm_plane_helper_disable, v2.

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

Changes since v1:
- Move commit so no frontbuffer_bits warnings are generated.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 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 4fbff2987d2e..3ec324651b8a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12682,8 +12682,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 v4 15/27] 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: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/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 862bb9d4b08d..0cdc81c0e6e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -376,7 +376,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 3ec324651b8a..3538a1059db0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4186,8 +4186,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;
@@ -4196,7 +4199,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;
 	}
@@ -4216,7 +4219,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;
 	}
@@ -4225,15 +4228,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;
 		}
@@ -4242,7 +4245,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;
@@ -4252,83 +4255,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];
 	}
 }
 
@@ -11975,13 +11928,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) {
@@ -12610,9 +12562,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)
@@ -12623,13 +12579,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;
 }
 
@@ -14324,6 +14277,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 b9082fd16792..ddae28ac4e6f 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;
@@ -628,6 +635,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)
@@ -1405,6 +1413,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 v4 16/27] drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.

[Maarten Lankhorst]

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>
Reviewed-by: Matt Roper <matthew.d.roper@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 v4 17/27] drm/i915: Swap planes on each crtc separately, v2.

[Maarten Lankhorst]

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.

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 crtc->config->base.crtc update may seem unrelated, but the helper
will use it to obtain the crtc for the state. Without the update it
will dereference NULL and crash.

Changes since v1:
- Use Matt Roper's commit message.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@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 3538a1059db0..b1457a53b2b5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12646,10 +12646,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;
 
@@ -15010,6 +15010,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 v4 18/27] drm/i915: Move cdclk and pll setup to intel_modeset_compute_config(), v2.

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

Changes since v1:
- Rename __intel_set_mode_checks to intel_modeset_checks (Matt Roper)
- Move intel_modeset_checks to before check_planes, so it won't
  have to be moved later.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1457a53b2b5..a134d83b15ca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12506,38 +12506,7 @@ 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)
+static int intel_modeset_setup_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -12587,7 +12556,7 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 }
 
 /* Code that should eventually be part of atomic_check() */
-static int __intel_set_mode_checks(struct drm_atomic_state *state)
+static int intel_modeset_checks(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	int ret;
@@ -12605,11 +12574,42 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 			return ret;
 	}
 
-	ret = __intel_set_mode_setup_plls(state);
+	return intel_modeset_setup_plls(state);
+}
+
+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;
 
-	return 0;
+	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 = intel_modeset_checks(state);
+	if (ret)
+		return ret;
+
+	return drm_atomic_helper_check_planes(state->dev, state);
 }
 
 static int __intel_set_mode(struct drm_atomic_state *state)
@@ -12621,10 +12621,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 v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

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

Changes since v1:
- Make sure all possible planes are added with their crtc set,
  so they will be turned off on first 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 | 379 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 243 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 a134d83b15ca..816f509b577a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10060,7 +10060,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:
@@ -10078,10 +10078,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;
@@ -10100,9 +10100,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;
@@ -10113,20 +10110,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;
@@ -10194,9 +10188,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;
 
@@ -10242,10 +10234,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;
 
@@ -11392,33 +11380,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.
@@ -11949,7 +11910,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) {
@@ -12253,11 +12213,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");
 	}
 }
@@ -12277,8 +12240,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");
 
@@ -12288,6 +12249,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
@@ -12713,11 +12677,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);
@@ -12730,14 +12694,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",
@@ -12746,9 +12706,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);
 	}
@@ -14823,6 +14780,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * ...  */
 		plane = crtc->plane;
 		to_intel_plane_state(crtc->base.primary->state)->visible = true;
+		crtc->base.primary->crtc = &crtc->base;
 		crtc->plane = !plane;
 		intel_crtc_control(&crtc->base, false);
 		crtc->plane = plane;
@@ -14986,99 +14944,225 @@ 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) {
+		primary->crtc = &crtc->base;
+		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
@@ -15087,37 +15171,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]");
 	}
 
@@ -15141,20 +15245,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 ddae28ac4e6f..01830532b33e 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 *);
@@ -535,7 +524,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;
@@ -1417,6 +1405,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 v4 20/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 816f509b577a..c2767d02a2c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6184,38 +6184,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 01830532b33e..889ce0ef40c3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -990,7 +990,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 v4 21/27] 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>
Reviewed-by: Matt Roper <matthew.d.roper@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 78ef0bb53c36..d3632c56fdf7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -634,6 +634,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 c2767d02a2c6..38c189d6cefd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6160,27 +6160,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 889ce0ef40c3..5ae6d713b16f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -989,7 +989,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 v4 22/27] drm/i915: move swap state to the right place

[Maarten Lankhorst]

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>
Reviewed-by: Matt Roper <matthew.d.roper@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 38c189d6cefd..5b98a7aacfd3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11937,7 +11937,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)
@@ -12631,8 +12630,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 v4 23/27] drm/i915: Use crtc->hwmode for vblanks, v2.

[Maarten Lankhorst]

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>
Reviewed-by: Matt Roper <matthew.d.roper@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 dadd586f0527..56db9e747464 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 5b98a7aacfd3..00c7860ede3a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11957,6 +11957,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 v4 24/27] drm/i915: Remove use of crtc->config from i915_debugfs.c

[Maarten Lankhorst]

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>
Reviewed-by: Matt Roper <matthew.d.roper@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 9a6159dd1972..51fe0fbcd00c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2777,13 +2777,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);
@@ -3024,7 +3027,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);
@@ -3076,7 +3079,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);
 
@@ -3618,22 +3621,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));
@@ -3649,6 +3657,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);
 	/*
@@ -3657,13 +3666,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));
@@ -3785,7 +3797,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 v4 25/27] drm/i915: Calculate haswell plane workaround, v5.

[Maarten Lankhorst]

This needs to be done last after all modesets have been calculated.

A modeset first disables all crtc's, so any crtc that undergoes a
modeset counts as inactive.

If no modeset's done, or > 1 crtc's stay w/a doesn't apply.
Apply workaround on the first crtc if 1 crtc stays active.
Apply workaround on the second crtc if no crtc was active.

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.
Changes since v4:
- Clean up commit message.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 2 files changed, 84 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00c7860ede3a..bc7e8c8a3d43 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4864,42 +4864,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;
@@ -4976,7 +4949,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)
@@ -12566,6 +12543,71 @@ static int intel_modeset_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_modeset_checks(struct drm_atomic_state *state)
 {
@@ -12585,7 +12627,14 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 			return ret;
 	}
 
-	return intel_modeset_setup_plls(state);
+	ret = intel_modeset_setup_plls(state);
+	if (ret)
+		return ret;
+
+	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 5ae6d713b16f..807e001e4cee 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 v4 26/27] drm/i915: Use atomic state for calculating DVO_2X_MODE on i830.

[Maarten Lankhorst]

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>
Reviewed-by: Matt Roper <matthew.d.roper@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 bc7e8c8a3d43..e649662acd18 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 v4 27/27] drm/i915: use calculated state for vblank evasion

[Maarten Lankhorst]

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 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 e649662acd18..fb9f07b1e5ca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13400,6 +13400,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;
@@ -13443,7 +13444,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

Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Matt Roper]

On Mon, Jun 01, 2015 at 12:50:03PM +0200, Maarten Lankhorst 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.
> 
> Changes since v1:
> - Make sure all possible planes are added with their crtc set,
>   so they will be turned off on first modeset.

Correct me if I'm wrong, but I don't think we're actually doing this
yet.  We're using drm_atomic_add_affected_planes() which looks at the
current (and possibly incomplete) software state to figure out what an
'affected' plane is.  If pre-driver firmware has enabled a sprite or
cursor plane, that's not going to be reflected in our state's
plane_mask fields, so those planes won't be added by the helper.

I think what you want is an i915-specific loop that operates over *all*
drm_plane's and compares intel_plane->pipe to intel_crtc->pipe to
determine which ones need to be pulled into the state.  Then the
sprites/cursors will be automatically turned off on the first modeset,
ensuring our hardware and software state stay in sync.

The other alternative is to actually look at the hardware state for each
sprite/cursor and try to inherit their real state as we do for the
primary plane.  But I'm not sure it's worth putting in that effort right
now.


Matt

> 
> 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 | 379 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |  14 +-
>  3 files changed, 243 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 a134d83b15ca..816f509b577a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10060,7 +10060,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:
> @@ -10078,10 +10078,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;
> @@ -10100,9 +10100,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;
> @@ -10113,20 +10110,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;
> @@ -10194,9 +10188,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;
>  
> @@ -10242,10 +10234,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;
>  
> @@ -11392,33 +11380,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.
> @@ -11949,7 +11910,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) {
> @@ -12253,11 +12213,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");
>  	}
>  }
> @@ -12277,8 +12240,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");
>  
> @@ -12288,6 +12249,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
> @@ -12713,11 +12677,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);
> @@ -12730,14 +12694,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",
> @@ -12746,9 +12706,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);
>  	}
> @@ -14823,6 +14780,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 * ...  */
>  		plane = crtc->plane;
>  		to_intel_plane_state(crtc->base.primary->state)->visible = true;
> +		crtc->base.primary->crtc = &crtc->base;
>  		crtc->plane = !plane;
>  		intel_crtc_control(&crtc->base, false);
>  		crtc->plane = plane;
> @@ -14986,99 +14944,225 @@ 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) {
> +		primary->crtc = &crtc->base;
> +		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
> @@ -15087,37 +15171,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]");
>  	}
>  
> @@ -15141,20 +15245,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 ddae28ac4e6f..01830532b33e 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 *);
> @@ -535,7 +524,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;
> @@ -1417,6 +1405,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 v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Maarten Lankhorst]

Op 01-06-15 om 23:47 schreef Matt Roper:
> On Mon, Jun 01, 2015 at 12:50:03PM +0200, Maarten Lankhorst 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.
>>
>> Changes since v1:
>> - Make sure all possible planes are added with their crtc set,
>>   so they will be turned off on first modeset.
> Correct me if I'm wrong, but I don't think we're actually doing this
> yet.  We're using drm_atomic_add_affected_planes() which looks at the
> current (and possibly incomplete) software state to figure out what an
> 'affected' plane is.  If pre-driver firmware has enabled a sprite or
> cursor plane, that's not going to be reflected in our state's
> plane_mask fields, so those planes won't be added by the helper.
>
> I think what you want is an i915-specific loop that operates over *all*
> drm_plane's and compares intel_plane->pipe to intel_crtc->pipe to
> determine which ones need to be pulled into the state.  Then the
> sprites/cursors will be automatically turned off on the first modeset,
> ensuring our hardware and software state stay in sync.
>
> The other alternative is to actually look at the hardware state for each
> sprite/cursor and try to inherit their real state as we do for the
> primary plane.  But I'm not sure it's worth putting in that effort right
> now.
All these interdepencies make it hard. I think what I'm doing here is no worse than what we were
doing previously, and cursors will still be unconditionally disabled until part 3 of converting to atomic.

So for now this is good enough, but in part 3 I will have to resolve this issue, I'll work on it today.

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

Re: [PATCH v4 15/27] drm/i915: Use global atomic state for staged pll config, v2.

[Jani Nikula]

On Mon, 01 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> 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: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/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 862bb9d4b08d..0cdc81c0e6e1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -376,7 +376,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));

The kzalloc parameters are the wrong way around.

Sparse would've told you:

  CHECK   drivers/gpu/drm/i915/intel_atomic.c
drivers/gpu/drm/i915/intel_atomic.c:459:52: warning: incorrect type in argument 1 (different base types)
drivers/gpu/drm/i915/intel_atomic.c:459:52:    expected unsigned long [unsigned] [usertype] size
drivers/gpu/drm/i915/intel_atomic.c:459:52:    got restricted gfp_t
drivers/gpu/drm/i915/intel_atomic.c:459:64: warning: incorrect type in argument 2 (different base types)
drivers/gpu/drm/i915/intel_atomic.c:459:64:    expected restricted gfp_t [usertype] flags
drivers/gpu/drm/i915/intel_atomic.c:459:64:    got unsigned long

BR,
Jani.


> +
> +	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 3ec324651b8a..3538a1059db0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4186,8 +4186,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;
> @@ -4196,7 +4199,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;
>  	}
> @@ -4216,7 +4219,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;
>  	}
> @@ -4225,15 +4228,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;
>  		}
> @@ -4242,7 +4245,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;
> @@ -4252,83 +4255,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];
>  	}
>  }
>  
> @@ -11975,13 +11928,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) {
> @@ -12610,9 +12562,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)
> @@ -12623,13 +12579,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;
>  }
>  
> @@ -14324,6 +14277,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 b9082fd16792..ddae28ac4e6f 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;
> @@ -628,6 +635,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)
> @@ -1405,6 +1413,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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 14/27] drm/i915: Zap call to drm_plane_helper_disable, v2.

[Jani Nikula]

On Mon, 01 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> 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.
>
> Changes since v1:
> - Move commit so no frontbuffer_bits warnings are generated.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Pushed patches 1-14 to drm-intel-next-queued. Thanks for the patches and
review.

BR,
Jani.


> ---
>  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 4fbff2987d2e..3ec324651b8a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12682,8 +12682,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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4.1 01/13] drm/i915: Use global atomic state for staged pll, config, v3.

[Maarten Lankhorst]

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.
Changes since v2:
- Fix swapped arguments to kzalloc for intel_atomic_state_alloc. (Jani Nikula)

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
Oops, thanks for catching it, can you apply rest of the series too?

 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 862bb9d4b08d..0cdc81c0e6e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -376,7 +376,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..45af3cc9d04c 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(sizeof(*state), GFP_KERNEL);
+
+	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 3ec324651b8a..3538a1059db0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4186,8 +4186,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;
@@ -4196,7 +4199,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;
 	}
@@ -4216,7 +4219,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;
 	}
@@ -4225,15 +4228,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;
 		}
@@ -4242,7 +4245,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;
@@ -4252,83 +4255,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];
 	}
 }
 
@@ -11975,13 +11928,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) {
@@ -12610,9 +12562,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)
@@ -12623,13 +12579,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;
 }
 
@@ -14324,6 +14277,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 b9082fd16792..ddae28ac4e6f 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;
@@ -628,6 +635,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)
@@ -1405,6 +1413,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

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

[Jani Nikula]

On Mon, 01 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> 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 uses of crtc->config and
> crtc->active, but doesn't remove it completely.
>
> Changes since previous versions are noted in each commit, but roughly:
>  - Add intel_display_suspend helper function.
>  - Move 'Zap call to drm_plane_helper_disable' to some place later
>    where it doesn't introduce warnings.
>  - Add some more patches to silence some warnings:
>    + drm/i915: Make sure all planes and connectors are added on modeset.
>    + drm/i915: update plane state during init
>    + drm/i915: do not wait for vblank when crtc is off
>  - Make sure plane_mask is kept closer to reality, so plane disabling
>    will work correctly later on.

All patches pushed to drm-intel-next-queued, thanks for the patches and
review.

BR,
Jani.


>
> 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(), v2.
>   drm/i915: Read hw state into an atomic state struct, v2.
>
> Maarten Lankhorst (21):
>   drm/i915: get rid of put_shared_dpll
>   drm/i915: get rid of intel_crtc_disable and related code, v3
>   drm/i915: add intel_display_suspend, v2
>   drm/i915: use intel_crtc_control everywhere, v3.
>   drm/i915: Use drm_atomic_helper_update_legacy_modeset_state, v2.
>   drm/i915: Use crtc_state->active instead of crtc_state->enable
>   drm/i915: Make sure all planes and connectors are added on modeset.
>   drm/i915: update plane state during init
>   drm/i915: do not wait for vblank when crtc is off
>   drm/i915: calculate primary visibility changes instead of calling from set_config
>   drm/i915: Zap call to drm_plane_helper_disable, v2.
>   drm/i915: Use drm_atomic_helper_swap_state in intel_atomic_commit.
>   drm/i915: Swap planes on each crtc separately, v2.
>   drm/i915: Implement intel_crtc_control using atomic state, v4
>   drm/i915: Make intel_display_suspend atomic, v2.
>   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, v5.
>   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.c      |    7 +-
>  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 | 1248 +++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h     |   35 +-
>  7 files changed, 744 insertions(+), 707 deletions(-)
>
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Tvrtko Ursulin]

On 06/01/2015 11:50 AM, Maarten Lankhorst 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.
>
> Changes since v1:
> - Make sure all possible planes are added with their crtc set,
>    so they will be turned off on first modeset.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This breaks display for me, which is eDP on SKL. At least bisect points 
to it. A lot of these in the logs:

*ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)

And the display does not light up.

Regards,

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

Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Maarten Lankhorst]

Hey,

Op 09-06-15 om 13:48 schreef Tvrtko Ursulin:
>
> On 06/01/2015 11:50 AM, Maarten Lankhorst 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.
>>
>> Changes since v1:
>> - Make sure all possible planes are added with their crtc set,
>>    so they will be turned off on first modeset.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> This breaks display for me, which is eDP on SKL. At least bisect points to it. A lot of these in the logs:
>
> *ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)
>
> And the display does not light up.

Yeah, it probably relies on better hw readout. This is partially mitigated by convert to atomic, part 3.
But it requires 2 additional patches.

commit 5a97529becb25fabf18a3507a94f892c365a4a1d
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Mon Jun 8 11:31:28 2015 +0200

    drm/i915: update more sw state with hw state during atomic readout
    
    I've noticed the following during initial readout:
    state->adjusted_mode's non crtc_* members were not set,
    but some code relies hdisplay and vdisplay, so make sure it's
    set correctly.
    
    Also vblank was not enabled because constants were not calculated,
    this shows up in dmesg as:
    [drm:drm_vblank_enable] enabling vblank on crtc 0, ret: 0
    [drm:drm_calc_vbltimestamp_from_scanoutpos] crtc 0: Noop due to uninitialized mode.
    
    This commit fixes the regression from the following commit:
    
    commit 3bae26eb2991c00670df377cf6c3bc2b0577e82a
    Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
    Date:   Mon Jun 1 12:50:03 2015 +0200
    
        drm/i915: Read hw state into an atomic state struct, v2.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90861
    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 fb9f07b1e5ca..dc29bab527d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14859,8 +14859,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 	/* restore vblank interrupts to correct state */
 	drm_crtc_vblank_reset(&crtc->base);
-	if (crtc->active) {
+	if (crtc->base.state->active) {
 		update_scanline_offset(crtc);
+		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
 		drm_crtc_vblank_on(&crtc->base);
 	}
 
@@ -15307,6 +15308,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		if (crtc->enabled) {
 			intel_mode_from_pipe_config(&crtc->state->mode,
 				to_intel_crtc_state(crtc->state));
+			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
+				to_intel_crtc_state(crtc->state));
 
 			drm_mode_copy(&crtc->mode, &crtc->state->mode);
 			drm_mode_copy(&crtc->hwmode,

commit d0f7e7ae8a151e9d018e2dbf36a5afba812bab4f
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue Jun 9 09:01:17 2015 +0200

    drm/i915: only perform a single modeset in intel_modeset_setup_hw_state

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 29ae92e5c8a9..77a553e21a7a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11994,24 +11994,35 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
 static bool
 intel_pipe_config_compare(struct drm_device *dev,
 			  struct intel_crtc_state *current_config,
-			  struct intel_crtc_state *pipe_config)
+			  struct intel_crtc_state *pipe_config,
+			  bool check_only)
 {
+	bool ret = true;
+
+#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
+	do { \
+		if (check_only) \
+			DRM_ERROR(fmt, ##__VA_ARGS__); \
+		else \
+			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
+	} while (0)
+
 #define PIPE_CONF_CHECK_X(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected 0x%08x, found 0x%08x)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_I(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 /* This is required for BDW+ where there is only one set of registers for
@@ -12022,30 +12033,30 @@ intel_pipe_config_compare(struct drm_device *dev,
 #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
 	if ((current_config->name != pipe_config->name) && \
 		(current_config->alt_name != pipe_config->name)) { \
-			DRM_ERROR("mismatch in " #name " " \
+			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 				  "(expected %i or %i, found %i)\n", \
 				  current_config->name, \
 				  current_config->alt_name, \
 				  pipe_config->name); \
-			return false; \
+			ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
 	if ((current_config->name ^ pipe_config->name) & (mask)) { \
-		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") "	   \
 			  "(expected %i, found %i)\n", \
 			  current_config->name & (mask), \
 			  pipe_config->name & (mask)); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
 	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_QUIRK(quirk)	\
@@ -12179,8 +12190,9 @@ intel_pipe_config_compare(struct drm_device *dev,
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
 #undef PIPE_CONF_QUIRK
+#undef INTEL_ERR_OR_DBG_KMS
 
-	return true;
+	return ret;
 }
 
 static void check_wm_state(struct drm_device *dev)
@@ -12377,7 +12389,7 @@ check_crtc_state(struct drm_device *dev)
 		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
 
 		if (active &&
-		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
+		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
 			intel_dump_pipe_config(crtc, &pipe_config,
 					       "[hw state]");
@@ -12734,6 +12746,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 			intel_crtc->active = false;
 			intel_disable_shared_dpll(intel_crtc);
 		}
+
+		/* FIXME: Move this to i9xx_crtc_disable when it gets a pointer
+		 * to the old crtc_state. */
+		if (to_intel_crtc_state(crtc_state)->quirks &
+		    PIPE_CONFIG_QUIRK_WRONG_PLANE)
+			intel_crtc->plane = !intel_crtc->plane;
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -14464,13 +14482,16 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
 }
 
 static void intel_sanitize_crtc(struct intel_crtc *crtc,
-				struct intel_crtc_state *pipe_config)
+				struct intel_crtc_state *pipe_config,
+				bool force_restore)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
+	struct drm_atomic_state *state = pipe_config->base.state;
+	struct intel_crtc_state *hw_state =
+		to_intel_crtc_state(crtc->base.state);
 	u32 reg;
-	bool enable;
 
 	/* Clear any frame start delays used for debugging left by the BIOS */
 	reg = PIPECONF(crtc->config->cpu_transcoder);
@@ -14484,28 +14505,64 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		drm_crtc_vblank_on(&crtc->base);
 	}
 
+	/* set up current state */
+	if (!force_restore && hw_state->base.active) {
+		bool enable;
+
+		memcpy(pipe_config, hw_state, sizeof(*pipe_config));
+		__drm_atomic_helper_crtc_duplicate_state(&crtc->base, &pipe_config->base);
+		pipe_config->base.state = state;
+
+		enable = false;
+		for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
+			enable |= intel_encoder->connectors_active;
+
+		pipe_config->base.active = !!enable;
+	}
+
 	/* We need to sanitize the plane -> pipe mapping first because this will
 	 * disable the crtc (and hence change the state) if it is wrong. Note
 	 * that gen4+ has a fixed plane -> pipe mapping.  */
 	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
-		bool plane;
-
 		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
 			      crtc->base.base.id);
 
 		/* Pipe has the wrong plane attached and the plane is active.
 		 * Temporarily change the plane mapping and disable everything
 		 * ...  */
-		plane = crtc->plane;
-		to_intel_plane_state(crtc->base.primary->state)->visible = true;
-		crtc->base.primary->crtc = &crtc->base;
-		crtc->plane = !plane;
-		intel_crtc_control(&crtc->base, false, true);
-		crtc->plane = plane;
+		hw_state->quirks |=
+			PIPE_CONFIG_QUIRK_WRONG_PLANE;
+
+		crtc->plane = !crtc->plane;
+
+		if (force_restore)
+			pipe_config->base.mode_changed = true;
+		else
+			pipe_config->base.active = false;
 	}
 
-	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
-	    crtc->pipe == PIPE_A && (!pipe_config || !pipe_config->base.active)) {
+	/* XXX: This is not active right now */
+	if (hw_state->base.active && pipe_config->base.active &&
+	    !i915.fastboot) {
+		struct intel_crtc_state sw_state;
+
+		memset(&sw_state, 0, sizeof(sw_state));
+		sw_state.base = pipe_config->base;
+		sw_state.scaler_state = pipe_config->scaler_state;
+		sw_state.shared_dpll = pipe_config->shared_dpll;
+		sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
+		sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
+
+		intel_modeset_pipe_config(&crtc->base, &sw_state);
+
+		/* Check if we need to force a modeset */
+		if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true))
+			pipe_config->base.mode_changed = true;
+	}
+
+
+	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
+	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
 		/* BIOS forgot to enable pipe A, this mostly happens after
 		 * resume. Force-enable the pipe to fix this, the update_dpms
 		 * call below we restore the pipe to the right state, but leave
@@ -14513,19 +14570,24 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		intel_enable_pipe_a(dev);
 	}
 
-	/* Adjust the state of the output pipe according to whether we
-	 * have active connectors/encoders */
-	enable = false;
-	for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
-		enable |= intel_encoder->connectors_active;
+	/* not restoring state, kill off all connectors and disable this thing */
+	if (!force_restore && !pipe_config->base.active) {
+		struct drm_connector_state *conn_state;
+		struct drm_connector *connector;
+		int i, ret;
 
-	/* only turn off separately if configuration's not restored,
-	 * if it's restored it will change mode or turn off anyway.
-	 */
-	if (!enable && crtc->base.state->active && !pipe_config)
-		intel_crtc_control(&crtc->base, false, true);
+		ret = drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL);
+
+		for_each_connector_in_state(state, connector, conn_state, i) {
+			if (conn_state->crtc != &crtc->base)
+				continue;
 
-	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
+			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+			WARN_ON(ret);
+		}
+	}
+
+	if (hw_state->base.active || HAS_GMCH_DISPLAY(dev)) {
 		/*
 		 * We start out with underrun reporting disabled to avoid races.
 		 * For correct bookkeeping mark this on active crtcs.
@@ -14581,6 +14643,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 		for_each_intel_connector(dev, connector) {
 			if (connector->encoder != encoder)
 				continue;
+
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
 			connector->base.encoder = NULL;
 		}
@@ -14887,7 +14950,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	struct drm_atomic_state *state;
 	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
-	int i;
+	int i, ret;
 
 	state = intel_modeset_readout_hw_state(dev);
 	if (IS_ERR(state)) {
@@ -14897,6 +14960,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	/* swap sw/hw state */
 	drm_atomic_helper_swap_state(dev, state);
+
 	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
 	intel_shared_dpll_commit(state);
 	memcpy(to_intel_atomic_state(state)->shared_dpll,
@@ -14919,31 +14983,19 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		crtc_state->planes_changed = false;
 
 		if (crtc->state->enable) {
-			intel_mode_from_pipe_config(&crtc->state->mode,
+			intel_mode_from_pipe_config(&crtc->mode,
 				to_intel_crtc_state(crtc->state));
 			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
 				to_intel_crtc_state(crtc->state));
 
-			drm_mode_copy(&crtc->mode, &crtc->state->mode);
+			if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode))
+				drm_mode_copy(&crtc->state->mode, &crtc->mode);
+
 			drm_mode_copy(&crtc->hwmode,
 				      &crtc->state->adjusted_mode);
-
-			/* Check if we need to force a modeset */
-			if (to_intel_crtc_state(crtc_state)->has_audio !=
-			    to_intel_crtc_state(crtc->state)->has_audio)
-				crtc_state->mode_changed = true;
-
-			if (to_intel_crtc_state(crtc_state)->has_infoframe !=
-			    to_intel_crtc_state(crtc->state)->has_infoframe)
-				crtc_state->mode_changed = true;
 		}
 
-		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
-				    to_intel_crtc_state(crtc_state));
-
-		/* turn CRTC off if a modeset is requested. */
-		if (crtc_state->mode_changed && !force_restore)
-			intel_crtc_control(crtc, false, true);
+		intel_sanitize_crtc(intel_crtc, to_intel_crtc_state(crtc_state), force_restore);
 
 		/*
 		 * sanitize_crtc may have forced an update of crtc->state,
@@ -14973,17 +15025,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	else if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
 
-	if (force_restore) {
-		int ret;
-
-		i915_redisable_vga(dev);
-
-		ret = drm_atomic_commit(state);
-		if (ret) {
-			DRM_ERROR("Failed to restore previous mode\n");
-			drm_atomic_state_free(state);
-		}
-	} else {
+	i915_redisable_vga(dev);
+	ret = drm_atomic_commit(state);
+	if (ret) {
+		DRM_ERROR("Failed to restore previous mode\n");
 		drm_atomic_state_free(state);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2bf6873a5b89..2d19b5d67d9d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -330,6 +330,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
 #define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
+#define PIPE_CONFIG_QUIRK_WRONG_PLANE		(1<<3) /* intel_crtc->plane is wrong */
 	unsigned long quirks;
 
 	/* Pipe source size (ie. panel fitter input size)

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

Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Jani Nikula]

On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Hey,
>
> Op 09-06-15 om 13:48 schreef Tvrtko Ursulin:
>>
>> On 06/01/2015 11:50 AM, Maarten Lankhorst 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.
>>>
>>> Changes since v1:
>>> - Make sure all possible planes are added with their crtc set,
>>>    so they will be turned off on first modeset.
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> This breaks display for me, which is eDP on SKL. At least bisect points to it. A lot of these in the logs:
>>
>> *ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)
>>
>> And the display does not light up.
>
> Yeah, it probably relies on better hw readout. This is partially mitigated by convert to atomic, part 3.
> But it requires 2 additional patches.

Maarten, I'd like to have the fallout from this series fixed before
moving on to merging another big series...

BR,
Jani.


>
> commit 5a97529becb25fabf18a3507a94f892c365a4a1d
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Mon Jun 8 11:31:28 2015 +0200
>
>     drm/i915: update more sw state with hw state during atomic readout
>     
>     I've noticed the following during initial readout:
>     state->adjusted_mode's non crtc_* members were not set,
>     but some code relies hdisplay and vdisplay, so make sure it's
>     set correctly.
>     
>     Also vblank was not enabled because constants were not calculated,
>     this shows up in dmesg as:
>     [drm:drm_vblank_enable] enabling vblank on crtc 0, ret: 0
>     [drm:drm_calc_vbltimestamp_from_scanoutpos] crtc 0: Noop due to uninitialized mode.
>     
>     This commit fixes the regression from the following commit:
>     
>     commit 3bae26eb2991c00670df377cf6c3bc2b0577e82a
>     Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>     Date:   Mon Jun 1 12:50:03 2015 +0200
>     
>         drm/i915: Read hw state into an atomic state struct, v2.
>     
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90861
>     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 fb9f07b1e5ca..dc29bab527d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14859,8 +14859,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  	/* restore vblank interrupts to correct state */
>  	drm_crtc_vblank_reset(&crtc->base);
> -	if (crtc->active) {
> +	if (crtc->base.state->active) {
>  		update_scanline_offset(crtc);
> +		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
>  
> @@ -15307,6 +15308,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		if (crtc->enabled) {
>  			intel_mode_from_pipe_config(&crtc->state->mode,
>  				to_intel_crtc_state(crtc->state));
> +			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> +				to_intel_crtc_state(crtc->state));
>  
>  			drm_mode_copy(&crtc->mode, &crtc->state->mode);
>  			drm_mode_copy(&crtc->hwmode,
>
> commit d0f7e7ae8a151e9d018e2dbf36a5afba812bab4f
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Tue Jun 9 09:01:17 2015 +0200
>
>     drm/i915: only perform a single modeset in intel_modeset_setup_hw_state
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 29ae92e5c8a9..77a553e21a7a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11994,24 +11994,35 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
>  static bool
>  intel_pipe_config_compare(struct drm_device *dev,
>  			  struct intel_crtc_state *current_config,
> -			  struct intel_crtc_state *pipe_config)
> +			  struct intel_crtc_state *pipe_config,
> +			  bool check_only)
>  {
> +	bool ret = true;
> +
> +#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
> +	do { \
> +		if (check_only) \
> +			DRM_ERROR(fmt, ##__VA_ARGS__); \
> +		else \
> +			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
> +	} while (0)
> +
>  #define PIPE_CONF_CHECK_X(name)	\
>  	if (current_config->name != pipe_config->name) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected 0x%08x, found 0x%08x)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_I(name)	\
>  	if (current_config->name != pipe_config->name) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  /* This is required for BDW+ where there is only one set of registers for
> @@ -12022,30 +12033,30 @@ intel_pipe_config_compare(struct drm_device *dev,
>  #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
>  	if ((current_config->name != pipe_config->name) && \
>  		(current_config->alt_name != pipe_config->name)) { \
> -			DRM_ERROR("mismatch in " #name " " \
> +			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  				  "(expected %i or %i, found %i)\n", \
>  				  current_config->name, \
>  				  current_config->alt_name, \
>  				  pipe_config->name); \
> -			return false; \
> +			ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
>  	if ((current_config->name ^ pipe_config->name) & (mask)) { \
> -		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") "	   \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name & (mask), \
>  			  pipe_config->name & (mask)); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
>  	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_QUIRK(quirk)	\
> @@ -12179,8 +12190,9 @@ intel_pipe_config_compare(struct drm_device *dev,
>  #undef PIPE_CONF_CHECK_FLAGS
>  #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>  #undef PIPE_CONF_QUIRK
> +#undef INTEL_ERR_OR_DBG_KMS
>  
> -	return true;
> +	return ret;
>  }
>  
>  static void check_wm_state(struct drm_device *dev)
> @@ -12377,7 +12389,7 @@ check_crtc_state(struct drm_device *dev)
>  		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
>  
>  		if (active &&
> -		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> +		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config, false)) {
>  			I915_STATE_WARN(1, "pipe state doesn't match!\n");
>  			intel_dump_pipe_config(crtc, &pipe_config,
>  					       "[hw state]");
> @@ -12734,6 +12746,12 @@ static int intel_atomic_commit(struct drm_device *dev,
>  			intel_crtc->active = false;
>  			intel_disable_shared_dpll(intel_crtc);
>  		}
> +
> +		/* FIXME: Move this to i9xx_crtc_disable when it gets a pointer
> +		 * to the old crtc_state. */
> +		if (to_intel_crtc_state(crtc_state)->quirks &
> +		    PIPE_CONFIG_QUIRK_WRONG_PLANE)
> +			intel_crtc->plane = !intel_crtc->plane;
>  	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
> @@ -14464,13 +14482,16 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>  }
>  
>  static void intel_sanitize_crtc(struct intel_crtc *crtc,
> -				struct intel_crtc_state *pipe_config)
> +				struct intel_crtc_state *pipe_config,
> +				bool force_restore)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder;
> +	struct drm_atomic_state *state = pipe_config->base.state;
> +	struct intel_crtc_state *hw_state =
> +		to_intel_crtc_state(crtc->base.state);
>  	u32 reg;
> -	bool enable;
>  
>  	/* Clear any frame start delays used for debugging left by the BIOS */
>  	reg = PIPECONF(crtc->config->cpu_transcoder);
> @@ -14484,28 +14505,64 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
>  
> +	/* set up current state */
> +	if (!force_restore && hw_state->base.active) {
> +		bool enable;
> +
> +		memcpy(pipe_config, hw_state, sizeof(*pipe_config));
> +		__drm_atomic_helper_crtc_duplicate_state(&crtc->base, &pipe_config->base);
> +		pipe_config->base.state = state;
> +
> +		enable = false;
> +		for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
> +			enable |= intel_encoder->connectors_active;
> +
> +		pipe_config->base.active = !!enable;
> +	}
> +
>  	/* We need to sanitize the plane -> pipe mapping first because this will
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
>  	 * that gen4+ has a fixed plane -> pipe mapping.  */
>  	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
> -		bool plane;
> -
>  		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
>  			      crtc->base.base.id);
>  
>  		/* Pipe has the wrong plane attached and the plane is active.
>  		 * Temporarily change the plane mapping and disable everything
>  		 * ...  */
> -		plane = crtc->plane;
> -		to_intel_plane_state(crtc->base.primary->state)->visible = true;
> -		crtc->base.primary->crtc = &crtc->base;
> -		crtc->plane = !plane;
> -		intel_crtc_control(&crtc->base, false, true);
> -		crtc->plane = plane;
> +		hw_state->quirks |=
> +			PIPE_CONFIG_QUIRK_WRONG_PLANE;
> +
> +		crtc->plane = !crtc->plane;
> +
> +		if (force_restore)
> +			pipe_config->base.mode_changed = true;
> +		else
> +			pipe_config->base.active = false;
>  	}
>  
> -	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> -	    crtc->pipe == PIPE_A && (!pipe_config || !pipe_config->base.active)) {
> +	/* XXX: This is not active right now */
> +	if (hw_state->base.active && pipe_config->base.active &&
> +	    !i915.fastboot) {
> +		struct intel_crtc_state sw_state;
> +
> +		memset(&sw_state, 0, sizeof(sw_state));
> +		sw_state.base = pipe_config->base;
> +		sw_state.scaler_state = pipe_config->scaler_state;
> +		sw_state.shared_dpll = pipe_config->shared_dpll;
> +		sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> +		sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> +
> +		intel_modeset_pipe_config(&crtc->base, &sw_state);
> +
> +		/* Check if we need to force a modeset */
> +		if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true))
> +			pipe_config->base.mode_changed = true;
> +	}
> +
> +
> +	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
> +	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
>  		/* BIOS forgot to enable pipe A, this mostly happens after
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
> @@ -14513,19 +14570,24 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		intel_enable_pipe_a(dev);
>  	}
>  
> -	/* Adjust the state of the output pipe according to whether we
> -	 * have active connectors/encoders */
> -	enable = false;
> -	for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
> -		enable |= intel_encoder->connectors_active;
> +	/* not restoring state, kill off all connectors and disable this thing */
> +	if (!force_restore && !pipe_config->base.active) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *connector;
> +		int i, ret;
>  
> -	/* only turn off separately if configuration's not restored,
> -	 * if it's restored it will change mode or turn off anyway.
> -	 */
> -	if (!enable && crtc->base.state->active && !pipe_config)
> -		intel_crtc_control(&crtc->base, false, true);
> +		ret = drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL);
> +
> +		for_each_connector_in_state(state, connector, conn_state, i) {
> +			if (conn_state->crtc != &crtc->base)
> +				continue;
>  
> -	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
> +			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +			WARN_ON(ret);
> +		}
> +	}
> +
> +	if (hw_state->base.active || HAS_GMCH_DISPLAY(dev)) {
>  		/*
>  		 * We start out with underrun reporting disabled to avoid races.
>  		 * For correct bookkeeping mark this on active crtcs.
> @@ -14581,6 +14643,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  		for_each_intel_connector(dev, connector) {
>  			if (connector->encoder != encoder)
>  				continue;
> +
>  			connector->base.dpms = DRM_MODE_DPMS_OFF;
>  			connector->base.encoder = NULL;
>  		}
> @@ -14887,7 +14950,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	struct intel_encoder *encoder;
>  	struct drm_atomic_state *state;
>  	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
> -	int i;
> +	int i, ret;
>  
>  	state = intel_modeset_readout_hw_state(dev);
>  	if (IS_ERR(state)) {
> @@ -14897,6 +14960,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  
>  	/* swap sw/hw state */
>  	drm_atomic_helper_swap_state(dev, state);
> +
>  	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
>  	intel_shared_dpll_commit(state);
>  	memcpy(to_intel_atomic_state(state)->shared_dpll,
> @@ -14919,31 +14983,19 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		crtc_state->planes_changed = false;
>  
>  		if (crtc->state->enable) {
> -			intel_mode_from_pipe_config(&crtc->state->mode,
> +			intel_mode_from_pipe_config(&crtc->mode,
>  				to_intel_crtc_state(crtc->state));
>  			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
>  				to_intel_crtc_state(crtc->state));
>  
> -			drm_mode_copy(&crtc->mode, &crtc->state->mode);
> +			if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode))
> +				drm_mode_copy(&crtc->state->mode, &crtc->mode);
> +
>  			drm_mode_copy(&crtc->hwmode,
>  				      &crtc->state->adjusted_mode);
> -
> -			/* Check if we need to force a modeset */
> -			if (to_intel_crtc_state(crtc_state)->has_audio !=
> -			    to_intel_crtc_state(crtc->state)->has_audio)
> -				crtc_state->mode_changed = true;
> -
> -			if (to_intel_crtc_state(crtc_state)->has_infoframe !=
> -			    to_intel_crtc_state(crtc->state)->has_infoframe)
> -				crtc_state->mode_changed = true;
>  		}
>  
> -		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
> -				    to_intel_crtc_state(crtc_state));
> -
> -		/* turn CRTC off if a modeset is requested. */
> -		if (crtc_state->mode_changed && !force_restore)
> -			intel_crtc_control(crtc, false, true);
> +		intel_sanitize_crtc(intel_crtc, to_intel_crtc_state(crtc_state), force_restore);
>  
>  		/*
>  		 * sanitize_crtc may have forced an update of crtc->state,
> @@ -14973,17 +15025,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	else if (HAS_PCH_SPLIT(dev))
>  		ilk_wm_get_hw_state(dev);
>  
> -	if (force_restore) {
> -		int ret;
> -
> -		i915_redisable_vga(dev);
> -
> -		ret = drm_atomic_commit(state);
> -		if (ret) {
> -			DRM_ERROR("Failed to restore previous mode\n");
> -			drm_atomic_state_free(state);
> -		}
> -	} else {
> +	i915_redisable_vga(dev);
> +	ret = drm_atomic_commit(state);
> +	if (ret) {
> +		DRM_ERROR("Failed to restore previous mode\n");
>  		drm_atomic_state_free(state);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2bf6873a5b89..2d19b5d67d9d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -330,6 +330,7 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
>  #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
>  #define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
> +#define PIPE_CONFIG_QUIRK_WRONG_PLANE		(1<<3) /* intel_crtc->plane is wrong */
>  	unsigned long quirks;
>  
>  	/* Pipe source size (ie. panel fitter input size)
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Maarten Lankhorst]

Hey,

Op 09-06-15 om 16:24 schreef Jani Nikula:
> On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>> Hey,
>>
>> Op 09-06-15 om 13:48 schreef Tvrtko Ursulin:
>>> On 06/01/2015 11:50 AM, Maarten Lankhorst 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.
>>>>
>>>> Changes since v1:
>>>> - Make sure all possible planes are added with their crtc set,
>>>>    so they will be turned off on first modeset.
>>>>
>>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> This breaks display for me, which is eDP on SKL. At least bisect points to it. A lot of these in the logs:
>>>
>>> *ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)
>>>
>>> And the display does not light up.
>> Yeah, it probably relies on better hw readout. This is partially mitigated by convert to atomic, part 3.
>> But it requires 2 additional patches.
> Maarten, I'd like to have the fallout from this series fixed before
> moving on to merging another big series...
Nasty, part of the fix is in the following series.

Could we revert only 490f400db5d886fc28566af69b02f6497f31be4b and 3bae26eb2991c00670df377cf6c3bc2b0577e82a ?

First commit for https://bugs.freedesktop.org/show_bug.cgi?id=90868 and because it would break not-atomic resume otherwise.
Second commit for https://bugs.freedesktop.org/show_bug.cgi?id=90861 .

I'll rework convert to atomic part 3 to incorporate the commits again.

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

Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Jani Nikula]

On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Hey,
>
> Op 09-06-15 om 16:24 schreef Jani Nikula:
>> On Tue, 09 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>>> Hey,
>>>
>>> Op 09-06-15 om 13:48 schreef Tvrtko Ursulin:
>>>> On 06/01/2015 11:50 AM, Maarten Lankhorst 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.
>>>>>
>>>>> Changes since v1:
>>>>> - Make sure all possible planes are added with their crtc set,
>>>>>    so they will be turned off on first modeset.
>>>>>
>>>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> This breaks display for me, which is eDP on SKL. At least bisect points to it. A lot of these in the logs:
>>>>
>>>> *ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)
>>>>
>>>> And the display does not light up.
>>> Yeah, it probably relies on better hw readout. This is partially mitigated by convert to atomic, part 3.
>>> But it requires 2 additional patches.
>> Maarten, I'd like to have the fallout from this series fixed before
>> moving on to merging another big series...
> Nasty, part of the fix is in the following series.
>
> Could we revert only 490f400db5d886fc28566af69b02f6497f31be4b and 3bae26eb2991c00670df377cf6c3bc2b0577e82a ?
>
> First commit for https://bugs.freedesktop.org/show_bug.cgi?id=90868 and because it would break not-atomic resume otherwise.
> Second commit for https://bugs.freedesktop.org/show_bug.cgi?id=90861 .
>
> I'll rework convert to atomic part 3 to incorporate the commits again.

This sounds like a good plan to me.

Please send the reverts as patches, citing the bugs and failures.

BR,
Jani.


>
> ~Maarten

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx