[PATCH RFC 0/5] Convert planes and crtc state updates to atomic.

[PATCH RFC 0/5] Convert planes and crtc state updates to atomic.

[maarten.lankhorst]

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

This applies on top of the patch series:
"[PATCH 00/35] Make legacy modeset a lot more atomic-like"

after the last patch crtc will no longer be enabled/disabled except through
atomic updates, which should make it a lot easier to rely on the atomic state
in the future.

Maarten Lankhorst (5):
  drm/i915: Get rid of intel_crtc_disable and related code.
  drm/i915: Only update required power domains.
  drm/i915: use intel_crtc_control everywhere
  drm/i915: make plane helpers fully atomic
  drm/i915: Implement intel_crtc_toggle using atomic state

 drivers/gpu/drm/i915/i915_debugfs.c       |  18 +-
 drivers/gpu/drm/i915/i915_drv.h           |   1 -
 drivers/gpu/drm/i915/intel_atomic_plane.c |  49 +-
 drivers/gpu/drm/i915/intel_display.c      | 796 ++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h          |   2 -
 drivers/gpu/drm/i915/intel_sprite.c       |  33 +-
 6 files changed, 467 insertions(+), 432 deletions(-)

-- 
2.1.0

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

[PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code.

[maarten.lankhorst]

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

Now that the dpll updates are (mostly) atomic, the .off() code is no longer used,
and there are no more callers for intel_put_shared_dpll. Move all the updates
done in intel_crtc_disable to intel_modeset_update_state, one less special case
to worry about.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 61b756bdbaad..9e62926e71f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -572,7 +572,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 35fde239c200..92d54dd30d7e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4123,27 +4123,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)
 {
@@ -5089,13 +5068,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;
@@ -5722,10 +5694,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)
 {
@@ -5775,34 +5743,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);
@@ -11219,7 +11159,6 @@ 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);
 
@@ -11227,15 +11166,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		if (!intel_encoder->base.crtc)
 			continue;
 
-		for_each_crtc_in_state(state, crtc, crtc_state, i)
-			if (crtc == intel_encoder->base.crtc)
-				break;
-
-		if (crtc != intel_encoder->base.crtc)
+		crtc = intel_encoder->base.crtc;
+		crtc_state = state->crtc_states[drm_crtc_index(crtc)];
+		if (!crtc_state || !needs_modeset(crtc->state))
 			continue;
 
-		if (crtc_state->enable && needs_modeset(crtc_state))
-			intel_encoder->connectors_active = false;
+		intel_encoder->connectors_active = false;
 	}
 
 	drm_atomic_helper_swap_state(state->dev, state);
@@ -11250,14 +11186,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		if (!connector->encoder || !connector->encoder->crtc)
 			continue;
 
-		for_each_crtc_in_state(state, crtc, crtc_state, i)
-			if (crtc == connector->encoder->crtc)
-				break;
-
-		if (crtc != connector->encoder->crtc)
+		crtc = connector->encoder->crtc;
+		crtc_state = state->crtc_states[drm_crtc_index(crtc)];
+		if (!crtc_state || !needs_modeset(crtc->state))
 			continue;
 
-		if (crtc->state->enable && needs_modeset(crtc->state)) {
+		if (crtc->state->enable) {
 			struct drm_property *dpms_property =
 				dev->mode_config.dpms_property;
 
@@ -11268,7 +11202,8 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 
 			intel_encoder = to_intel_encoder(connector->encoder);
 			intel_encoder->connectors_active = true;
-		}
+		} else
+			connector->dpms = DRM_MODE_DPMS_OFF;
 	}
 
 }
@@ -11946,12 +11881,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
@@ -13615,7 +13548,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 = ironlake_crtc_off;
 		dev_priv->display.update_primary_plane =
 			skylake_update_primary_plane;
 	} else if (HAS_DDI(dev)) {
@@ -13626,7 +13558,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 = ironlake_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
@@ -13637,7 +13568,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 = ironlake_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
@@ -13647,7 +13577,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 {
@@ -13657,7 +13586,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;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 70b70e9be167..fb89f5f3498c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1078,7 +1078,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

Re: [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code.

[Ander Conselvan De Oliveira]

On Wed, 2015-04-22 at 13:24 +0200, maarten.lankhorst@linux.intel.com
wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Now that the dpll updates are (mostly) atomic, the .off() code is no longer used,
> and there are no more callers for intel_put_shared_dpll.

This is a bit confusing since until this patch the .off() code is
actually called from intel_crtc_disable, which calls
intel_put_shared_dpll(). I think what you intended to say here is that
intel_shared_dpll_commit() has the same effect as put_shared_dpll(), so
it is safe to remove that call, at which point the .off() hook is not
necessary anymore.

>  Move all the updates
> done in intel_crtc_disable to intel_modeset_update_state, one less special case
> to worry about.

Maybe split this to a separate patch?

Ander

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   1 -
>  drivers/gpu/drm/i915/intel_display.c | 100 +++++------------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  3 files changed, 14 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 61b756bdbaad..9e62926e71f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -572,7 +572,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 35fde239c200..92d54dd30d7e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4123,27 +4123,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)
>  {
> @@ -5089,13 +5068,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;
> @@ -5722,10 +5694,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)
>  {
> @@ -5775,34 +5743,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);
> @@ -11219,7 +11159,6 @@ 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);
>  
> @@ -11227,15 +11166,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		if (!intel_encoder->base.crtc)
>  			continue;
>  
> -		for_each_crtc_in_state(state, crtc, crtc_state, i)
> -			if (crtc == intel_encoder->base.crtc)
> -				break;
> -
> -		if (crtc != intel_encoder->base.crtc)
> +		crtc = intel_encoder->base.crtc;
> +		crtc_state = state->crtc_states[drm_crtc_index(crtc)];
> +		if (!crtc_state || !needs_modeset(crtc->state))
>  			continue;
>  
> -		if (crtc_state->enable && needs_modeset(crtc_state))
> -			intel_encoder->connectors_active = false;
> +		intel_encoder->connectors_active = false;
>  	}
>  
>  	drm_atomic_helper_swap_state(state->dev, state);
> @@ -11250,14 +11186,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		if (!connector->encoder || !connector->encoder->crtc)
>  			continue;
>  
> -		for_each_crtc_in_state(state, crtc, crtc_state, i)
> -			if (crtc == connector->encoder->crtc)
> -				break;
> -
> -		if (crtc != connector->encoder->crtc)
> +		crtc = connector->encoder->crtc;
> +		crtc_state = state->crtc_states[drm_crtc_index(crtc)];
> +		if (!crtc_state || !needs_modeset(crtc->state))
>  			continue;
>  
> -		if (crtc->state->enable && needs_modeset(crtc->state)) {
> +		if (crtc->state->enable) {
>  			struct drm_property *dpms_property =
>  				dev->mode_config.dpms_property;
>  
> @@ -11268,7 +11202,8 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  
>  			intel_encoder = to_intel_encoder(connector->encoder);
>  			intel_encoder->connectors_active = true;
> -		}
> +		} else
> +			connector->dpms = DRM_MODE_DPMS_OFF;
>  	}
>  
>  }
> @@ -11946,12 +11881,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
> @@ -13615,7 +13548,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 = ironlake_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			skylake_update_primary_plane;
>  	} else if (HAS_DDI(dev)) {
> @@ -13626,7 +13558,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 = ironlake_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			ironlake_update_primary_plane;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> @@ -13637,7 +13568,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 = ironlake_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			ironlake_update_primary_plane;
>  	} else if (IS_VALLEYVIEW(dev)) {
> @@ -13647,7 +13577,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 {
> @@ -13657,7 +13586,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;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 70b70e9be167..fb89f5f3498c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1078,7 +1078,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);


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

[PATCH RFC 2/5] drm/i915: Only update required power domains.

[maarten.lankhorst]

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

This prevents unnecessarily updating power domains, while still
enabling all power domains on initial setup.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92d54dd30d7e..438d8e213748 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5163,36 +5163,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	return mask;
 }
 
+static bool
+needs_modeset(struct drm_crtc_state *state)
+{
+	return state->mode_changed || state->active_changed;
+}
+
 static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
 	struct intel_crtc *crtc;
+	bool init_power = dev_priv->power_domains.init_power_on;
+	bool any_power = init_power, any_modeset = false;
+	unsigned long domains;
 
 	/*
 	 * First get all needed power domains, then put all unneeded, to avoid
 	 * any unnecessary toggling of the power wells.
 	 */
 	for_each_intel_crtc(dev, crtc) {
+		int idx = drm_crtc_index(&crtc->base);
+		struct drm_crtc_state *crtc_state = state->crtc_states[idx];
 		enum intel_display_power_domain domain;
 
-		if (!crtc->base.state->enable)
+		if (!init_power && !crtc_state)
+			continue;
+
+		if (needs_modeset(crtc->base.state))
+			any_modeset = true;
+
+		if (crtc->base.state->enable)
+			pipe_domains[crtc->pipe] =
+				get_crtc_power_domains(&crtc->base);
+
+		if (pipe_domains[crtc->pipe] == crtc->enabled_power_domains)
 			continue;
 
-		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
+		WARN_ON(!init_power && !needs_modeset(crtc->base.state));
+
+		any_power = true;
+		domains = pipe_domains[crtc->pipe] &
+			  ~crtc->enabled_power_domains;
 
-		for_each_power_domain(domain, pipe_domains[crtc->pipe])
+		for_each_power_domain(domain, domains)
 			intel_display_power_get(dev_priv, domain);
 	}
 
-	if (dev_priv->display.modeset_global_resources)
+	if (any_modeset && dev_priv->display.modeset_global_resources)
 		dev_priv->display.modeset_global_resources(state);
 
