[07/42] drm/i915: Get rid of crtc->new_enabled, v2.
diff mbox

Message ID 1431354318-11995-8-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 11, 2015, 2:24 p.m. UTC
No longer any different from state->enable.

v2: Keep track of enabled crtc's for calling intel_crtc_restore_mode.

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

Comments

Daniel Vetter May 11, 2015, 5:33 p.m. UTC | #1
On Mon, May 11, 2015 at 04:24:43PM +0200, Maarten Lankhorst wrote:
> @@ -14679,7 +14646,8 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>  }
>  
> -static void intel_modeset_readout_hw_state(struct drm_device *dev)
> +static void intel_modeset_readout_hw_state(struct drm_device *dev,
> +					   unsigned *crtc_mask)

Why is crtc_mask a paramter?

>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum pipe pipe;
> @@ -14688,6 +14656,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	struct intel_connector *connector;
>  	int i;
>  
> +	*crtc_mask = 0;
>  	for_each_intel_crtc(dev, crtc) {
>  		struct drm_plane *primary = crtc->base.primary;
>  		struct intel_plane_state *plane_state;
> @@ -14696,6 +14665,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> +		if (crtc->active)
> +			*crtc_mask |= drm_crtc_index(&crtc->base);
> +
>  		crtc->active = dev_priv->display.get_pipe_config(crtc,
>  								 crtc->config);
>  
> @@ -14778,7 +14750,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	struct intel_encoder *encoder;
>  	int i;
>  
> -	intel_modeset_readout_hw_state(dev);
> +	unsigned crtc_mask;
> +
> +	intel_modeset_readout_hw_state(dev, &crtc_mask);
>  
>  	/*
>  	 * Now that we have the config, copy it to each CRTC struct
> @@ -14837,10 +14811,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  			struct drm_crtc *crtc =
>  				dev_priv->pipe_to_crtc_mapping[pipe];
>  
> -			intel_crtc_restore_mode(crtc);
> +			if (crtc_mask & (1 << drm_crtc_index(crtc)))
> +				intel_crtc_restore_mode(crtc);

this seems a bit backwards. If we push the crtc loop down into
intel_crct_restore_mode (and call it intel_restore_mode or so) then we
could reuse the ->enable state from atomic. I.e.

	/* all the code in intel_crtc_restore_mode except the set_mode
	 * call at the bottom */

	for_crc_in_atomic_state(crtc_state) 
		if (crtc_state->enable)
			intel_set_mode

That would look more atomic imo (we'll just need to replace this loop with
a drm_atomic_commit once we're there). And contain the restore state
tracking to one function instead of spreading it over readout_hw_state.

Thoughts?
-Daniel

>  		}
> -	} else {
> -		intel_modeset_update_staged_output_state(dev);
>  	}
>  
>  	intel_modeset_check_state(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f85761494dd1..1e892098eea2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -515,7 +515,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;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter May 11, 2015, 5:44 p.m. UTC | #2
On Mon, May 11, 2015 at 07:33:01PM +0200, Daniel Vetter wrote:
> On Mon, May 11, 2015 at 04:24:43PM +0200, Maarten Lankhorst wrote:
> > @@ -14679,7 +14646,8 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
> >  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> >  }
> >  
> > -static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > +static void intel_modeset_readout_hw_state(struct drm_device *dev,
> > +					   unsigned *crtc_mask)
> 
> Why is crtc_mask a paramter?
> 
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	enum pipe pipe;
> > @@ -14688,6 +14656,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  	struct intel_connector *connector;
> >  	int i;
> >  
> > +	*crtc_mask = 0;
> >  	for_each_intel_crtc(dev, crtc) {
> >  		struct drm_plane *primary = crtc->base.primary;
> >  		struct intel_plane_state *plane_state;
> > @@ -14696,6 +14665,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  
> >  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
> >  
> > +		if (crtc->active)
> > +			*crtc_mask |= drm_crtc_index(&crtc->base);
> > +
> >  		crtc->active = dev_priv->display.get_pipe_config(crtc,
> >  								 crtc->config);
> >  
> > @@ -14778,7 +14750,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >  	struct intel_encoder *encoder;
> >  	int i;
> >  
> > -	intel_modeset_readout_hw_state(dev);
> > +	unsigned crtc_mask;
> > +
> > +	intel_modeset_readout_hw_state(dev, &crtc_mask);
> >  
> >  	/*
> >  	 * Now that we have the config, copy it to each CRTC struct
> > @@ -14837,10 +14811,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >  			struct drm_crtc *crtc =
> >  				dev_priv->pipe_to_crtc_mapping[pipe];
> >  
> > -			intel_crtc_restore_mode(crtc);
> > +			if (crtc_mask & (1 << drm_crtc_index(crtc)))
> > +				intel_crtc_restore_mode(crtc);
> 
> this seems a bit backwards. If we push the crtc loop down into
> intel_crct_restore_mode (and call it intel_restore_mode or so) then we
> could reuse the ->enable state from atomic. I.e.
> 
> 	/* all the code in intel_crtc_restore_mode except the set_mode
> 	 * call at the bottom */
> 
> 	for_crc_in_atomic_state(crtc_state) 
> 		if (crtc_state->enable)
> 			intel_set_mode
> 
> That would look more atomic imo (we'll just need to replace this loop with
> a drm_atomic_commit once we're there). And contain the restore state
> tracking to one function instead of spreading it over readout_hw_state.

This doesn't work, and we actually need to pull the atomic state copying
out of restore_mode up into readout_hw_state so that we can use it instead
of ->new_* pointers and intel_crtc->config. And that needs to happen
_before_ remove the new_ pointers ofc.

Otherwise (i.e. after this patch) well overwrite object linking in
readout_hw_state and then restore_mode will do something silly.

Or do I still miss something?
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a9ce827601d8..8c2fb951029b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9733,7 +9733,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:
@@ -9751,10 +9751,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;
@@ -9773,9 +9773,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;
@@ -9786,18 +9783,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;
+		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;
@@ -9865,9 +9861,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;
 
@@ -9913,8 +9907,6 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (IS_ERR(crtc_state))
 			goto fail;
 
-		intel_crtc->new_enabled = false;
-
 		connector_state->best_encoder = NULL;
 		connector_state->crtc = NULL;
 
@@ -11052,21 +11044,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;
-
-	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.
@@ -11119,10 +11096,6 @@  static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 		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
@@ -12415,9 +12388,6 @@  void intel_crtc_restore_mode(struct drm_crtc *crtc)
 	}
 
 	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",
@@ -12426,9 +12396,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);
 	}
