Skip to contentSkip to conversation

Convert planes and crtc state updates to atomic.

RFC PATCH In review [email protected] 3 participants 14 messages Apr 22, 2015 – May 4, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Convert planes and crtc state updates to atomic.

From: Maarten Lankhorst <[email protected]>

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(-)
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw

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

From: Maarten Lankhorst <[email protected]>

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.

---
 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(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
index 61b756bdbaad..9e62926e71f0 100644
@@ -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);
drivers/gpu/drm/i915/intel_display.c +14 -86

188 lines not on this page · raw message

drivers/gpu/drm/i915/intel_drv.h +0 -1
index 70b70e9be167..fb89f5f3498c 100644
@@ -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);
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#2 lore raw

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

Quoted 4 lines · On Wed, 2015-04-22 at 13:24 +0200, [email protected] wrote:

From: Maarten Lankhorst <[email protected]>

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.

Quoted 3 lines

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

Quoted 230 lines
---
 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(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
index 61b756bdbaad..9e62926e71f0 100644
@@ -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);
drivers/gpu/drm/i915/intel_display.c +14 -86
index 35fde239c200..92d54dd30d7e 100644
@@ -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;
 	}
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 70b70e9be167..fb89f5f3498c 100644

drivers/gpu/drm/i915/intel_drv.h void assert_shared_dpll(struct drm_i915_private *dev_priv, · line 1083

Quoted 7 lines
drivers/gpu/drm/i915/intel_drv.h +0 -1
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#3 lore raw

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

In reply to [email protected]

From: Maarten Lankhorst <[email protected]>

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

---
 drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 11 deletions(-)
drivers/gpu/drm/i915/intel_display.c +41 -11
index 92d54dd30d7e..438d8e213748 100644
@@ -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)
 {
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#4 lore raw

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

Quoted 57 lines · On Wed, 2015-04-22 at 13:24 +0200, [email protected] wrote:

From: Maarten Lankhorst <[email protected]>

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

---
 drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 11 deletions(-)
drivers/gpu/drm/i915/intel_display.c +22 -1
index 92d54dd30d7e..438d8e213748 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_display.c static unsigned long get_crtc_power_domains(struct drm_crtc *crtc) · line 5211

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +6 -2
-		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

Quoted 28 lines
drivers/gpu/drm/i915/intel_display.c +13 -2
 	}
 
-	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;
 }

drivers/gpu/drm/i915/intel_display.c static bool intel_crtc_in_use(struct drm_crtc *crtc) · line 11185

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +0 -6
 
-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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw

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

In reply to [email protected]

From: Maarten Lankhorst <[email protected]>

---
 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +15 -3
index 60e86f331313..91c9a4fc8b6a 100644
@@ -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);
 }
drivers/gpu/drm/i915/intel_display.c +24 -44

106 lines not on this page · raw message

drivers/gpu/drm/i915/intel_drv.h +0 -1
index fb89f5f3498c..9668b17d7e0e 100644
@@ -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 *);
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#6 lore raw

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

Quoted 1 line · On Wed, Apr 22, 2015 at 01:24:20PM +0200, [email protected]…

From: Maarten Lankhorst <[email protected]>

A bit more explanation in the commit message here would be useful, i.e. what changed that we need this now.
-Daniel

Quoted 187 lines
---
 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +15 -3
index 60e86f331313..91c9a4fc8b6a 100644
@@ -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);
 }
drivers/gpu/drm/i915/intel_display.c +24 -44
index 438d8e213748..2ffacb4c3a12 100644
@@ -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. */
drivers/gpu/drm/i915/intel_drv.h +0 -1
index fb89f5f3498c..9668b17d7e0e 100644
@@ -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 *);
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#7 lore raw

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

In reply to [email protected]

From: Maarten Lankhorst <[email protected]>

---
 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(-)
drivers/gpu/drm/i915/intel_atomic_plane.c +16 -33
index f7bd3b8fa245..0be720edb971 100644
@@ -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 ||
drivers/gpu/drm/i915/intel_display.c +334 -147

711 lines not on this page · raw message

drivers/gpu/drm/i915/intel_sprite.c +3 -30
index 7419e04b113f..5a277757ac2d 100644
@@ -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;
 }
 
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#8 lore raw

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

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

---
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(-)
drivers/gpu/drm/i915/intel_atomic_plane.c +25 -33
index f7bd3b8fa245..ba4ab392b6b0 100644
@@ -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 ||
drivers/gpu/drm/i915/intel_display.c +338 -147

715 lines not on this page · raw message

drivers/gpu/drm/i915/intel_sprite.c +3 -30
index 7419e04b113f..5a277757ac2d 100644
@@ -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;
 }
 
Signature · 7 lines
2.1.0


_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#9 lore raw

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

Quoted 212 lines · Maarten Lankhorst

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

