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

From: Ville Syrjälä ville.syrjala@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@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;