+	if (!any_power)
+		return;
+
 	for_each_intel_crtc(dev, crtc) {
+		int idx = drm_crtc_index(&crtc->base);
+		struct drm_crtc_state *crtc_state = state->crtc_states[idx];
 		enum intel_display_power_domain domain;
 
-		for_each_power_domain(domain, crtc->enabled_power_domains)
+		if (!init_power && !crtc_state)
+			continue;
+
+		domains = crtc->enabled_power_domains &
+			  ~pipe_domains[crtc->pipe];
+
+		for_each_power_domain(domain, domains)
 			intel_display_power_put(dev_priv, domain);
 
 		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
@@ -11144,12 +11180,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)
 {
-- 
2.1.0

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

Re: [PATCH RFC 2/5] drm/i915: Only update required power domains.

[Ander Conselvan De Oliveira]

On Wed, 2015-04-22 at 13:24 +0200, maarten.lankhorst@linux.intel.com
wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> This prevents unnecessarily updating power domains, while still
> enabling all power domains on initial setup.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92d54dd30d7e..438d8e213748 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5163,36 +5163,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>  	return mask;
>  }
>  
> +static bool
> +needs_modeset(struct drm_crtc_state *state)
> +{
> +	return state->mode_changed || state->active_changed;
> +}
> +
>  static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
>  	struct intel_crtc *crtc;
> +	bool init_power = dev_priv->power_domains.init_power_on;
> +	bool any_power = init_power, any_modeset = false;
> +	unsigned long domains;
>  
>  	/*
>  	 * First get all needed power domains, then put all unneeded, to avoid
>  	 * any unnecessary toggling of the power wells.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> +		int idx = drm_crtc_index(&crtc->base);
> +		struct drm_crtc_state *crtc_state = state->crtc_states[idx];
>  		enum intel_display_power_domain domain;
>  
> -		if (!crtc->base.state->enable)
> +		if (!init_power && !crtc_state)
> +			continue;
> +
> +		if (needs_modeset(crtc->base.state))
> +			any_modeset = true;
> +
> +		if (crtc->base.state->enable)
> +			pipe_domains[crtc->pipe] =
> +				get_crtc_power_domains(&crtc->base);
> +
> +		if (pipe_domains[crtc->pipe] == crtc->enabled_power_domains)
>  			continue;
>  
> -		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
> +		WARN_ON(!init_power && !needs_modeset(crtc->base.state));
> +
> +		any_power = true;
> +		domains = pipe_domains[crtc->pipe] &
> +			  ~crtc->enabled_power_domains;
>  
> -		for_each_power_domain(domain, pipe_domains[crtc->pipe])
> +		for_each_power_domain(domain, domains)
>  			intel_display_power_get(dev_priv, domain);

Isn't intel_display_power_get() already a no-op if the power domain is
already active, except for the reference counting? Or did I miss
something?

Ander


>  	}
>  
> -	if (dev_priv->display.modeset_global_resources)
> +	if (any_modeset && dev_priv->display.modeset_global_resources)
>  		dev_priv->display.modeset_global_resources(state);
>  
> +	if (!any_power)
> +		return;
> +
>  	for_each_intel_crtc(dev, crtc) {
> +		int idx = drm_crtc_index(&crtc->base);
> +		struct drm_crtc_state *crtc_state = state->crtc_states[idx];
>  		enum intel_display_power_domain domain;
>  
> -		for_each_power_domain(domain, crtc->enabled_power_domains)
> +		if (!init_power && !crtc_state)
> +			continue;
> +
> +		domains = crtc->enabled_power_domains &
> +			  ~pipe_domains[crtc->pipe];
> +
> +		for_each_power_domain(domain, domains)
>  			intel_display_power_put(dev_priv, domain);
>  
>  		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> @@ -11144,12 +11180,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)
>  {


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

[PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere

[maarten.lankhorst]

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

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 60e86f331313..91c9a4fc8b6a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3589,12 +3589,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->base);
+		if (active)
+			intel_crtc_control(&crtc->base, true);
 	}
 	drm_modeset_unlock_all(dev);
 }
@@ -3613,12 +3619,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->base);
+		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 438d8e213748..2ffacb4c3a12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3101,22 +3101,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
 	}
 }
 
-void intel_crtc_reset(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-
-	if (!crtc->active)
-		return;
-
-	intel_crtc_disable_planes(&crtc->base);
-	dev_priv->display.crtc_disable(&crtc->base);
-	dev_priv->display.crtc_enable(&crtc->base);
-	intel_crtc_enable_planes(&crtc->base);
-}
-
 void intel_prepare_reset(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *crtc;
 
 	/* no reset support for gen2 */
@@ -3128,18 +3114,12 @@ void intel_prepare_reset(struct drm_device *dev)
 		return;
 
 	drm_modeset_lock_all(dev);
-
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
-	}
+	for_each_intel_crtc(dev, crtc)
+		intel_crtc_control(&crtc->base, false);
 }
 
 void intel_finish_reset(struct drm_device *dev)
@@ -5739,26 +5719,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;
-
-			dev_priv->display.crtc_enable(crtc);
-			intel_crtc_enable_planes(crtc);
-		}
+		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);
 	} else {
-		if (intel_crtc->active) {
-			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;
-		}
+		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;
 	}
 }
 
@@ -5775,8 +5758,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)
@@ -14087,8 +14068,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 fb89f5f3498c..9668b17d7e0e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -987,7 +987,6 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
-void intel_crtc_reset(struct intel_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
-- 
2.1.0

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

Re: [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere

[Daniel Vetter]

On Wed, Apr 22, 2015 at 01:24:20PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

A bit more explanation in the commit message here would be useful, i.e.
what changed that we need this now.
-Daniel
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 18 ++++++++--
>  drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++-----------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  3 files changed, 39 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 60e86f331313..91c9a4fc8b6a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3589,12 +3589,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->base);
> +		if (active)
> +			intel_crtc_control(&crtc->base, true);
>  	}
>  	drm_modeset_unlock_all(dev);
>  }
> @@ -3613,12 +3619,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->base);
> +		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 438d8e213748..2ffacb4c3a12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3101,22 +3101,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
>  	}
>  }
>  
> -void intel_crtc_reset(struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -
> -	if (!crtc->active)
> -		return;
> -
> -	intel_crtc_disable_planes(&crtc->base);
> -	dev_priv->display.crtc_disable(&crtc->base);
> -	dev_priv->display.crtc_enable(&crtc->base);
> -	intel_crtc_enable_planes(&crtc->base);
> -}
> -
>  void intel_prepare_reset(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *crtc;
>  
>  	/* no reset support for gen2 */
> @@ -3128,18 +3114,12 @@ void intel_prepare_reset(struct drm_device *dev)
>  		return;
>  
>  	drm_modeset_lock_all(dev);
> -
>  	/*
>  	 * Disabling the crtcs gracefully seems nicer. Also the
>  	 * g33 docs say we should at least disable all the planes.
>  	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> -
> -		intel_crtc_disable_planes(&crtc->base);
> -		dev_priv->display.crtc_disable(&crtc->base);
> -	}
> +	for_each_intel_crtc(dev, crtc)
> +		intel_crtc_control(&crtc->base, false);
>  }
>  
>  void intel_finish_reset(struct drm_device *dev)
> @@ -5739,26 +5719,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;
> -
> -			dev_priv->display.crtc_enable(crtc);
> -			intel_crtc_enable_planes(crtc);
> -		}
> +		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);
>  	} else {
> -		if (intel_crtc->active) {
> -			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;
> -		}
> +		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;
>  	}
>  }
>  
> @@ -5775,8 +5758,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)
> @@ -14087,8 +14068,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 fb89f5f3498c..9668b17d7e0e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -987,7 +987,6 @@ void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>  void intel_crtc_control(struct drm_crtc *crtc, bool enable);
> -void intel_crtc_reset(struct intel_crtc *crtc);
>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH RFC 4/5] drm/i915: make plane helpers fully atomic

[maarten.lankhorst]

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  49 +--
 drivers/gpu/drm/i915/intel_display.c      | 481 +++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_sprite.c       |  33 +-
 3 files changed, 353 insertions(+), 210 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index f7bd3b8fa245..0be720edb971 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -110,32 +110,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
+	intel_state->visible = false;
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
 	 * anything driver-specific we need to test in that case, so
 	 * just return success.
 	 */
-	if (!crtc)
+	if (!crtc) {
+		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
+		return 0;
+	}
+
+	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];
+	if (WARN_ON(!crtc_state) ||
+	    WARN_ON(!crtc_state->enable)) {
 		return 0;
+	}
 
-	/* FIXME: temporary hack necessary while we still use the plane update
-	 * helper. */
-	if (state->state) {
-		crtc_state =
-			intel_atomic_get_crtc_state(state->state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	} else {
-		crtc_state = intel_crtc->config;
+	if (!crtc_state->active) {
+		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
+		return 0;
 	}
 
 	/*
@@ -155,24 +154,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
-	intel_state->clip.x2 =
-		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
-	intel_state->clip.y2 =
-		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
-
-	/*
-	 * Disabling a plane is always okay; we just need to update
-	 * fb tracking in a special way since cleanup_fb() won't
-	 * get called by the plane helpers.
-	 */
-	if (state->fb == NULL && plane->state->fb != NULL) {
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking as a special case
-		 */
-		intel_crtc->atomic.disabled_planes |=
-			(1 << drm_plane_index(plane));
-	}
+	intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w;
+	intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h;
 
 	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ffacb4c3a12..99a45bee20d8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
+static int intel_atomic_check_crtc(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state);
 static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
 static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
 static void intel_crtc_enable_planes(struct drm_crtc *crtc);
 static void intel_crtc_disable_planes(struct drm_crtc *crtc);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
+static void intel_post_enable_primary(struct drm_crtc *crtc);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane_state *plane_state =
+		to_intel_plane_state(crtc->primary->state);
+	bool was_visible = plane_state->visible;
 
-	if (dev_priv->display.disable_fbc)
+	/* Not supported right now by the helper, but lets be thorough. */
+	if (was_visible && !fb)
+		intel_pre_disable_primary(crtc);
+	else if (was_visible && dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
 
+	plane_state->visible = !!fb;
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
+	if (!was_visible && fb)
+		intel_post_enable_primary(crtc);
 
 	return 0;
 }
@@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev)
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		drm_modeset_lock(&crtc->mutex, NULL);
-		/*
-		 * FIXME: Once we have proper support for primary planes (and
-		 * disabling them without disabling the entire crtc) allow again
-		 * a NULL crtc->primary->fb.
-		 */
-		if (intel_crtc->active && crtc->primary->fb)
+
+		if (intel_crtc->active) {
+			const struct intel_plane_state *state =
+				to_intel_plane_state(crtc->primary->state);
+
 			dev_priv->display.update_primary_plane(crtc,
-							       crtc->primary->fb,
-							       crtc->x,
-							       crtc->y);
+							state->base.fb,
+							state->src.x1 >> 16,
+							state->src.y1 >> 16);
+		}
+
 		drm_modeset_unlock(&crtc->mutex);
 	}
 }
@@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.load_lut = intel_crtc_load_lut,
 	.atomic_begin = intel_begin_crtc_commit,
 	.atomic_flush = intel_finish_crtc_commit,
+	.atomic_check = intel_atomic_check_crtc,
 };
 
 /**
@@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
  */
 static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 {
-	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
@@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 			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);
@@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
 
 	intel_shared_dpll_commit(dev_priv);
 
@@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		intel_encoder->connectors_active = false;
 	}
 
-	drm_atomic_helper_swap_state(state->dev, state);
+	/*
+	 * swap crtc and connector state, plane state is already swapped in
+	 * __intel_set_mode_update_planes. Once .crtc_disable is fixed
+	 * all state should be swapped before disabling crtc's.
+	 */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		crtc->enabled = crtc_state->enable;
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state);
+
+		crtc->state->state = state;
+		swap(state->crtc_states[i], crtc->state);
+		crtc->state->state = NULL;
+	}
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		connector->state->state = state;
+		swap(state->connector_states[i], connector->state);
+		connector->state->state = NULL;
+	}
+
 	intel_modeset_fixup_state(state);
 
 	/* Double check state. */
@@ -11740,6 +11770,26 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
+static int intel_modeset_compute_planes(struct drm_atomic_state *state,
+					struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+
+	/* Add all overlay planes */
+	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
+		if (to_intel_plane(plane)->pipe == pipe) {
+			plane_state = drm_atomic_get_plane_state(state, plane);
+			if (IS_ERR(plane_state))
+				return PTR_ERR(plane_state);
+		}
+	}
+
+	return 0;
+}
+
 static struct intel_crtc_state *
 intel_modeset_compute_config(struct drm_crtc *crtc,
 			     struct drm_atomic_state *state)
@@ -11765,8 +11815,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (IS_ERR(pipe_config))
 		return pipe_config;
 
+	if (needs_modeset(&pipe_config->base)) {
+		ret = intel_modeset_compute_planes(state, crtc);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	if (!pipe_config->base.enable)
-		return pipe_config;
+		goto done;
 
 	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
 	if (ret)
@@ -11784,8 +11840,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	 * 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]");
+done:
 	ret = drm_atomic_helper_check_planes(state->dev, state);
 	if (ret)
 		return ERR_PTR(ret);