---
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(-)
drivers/gpu/drm/i915/intel_atomic_plane.c +25 -33
index f7bd3b8fa245..ba4ab392b6b0 100644
@@ -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 ||
drivers/gpu/drm/i915/intel_display.c +27 -16
index 2ffacb4c3a12..acb5c5bea428 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_display.c intel_modeset_update_state(struct drm_atomic_state *state) · line 11203

Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c +5 -1
@@ -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?

Quoted 125 lines
drivers/gpu/drm/i915/intel_display.c +93 -3
+	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;
+

drivers/gpu/drm/i915/intel_display.c static int __intel_set_mode_checks(struct drm_atomic_state *state) · line 11986

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
+		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?

Quoted 39 lines
drivers/gpu/drm/i915/intel_display.c +39 -0
+		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)

drivers/gpu/drm/i915/intel_display.c static int __intel_set_mode_checks(struct drm_atomic_state *state) · line 12035

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
+			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.

Quoted 43 lines
drivers/gpu/drm/i915/intel_display.c +7 -6
+	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)

drivers/gpu/drm/i915/intel_display.c static int __intel_set_mode(struct drm_crtc *modeset_crtc, · line 12104

Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c +2 -2
@@ -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().

Quoted 11 lines
drivers/gpu/drm/i915/intel_display.c +2 -2
 	}
 
-	/* 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);
 

drivers/gpu/drm/i915/intel_display.c intel_modeset_stage_output_state(struct drm_device *dev, · line 12338

Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -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?

Quoted 50 lines
drivers/gpu/drm/i915/intel_display.c +0 -30
 	}
 
 	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

drivers/gpu/drm/i915/intel_display.c static int intel_crtc_set_config(struct drm_mode_set *set) · line 12276

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +0 -10
-		 * 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?

Quoted 218 lines
drivers/gpu/drm/i915/intel_display.c +120 -60
 	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) {

drivers/gpu/drm/i915/intel_display.c intel_disable_primary_plane(struct drm_plane *plane, · line 12814

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
+				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.

Quoted 89 lines
drivers/gpu/drm/i915/intel_display.c +11 -17
 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 ->

drivers/gpu/drm/i915/intel_display.c static void intel_modeset_readout_hw_state(struct drm_device *dev) · line 14450

Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -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

Quoted 49 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
 		plane_state = to_intel_plane_state(primary->state);
drivers/gpu/drm/i915/intel_sprite.c +3 -23
index 7419e04b113f..5a277757ac2d 100644
@@ -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.

drivers/gpu/drm/i915/intel_sprite.c intel_check_sprite_plane(struct drm_plane *plane, · line 953

Quoted 10 lines
drivers/gpu/drm/i915/intel_sprite.c +0 -7
-			 */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_sprite_watermarks |=
-				(1 << drm_plane_index(plane));
-		}
-	}
-
 	return 0;
 }
 

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#10 lore raw

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

In reply to [email protected]

From: Maarten Lankhorst <[email protected]>

Assume the function is locked with drm_modeset_lock_all for now.

---
 drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++-------------------------
 1 file changed, 38 insertions(+), 93 deletions(-)
drivers/gpu/drm/i915/intel_display.c +38 -93
index 99a45bee20d8..5a43ac02b925 100644
@@ -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);
 }
 
 /**
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#11 lore raw

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

Op 22-04-15 om 13:24 schreef [email protected]:

Quoted 30 lines

From: Maarten Lankhorst <[email protected]>

Assume the function is locked with drm_modeset_lock_all for now.

---
 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) {

drivers/gpu/drm/i915/intel_display.c +6 -3
 	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;

drivers/gpu/drm/i915/intel_display.c void intel_crtc_control(struct drm_crtc *crtc, bool enable) · line 5669

Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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. :-)

Quoted 42 lines
drivers/gpu/drm/i915/intel_display.c +25 -14
-	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)

drivers/gpu/drm/i915/intel_display.c void intel_crtc_control(struct drm_crtc *crtc, bool enable) · line 5707

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +7 -0
+		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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#12 lore raw

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

In reply to [email protected]

Assume the function is locked with drm_modeset_lock_all for now.

---
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(-)
drivers/gpu/drm/i915/i915_irq.c +1 -1
index 9da955e4f355..a6816503a080 100644
@@ -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;
 	}
drivers/gpu/drm/i915/intel_display.c +64 -110

309 lines not on this page · raw message

Signature · 7 lines
2.1.0


_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#13 lore raw

[PATCH v2 RFC 6/5] drm/i915: Update less state during modeset.

In reply to [email protected]

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

---
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(-)
drivers/gpu/drm/i915/intel_display.c +28 -51
index 2d2ada580b36..16204c525004 100644
@@ -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,
Signature · 7 lines
2.1.0


_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx