[Intel-gfx] [PATCH] drm/i915: Nuke intel_atomic_lock/serialize_global_state()

* [Intel-gfx] [PATCH] drm/i915: Nuke intel_atomic_lock/serialize_global_state()
@ 2020-01-18 23:21 José Roberto de Souza
  2020-01-18 23:59 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: José Roberto de Souza @ 2020-01-18 23:21 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula

When computing GEN 9+ watermarks all the pipes were being added to
the state whatever the state needs a modeset or the active pipes
changed what is redundant.
As when a pipe/CRTC is added to the state it means that the state
also holds the CRTC lock, so we can remove a lot of code and
complexity by adding all CRTCs to the state earlier with no drawbacks
for GEN 9+ and little to nothing in older GENs.

Also it brings other improvements like atomic state has now a device
wide correctly active_pipes and it will fix the enabling of the right
number of the dbufs as if the state was only enabling one pipe and
only had one CRTC state it would only enable one slice.

Bellow some justifications that might not be so easy to understand:

- intel_atomic_lock_global_state() is not needed anymore in
glk_force_audio_cdclk() because setting force_min_cdclk_changed will
force a modeset
- intel_cdclk.c also don't need intel_atomic_lock_global_state() or
intel_atomic_serialize_global_state() as those functions are called
by intel_modeset_checks() call chain after the CRCTs are added to the
state
- no need to call intel_atomic_lock_global_state() in
intel_modeset_checks() as we just got the CRTCs locks.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c   | 38 -----------------
 drivers/gpu/drm/i915/display/intel_atomic.h   |  4 --
 drivers/gpu/drm/i915/display/intel_audio.c    |  6 +--
 drivers/gpu/drm/i915/display/intel_cdclk.c    | 33 +++------------
 drivers/gpu/drm/i915/display/intel_display.c  | 20 ++++-----
 .../drm/i915/display/intel_display_types.h    | 18 --------
 drivers/gpu/drm/i915/intel_pm.c               | 42 +++----------------
 7 files changed, 18 insertions(+), 143 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index c362eecdd414..03938ae2fb05 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -499,7 +499,6 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
 	struct intel_atomic_state *state = to_intel_atomic_state(s);
 	drm_atomic_state_default_clear(&state->base);
 	state->dpll_set = state->modeset = false;
-	state->global_state_changed = false;
 	state->active_pipes = 0;
 	memset(&state->min_cdclk, 0, sizeof(state->min_cdclk));
 	memset(&state->min_voltage_level, 0, sizeof(state->min_voltage_level));
@@ -519,40 +518,3 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
-
-int intel_atomic_lock_global_state(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_crtc *crtc;
-
-	state->global_state_changed = true;
-
-	for_each_intel_crtc(&dev_priv->drm, crtc) {
-		int ret;
-
-		ret = drm_modeset_lock(&crtc->base.mutex,
-				       state->base.acquire_ctx);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
-int intel_atomic_serialize_global_state(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_crtc *crtc;
-
-	state->global_state_changed = true;
-
-	for_each_intel_crtc(&dev_priv->drm, crtc) {
-		struct intel_crtc_state *crtc_state;
-
-		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	}
-
-	return 0;
-}
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index 74c749dbfb4f..b324d12bdd97 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -55,8 +55,4 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 			       struct intel_crtc *intel_crtc,
 			       struct intel_crtc_state *crtc_state);
 
-int intel_atomic_lock_global_state(struct intel_atomic_state *state);
-
-int intel_atomic_serialize_global_state(struct intel_atomic_state *state);
-
 #endif /* __INTEL_ATOMIC_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index b18040793d9e..0147e1f382b6 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -819,11 +819,7 @@ static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
 	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
 		enable ? 2 * 96000 : 0;
 
-	/* Protects dev_priv->cdclk.force_min_cdclk */
-	ret = intel_atomic_lock_global_state(to_intel_atomic_state(state));
-	if (!ret)
-		ret = drm_atomic_commit(state);
-
+	ret = drm_atomic_commit(state);
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
 		drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 0ce5926006ca..469617436815 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2037,8 +2037,6 @@ static int intel_compute_min_cdclk(struct intel_atomic_state *state)
 	       sizeof(state->min_cdclk));
 
 	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