@@ -11869,6 +11925,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void __intel_set_mode_update_planes(struct drm_device *dev,
+					   struct drm_atomic_state *state)
+{
+	int i;
+	struct drm_plane_state *old_plane_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+
+	/*
+	 * For now only swap plane state, should be replaced with a
+	 * call to drm_atomic_helper_swap_state
+	 */
+	for_each_plane_in_state(state, plane, old_plane_state, 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;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		const struct drm_crtc_helper_funcs *funcs;
+
+		funcs = crtc->helper_private;
+
+		if (!funcs || !funcs->atomic_begin)
+			continue;
+
+		/* XXX: Hack because crtc state is not swapped */
+		crtc->state->mode_changed = crtc_state->mode_changed;
+		crtc->state->active_changed = crtc_state->active_changed;
+
+		DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i);
+		funcs->atomic_begin(crtc);
+	}
+
+	for_each_plane_in_state(state, plane, old_plane_state, i) {
+		bool visible = to_intel_plane_state(plane->state)->visible;
+		struct intel_plane *intel_plane = to_intel_plane(plane);
+		const struct drm_plane_helper_funcs *funcs;
+
+		crtc = plane->state->crtc;
+		funcs = plane->helper_private;
+
+		if (!funcs)
+			continue;
+
+		DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible);
+
+		if (!visible)
+			funcs->atomic_update(plane, old_plane_state);
+		else if (crtc->state->mode_changed)
+			intel_plane->disable_plane(plane, crtc, true);
+	}
+}
+
+static void __intel_set_mode_cleanup_planes(struct drm_device *dev,
+					    struct drm_atomic_state *old_state)
+{
+	int nplanes = dev->mode_config.num_total_plane;
+	int ncrtcs = dev->mode_config.num_crtc;
+	int i;
+
+	for (i = 0; i < nplanes; i++) {
+		const struct drm_plane_helper_funcs *funcs;
+		struct drm_plane *plane = old_state->planes[i];
+		struct drm_plane_state *old_plane_state;
+
+		if (!plane)
+			continue;
+
+		funcs = plane->helper_private;
+
+		if (!funcs)
+			continue;
+
+		old_plane_state = old_state->plane_states[i];
+
+		if (to_intel_plane_state(plane->state)->visible) {
+			DRM_DEBUG_ATOMIC("Plane %i is updated\n", i);
+			funcs->atomic_update(plane, old_plane_state);
+		} else
+			DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i);
+	}
+
+	for (i = 0; i < ncrtcs; i++) {
+		const struct drm_crtc_helper_funcs *funcs;
+		struct drm_crtc *crtc = old_state->crtcs[i];
+
+		if (!crtc)
+			continue;
+
+		funcs = crtc->helper_private;
+
+		if (!funcs || !funcs->atomic_flush)
+			continue;
+
+		DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i);
+		funcs->atomic_flush(crtc);
+	}
+	drm_atomic_helper_cleanup_planes(dev, old_state);
+}
+
 static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			    struct intel_crtc_state *pipe_config)
 {
@@ -11877,7 +12040,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	struct drm_atomic_state *state = pipe_config->base.state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	int ret = 0;
+	int ret;
 	int i;
 
 	ret = __intel_set_mode_checks(state);
@@ -11888,14 +12051,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	if (ret)
 		return ret;
 
+	__intel_set_mode_update_planes(dev, state);
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state))
 			continue;
 
-		intel_crtc_disable_planes(crtc);
+		intel_crtc_dpms_overlay_disable(to_intel_crtc(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
@@ -11926,8 +12089,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	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) {
 		if (!crtc->state->enable)
@@ -11935,13 +12096,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 		update_scanline_offset(to_intel_crtc(crtc));
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
+		if (needs_modeset(crtc->state))
+			dev_priv->display.crtc_enable(crtc);
 	}
 
-	/* FIXME: add subpixel order */
+	__intel_set_mode_cleanup_planes(dev, state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+	/* FIXME: add subpixel order */
 
 	drm_atomic_state_free(state);
 
@@ -12170,6 +12331,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 			return ret;
 
 		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
+		crtc_state->active = crtc_state->enable;
 	}
 
 	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
@@ -12190,20 +12352,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);
@@ -12242,38 +12395,7 @@ 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);
@@ -12413,6 +12535,9 @@ bool intel_wm_need_update(struct drm_plane *plane,
 	    plane->state->rotation != state->rotation)
 		return true;
 
+	if (plane->state->crtc_w != state->crtc_w)
+		return true;
+
 	return false;
 }
 
@@ -12507,74 +12632,22 @@ intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	bool can_position = false;
-	int ret;
-
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
 
 	if (INTEL_INFO(dev)->gen >= 9)
 		can_position = true;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    src, dest, clip,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    can_position, true,
-					    &state->visible);
-	if (ret)
-		return ret;
-
-	if (intel_crtc->active) {
-		struct intel_plane_state *old_state =
-			to_intel_plane_state(plane->state);
-
-		intel_crtc->atomic.wait_for_flips = true;
-
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-		if (state->visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    state->base.rotation != BIT(DRM_ROTATE_0)) {
-			intel_crtc->atomic.disable_fbc = true;
-		}
-
-		if (state->visible && !old_state->visible) {
-			/*
-			 * BDW signals flip done immediately if the plane
-			 * is disabled, even if the plane enable is already
-			 * armed to occur at the next vblank :(
-			 */
-			if (IS_BROADWELL(dev))
-				intel_crtc->atomic.wait_vblank = true;
-		}
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
-
-		intel_crtc->atomic.update_fbc = true;
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-	}
-
-	return 0;
+	return drm_plane_helper_check_update(plane, crtc, fb,
+					     src, dest, clip,
+					     DRM_PLANE_HELPER_NO_SCALING,
+					     DRM_PLANE_HELPER_NO_SCALING,
+					     can_position, true,
+					     &state->visible);
 }
 
 static void
@@ -12600,8 +12673,10 @@ intel_commit_primary_plane(struct drm_plane *plane,
 			/* FIXME: kill this fastboot hack */
 			intel_update_pipe_size(intel_crtc);
 
-		dev_priv->display.update_primary_plane(crtc, plane->fb,
-						       crtc->x, crtc->y);
+		dev_priv->display.update_primary_plane(crtc,
+						       state->base.fb,
+						       state->src.x1 >> 16,
+						       state->src.y1 >> 16);
 	}
 }
 
@@ -12616,6 +12691,123 @@ intel_disable_primary_plane(struct drm_plane *plane,
 	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
 }
 
+/* Transitional checking here, mostly for plane updates */
+static int intel_atomic_check_crtc(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state)
+{
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned plane_mask;
+	int i, nplanes = dev->mode_config.num_total_plane, idx;
+	bool mode_changed = needs_modeset(crtc_state);
+	bool is_crtc_enabled = crtc_state->active;
+	bool was_crtc_enabled = crtc->state->active && intel_crtc->active;
+
+	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	intel_crtc->atomic.update_wm = mode_changed;
+
+	idx = drm_crtc_index(crtc);
+	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
+			 idx, was_crtc_enabled, is_crtc_enabled);
+
+	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
+	for (i = 0; i < nplanes; i++) {
+		struct intel_plane_state *plane_state, *old_plane_state;
+		struct intel_plane *plane = to_intel_plane(state->planes[i]);
+		bool turn_off, turn_on, visible, was_visible;
+		struct drm_framebuffer *fb;
+
+		if (!plane)
+			continue;
+
+		plane_state = to_intel_plane_state(state->plane_states[i]);
+		old_plane_state = to_intel_plane_state(plane->base.state);
+
+		was_visible = old_plane_state->visible && was_crtc_enabled;
+		visible = plane_state->visible && is_crtc_enabled;
+
+		turn_off = was_visible && (!visible || mode_changed);
+		turn_on = visible && (!was_visible || mode_changed);
+		fb = plane_state->base.fb;
+
+		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
+			drm_plane_index(&plane->base), fb ? fb->base.id : -1);
+		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
+			was_visible, visible, turn_off, turn_on, mode_changed);
+
+		/* plane being turned off as part of modeset or changes? */
+		if (intel_wm_need_update(&plane->base, &plane_state->base))
+			intel_crtc->atomic.update_wm = true;
+
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking as a special case
+		 */
+		if (old_plane_state->base.fb && !plane_state->base.fb)
+			intel_crtc->atomic.disabled_planes |=
+				(1 << drm_plane_index(&plane->base));
+
+		switch (plane->base.type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			intel_crtc->atomic.fb_bits |=
+				INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+
+			intel_crtc->atomic.wait_for_flips = true;
+			intel_crtc->atomic.pre_disable_primary = turn_off;
+			intel_crtc->atomic.post_enable_primary = turn_on;
+
+			if (turn_off)
+				intel_crtc->atomic.disable_fbc = true;
+
+			/*
+			 * FBC does not work on some platforms for rotated
+			 * planes, so disable it when rotation is not 0 and
+			 * update it when rotation is set back to 0.
+			 *
+			 * FIXME: This is redundant with the fbc update done in
+			 * the primary plane enable function except that that
+			 * one is done too late. We eventually need to unify
+			 * this.
+			 */
+
+			if (visible &&
+			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+			    dev_priv->fbc.crtc == intel_crtc &&
+			    plane_state->base.rotation != BIT(DRM_ROTATE_0))
+				intel_crtc->atomic.disable_fbc = true;
+
+			/*
+			 * BDW signals flip done immediately if the plane
+			 * is disabled, even if the plane enable is already
+			 * armed to occur at the next vblank :(
+			 */
+			if (turn_on && IS_BROADWELL(dev))
+				intel_crtc->atomic.wait_vblank = true;
+
+			intel_crtc->atomic.update_fbc = true;
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			intel_crtc->atomic.fb_bits |=
+				INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			intel_crtc->atomic.fb_bits |=
+				INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
+
+			if (turn_off) {
+				intel_crtc->atomic.wait_vblank = true;
+				intel_crtc->atomic.update_sprite_watermarks |=
+					(1 << drm_plane_index(&plane->base));
+			}
+			break;
+		}
+	}
+	return 0;
+}
+
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -12664,10 +12856,13 @@ 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 (intel_crtc->active && !needs_modeset(crtc->state) &&
+	    !dev_priv->power_domains.init_power_on)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
+	else
+		intel_crtc->atomic.evade = false;
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
@@ -12703,6 +12898,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 						       false, false);
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	crtc->state->mode_changed = false;
+	crtc->state->active_changed = false;
 }
 
 /**
@@ -12812,13 +13009,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct intel_crtc *intel_crtc;
 	unsigned stride;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -12830,7 +13023,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj)
-		goto finish;
+		return 0;
 
 	/* Check for which cursor types we support */
 	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
@@ -12847,19 +13040,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
 		DRM_DEBUG_KMS("cursor cannot be tiled\n");
-		ret = -EINVAL;
-	}
-
-finish:
-	if (intel_crtc->active) {
-		if (plane->state->crtc_w != state->base.crtc_w)
-			intel_crtc->atomic.update_wm = true;
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static void
@@ -14089,6 +14273,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 		WARN_ON(crtc->active);
 		crtc->base.state->enable = false;
+		crtc->base.state->active = false;
 		crtc->base.enabled = false;
 	}
 
@@ -14117,6 +14302,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			      crtc->active ? "enabled" : "disabled");
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		/* Because we only establish the connector -> encoder ->
@@ -14255,6 +14441,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 								 crtc->config);
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		plane_state = to_intel_plane_state(primary->state);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7419e04b113f..5a277757ac2d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int max_scale, min_scale;
 	int pixel_size;
 
-	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
-
-	if (!fb) {
-		state->visible = false;
-		goto finish;
-	}
+	if (!fb)
+		return 0;
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
@@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
 	BUG_ON(vscale < 0);
 
-	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	crtc_x = dst->x1;
 	crtc_y = dst->y1;
@@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	dst->y1 = crtc_y;
 	dst->y2 = crtc_y + crtc_h;
 
-finish:
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	if (intel_crtc->active) {
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-
-		if (!state->visible) {
-			/*
-			 * Avoid underruns when disabling the sprite.
-			 * FIXME remove once watermark updates are done properly.
-			 */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_sprite_watermarks |=
-				(1 << drm_plane_index(plane));
-		}
-	}
-
 	return 0;
 }
 
-- 
2.1.0

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

[PATCH v2 RFC 4/5] drm/i915: make plane helpers fully atomic

[Maarten Lankhorst]

This kills off most of the transitional sers and uses atomic plane updates
in the modeset path to update everything.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Changes since v1:
 - Add atomic and sprite planes during a modeset too so they
   will be restored.
 - Drop a WARN_ON(!crtc_state->enabled) in atomic_plane_check for
   cursor and sprite planes. Keep it for primary as this probably
   indicates we messed up somewhere.

 drivers/gpu/drm/i915/intel_atomic_plane.c |  58 ++--
 drivers/gpu/drm/i915/intel_display.c      | 485 +++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_sprite.c       |  33 +-
 3 files changed, 366 insertions(+), 210 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index f7bd3b8fa245..ba4ab392b6b0 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
+	intel_state->visible = false;
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
 	 * anything driver-specific we need to test in that case, so
 	 * just return success.
 	 */
-	if (!crtc)
+	if (!crtc) {
+		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
 		return 0;
+	}
+
+	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];
+	if (WARN_ON(!crtc_state))
+		return 0;
+
+	if (!crtc_state->enable) {
+		DRM_DEBUG_ATOMIC("Invisible: crtc off\n");
 
-	/* FIXME: temporary hack necessary while we still use the plane update
-	 * helper. */
-	if (state->state) {
-		crtc_state =
-			intel_atomic_get_crtc_state(state->state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	} else {
-		crtc_state = intel_crtc->config;
+		/*
+		 * Probably allowed after converting to atomic. Right
+		 * now it probably means we have the state confused.
+		 */
+		I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY);
+		return 0;
+	}
+
+	if (!crtc_state->active) {
+		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
+		return 0;
 	}
 
 	/*
@@ -155,24 +163,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
-	intel_state->clip.x2 =
-		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
-	intel_state->clip.y2 =
-		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
-
-	/*
-	 * Disabling a plane is always okay; we just need to update
-	 * fb tracking in a special way since cleanup_fb() won't
-	 * get called by the plane helpers.
-	 */
-	if (state->fb == NULL && plane->state->fb != NULL) {
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking as a special case
-		 */
-		intel_crtc->atomic.disabled_planes |=
-			(1 << drm_plane_index(plane));
-	}
+	intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w;
+	intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h;
 
 	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ffacb4c3a12..acb5c5bea428 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
+static int intel_atomic_check_crtc(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state);
 static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
 static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
 static void intel_crtc_enable_planes(struct drm_crtc *crtc);
 static void intel_crtc_disable_planes(struct drm_crtc *crtc);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
+static void intel_post_enable_primary(struct drm_crtc *crtc);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane_state *plane_state =
+		to_intel_plane_state(crtc->primary->state);
+	bool was_visible = plane_state->visible;
 
-	if (dev_priv->display.disable_fbc)
+	/* Not supported right now by the helper, but lets be thorough. */
+	if (was_visible && !fb)
+		intel_pre_disable_primary(crtc);
+	else if (was_visible && dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
 
+	plane_state->visible = !!fb;
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
+	if (!was_visible && fb)
+		intel_post_enable_primary(crtc);
 
 	return 0;
 }
@@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev)
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		drm_modeset_lock(&crtc->mutex, NULL);
-		/*
-		 * FIXME: Once we have proper support for primary planes (and
-		 * disabling them without disabling the entire crtc) allow again
-		 * a NULL crtc->primary->fb.
-		 */
-		if (intel_crtc->active && crtc->primary->fb)
+
+		if (intel_crtc->active) {
+			const struct intel_plane_state *state =
+				to_intel_plane_state(crtc->primary->state);
+
 			dev_priv->display.update_primary_plane(crtc,
-							       crtc->primary->fb,
-							       crtc->x,
-							       crtc->y);
+							state->base.fb,
+							state->src.x1 >> 16,
+							state->src.y1 >> 16);
+		}
+
 		drm_modeset_unlock(&crtc->mutex);
 	}
 }
@@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.load_lut = intel_crtc_load_lut,
 	.atomic_begin = intel_begin_crtc_commit,
 	.atomic_flush = intel_finish_crtc_commit,
+	.atomic_check = intel_atomic_check_crtc,
 };
 
 /**
@@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
  */
 static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 {
-	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
@@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 			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);
@@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
 
 	intel_shared_dpll_commit(dev_priv);
 
@@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		intel_encoder->connectors_active = false;
 	}
 
-	drm_atomic_helper_swap_state(state->dev, state);
+	/*
+	 * swap crtc and connector state, plane state is already swapped in
+	 * __intel_set_mode_update_planes. Once .crtc_disable is fixed
+	 * all state should be swapped before disabling crtc's.
+	 */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		crtc->enabled = crtc_state->enable;
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state);
+
+		crtc->state->state = state;
+		swap(state->crtc_states[i], crtc->state);
+		crtc->state->state = NULL;
+	}
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		connector->state->state = state;
+		swap(state->connector_states[i], connector->state);
+		connector->state->state = NULL;
+	}
+
 	intel_modeset_fixup_state(state);
 
 	/* Double check state. */
@@ -11740,6 +11770,30 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
+static int intel_modeset_compute_planes(struct drm_atomic_state *state,
+					struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+
+	plane_state = drm_atomic_get_plane_state(state, crtc->cursor);
+	if (IS_ERR(plane_state))
+		return PTR_ERR(plane_state);
+
+	/* Add all overlay planes */
+	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
+		if (to_intel_plane(plane)->pipe == pipe) {
+			plane_state = drm_atomic_get_plane_state(state, plane);
+			if (IS_ERR(plane_state))
+				return PTR_ERR(plane_state);
+		}
+	}
+
+	return 0;
+}
+
 static struct intel_crtc_state *
 intel_modeset_compute_config(struct drm_crtc *crtc,
 			     struct drm_atomic_state *state)
@@ -11765,8 +11819,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (IS_ERR(pipe_config))
 		return pipe_config;
 
+	if (needs_modeset(&pipe_config->base)) {
+		ret = intel_modeset_compute_planes(state, crtc);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	if (!pipe_config->base.enable)
-		return pipe_config;
+		goto done;
 
 	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
 	if (ret)
@@ -11784,8 +11844,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	 * 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]");
+done:
 	ret = drm_atomic_helper_check_planes(state->dev, state);
 	if (ret)
 		return ERR_PTR(ret);
@@ -11869,6 +11929,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void __intel_set_mode_update_planes(struct drm_device *dev,
+					   struct drm_atomic_state *state)
+{
+	int i;
+	struct drm_plane_state *old_plane_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+
+	/*
+	 * For now only swap plane state, should be replaced with a
+	 * call to drm_atomic_helper_swap_state
+	 */
+	for_each_plane_in_state(state, plane, old_plane_state, 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;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		const struct drm_crtc_helper_funcs *funcs;
+
+		funcs = crtc->helper_private;
+
+		if (!funcs || !funcs->atomic_begin)
+			continue;
+
+		/* XXX: Hack because crtc state is not swapped */
+		crtc->state->mode_changed = crtc_state->mode_changed;
+		crtc->state->active_changed = crtc_state->active_changed;
+
+		DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i);
+		funcs->atomic_begin(crtc);
+	}
+
+	for_each_plane_in_state(state, plane, old_plane_state, i) {
+		bool visible = to_intel_plane_state(plane->state)->visible;
+		struct intel_plane *intel_plane = to_intel_plane(plane);
+		const struct drm_plane_helper_funcs *funcs;
+
+		crtc = plane->state->crtc;
+		funcs = plane->helper_private;
+
+		if (!funcs)
+			continue;
+
+		DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible);
+
+		if (!visible)
+			funcs->atomic_update(plane, old_plane_state);
+		else if (crtc->state->mode_changed)
+			intel_plane->disable_plane(plane, crtc, true);
+	}
+}
+
+static void __intel_set_mode_cleanup_planes(struct drm_device *dev,
+					    struct drm_atomic_state *old_state)
+{
+	int nplanes = dev->mode_config.num_total_plane;
+	int ncrtcs = dev->mode_config.num_crtc;
+	int i;
+
+	for (i = 0; i < nplanes; i++) {
+		const struct drm_plane_helper_funcs *funcs;
+		struct drm_plane *plane = old_state->planes[i];
+		struct drm_plane_state *old_plane_state;
+
+		if (!plane)
+			continue;
+
+		funcs = plane->helper_private;
+
+		if (!funcs)
+			continue;
+
+		old_plane_state = old_state->plane_states[i];
+
+		if (to_intel_plane_state(plane->state)->visible) {
+			DRM_DEBUG_ATOMIC("Plane %i is updated\n", i);
+			funcs->atomic_update(plane, old_plane_state);
+		} else
+			DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i);
+	}
+
+	for (i = 0; i < ncrtcs; i++) {
+		const struct drm_crtc_helper_funcs *funcs;
+		struct drm_crtc *crtc = old_state->crtcs[i];
+
+		if (!crtc)
+			continue;
+
+		funcs = crtc->helper_private;
+
+		if (!funcs || !funcs->atomic_flush)
+			continue;
+
+		DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i);
+		funcs->atomic_flush(crtc);
+	}
+	drm_atomic_helper_cleanup_planes(dev, old_state);
+}
+
 static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			    struct intel_crtc_state *pipe_config)
 {
@@ -11877,7 +12044,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	struct drm_atomic_state *state = pipe_config->base.state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	int ret = 0;
+	int ret;
 	int i;
 
 	ret = __intel_set_mode_checks(state);
@@ -11888,14 +12055,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	if (ret)
 		return ret;
 
+	__intel_set_mode_update_planes(dev, state);
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state))
 			continue;
 
-		intel_crtc_disable_planes(crtc);
+		intel_crtc_dpms_overlay_disable(to_intel_crtc(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
@@ -11926,8 +12093,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	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) {
 		if (!crtc->state->enable)
@@ -11935,13 +12100,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 		update_scanline_offset(to_intel_crtc(crtc));
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
+		if (needs_modeset(crtc->state))
+			dev_priv->display.crtc_enable(crtc);
 	}
 
-	/* FIXME: add subpixel order */
+	__intel_set_mode_cleanup_planes(dev, state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+	/* FIXME: add subpixel order */
 
 	drm_atomic_state_free(state);
 
@@ -12170,6 +12335,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 			return ret;
 
 		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
+		crtc_state->active = crtc_state->enable;
 	}
 
 	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
@@ -12190,20 +12356,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);
@@ -12242,38 +12399,7 @@ 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);
@@ -12413,6 +12539,9 @@ bool intel_wm_need_update(struct drm_plane *plane,
 	    plane->state->rotation != state->rotation)
 		return true;
 
+	if (plane->state->crtc_w != state->crtc_w)
+		return true;
+
 	return false;
 }
 
@@ -12507,74 +12636,22 @@ intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	bool can_position = false;
-	int ret;
-
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
 
 	if (INTEL_INFO(dev)->gen >= 9)
 		can_position = true;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    src, dest, clip,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    can_position, true,
-					    &state->visible);
-	if (ret)
-		return ret;
-
-	if (intel_crtc->active) {
-		struct intel_plane_state *old_state =
-			to_intel_plane_state(plane->state);
-
-		intel_crtc->atomic.wait_for_flips = true;
-
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-		if (state->visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    state->base.rotation != BIT(DRM_ROTATE_0)) {
-			intel_crtc->atomic.disable_fbc = true;
-		}
-
-		if (state->visible && !old_state->visible) {
-			/*
-			 * BDW signals flip done immediately if the plane
-			 * is disabled, even if the plane enable is already
-			 * armed to occur at the next vblank :(
-			 */
-			if (IS_BROADWELL(dev))
-				intel_crtc->atomic.wait_vblank = true;
-		}
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
-
-		intel_crtc->atomic.update_fbc = true;
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-	}
-
-	return 0;
+	return drm_plane_helper_check_update(plane, crtc, fb,
+					     src, dest, clip,
+					     DRM_PLANE_HELPER_NO_SCALING,
+					     DRM_PLANE_HELPER_NO_SCALING,
+					     can_position, true,
+					     &state->visible);
 }
 
 static void
