[Intel-gfx] [PATCH 11/19] drm/i915: Rework global state locking

Ville Syrjala ville.syrjala at linux.intel.com
Mon Jul 8 12:53:17 UTC 2019


From: Ville Syrjälä <ville.syrjala at linux.intel.com>

So far we've sort of protected the global state under dev_priv with
the connection_mutex. I wan to change that so that we can change the
cdclk even for pure plane updates. To that end let's formalize the
protection of the global state to follow what I started with the cdclk
code already (though not entirely properly) such that any crtc mutex
will suffice as a read lock, and all crtcs mutexes act as the write
lock.

We'll also pimp intel_atomic_state_clear() to clear the entire global
state, so that we don't accidentally leak stale information between
the locking retries.

Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c  |  27 +++++
 drivers/gpu/drm/i915/display/intel_atomic.h  |   3 +
 drivers/gpu/drm/i915/display/intel_audio.c   |  10 +-
 drivers/gpu/drm/i915/display/intel_cdclk.c   | 116 ++++++++++---------
 drivers/gpu/drm/i915/display/intel_display.c |  29 ++++-
 drivers/gpu/drm/i915/i915_drv.h              |  11 +-
 drivers/gpu/drm/i915/intel_drv.h             |   8 ++
 7 files changed, 139 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 954d4a930864..de4cd482dbe5 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -418,6 +418,13 @@ 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_crtcs = 0;
+	memset(&state->min_cdclk, 0, sizeof(state->min_cdclk));
+	memset(&state->min_voltage_level, 0, sizeof(state->min_voltage_level));
+	memset(&state->cdclk.logical, 0, sizeof(state->cdclk.logical));
+	memset(&state->cdclk.actual, 0, sizeof(state->cdclk.actual));
+	state->cdclk.pipe = INVALID_PIPE;
 }
 
 struct intel_crtc_state *
@@ -431,3 +438,23 @@ 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;
+
+	/* Lock all crtc mutexes */
+	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;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index 58065d3161a3..0d6cd22b7e5f 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -16,6 +16,7 @@ struct drm_crtc_state;
 struct drm_device;
 struct drm_i915_private;
 struct drm_property;
+struct intel_atomic_state;
 struct intel_crtc;
 struct intel_crtc_state;
 
@@ -46,4 +47,6 @@ 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);
+
 #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 c8fd35a7ca42..22ccb824c716 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -28,6 +28,7 @@
 #include <drm/i915_component.h>
 
 #include "i915_drv.h"
+#include "intel_atomic.h"
 #include "intel_audio.h"
 #include "intel_drv.h"
 #include "intel_lpe_audio.h"
@@ -816,13 +817,8 @@ 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
-	 * Need to lock this here in case we have no active pipes
-	 * and thus wouldn't lock it during the commit otherwise.
-	 */
-	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
-			       &ctx);
+	/* 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);
 
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 4649485fee33..40583d8d259b 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -442,7 +442,7 @@ static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
 		return 200000;
 }
 
-static u8 vlv_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk)
+static int vlv_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk)
 {
 	if (IS_VALLEYVIEW(dev_priv)) {
 		if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
@@ -669,7 +669,7 @@ static int bdw_calc_cdclk(int min_cdclk)
 		return 337500;
 }
 
-static u8 bdw_calc_voltage_level(int cdclk)
+static int bdw_calc_voltage_level(int cdclk)
 {
 	switch (cdclk) {
 	default:
@@ -808,7 +808,7 @@ static int skl_calc_cdclk(int min_cdclk, int vco)
 	}
 }
 
-static u8 skl_calc_voltage_level(int cdclk)
+static int skl_calc_voltage_level(int cdclk)
 {
 	if (cdclk > 540000)
 		return 3;
@@ -1190,7 +1190,7 @@ static int glk_calc_cdclk(int min_cdclk)
 		return 79200;
 }
 
-static u8 bxt_calc_voltage_level(int cdclk)
+static int bxt_calc_voltage_level(int cdclk)
 {
 	return DIV_ROUND_UP(cdclk, 25000);
 }
@@ -1524,7 +1524,7 @@ static int cnl_calc_cdclk(int min_cdclk)
 		return 168000;
 }
 
-static u8 cnl_calc_voltage_level(int cdclk)
+static int cnl_calc_voltage_level(int cdclk)
 {
 	if (cdclk > 336000)
 		return 2;
@@ -1867,7 +1867,7 @@ static void icl_set_cdclk(struct drm_i915_private *dev_priv,
 	dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level;
 }
 
-static u8 icl_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk)
+static int icl_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk)
 {
 	if (IS_ELKHARTLAKE(dev_priv)) {
 		if (cdclk > 312000)
@@ -2305,11 +2305,20 @@ 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;
 
+		if (state->min_cdclk[i] == min_cdclk)
+			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;
@@ -2328,7 +2337,7 @@ static int intel_compute_min_cdclk(struct intel_atomic_state *state)
  * future platforms this code will need to be
  * adjusted.
  */
