All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 0/5] Convert planes and crtc state updates to atomic.
@ 2015-04-22 11:24 maarten.lankhorst
  2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code.
  2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
@ 2015-04-22 11:24 ` maarten.lankhorst
  2015-04-24  8:46   ` Ander Conselvan De Oliveira
  2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH RFC 2/5] drm/i915: Only update required power domains.
  2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
  2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst
@ 2015-04-22 11:24 ` maarten.lankhorst
  2015-04-24  8:47   ` Ander Conselvan De Oliveira
  2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere
  2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
  2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst
  2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst
@ 2015-04-22 11:24 ` maarten.lankhorst
  2015-05-04 13:44   ` Daniel Vetter
  2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic
  2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
                   ` (2 preceding siblings ...)
  2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst
@ 2015-04-22 11:24 ` maarten.lankhorst
  2015-04-23  6:19   ` [PATCH v2 " Maarten Lankhorst
  2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst
  2015-04-23  6:29 ` [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset Maarten Lankhorst
  5 siblings, 1 reply; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state
  2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
                   ` (3 preceding siblings ...)
  2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst
@ 2015-04-22 11:24 ` maarten.lankhorst
  2015-04-22 14:18   ` Maarten Lankhorst
  2015-04-23  6:23   ` [PATCH v2 " Maarten Lankhorst
  2015-04-23  6:29 ` [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset Maarten Lankhorst
  5 siblings, 2 replies; 14+ messages in thread
From: maarten.lankhorst @ 2015-04-22 11:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state
  2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst
@ 2015-04-22 14:18   ` Maarten Lankhorst
  2015-04-23  6:23   ` [PATCH v2 " Maarten Lankhorst
  1 sibling, 0 replies; 14+ messages in thread
From: Maarten Lankhorst @ 2015-04-22 14:18 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 RFC 4/5] drm/i915: make plane helpers fully atomic
  2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst
@ 2015-04-23  6:19   ` Maarten Lankhorst
  2015-04-24  8:52     ` Ander Conselvan De Oliveira
  0 siblings, 1 reply; 14+ messages in thread
From: Maarten Lankhorst @ 2015-04-23  6:19 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state
  2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst
  2015-04-22 14:18   ` Maarten Lankhorst
@ 2015-04-23  6:23   ` Maarten Lankhorst
  1 sibling, 0 replies; 14+ messages in thread
From: Maarten Lankhorst @ 2015-04-23  6:23 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset.
  2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
                   ` (4 preceding siblings ...)
  2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst
@ 2015-04-23  6:29 ` Maarten Lankhorst
  5 siblings, 0 replies; 14+ messages in thread
From: Maarten Lankhorst @ 2015-04-23  6:29 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code.
  2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst
@ 2015-04-24  8:46   ` Ander Conselvan De Oliveira
  0 siblings, 0 replies; 14+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-04-24  8:46 UTC (permalink / raw)
  To: maarten.lankhorst; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC 2/5] drm/i915: Only update required power domains.
  2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst
@ 2015-04-24  8:47   ` Ander Conselvan De Oliveira
  0 siblings, 0 replies; 14+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-04-24  8:47 UTC (permalink / raw)
  To: maarten.lankhorst; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 RFC 4/5] drm/i915: make plane helpers fully atomic
  2015-04-23  6:19   ` [PATCH v2 " Maarten Lankhorst
@ 2015-04-24  8:52     ` Ander Conselvan De Oliveira
  0 siblings, 0 replies; 14+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-04-24  8:52 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere
  2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst
@ 2015-05-04 13:44   ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2015-05-04 13:44 UTC (permalink / raw)
  To: maarten.lankhorst; +Cc: Ander Conselvan de Oliveira, intel-gfx

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-05-04 13:42 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-22 11:24 [PATCH RFC 0/5] Convert planes and crtc state updates to atomic maarten.lankhorst
2015-04-22 11:24 ` [PATCH RFC 1/5] drm/i915: Get rid of intel_crtc_disable and related code maarten.lankhorst
2015-04-24  8:46   ` Ander Conselvan De Oliveira
2015-04-22 11:24 ` [PATCH RFC 2/5] drm/i915: Only update required power domains maarten.lankhorst
2015-04-24  8:47   ` Ander Conselvan De Oliveira
2015-04-22 11:24 ` [PATCH RFC 3/5] drm/i915: use intel_crtc_control everywhere maarten.lankhorst
2015-05-04 13:44   ` Daniel Vetter
2015-04-22 11:24 ` [PATCH RFC 4/5] drm/i915: make plane helpers fully atomic maarten.lankhorst
2015-04-23  6:19   ` [PATCH v2 " Maarten Lankhorst
2015-04-24  8:52     ` Ander Conselvan De Oliveira
2015-04-22 11:24 ` [PATCH RFC 5/5] drm/i915: Implement intel_crtc_toggle using atomic state maarten.lankhorst
2015-04-22 14:18   ` Maarten Lankhorst
2015-04-23  6:23   ` [PATCH v2 " Maarten Lankhorst
2015-04-23  6:29 ` [PATCH v2 RFC 6/5] drm/i915: Update less state during modeset Maarten Lankhorst

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.