@@ -14679,7 +14646,8 @@  static bool primary_get_hw_state(struct intel_crtc *crtc)
 	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
 }
 
-static void intel_modeset_readout_hw_state(struct drm_device *dev)
+static void intel_modeset_readout_hw_state(struct drm_device *dev,
+					   unsigned *crtc_mask)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
@@ -14688,6 +14656,7 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	struct intel_connector *connector;
 	int i;
 
+	*crtc_mask = 0;
 	for_each_intel_crtc(dev, crtc) {
 		struct drm_plane *primary = crtc->base.primary;
 		struct intel_plane_state *plane_state;
@@ -14696,6 +14665,9 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
+		if (crtc->active)
+			*crtc_mask |= drm_crtc_index(&crtc->base);
+
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 crtc->config);
 
@@ -14778,7 +14750,9 @@  void intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	int i;
 
-	intel_modeset_readout_hw_state(dev);
+	unsigned crtc_mask;
+
+	intel_modeset_readout_hw_state(dev, &crtc_mask);
 
 	/*
 	 * Now that we have the config, copy it to each CRTC struct
@@ -14837,10 +14811,9 @@  void intel_modeset_setup_hw_state(struct drm_device *dev,
 			struct drm_crtc *crtc =
 				dev_priv->pipe_to_crtc_mapping[pipe];
 
-			intel_crtc_restore_mode(crtc);
+			if (crtc_mask & (1 << drm_crtc_index(crtc)))
+				intel_crtc_restore_mode(crtc);
 		}
-	} else {
-		intel_modeset_update_staged_output_state(dev);
 	}
 
 	intel_modeset_check_state(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f85761494dd1..1e892098eea2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -515,7 +515,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;