-static u8 cnl_compute_min_voltage_level(struct intel_atomic_state *state)
+static int cnl_compute_min_voltage_level(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct intel_crtc *crtc;
@@ -2341,11 +2350,21 @@ static u8 cnl_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->base.enable)
-			state->min_voltage_level[i] =
-				crtc_state->min_voltage_level;
+			min_voltage_level = crtc_state->min_voltage_level;
 		else
-			state->min_voltage_level[i] = 0;
+			min_voltage_level = 0;
+
+		if (state->min_voltage_level[i] == min_voltage_level)
+			continue;
+
+		state->min_voltage_level[i] = min_voltage_level;
+
+		ret = intel_atomic_lock_global_state(state);
+		if (ret)
+			return ret;
 	}
 
 	min_voltage_level = 0;
@@ -2531,12 +2550,16 @@ static int bxt_modeset_calc_cdclk(struct intel_atomic_state *state)
 static int cnl_modeset_calc_cdclk(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	int min_cdclk, cdclk, vco;
+	int min_cdclk, min_voltage_level, cdclk, vco;
 
 	min_cdclk = intel_compute_min_cdclk(state);
 	if (min_cdclk < 0)
 		return min_cdclk;
 
+	min_voltage_level = cnl_compute_min_voltage_level(state);
+	if (min_voltage_level < 0)
+		return min_voltage_level;
+
 	cdclk = cnl_calc_cdclk(min_cdclk);
 	vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
 
@@ -2544,7 +2567,7 @@ static int cnl_modeset_calc_cdclk(struct intel_atomic_state *state)
 	state->cdclk.logical.cdclk = cdclk;
 	state->cdclk.logical.voltage_level =
 		max(cnl_calc_voltage_level(cdclk),
-		    cnl_compute_min_voltage_level(state));
+		    min_voltage_level);
 
 	if (!state->active_crtcs) {
 		cdclk = cnl_calc_cdclk(state->cdclk.force_min_cdclk);
@@ -2561,23 +2584,6 @@ static int cnl_modeset_calc_cdclk(struct intel_atomic_state *state)
 	return 0;
 }
 
