[RFC,2/7] drm/i915: Read hw state into an atomic state struct, try 2.
diff mbox

Message ID 1434700950-16242-3-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst June 19, 2015, 8:02 a.m. UTC
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.

Change since try1:
- Fix the regressions caused by not updating drm core sw state.

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 | 381 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 243 insertions(+), 154 deletions(-)

Comments

Daniel Stone June 19, 2015, 9:38 a.m. UTC | #1
Hi,

On 19 June 2015 at 09:02, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> +               if (crtc->state->enable) {
> +                       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));
> +
> +                       if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode) < 0)
> +                               drm_mode_copy(&crtc->state->mode, &crtc->mode);

I've never really understood why this is here. There are only two ways
in which set_mode_for_crtc can fail: either the mode fails validation
(fixed in the patch I sent you the other day, which it seems like
you've now pulled in), or -ENOMEM. The first one we shouldn't really
be papering over - and I haven't seen it happen since that patch - and
the second one isn't really recoverable anyway. Plus I would argue
that breaking the state like that is pretty bad.

Cheers,
Daniel
Daniel Vetter June 22, 2015, 3:31 p.m. UTC | #2
On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
>  /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
> @@ -15491,37 +15569,61 @@ 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);

This is imo way too much open-coding of stuff that's already there. What I
had in mind for force_restore was a generic helper to duplicate all the
state objects into a drm_atomic_state, without calling any of the
->atomic_check functions or anything. Then the sequence for restoring
state would be.

1. saved_state = drm_atomic_helper_duplicate_state()
2. intel_readout_hw_sate() <- this would just update ->state and sanitize
it.
3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
always works.

Ofc we still need the ww-mutex deadlock avoidance wrapped around this, but
since atomic_commit will first compute all derived state there's no need
to duplicate all the i915-internal state too. That should all work like
with any other normal modeset.
-Daniel
Maarten Lankhorst June 23, 2015, 10:49 a.m. UTC | #3
Op 22-06-15 om 17:31 schreef Daniel Vetter:
> On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
>>  /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
>> @@ -15491,37 +15569,61 @@ 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);
> This is imo way too much open-coding of stuff that's already there. What I
> had in mind for force_restore was a generic helper to duplicate all the
> state objects into a drm_atomic_state, without calling any of the
> ->atomic_check functions or anything. Then the sequence for restoring
> state would be.
>
> 1. saved_state = drm_atomic_helper_duplicate_state()
> 2. intel_readout_hw_sate() <- this would just update ->state and sanitize
> it.
> 3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
> always works.
Well, for !force_restore we want to do an initial sanitizing modeset too, but with the full disabled state as reference point.
I'm not sure dupe state followed by a swap, is different from save old state, and read out to current state.
> Ofc we still need the ww-mutex deadlock avoidance wrapped around this, but
> since atomic_commit will first compute all derived state there's no need
> to duplicate all the i915-internal state too. That should all work like
> with any other normal modeset.
No need to wrap, readout touches everything so lock_all is fine.

~Maarten
Daniel Vetter June 23, 2015, 11:40 a.m. UTC | #4
On Tue, Jun 23, 2015 at 12:49:00PM +0200, Maarten Lankhorst wrote:
> Op 22-06-15 om 17:31 schreef Daniel Vetter:
> > On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
> >>  /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
> >> @@ -15491,37 +15569,61 @@ 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);
> > This is imo way too much open-coding of stuff that's already there. What I
> > had in mind for force_restore was a generic helper to duplicate all the
> > state objects into a drm_atomic_state, without calling any of the
> > ->atomic_check functions or anything. Then the sequence for restoring
> > state would be.
> >
> > 1. saved_state = drm_atomic_helper_duplicate_state()
> > 2. intel_readout_hw_sate() <- this would just update ->state and sanitize
> > it.
> > 3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
> > always works.
> Well, for !force_restore we want to do an initial sanitizing modeset too, but with the full disabled state as reference point.
> I'm not sure dupe state followed by a swap, is different from save old state, and read out to current state.