@@ -12600,8 +12677,10 @@ intel_commit_primary_plane(struct drm_plane *plane,
 			/* FIXME: kill this fastboot hack */
 			intel_update_pipe_size(intel_crtc);
 
-		dev_priv->display.update_primary_plane(crtc, plane->fb,
-						       crtc->x, crtc->y);
+		dev_priv->display.update_primary_plane(crtc,
+						       state->base.fb,
+						       state->src.x1 >> 16,
+						       state->src.y1 >> 16);
 	}
 }
 
@@ -12616,6 +12695,123 @@ intel_disable_primary_plane(struct drm_plane *plane,
 	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
 }
 
+/* Transitional checking here, mostly for plane updates */
+static int intel_atomic_check_crtc(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state)
+{
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned plane_mask;
+	int i, nplanes = dev->mode_config.num_total_plane, idx;
+	bool mode_changed = needs_modeset(crtc_state);
+	bool is_crtc_enabled = crtc_state->active;
+	bool was_crtc_enabled = crtc->state->active && intel_crtc->active;
+
+	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	intel_crtc->atomic.update_wm = mode_changed;
+
+	idx = drm_crtc_index(crtc);
+	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
+			 idx, was_crtc_enabled, is_crtc_enabled);
+
+	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
+	for (i = 0; i < nplanes; i++) {
+		struct intel_plane_state *plane_state, *old_plane_state;
+		struct intel_plane *plane = to_intel_plane(state->planes[i]);
+		bool turn_off, turn_on, visible, was_visible;
+		struct drm_framebuffer *fb;
+
+		if (!plane)
+			continue;
+
+		plane_state = to_intel_plane_state(state->plane_states[i]);
+		old_plane_state = to_intel_plane_state(plane->base.state);
+
+		was_visible = old_plane_state->visible && was_crtc_enabled;
+		visible = plane_state->visible && is_crtc_enabled;
+
+		turn_off = was_visible && (!visible || mode_changed);
+		turn_on = visible && (!was_visible || mode_changed);
+		fb = plane_state->base.fb;
+
+		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
+			drm_plane_index(&plane->base), fb ? fb->base.id : -1);
+		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
+			was_visible, visible, turn_off, turn_on, mode_changed);
+
+		/* plane being turned off as part of modeset or changes? */
+		if (intel_wm_need_update(&plane->base, &plane_state->base))
+			intel_crtc->atomic.update_wm = true;
+
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking as a special case
+		 */
+		if (old_plane_state->base.fb && !plane_state->base.fb)
+			intel_crtc->atomic.disabled_planes |=
+				(1 << drm_plane_index(&plane->base));
+
+		switch (plane->base.type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			intel_crtc->atomic.fb_bits |=
+				INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+
+			intel_crtc->atomic.wait_for_flips = true;
+			intel_crtc->atomic.pre_disable_primary = turn_off;
+			intel_crtc->atomic.post_enable_primary = turn_on;
+
+			if (turn_off)
+				intel_crtc->atomic.disable_fbc = true;
+
+			/*
+			 * FBC does not work on some platforms for rotated
+			 * planes, so disable it when rotation is not 0 and
+			 * update it when rotation is set back to 0.
+			 *
+			 * FIXME: This is redundant with the fbc update done in
+			 * the primary plane enable function except that that
+			 * one is done too late. We eventually need to unify
+			 * this.
+			 */
+
+			if (visible &&
+			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+			    dev_priv->fbc.crtc == intel_crtc &&
+			    plane_state->base.rotation != BIT(DRM_ROTATE_0))
+				intel_crtc->atomic.disable_fbc = true;
+
+			/*
+			 * BDW signals flip done immediately if the plane
+			 * is disabled, even if the plane enable is already
+			 * armed to occur at the next vblank :(
+			 */
+			if (turn_on && IS_BROADWELL(dev))
+				intel_crtc->atomic.wait_vblank = true;
+
+			intel_crtc->atomic.update_fbc = true;
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			intel_crtc->atomic.fb_bits |=
+				INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			intel_crtc->atomic.fb_bits |=
+				INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
+
+			if (turn_off) {
+				intel_crtc->atomic.wait_vblank = true;
+				intel_crtc->atomic.update_sprite_watermarks |=
+					(1 << drm_plane_index(&plane->base));
+			}
+			break;
+		}
+	}
+	return 0;
+}
+
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -12664,10 +12860,13 @@ 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 (intel_crtc->active && !needs_modeset(crtc->state) &&
+	    !dev_priv->power_domains.init_power_on)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
+	else
+		intel_crtc->atomic.evade = false;
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
@@ -12703,6 +12902,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 						       false, false);
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	crtc->state->mode_changed = false;
+	crtc->state->active_changed = false;
 }
 
 /**
@@ -12812,13 +13013,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct intel_crtc *intel_crtc;
 	unsigned stride;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -12830,7 +13027,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj)
-		goto finish;
+		return 0;
 
 	/* Check for which cursor types we support */
 	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
@@ -12847,19 +13044,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
 		DRM_DEBUG_KMS("cursor cannot be tiled\n");
-		ret = -EINVAL;
-	}
-
-finish:
-	if (intel_crtc->active) {
-		if (plane->state->crtc_w != state->base.crtc_w)
-			intel_crtc->atomic.update_wm = true;
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static void
@@ -14089,6 +14277,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 		WARN_ON(crtc->active);
 		crtc->base.state->enable = false;
+		crtc->base.state->active = false;
 		crtc->base.enabled = false;
 	}
 
@@ -14117,6 +14306,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			      crtc->active ? "enabled" : "disabled");
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		/* Because we only establish the connector -> encoder ->
@@ -14255,6 +14445,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 								 crtc->config);
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		plane_state = to_intel_plane_state(primary->state);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7419e04b113f..5a277757ac2d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int max_scale, min_scale;
 	int pixel_size;
 
-	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
-
-	if (!fb) {
-		state->visible = false;
-		goto finish;
-	}
+	if (!fb)
+		return 0;
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
@@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
 	BUG_ON(vscale < 0);
 
-	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	crtc_x = dst->x1;
 	crtc_y = dst->y1;
@@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	dst->y1 = crtc_y;
 	dst->y2 = crtc_y + crtc_h;
 
-finish:
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	if (intel_crtc->active) {
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-
-		if (!state->visible) {
-			/*
-			 * Avoid underruns when disabling the sprite.
-			 * FIXME remove once watermark updates are done properly.
-			 */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_sprite_watermarks |=
-				(1 << drm_plane_index(plane));
-		}
-	}
-
 	return 0;
 }
 
-- 
2.1.0


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

Re: [PATCH v2 RFC 4/5] drm/i915: make plane helpers fully atomic

[Ander Conselvan De Oliveira]

On Thu, 2015-04-23 at 08:19 +0200, Maarten Lankhorst wrote:
> This kills off most of the transitional sers and uses atomic plane updates
> in the modeset path to update everything.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Changes since v1:
>  - Add atomic and sprite planes during a modeset too so they
>    will be restored.
>  - Drop a WARN_ON(!crtc_state->enabled) in atomic_plane_check for
>    cursor and sprite planes. Keep it for primary as this probably
>    indicates we messed up somewhere.
> 
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  58 ++--
>  drivers/gpu/drm/i915/intel_display.c      | 485 +++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_sprite.c       |  33 +-
>  3 files changed, 366 insertions(+), 210 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index f7bd3b8fa245..ba4ab392b6b0 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  				    struct drm_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *crtc_state;
> +	struct drm_crtc_state *crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
>  
> -	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
> +	intel_state->visible = false;
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
>  	 * anything driver-specific we need to test in that case, so
>  	 * just return success.
>  	 */
> -	if (!crtc)
> +	if (!crtc) {
> +		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
>  		return 0;
> +	}
> +
> +	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];
> +	if (WARN_ON(!crtc_state))
> +		return 0;
> +
> +	if (!crtc_state->enable) {
> +		DRM_DEBUG_ATOMIC("Invisible: crtc off\n");
>  
> -	/* FIXME: temporary hack necessary while we still use the plane update
> -	 * helper. */
> -	if (state->state) {
> -		crtc_state =
> -			intel_atomic_get_crtc_state(state->state, intel_crtc);
> -		if (IS_ERR(crtc_state))
> -			return PTR_ERR(crtc_state);
> -	} else {
> -		crtc_state = intel_crtc->config;
> +		/*
> +		 * Probably allowed after converting to atomic. Right
> +		 * now it probably means we have the state confused.
> +		 */
> +		I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY);
> +		return 0;
> +	}
> +
> +	if (!crtc_state->active) {
> +		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
> +		return 0;
>  	}
>  
>  	/*
> @@ -155,24 +163,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> -	intel_state->clip.x2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> -	intel_state->clip.y2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> -
> -	/*
> -	 * Disabling a plane is always okay; we just need to update
> -	 * fb tracking in a special way since cleanup_fb() won't
> -	 * get called by the plane helpers.
> -	 */
> -	if (state->fb == NULL && plane->state->fb != NULL) {
> -		/*
> -		 * 'prepare' is never called when plane is being disabled, so
> -		 * we need to handle frontbuffer tracking as a special case
> -		 */
> -		intel_crtc->atomic.disabled_planes |=
> -			(1 << drm_plane_index(plane));
> -	}
> +	intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w;
> +	intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h;
>  
>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ffacb4c3a12..acb5c5bea428 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
>  static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *crtc_state);
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
>  static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state);
>  static void intel_crtc_enable_planes(struct drm_crtc *crtc);
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc);
> +static void intel_pre_disable_primary(struct drm_crtc *crtc);
> +static void intel_post_enable_primary(struct drm_crtc *crtc);
>  
>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>  {
> @@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane_state *plane_state =
> +		to_intel_plane_state(crtc->primary->state);
> +	bool was_visible = plane_state->visible;
>  
> -	if (dev_priv->display.disable_fbc)
> +	/* Not supported right now by the helper, but lets be thorough. */
> +	if (was_visible && !fb)
> +		intel_pre_disable_primary(crtc);
> +	else if (was_visible && dev_priv->display.disable_fbc)
>  		dev_priv->display.disable_fbc(dev);
>  
> +	plane_state->visible = !!fb;
>  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> +	if (!was_visible && fb)
> +		intel_post_enable_primary(crtc);
>  
>  	return 0;
>  }
> @@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev)
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  		drm_modeset_lock(&crtc->mutex, NULL);
> -		/*
> -		 * FIXME: Once we have proper support for primary planes (and
> -		 * disabling them without disabling the entire crtc) allow again
> -		 * a NULL crtc->primary->fb.
> -		 */
> -		if (intel_crtc->active && crtc->primary->fb)
> +
> +		if (intel_crtc->active) {
> +			const struct intel_plane_state *state =
> +				to_intel_plane_state(crtc->primary->state);
> +
>  			dev_priv->display.update_primary_plane(crtc,
> -							       crtc->primary->fb,
> -							       crtc->x,
> -							       crtc->y);
> +							state->base.fb,
> +							state->src.x1 >> 16,
> +							state->src.y1 >> 16);
> +		}
> +
>  		drm_modeset_unlock(&crtc->mutex);
>  	}
>  }
> @@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.load_lut = intel_crtc_load_lut,
>  	.atomic_begin = intel_begin_crtc_commit,
>  	.atomic_flush = intel_finish_crtc_commit,
> +	.atomic_check = intel_atomic_check_crtc,
>  };
>  
>  /**
> @@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>   */
>  static void intel_modeset_fixup_state(struct drm_atomic_state *state)
>  {
> -	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
>  
> @@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state)
>  			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);
> @@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *connector;
> +	struct drm_connector_state *connector_state;
> +	int i;
>  
>  	intel_shared_dpll_commit(dev_priv);
>  
> @@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		intel_encoder->connectors_active = false;
>  	}
>  
> -	drm_atomic_helper_swap_state(state->dev, state);
> +	/*
> +	 * swap crtc and connector state, plane state is already swapped in
> +	 * __intel_set_mode_update_planes. Once .crtc_disable is fixed
> +	 * all state should be swapped before disabling crtc's.
> +	 */