-static int intel_lock_all_pipes(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_crtc *crtc;
-
-	/* Add all pipes to the state */
-	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 intel_modeset_all_pipes(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
@@ -2621,12 +2627,16 @@ static int icl_modeset_calc_cdclk(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	unsigned int ref = state->cdclk.logical.ref;
-	int min_cdclk, cdclk, vco;
+	int min_cdclk, min_voltage_level, cdclk, vco;
 
 	min_cdclk = intel_compute_min_cdclk(state);
 	if (min_cdclk < 0)
 		return min_cdclk;
 
+	min_voltage_level = cnl_compute_min_voltage_level(state);
+	if (min_voltage_level < 0)
+		return min_voltage_level;
+
 	cdclk = icl_calc_cdclk(min_cdclk, ref);
 	vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
 
@@ -2634,7 +2644,7 @@ static int icl_modeset_calc_cdclk(struct intel_atomic_state *state)
 	state->cdclk.logical.cdclk = cdclk;
 	state->cdclk.logical.voltage_level =
 		max(icl_calc_voltage_level(dev_priv, cdclk),
-		    cnl_compute_min_voltage_level(state));
+		    min_voltage_level);
 
 	if (!state->active_crtcs) {
 		cdclk = icl_calc_cdclk(state->cdclk.force_min_cdclk, ref);
@@ -2677,47 +2687,49 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 	if (ret)
 		return ret;
 
+	if (!intel_cdclk_changed(&dev_priv->cdclk.logical,
+				 &state->cdclk.logical) &&
+	    !intel_cdclk_changed(&dev_priv->cdclk.actual,
+				 &state->cdclk.actual))
+		return 0;
+
 	/*
-	 * Writes to dev_priv->cdclk.logical must protected by
-	 * holding all the crtc locks, even if we don't end up
+	 * Writes to dev_priv->cdclk.{actual,logical} must protected
+	 * by holding all the crtc mutexes even if we don't end up
 	 * touching the hardware
 	 */
-	if (intel_cdclk_changed(&dev_priv->cdclk.logical,
-				&state->cdclk.logical)) {
-		ret = intel_lock_all_pipes(state);
-		if (ret < 0)
-			return ret;
-	}
+	ret = intel_atomic_lock_global_state(state);
+	if (ret)
+		return ret;
 
-	if (is_power_of_2(state->active_crtcs)) {
+	if (is_power_of_2(state->active_crtcs) &&
+	    intel_cdclk_needs_cd2x_update(dev_priv,
+					  &dev_priv->cdclk.actual,
+					  &state->cdclk.actual)) {
 		struct intel_crtc *crtc;
 		struct intel_crtc_state *crtc_state;
 
 		pipe = ilog2(state->active_crtcs);
 		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
-		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
-		if (crtc_state &&
-		    drm_atomic_crtc_needs_modeset(&crtc_state->base))
+
+		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		if (drm_atomic_crtc_needs_modeset(&crtc_state->base))
 			pipe = INVALID_PIPE;
 	} else {
 		pipe = INVALID_PIPE;
 	}
 
-	/* All pipes must be switched off while we change the cdclk. */
-	if (pipe != INVALID_PIPE &&
-	    intel_cdclk_needs_cd2x_update(dev_priv,
-					  &dev_priv->cdclk.actual,
-					  &state->cdclk.actual)) {
-		ret = intel_lock_all_pipes(state);
-		if (ret)
-			return ret;
-
+	if (pipe != INVALID_PIPE) {
 		state->cdclk.pipe = pipe;
 
 		DRM_DEBUG_KMS("Can change cdclk with pipe %c active\n",
 			      pipe_name(pipe));
 	} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
 					     &state->cdclk.actual)) {
+		/* All pipes must be switched off while we change the cdclk. */
 		ret = intel_modeset_all_pipes(state);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 2d3cfdc80fd3..5b6300b82c50 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12140,6 +12140,12 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
 	unsigned int used_mst_ports = 0;
 	bool ret = true;
 
+	/*
+	 * We're going to peek into connector->state,
+	 * hence connection_mutex must be held.
+	 */
+	drm_modeset_lock_assert_held(&dev->mode_config.connection_mutex);
+
 	/*
 	 * Walk the connector list instead of the encoder
 	 * list to detect the problem on ddi platforms
@@ -13387,7 +13393,6 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
 	state->active_crtcs = dev_priv->active_crtcs;
 	state->cdclk.logical = dev_priv->cdclk.logical;
 	state->cdclk.actual = dev_priv->cdclk.actual;
-	state->cdclk.pipe = INVALID_PIPE;
 
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
@@ -13400,6 +13405,12 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
 			state->active_pipe_changes |= drm_crtc_mask(&crtc->base);
 	}
 
+	if (state->active_pipe_changes) {
+		ret = intel_atomic_lock_global_state(state);
+		if (ret)
+			return ret;
+	}
+
 	ret = intel_modeset_calc_cdclk(state);
 	if (ret)
 		return ret;
@@ -13501,7 +13512,7 @@ static int intel_atomic_check(struct drm_device *dev,
 	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
 	struct intel_crtc *crtc;
 	int ret, i;
-	bool any_ms = state->cdclk.force_min_cdclk_changed;
+	bool any_ms = false;
 
 	/* Catch I915_MODE_FLAG_INHERITED */
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
@@ -13539,6 +13550,8 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	any_ms |= state->cdclk.force_min_cdclk_changed;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);
 		if (ret)
@@ -14028,6 +14041,14 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
 				  to_intel_plane(plane)->frontbuffer_bit);
 }
 
+static void assert_global_state_locked(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	for_each_intel_crtc(&dev_priv->drm, crtc)
+		drm_modeset_lock_assert_held(&crtc->base.mutex);
+}
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -14105,7 +14126,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_shared_dpll_swap_state(state);
 	intel_atomic_track_fbs(state);
 
-	if (intel_state->modeset) {
+	if (intel_state->global_state_changed) {
+		assert_global_state_locked(dev_priv);
+
 		memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
 		       sizeof(intel_state->min_cdclk));
 		memcpy(dev_priv->min_voltage_level,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e05bc3e1014d..d812ac6d86a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1437,13 +1437,14 @@ struct drm_i915_private {
 	unsigned int fdi_pll_freq;
 	unsigned int czclk_freq;
 
+	/*
+	 * For reading holding any crtc lock is sufficient,
+	 * for writing must hold all of them.
+	 */
 	struct {
 		/*
 		 * The current logical cdclk state.
 		 * See intel_atomic_state.cdclk.logical
-		 *
-		 * For reading holding any crtc lock is sufficient,
-		 * for writing must hold all of them.
 		 */
 		struct intel_cdclk_state logical;
 		/*
@@ -1508,6 +1509,10 @@ struct drm_i915_private {
 	 */
 	struct mutex dpll_lock;
 
+	/*
+	 * For reading active_crtcs,min_cdclk,min_voltage_level holding
+	 * any crtc lock is sufficient, for writing must hold all of them.
+	 */
 	unsigned int active_crtcs;
 	/* minimum acceptable cdclk for each pipe */
 	int min_cdclk[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 04eee5d880f5..c0bbf7a60944 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -476,6 +476,14 @@ struct intel_atomic_state {
 
 	bool rps_interactive;
 
+	/*
+	 * active_crtcs
+	 * min_cdclk[]
+	 * min_voltage_level[]
+	 * cdclk.*
+	 */
+	bool global_state_changed;
+
 	/* Gen9+ only */
 	struct skl_ddb_values wm_results;
 
-- 
2.21.0



More information about the Intel-gfx mailing list