-		int ret;
-
 		min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
 		if (min_cdclk < 0)
 			return min_cdclk;
@@ -2047,10 +2045,6 @@ static int intel_compute_min_cdclk(struct intel_atomic_state *state)
 			continue;
 
 		state->min_cdclk[i] = min_cdclk;
-
-		ret = intel_atomic_lock_global_state(state);
-		if (ret)
-			return ret;
 	}
 
 	min_cdclk = state->cdclk.force_min_cdclk;
@@ -2086,8 +2080,6 @@ static int bxt_compute_min_voltage_level(struct intel_atomic_state *state)
 	       sizeof(state->min_voltage_level));
 
 	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
-		int ret;
-
 		if (crtc_state->hw.enable)
 			min_voltage_level = crtc_state->min_voltage_level;
 		else
@@ -2097,10 +2089,6 @@ static int bxt_compute_min_voltage_level(struct intel_atomic_state *state)
 			continue;
 
 		state->min_voltage_level[i] = min_voltage_level;
-
-		ret = intel_atomic_lock_global_state(state);
-		if (ret)
-			return ret;
 	}
 
 	min_voltage_level = 0;
@@ -2348,23 +2336,12 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 	 * by holding all the crtc mutexes even if we don't end up
 	 * touching the hardware
 	 */
-	if (intel_cdclk_changed(&dev_priv->cdclk.actual,
-				&state->cdclk.actual)) {
-		/*
-		 * Also serialize commits across all crtcs
-		 * if the actual hw needs to be poked.
-		 */
-		ret = intel_atomic_serialize_global_state(state);
-		if (ret)
-			return ret;
-	} else if (intel_cdclk_changed(&dev_priv->cdclk.logical,
-				       &state->cdclk.logical)) {
-		ret = intel_atomic_lock_global_state(state);
-		if (ret)
-			return ret;
-	} else {
+	ret = intel_cdclk_changed(&dev_priv->cdclk.actual,
+				  &state->cdclk.actual);
+	ret |= intel_cdclk_changed(&dev_priv->cdclk.logical,
+				   &state->cdclk.logical);
+	if (!ret)
 		return 0;
-	}
 
 	if (is_power_of_2(state->active_pipes) &&
 	    intel_cdclk_needs_cd2x_update(dev_priv,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index dd03987cc24f..b3a6e39e48c8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14284,25 +14284,19 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
 		state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
 
 	state->modeset = true;
-	state->active_pipes = dev_priv->active_pipes;
 	state->cdclk.logical = dev_priv->cdclk.logical;
 	state->cdclk.actual = dev_priv->cdclk.actual;
 
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		new_crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(new_crtc_state))
+			return PTR_ERR(new_crtc_state);
+	}
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (new_crtc_state->hw.active)
 			state->active_pipes |= BIT(crtc->pipe);
-		else
-			state->active_pipes &= ~BIT(crtc->pipe);
-
-		if (old_crtc_state->hw.active != new_crtc_state->hw.active)
-			state->active_pipe_changes |= BIT(crtc->pipe);
-	}
-
-	if (state->active_pipe_changes) {
-		ret = intel_atomic_lock_global_state(state);
-		if (ret)
-			return ret;
 	}
 
 	ret = intel_modeset_calc_cdclk(state);
@@ -15586,7 +15580,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_shared_dpll_swap_state(state);
 	intel_atomic_track_fbs(state);
 
-	if (state->global_state_changed) {
+	if (state->modeset) {
 		assert_global_state_locked(dev_priv);
 
 		memcpy(dev_priv->min_cdclk, state->min_cdclk,
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 155ce49ae764..60d812234654 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -483,16 +483,6 @@ struct intel_atomic_state {
 
 	bool dpll_set, modeset;
 
-	/*
-	 * Does this transaction change the pipes that are active?  This mask
-	 * tracks which CRTC's have changed their active state at the end of
-	 * the transaction (not counting the temporary disable during modesets).
-	 * This mask should only be non-zero when intel_state->modeset is true,
-	 * but the converse is not necessarily true; simply changing a mode may
-	 * not flip the final active status of any CRTC's
-	 */
-	u8 active_pipe_changes;
-
 	u8 active_pipes;
 	/* minimum acceptable cdclk for each pipe */
 	int min_cdclk[I915_MAX_PIPES];
@@ -509,14 +499,6 @@ struct intel_atomic_state {
 
 	bool rps_interactive;
 
-	/*
-	 * active_pipes
-	 * min_cdclk[]
-	 * min_voltage_level[]
-	 * cdclk.*
-	 */
-	bool global_state_changed;
-
 	/* Gen9+ only */
 	struct skl_ddb_values wm_results;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 81e5a3278fda..5135d887f328 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3924,10 +3924,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 		return;
 	}
 
-	if (intel_state->active_pipe_changes)
-		*num_active = hweight8(intel_state->active_pipes);
-	else
-		*num_active = hweight8(dev_priv->active_pipes);
+	*num_active = hweight8(intel_state->active_pipes);
 
 	ddb_size = intel_get_ddb_size(dev_priv, crtc_state, total_data_rate,
 				      *num_active, ddb);
@@ -3940,7 +3937,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	 * that changes the active CRTC list or do modeset would need to
 	 * grab _all_ crtc locks, including the one we currently hold.
 	 */
-	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
+	if (!intel_state->modeset) {
 		/*
 		 * alloc may be cleared by clear_intel_crtc_state,
 		 * copy from old state to be sure
@@ -5379,27 +5376,10 @@ skl_print_wm_changes(struct intel_atomic_state *state)
 	}
 }
 
-static int intel_add_all_pipes(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_crtc *crtc;
-
-	for_each_intel_crtc(&dev_priv->drm, crtc) {
-		struct intel_crtc_state *crtc_state;
-
-		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	}
-
-	return 0;
-}
-
 static int
-skl_ddb_add_affected_pipes(struct intel_atomic_state *state)
+skl_ddb_mark_dirty_affected_pipes(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	int ret;
 
 	/*
 	 * If this is our first atomic update following hardware readout,
@@ -5408,13 +5388,6 @@ skl_ddb_add_affected_pipes(struct intel_atomic_state *state)
 	 * ensure a full DDB recompute.
 	 */
 	if (dev_priv->wm.distrust_bios_wm) {
-		ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
-				       state->base.acquire_ctx);
-		if (ret)
-			return ret;
-
-		state->active_pipe_changes = INTEL_INFO(dev_priv)->pipe_mask;
-
 		/*
 		 * We usually only initialize state->active_pipes if we
 		 * we're doing a modeset; make sure this field is always
@@ -5438,14 +5411,9 @@ skl_ddb_add_affected_pipes(struct intel_atomic_state *state)
 	 * any other display updates race with this transaction, so we need
 	 * to grab the lock on *all* CRTC's.
 	 */
-	if (state->active_pipe_changes || state->modeset) {
+	if (dev_priv->wm.distrust_bios_wm || state->modeset)
 		state->wm_results.dirty_pipes = INTEL_INFO(dev_priv)->pipe_mask;
 
-		ret = intel_add_all_pipes(state);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
@@ -5521,7 +5489,7 @@ skl_compute_wm(struct intel_atomic_state *state)
 	/* Clear all dirty flags */
 	results->dirty_pipes = 0;
 
-	ret = skl_ddb_add_affected_pipes(state);
+	ret = skl_ddb_mark_dirty_affected_pipes(state);
 	if (ret)
 		return ret;
 
-- 
2.25.0

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

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