Can't we just fix .crtc_disable() first?

> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		crtc->enabled = crtc_state->enable;
> +		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state);
> +
> +		crtc->state->state = state;
> +		swap(state->crtc_states[i], crtc->state);
> +		crtc->state->state = NULL;
> +	}
> +
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		connector->state->state = state;
> +		swap(state->connector_states[i], connector->state);
> +		connector->state->state = NULL;
> +	}
> +
>  	intel_modeset_fixup_state(state);
>  
>  	/* Double check state. */
> @@ -11740,6 +11770,30 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  		crtc->scanline_offset = 1;
>  }
>  
> +static int intel_modeset_compute_planes(struct drm_atomic_state *state,
> +					struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int pipe = intel_crtc->pipe;
> +	struct drm_plane_state *plane_state;
> +	struct drm_plane *plane;
> +
> +	plane_state = drm_atomic_get_plane_state(state, crtc->cursor);
> +	if (IS_ERR(plane_state))
> +		return PTR_ERR(plane_state);
> +
> +	/* Add all overlay planes */
> +	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
> +		if (to_intel_plane(plane)->pipe == pipe) {
> +			plane_state = drm_atomic_get_plane_state(state, plane);
> +			if (IS_ERR(plane_state))
> +				return PTR_ERR(plane_state);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static struct intel_crtc_state *
>  intel_modeset_compute_config(struct drm_crtc *crtc,
>  			     struct drm_atomic_state *state)
> @@ -11765,8 +11819,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	if (IS_ERR(pipe_config))
>  		return pipe_config;
>  
> +	if (needs_modeset(&pipe_config->base)) {
> +		ret = intel_modeset_compute_planes(state, crtc);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
>  	if (!pipe_config->base.enable)
> -		return pipe_config;
> +		goto done;
>  
>  	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
>  	if (ret)
> @@ -11784,8 +11844,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	 * 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]");
> +done:
>  	ret = drm_atomic_helper_check_planes(state->dev, state);
>  	if (ret)
>  		return ERR_PTR(ret);
> @@ -11869,6 +11929,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static void __intel_set_mode_update_planes(struct drm_device *dev,
> +					   struct drm_atomic_state *state)
> +{
> +	int i;
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +
> +	/*
> +	 * For now only swap plane state, should be replaced with a
> +	 * call to drm_atomic_helper_swap_state
> +	 */
> +	for_each_plane_in_state(state, plane, old_plane_state, 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;
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		const struct drm_crtc_helper_funcs *funcs;
> +
> +		funcs = crtc->helper_private;
> +
> +		if (!funcs || !funcs->atomic_begin)
> +			continue;
> +
> +		/* XXX: Hack because crtc state is not swapped */
> +		crtc->state->mode_changed = crtc_state->mode_changed;
> +		crtc->state->active_changed = crtc_state->active_changed;
> +
> +		DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i);
> +		funcs->atomic_begin(crtc);
> +	}
> +
> +	for_each_plane_in_state(state, plane, old_plane_state, i) {
> +		bool visible = to_intel_plane_state(plane->state)->visible;
> +		struct intel_plane *intel_plane = to_intel_plane(plane);
> +		const struct drm_plane_helper_funcs *funcs;
> +
> +		crtc = plane->state->crtc;
> +		funcs = plane->helper_private;
> +
> +		if (!funcs)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible);
> +
> +		if (!visible)
> +			funcs->atomic_update(plane, old_plane_state);

I'm getting a NULL pointer dereference in intel_commit_primary_plane()
because of a NULL crtc when an already disabled plane is disabled again.
Should we just skip the update here?

> +		else if (crtc->state->mode_changed)
> +			intel_plane->disable_plane(plane, crtc, true);
> +	}
> +}
> +
> +static void __intel_set_mode_cleanup_planes(struct drm_device *dev,
> +					    struct drm_atomic_state *old_state)
> +{
> +	int nplanes = dev->mode_config.num_total_plane;
> +	int ncrtcs = dev->mode_config.num_crtc;
> +	int i;
> +
> +	for (i = 0; i < nplanes; i++) {
> +		const struct drm_plane_helper_funcs *funcs;
> +		struct drm_plane *plane = old_state->planes[i];
> +		struct drm_plane_state *old_plane_state;
> +
> +		if (!plane)
> +			continue;
> +
> +		funcs = plane->helper_private;
> +
> +		if (!funcs)
> +			continue;
> +
> +		old_plane_state = old_state->plane_states[i];
> +
> +		if (to_intel_plane_state(plane->state)->visible) {
> +			DRM_DEBUG_ATOMIC("Plane %i is updated\n", i);
> +			funcs->atomic_update(plane, old_plane_state);
> +		} else
> +			DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i);
> +	}
> +
> +	for (i = 0; i < ncrtcs; i++) {
> +		const struct drm_crtc_helper_funcs *funcs;
> +		struct drm_crtc *crtc = old_state->crtcs[i];
> +
> +		if (!crtc)
> +			continue;
> +
> +		funcs = crtc->helper_private;
> +
> +		if (!funcs || !funcs->atomic_flush)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i);
> +		funcs->atomic_flush(crtc);
> +	}

These loops could use for_each_{crtc,plane}_in_state macros.

> +	drm_atomic_helper_cleanup_planes(dev, old_state);
> +}
> +
>  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  			    struct intel_crtc_state *pipe_config)
>  {
> @@ -11877,7 +12044,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  	struct drm_atomic_state *state = pipe_config->base.state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	int ret = 0;
> +	int ret;
>  	int i;
>  
>  	ret = __intel_set_mode_checks(state);
> @@ -11888,14 +12055,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  	if (ret)
>  		return ret;
>  
> +	__intel_set_mode_update_planes(dev, state);
> +
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (!needs_modeset(crtc_state))
>  			continue;
>  
> -		intel_crtc_disable_planes(crtc);
> +		intel_crtc_dpms_overlay_disable(to_intel_crtc(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
> @@ -11926,8 +12093,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  
>  	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) {
>  		if (!crtc->state->enable)
> @@ -11935,13 +12100,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  
>  		update_scanline_offset(to_intel_crtc(crtc));
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> +		if (needs_modeset(crtc->state))
> +			dev_priv->display.crtc_enable(crtc);

In v2 of the patch that removes modeset_pipes and friends, I added a
check for needs_modeset() before update_scanline_offset().

>  	}
>  
> -	/* FIXME: add subpixel order */
> +	__intel_set_mode_cleanup_planes(dev, state);
>  
> -	drm_atomic_helper_cleanup_planes(dev, state);
> +	/* FIXME: add subpixel order */
>  
>  	drm_atomic_state_free(state);
>  
> @@ -12170,6 +12335,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  			return ret;
>  
>  		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
> +		crtc_state->active = crtc_state->enable;

Should this be a separate fix?

>  	}
>  
>  	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
> @@ -12190,20 +12356,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);
> @@ -12242,38 +12399,7 @@ 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);
> -	}
> -

Maybe this could be a separate patch?

>  	if (ret) {
>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>  			      set->crtc->base.id, ret);
> @@ -12413,6 +12539,9 @@ bool intel_wm_need_update(struct drm_plane *plane,
>  	    plane->state->rotation != state->rotation)
>  		return true;
>  
> +	if (plane->state->crtc_w != state->crtc_w)
> +		return true;
> +
>  	return false;
>  }
>  
> @@ -12507,74 +12636,22 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
>  	bool can_position = false;
> -	int ret;
> -
> -	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
>  
>  	if (INTEL_INFO(dev)->gen >= 9)
>  		can_position = true;
>  
> -	ret = drm_plane_helper_check_update(plane, crtc, fb,
> -					    src, dest, clip,
> -					    DRM_PLANE_HELPER_NO_SCALING,
> -					    DRM_PLANE_HELPER_NO_SCALING,
> -					    can_position, true,
> -					    &state->visible);
> -	if (ret)
> -		return ret;
> -
> -	if (intel_crtc->active) {
> -		struct intel_plane_state *old_state =
> -			to_intel_plane_state(plane->state);
> -
> -		intel_crtc->atomic.wait_for_flips = true;
> -
> -		/*
> -		 * FBC does not work on some platforms for rotated
> -		 * planes, so disable it when rotation is not 0 and
> -		 * update it when rotation is set back to 0.
> -		 *
> -		 * FIXME: This is redundant with the fbc update done in
> -		 * the primary plane enable function except that that
> -		 * one is done too late. We eventually need to unify
> -		 * this.
> -		 */
> -		if (state->visible &&
> -		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -		    dev_priv->fbc.crtc == intel_crtc &&
> -		    state->base.rotation != BIT(DRM_ROTATE_0)) {
> -			intel_crtc->atomic.disable_fbc = true;
> -		}
> -
> -		if (state->visible && !old_state->visible) {
> -			/*
> -			 * BDW signals flip done immediately if the plane
> -			 * is disabled, even if the plane enable is already
> -			 * armed to occur at the next vblank :(
> -			 */
> -			if (IS_BROADWELL(dev))
> -				intel_crtc->atomic.wait_vblank = true;
> -		}
> -
> -		intel_crtc->atomic.fb_bits |=
> -			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> -
> -		intel_crtc->atomic.update_fbc = true;
> -
> -		if (intel_wm_need_update(plane, &state->base))
> -			intel_crtc->atomic.update_wm = true;
> -	}
> -
> -	return 0;
> +	return drm_plane_helper_check_update(plane, crtc, fb,
> +					     src, dest, clip,
> +					     DRM_PLANE_HELPER_NO_SCALING,
> +					     DRM_PLANE_HELPER_NO_SCALING,
> +					     can_position, true,
> +					     &state->visible);
>  }
>  
>  static void
> @@ -12600,8 +12677,10 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  			/* FIXME: kill this fastboot hack */
>  			intel_update_pipe_size(intel_crtc);
>  
> -		dev_priv->display.update_primary_plane(crtc, plane->fb,
> -						       crtc->x, crtc->y);
> +		dev_priv->display.update_primary_plane(crtc,
> +						       state->base.fb,
> +						       state->src.x1 >> 16,
> +						       state->src.y1 >> 16);
>  	}
>  }
>  
> @@ -12616,6 +12695,123 @@ intel_disable_primary_plane(struct drm_plane *plane,
>  	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
>  }
>  
> +/* Transitional checking here, mostly for plane updates */
> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *crtc_state)
> +{
> +	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned plane_mask;
> +	int i, nplanes = dev->mode_config.num_total_plane, idx;
> +	bool mode_changed = needs_modeset(crtc_state);
> +	bool is_crtc_enabled = crtc_state->active;
> +	bool was_crtc_enabled = crtc->state->active && intel_crtc->active;
> +
> +	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> +	intel_crtc->atomic.update_wm = mode_changed;
> +
> +	idx = drm_crtc_index(crtc);
> +	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> +			 idx, was_crtc_enabled, is_crtc_enabled);
> +
> +	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
> +	for (i = 0; i < nplanes; i++) {
> +		struct intel_plane_state *plane_state, *old_plane_state;
> +		struct intel_plane *plane = to_intel_plane(state->planes[i]);
> +		bool turn_off, turn_on, visible, was_visible;
> +		struct drm_framebuffer *fb;
> +
> +		if (!plane)
> +			continue;
> +
> +		plane_state = to_intel_plane_state(state->plane_states[i]);
> +		old_plane_state = to_intel_plane_state(plane->base.state);
> +
> +		was_visible = old_plane_state->visible && was_crtc_enabled;
> +		visible = plane_state->visible && is_crtc_enabled;
> +
> +		turn_off = was_visible && (!visible || mode_changed);
> +		turn_on = visible && (!was_visible || mode_changed);
> +		fb = plane_state->base.fb;
> +
> +		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
> +			drm_plane_index(&plane->base), fb ? fb->base.id : -1);
> +		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
> +			was_visible, visible, turn_off, turn_on, mode_changed);
> +
> +		/* plane being turned off as part of modeset or changes? */
> +		if (intel_wm_need_update(&plane->base, &plane_state->base))
> +			intel_crtc->atomic.update_wm = true;
> +
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking as a special case
> +		 */
> +		if (old_plane_state->base.fb && !plane_state->base.fb)
> +			intel_crtc->atomic.disabled_planes |=
> +				(1 << drm_plane_index(&plane->base));
> +
> +		switch (plane->base.type) {
> +		case DRM_PLANE_TYPE_PRIMARY:
> +			intel_crtc->atomic.fb_bits |=
> +				INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> +
> +			intel_crtc->atomic.wait_for_flips = true;
> +			intel_crtc->atomic.pre_disable_primary = turn_off;
> +			intel_crtc->atomic.post_enable_primary = turn_on;
> +
> +			if (turn_off)
> +				intel_crtc->atomic.disable_fbc = true;
> +
> +			/*
> +			 * FBC does not work on some platforms for rotated
> +			 * planes, so disable it when rotation is not 0 and
> +			 * update it when rotation is set back to 0.
> +			 *
> +			 * FIXME: This is redundant with the fbc update done in
> +			 * the primary plane enable function except that that
> +			 * one is done too late. We eventually need to unify
> +			 * this.
> +			 */
> +
> +			if (visible &&
> +			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> +			    dev_priv->fbc.crtc == intel_crtc &&
> +			    plane_state->base.rotation != BIT(DRM_ROTATE_0))
> +				intel_crtc->atomic.disable_fbc = true;
> +
> +			/*
> +			 * BDW signals flip done immediately if the plane
> +			 * is disabled, even if the plane enable is already
> +			 * armed to occur at the next vblank :(
> +			 */
> +			if (turn_on && IS_BROADWELL(dev))
> +				intel_crtc->atomic.wait_vblank = true;
> +
> +			intel_crtc->atomic.update_fbc = true;
> +			break;
> +		case DRM_PLANE_TYPE_CURSOR:
> +			intel_crtc->atomic.fb_bits |=
> +				INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
> +			break;
> +		case DRM_PLANE_TYPE_OVERLAY:
> +			intel_crtc->atomic.fb_bits |=
> +				INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
> +
> +			if (turn_off) {
> +				intel_crtc->atomic.wait_vblank = true;
> +				intel_crtc->atomic.update_sprite_watermarks |=
> +					(1 << drm_plane_index(&plane->base));
> +			}
> +			break;
> +		}
> +	}
> +	return 0;
> +}
> +