You also dupe internal state like dplls, that's what a helper-level
duplicat state would avoid. And imo that's cleaner. ->atomic_check should
then grab all the internal states needed, if any are necessary. The goal
really is to avoid another special case with too much knowledge of i915
internals, which atomic really should allow us to do.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 429677a111d5..4d87574a2032 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -398,7 +398,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 b3d8d1a30a04..41193dab58c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10231,7 +10231,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:
@@ -10249,10 +10249,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;
@@ -10271,9 +10271,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;
@@ -10284,20 +10281,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;
@@ -10365,9 +10359,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;
 
@@ -10413,10 +10405,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;
 
@@ -11869,33 +11857,6 @@  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.atomic_check = intel_crtc_atomic_check,
 };
 
-/**
- * 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.
@@ -12349,7 +12310,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) {
@@ -12659,11 +12619,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");
 	}
 }
@@ -12683,8 +12646,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");
 
@@ -12694,6 +12655,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
@@ -13255,11 +13219,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);
@@ -13272,14 +13236,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",
@@ -13288,9 +13248,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);
 	}
@@ -15181,8 +15138,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);
 	}
 
@@ -15350,12 +15308,14 @@  static bool primary_get_hw_state(struct intel_crtc *crtc)
 	return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
 }
 
-static void readout_plane_state(struct intel_crtc *crtc,
+static int readout_plane_state(struct drm_atomic_state *state,
+				struct intel_crtc *crtc,
 				struct intel_crtc_state *crtc_state)
 {
 	struct intel_plane *p;
 	struct drm_plane_state *drm_plane_state;
 	bool active = crtc_state->base.active;
+	int ret = 0;
 
 	if (active) {
 		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
@@ -15371,7 +15331,11 @@  static void readout_plane_state(struct intel_crtc *crtc,
 		if (crtc->pipe != p->pipe)
 			continue;
 
-		drm_plane_state = p->base.state;
+		drm_plane_state = drm_atomic_get_plane_state(state, &p->base);
+		if (IS_ERR(drm_plane_state)) {
+			ret = PTR_ERR(drm_plane_state);
+			break;
+		}
 
 		/* Plane scaler state is not touched here. The first atomic
 		 * commit will restore all plane scalers to its old state.
@@ -15388,101 +15352,215 @@  static void readout_plane_state(struct intel_crtc *crtc,
 			to_intel_plane_state(drm_plane_state)->visible = false;
 		}
 
+		p->base.crtc = drm_plane_state->crtc;
 		if (visible) {
 			crtc_state->base.plane_mask |=
 				1 << drm_plane_index(&p->base);
-		} else if (crtc_state->base.state) {
-			/* Make this unconditional for atomic hw readout. */
+		} else {
 			crtc_state->base.plane_mask &=
 				~(1 << drm_plane_index(&p->base));
 		}
 	}
+
+	return ret;
 }
 
-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;
+	int ret;
 
-	for_each_intel_crtc(dev, crtc) {
-		memset(crtc->config, 0, sizeof(*crtc->config));
-		crtc->config->base.crtc = &crtc->base;
+	crtc_state = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
 
-		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
+	ret = drm_atomic_add_affected_planes(state, &crtc->base);
+	if (ret)
+		return ret;
 
-		crtc->active = dev_priv->display.get_pipe_config(crtc,
-								 crtc->config);
+	memset(crtc_state, 0, sizeof(*crtc_state));
+	crtc_state->base.crtc = &crtc->base;
+	crtc_state->base.state = state;
 
-		crtc->base.state->enable = crtc->active;
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
-		crtc->base.hwmode = crtc->config->base.adjusted_mode;
+	crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
-		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
+	crtc_state->base.enable = crtc_state->base.active =
+	crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state);
 
-		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
-			      crtc->base.base.id,
-			      crtc->active ? "enabled" : "disabled");
-	}
+	/* 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_state->base.active ? "enabled" : "disabled");
+
+	return readout_plane_state(state, crtc, crtc_state);
+}
 
+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
@@ -15491,37 +15569,61 @@  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->state->enable) {
+			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));
+
+			if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode) < 0)
+				drm_mode_copy(&crtc->state->mode, &crtc->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]");
 	}
 
@@ -15545,20 +15647,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 4b185564aeea..bc7d0a003c8e 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 *);
@@ -541,7 +530,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;
@@ -1418,6 +1406,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,