From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
---
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