Is there a specific reason why this needs to be in check_crtc vs
check_plane? IMO, it would make review easier if this move was a
separate patch.

>  static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -12664,10 +12860,13 @@ 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 (intel_crtc->active && !needs_modeset(crtc->state) &&
> +	    !dev_priv->power_domains.init_power_on)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> +	else
> +		intel_crtc->atomic.evade = false;
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> @@ -12703,6 +12902,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  						       false, false);
>  
>  	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> +	crtc->state->mode_changed = false;
> +	crtc->state->active_changed = false;
>  }
>  
>  /**
> @@ -12812,13 +13013,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct intel_crtc *intel_crtc;
>  	unsigned stride;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12830,7 +13027,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  
>  	/* if we want to turn off the cursor ignore width and height */
>  	if (!obj)
> -		goto finish;
> +		return 0;
>  
>  	/* Check for which cursor types we support */
>  	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
> @@ -12847,19 +13044,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  
>  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
>  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> -		ret = -EINVAL;
> -	}
> -
> -finish:
> -	if (intel_crtc->active) {
> -		if (plane->state->crtc_w != state->base.crtc_w)
> -			intel_crtc->atomic.update_wm = true;
> -
> -		intel_crtc->atomic.fb_bits |=
> -			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
> +		return -EINVAL;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static void
> @@ -14089,6 +14277,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  		WARN_ON(crtc->active);
>  		crtc->base.state->enable = false;
> +		crtc->base.state->active = false;
>  		crtc->base.enabled = false;
>  	}
>  
> @@ -14117,6 +14306,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  			      crtc->active ? "enabled" : "disabled");
>  
>  		crtc->base.state->enable = crtc->active;
> +		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
>  		/* Because we only establish the connector -> encoder ->
> @@ -14255,6 +14445,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  								 crtc->config);
>  
>  		crtc->base.state->enable = crtc->active;
> +		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  

Should this be a separate bug fix?


Ander

>  		plane_state = to_intel_plane_state(primary->state);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 7419e04b113f..5a277757ac2d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int max_scale, min_scale;
>  	int pixel_size;
>  
> -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> -
> -	if (!fb) {
> -		state->visible = false;
> -		goto finish;
> -	}
> +	if (!fb)
> +		return 0;
>  
>  	/* Don't modify another pipe's plane */
>  	if (intel_plane->pipe != intel_crtc->pipe) {
> @@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
>  	BUG_ON(vscale < 0);
>  
> -	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> +	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
>  
>  	crtc_x = dst->x1;
>  	crtc_y = dst->y1;
> @@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	dst->y1 = crtc_y;
>  	dst->y2 = crtc_y + crtc_h;
>  
> -finish:
> -	/*
> -	 * If the sprite is completely covering the primary plane,
> -	 * we can disable the primary and save power.
> -	 */
> -	if (intel_crtc->active) {
> -		intel_crtc->atomic.fb_bits |=
> -			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
> -
> -		if (intel_wm_need_update(plane, &state->base))
> -			intel_crtc->atomic.update_wm = true;
> -
> -		if (!state->visible) {
> -			/*
> -			 * Avoid underruns when disabling the sprite.
> -			 * FIXME remove once watermark updates are done properly.
> -			 */
> -			intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.update_sprite_watermarks |=
> -				(1 << drm_plane_index(plane));
> -		}
> -	}
> -
>  	return 0;
>  }
>  


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

[PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state

[maarten.lankhorst]

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

Assume the function is locked with drm_modeset_lock_all for now.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 99a45bee20d8..5a43ac02b925 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -106,8 +106,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
 static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
-static void intel_crtc_enable_planes(struct drm_crtc *crtc);
-static void intel_crtc_disable_planes(struct drm_crtc *crtc);
 static void intel_pre_disable_primary(struct drm_crtc *crtc);
 static void intel_post_enable_primary(struct drm_crtc *crtc);
 
@@ -2197,28 +2195,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 	POSTING_READ(reg);
 }
 
-/**
- * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
- * @plane:  plane to be enabled
- * @crtc: crtc for the plane
- *
- * Enable @plane on @crtc, making sure that the pipe is running first.
- */
-static void intel_enable_primary_hw_plane(struct drm_plane *plane,
-					  struct drm_crtc *crtc)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* If the pipe isn't enabled, we can't pump pixels and may hang */
-	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
-	to_intel_plane_state(plane->state)->visible = true;
-
-	dev_priv->display.update_primary_plane(crtc, plane->fb,
-					       crtc->x, crtc->y);
-}
-
 static bool need_vtd_wa(struct drm_device *dev)
 {
 #ifdef CONFIG_INTEL_IOMMU
@@ -4455,20 +4431,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-static void intel_enable_sprite_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	enum pipe pipe = to_intel_crtc(crtc)->pipe;
-	struct drm_plane *plane;
-	struct intel_plane *intel_plane;
-
-	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
-		intel_plane = to_intel_plane(plane);
-		if (intel_plane->pipe == pipe)
-			intel_plane_restore(&intel_plane->base);
-	}
-}
-
 void hsw_enable_ips(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -4698,44 +4660,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_crtc_enable_planes(struct drm_crtc *crtc)
-{
-	intel_enable_primary_hw_plane(crtc->primary, crtc);
-	intel_enable_sprite_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
-
-	intel_post_enable_primary(crtc);
-}
-
-static void intel_crtc_disable_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-	int pipe = intel_crtc->pipe;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	intel_pre_disable_primary(crtc);
-
-	intel_crtc_dpms_overlay_disable(intel_crtc);
-	for_each_intel_plane(dev, intel_plane) {
-		if (intel_plane->pipe == pipe) {
-			struct drm_crtc *from = intel_plane->base.crtc;
-
-			intel_plane->disable_plane(&intel_plane->base,
-						   from ?: crtc, true);
-		}
-	}
-
-	/*
-	 * FIXME: Once we grow proper nuclear flip support out of this we need
-	 * to compute the mask of flip planes precisely. For the time being
-	 * consider this a flip to a NULL plane.
-	 */
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
-}
-
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -5728,10 +5652,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum intel_display_power_domain domain;
-	unsigned long domains;
+	struct intel_crtc_state *pipe_config;
+	struct drm_plane_state *plane_state;
+	struct drm_atomic_state *state;
+	int ret;
 
 	if (enable == intel_crtc->active)
 		return;
@@ -5740,23 +5667,41 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 		return;
 
 	crtc->state->active = enable;
-	if (enable) {
-		domains = get_crtc_power_domains(crtc);
-		for_each_power_domain(domain, domains)
-			intel_display_power_get(dev_priv, domain);
-		intel_crtc->enabled_power_domains = domains;
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
-	} else {
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+	/* this function should be called with drm_modeset_lock_all for now */
+	if (WARN_ON(!ctx))
+		return;
+	lockdep_assert_held(&ctx->ww_ctx);
 
-		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 = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return;
+
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
+
+	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto err;
+	}
+	pipe_config->base.active = enable;
+
+	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
+	if (IS_ERR(plane_state)) {
+		ret = PTR_ERR(plane_state);
+		goto err;
 	}
+
+	ret = intel_set_mode(crtc, state);
+	if (!ret)
+		return;
+
+	DRM_ERROR("Failed to toggle crtc!\n");
+
+err:
+	DRM_ERROR("Updating crtc active failed with %i\n", ret);
+	drm_atomic_state_free(state);
 }
 
 /**
-- 
2.1.0

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

Re: [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state

[Maarten Lankhorst]

Op 22-04-15 om 13:24 schreef maarten.lankhorst@linux.intel.com:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Assume the function is locked with drm_modeset_lock_all for now.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++-------------------------
>  1 file changed, 38 insertions(+), 93 deletions(-)
> <snip>
>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -5728,10 +5652,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum intel_display_power_domain domain;
> -	unsigned long domains;
> +	struct intel_crtc_state *pipe_config;
> +	struct drm_plane_state *plane_state;
> +	struct drm_atomic_state *state;
> +	int ret;
>  
>  	if (enable == intel_crtc->active)
>  		return;
> @@ -5740,23 +5667,41 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  		return;
>  
>  	crtc->state->active = enable;
^Oops, this line should be removed now or this patch will glitch badly.

And a lot of places that use state->enable should use state->active instead.

Still leaving this here as it's a RFC. :-)
> -	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;
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> -	} else {
> -		intel_crtc_disable_planes(crtc);
> -		dev_priv->display.crtc_disable(crtc);
> +	/* this function should be called with drm_modeset_lock_all for now */
> +	if (WARN_ON(!ctx))
> +		return;
> +	lockdep_assert_held(&ctx->ww_ctx);
>  
> -		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 = drm_atomic_state_alloc(dev);
> +	if (WARN_ON(!state))
> +		return;
> +
> +	state->acquire_ctx = ctx;
> +	state->allow_modeset = true;
> +
> +	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto err;
> +	}
> +	pipe_config->base.active = enable;
> +
> +	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
> +	if (IS_ERR(plane_state)) {
> +		ret = PTR_ERR(plane_state);
> +		goto err;
>  	}
> +
> +	ret = intel_set_mode(crtc, state);
> +	if (!ret)
> +		return;
> +
> +	DRM_ERROR("Failed to toggle crtc!\n");
> +
> +err:
> +	DRM_ERROR("Updating crtc active failed with %i\n", ret);
> +	drm_atomic_state_free(state);
>  }
>  
>  /**

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

[PATCH v2 RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state

[Maarten Lankhorst]

Assume the function is locked with drm_modeset_lock_all for now.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Changes since RFC v1:
 - Get rid of the crtc->state->active assignment in intel_crtc_control,
   it caused the whole state to be confused.
 - Convert some places that use state->enable to state->active.
 - crtc->state->active should mirror crtc->active now, latter should
   be removed at some point.

 drivers/gpu/drm/i915/i915_irq.c      |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 174 +++++++++++++----------------------
 2 files changed, 65 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9da955e4f355..a6816503a080 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 acb5c5bea428..2d2ada580b36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -106,8 +106,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
 static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
-static void intel_crtc_enable_planes(struct drm_crtc *crtc);
-static void intel_crtc_disable_planes(struct drm_crtc *crtc);
 static void intel_pre_disable_primary(struct drm_crtc *crtc);
 static void intel_post_enable_primary(struct drm_crtc *crtc);
 
@@ -2197,28 +2195,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 	POSTING_READ(reg);
 }
 
-/**
- * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
- * @plane:  plane to be enabled
- * @crtc: crtc for the plane
- *
- * Enable @plane on @crtc, making sure that the pipe is running first.
- */
-static void intel_enable_primary_hw_plane(struct drm_plane *plane,
-					  struct drm_crtc *crtc)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* If the pipe isn't enabled, we can't pump pixels and may hang */
-	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
-	to_intel_plane_state(plane->state)->visible = true;
-
-	dev_priv->display.update_primary_plane(crtc, plane->fb,
-					       crtc->x, crtc->y);
-}
-
 static bool need_vtd_wa(struct drm_device *dev)
 {
 #ifdef CONFIG_INTEL_IOMMU
@@ -4455,20 +4431,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-static void intel_enable_sprite_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	enum pipe pipe = to_intel_crtc(crtc)->pipe;
-	struct drm_plane *plane;
-	struct intel_plane *intel_plane;
-
-	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
-		intel_plane = to_intel_plane(plane);
-		if (intel_plane->pipe == pipe)
-			intel_plane_restore(&intel_plane->base);
-	}
-}
-
 void hsw_enable_ips(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -4539,7 +4501,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 || !intel_crtc->active)
 		return;
 
 	if (!HAS_PCH_SPLIT(dev_priv->dev)) {
@@ -4698,44 +4660,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_crtc_enable_planes(struct drm_crtc *crtc)
-{
-	intel_enable_primary_hw_plane(crtc->primary, crtc);
-	intel_enable_sprite_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
-
-	intel_post_enable_primary(crtc);
-}
-
-static void intel_crtc_disable_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-	int pipe = intel_crtc->pipe;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	intel_pre_disable_primary(crtc);
-
-	intel_crtc_dpms_overlay_disable(intel_crtc);
-	for_each_intel_plane(dev, intel_plane) {
-		if (intel_plane->pipe == pipe) {
-			struct drm_crtc *from = intel_plane->base.crtc;
-
-			intel_plane->disable_plane(&intel_plane->base,
-						   from ?: crtc, true);
-		}
-	}
-
-	/*
-	 * FIXME: Once we grow proper nuclear flip support out of this we need
-	 * to compute the mask of flip planes precisely. For the time being
-	 * consider this a flip to a NULL plane.
-	 */
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
-}
-
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -5441,7 +5365,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);
@@ -5539,7 +5463,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	bool is_dsi;
 
-	WARN_ON(!crtc->state->enable);
+	WARN_ON(!crtc->state->active);
 
 	if (intel_crtc->active)
 		return;
@@ -5617,7 +5541,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
+	WARN_ON(!crtc->state->active);
 
 	if (intel_crtc->active)
 		return;
@@ -5728,10 +5652,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum intel_display_power_domain domain;
-	unsigned long domains;
+	struct intel_crtc_state *pipe_config;
+	struct drm_plane_state *plane_state;
+	struct drm_atomic_state *state;
+	int ret;
 
 	if (enable == intel_crtc->active)
 		return;
@@ -5739,24 +5666,40 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	if (enable && !crtc->state->enable)
 		return;
 
-	crtc->state->active = enable;
-	if (enable) {
-		domains = get_crtc_power_domains(crtc);
-		for_each_power_domain(domain, domains)
-			intel_display_power_get(dev_priv, domain);
-		intel_crtc->enabled_power_domains = domains;
+	/* this function should be called with drm_modeset_lock_all for now */
+	if (WARN_ON(!ctx))
+		return;
+	lockdep_assert_held(&ctx->ww_ctx);
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
-	} else {
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return;
 
-		domains = intel_crtc->enabled_power_domains;
-		for_each_power_domain(domain, domains)
-			intel_display_power_put(dev_priv, domain);
-		intel_crtc->enabled_power_domains = 0;
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
+
+	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto err;
 	}
+	pipe_config->base.active = enable;
+
+	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
+	if (IS_ERR(plane_state)) {
+		ret = PTR_ERR(plane_state);
+		goto err;
+	}
+
+	ret = intel_set_mode(crtc, state);
+	if (!ret)
+		return;
+
+	DRM_ERROR("Failed to toggle crtc!\n");
+
+err:
+	DRM_ERROR("Updating crtc active failed with %i\n", ret);
+	drm_atomic_state_free(state);
 }
 
 /**
@@ -5832,7 +5775,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
 
 			crtc = encoder->base.crtc;
 
-			I915_STATE_WARN(!crtc->state->enable,
+			I915_STATE_WARN(!crtc->state->active,
 					"crtc not enabled\n");
 			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
 			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
@@ -11232,7 +11175,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;
 
@@ -11243,8 +11186,15 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 
 			intel_encoder = to_intel_encoder(connector->encoder);
 			intel_encoder->connectors_active = true;
-		} else
+		} else {
+			struct drm_property *dpms_property =
+				dev->mode_config.dpms_property;
+
 			connector->dpms = DRM_MODE_DPMS_OFF;
+			drm_object_property_set_value(&connector->base,
+							 dpms_property,
+							 DRM_MODE_DPMS_OFF);
+		}
 	}
 
 }
@@ -11692,7 +11642,7 @@ check_shared_dpll_state(struct drm_device *dev)
 		     pll->on, active);
 
 		for_each_intel_crtc(dev, crtc) {
-			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
+			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
 				enabled_crtcs++;
 			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
 				active_crtcs++;
@@ -11825,7 +11775,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 			return ERR_PTR(ret);
 	}
 
-	if (!pipe_config->base.enable)
+	if (!pipe_config->base.active)
 		goto done;
 
 	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
@@ -11885,7 +11835,7 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 		goto done;
 
 	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;
 
 		intel_crtc = to_intel_crtc(crtc);
@@ -12072,7 +12022,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	 * 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)) {
+	if (pipe_config->base.active && needs_modeset(&pipe_config->base)) {
 		modeset_crtc->mode = pipe_config->base.mode;
 
 		/*
@@ -12095,7 +12045,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc->state->enable)
+		if (!crtc->state->active)
 			continue;
 
 		update_scanline_offset(to_intel_crtc(crtc));
@@ -12708,12 +12658,16 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 	int i, nplanes = dev->mode_config.num_total_plane, idx;
 	bool mode_changed = needs_modeset(crtc_state);
 	bool is_crtc_enabled = crtc_state->active;
-	bool was_crtc_enabled = crtc->state->active && intel_crtc->active;
+	bool was_crtc_enabled = crtc->state->active;
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 	intel_crtc->atomic.update_wm = mode_changed;
 
-	idx = drm_crtc_index(crtc);
+	idx = crtc->base.id;
+	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
+		"Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n",
+		idx, crtc->state->active, intel_crtc->active);
+
 	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
 			 idx, was_crtc_enabled, is_crtc_enabled);
 
@@ -12738,7 +12692,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 		fb = plane_state->base.fb;
 
 		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
-			drm_plane_index(&plane->base), fb ? fb->base.id : -1);
+			plane->base.base.id, fb ? fb->base.id : -1);
 		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
 			was_visible, visible, turn_off, turn_on, mode_changed);
 
@@ -14294,7 +14248,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 v2 RFC 6/5] drm/i915: Update less state during modeset.

[Maarten Lankhorst]

No need to repeatedly call update_watermarks, or update_fbc.
For update_watermarks once should be enough after disabling crtc's
and swapping the state.

Down to a single call to update_watermarks in .crtc_enable

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
There is no v1, this patch was added later to the series. :-)

The order in ironlake_crtc_disable is slightly changed with
intel_disable_shared_dpll and ironlake_fdi_pll_disable order changed.
I have no idea if this can cause problems, but it works on my system. 

 drivers/gpu/drm/i915/intel_display.c | 79 +++++++++++++-----------------------
 1 file changed, 28 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2d2ada580b36..16204c525004 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4588,10 +4588,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 */
 	hsw_enable_ips(intel_crtc);
 
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes
@@ -4646,11 +4642,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		intel_set_memory_cxsr(dev_priv, false);
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->fbc.crtc == intel_crtc)
-		intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
@@ -4876,9 +4867,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	if (!intel_crtc->active)
-		return;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
@@ -4916,18 +4904,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 			I915_WRITE(PCH_DPLL_SEL, temp);
 		}
 
-		/* disable PCH DPLL */
-		intel_disable_shared_dpll(intel_crtc);
-
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -4938,9 +4916,6 @@ 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)
-		return;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
@@ -4974,16 +4949,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	if (intel_crtc_to_shared_dpll(intel_crtc))
-		intel_disable_shared_dpll(intel_crtc);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
@@ -5603,9 +5568,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (!intel_crtc->active)
-		return;
-
 	/*
 	 * On gen2 planes are double buffered but the pipe isn't, so we must
 	 * wait for planes to fully turn off before disabling the pipe.
@@ -5639,13 +5601,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	if (!IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
@@ -12008,11 +11963,21 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	__intel_set_mode_update_planes(dev, state);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
 		if (!needs_modeset(crtc_state))
 			continue;
 
+		if (!crtc->state->active)
+			continue;
+
 		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
 		dev_priv->display.crtc_disable(crtc);
+
+		intel_crtc->active = false;
+
+		if (intel_crtc_to_shared_dpll(intel_crtc))
+			intel_disable_shared_dpll(intel_crtc);
 	}
 
 	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
@@ -12045,8 +12010,11 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc->state->active)
+		if (!crtc->state->active) {
+			if (needs_modeset(crtc->state))
+				intel_update_watermarks(crtc);
 			continue;
+		}
 
 		update_scanline_offset(to_intel_crtc(crtc));
 
@@ -12662,6 +12630,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 	intel_crtc->atomic.update_wm = mode_changed;
+	intel_crtc->atomic.disable_fbc = mode_changed;
 
 	idx = crtc->base.id;
 	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
@@ -12763,6 +12732,9 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 			break;
 		}
 	}
+
+	if (mode_changed)
+		intel_crtc->atomic.update_wm = false;
 	return 0;
 }
 
@@ -12802,8 +12774,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.wait_for_flips)
 		intel_crtc_wait_for_pending_flips(crtc);
 
-	if (intel_crtc->atomic.disable_fbc)
-		intel_fbc_disable(dev);
+	if (intel_crtc->atomic.disable_fbc &&
+	    dev_priv->fbc.crtc == intel_crtc) {
+		mutex_lock(&dev->struct_mutex);
+		if (dev_priv->fbc.crtc == intel_crtc)
+			intel_fbc_disable(dev);
+		mutex_unlock(&dev->struct_mutex);
+	}
 
 	if (intel_crtc->atomic.pre_disable_primary)
 		intel_pre_disable_primary(crtc);
@@ -12841,15 +12818,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
 
+	if (intel_crtc->atomic.post_enable_primary)
+		intel_post_enable_primary(crtc);
+
 	if (intel_crtc->atomic.update_fbc) {
 		mutex_lock(&dev->struct_mutex);
 		intel_fbc_update(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	if (intel_crtc->atomic.post_enable_primary)
-		intel_post_enable_primary(crtc);
-
 	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
 		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
 			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
-- 
2.1.0


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