On Tue, 2019-10-15 at 22:30 +0300, Ville Syrjala wrote:
> 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.
>
> As a slight optimization we'll only lock the crtc mutexes to protect
> the global state, however if and when we actually have to poke the
> hw (eg. if the actual cdclk changes) we must serialize commits
> across all crtcs so that a parallel nonblocking commit can't get
> ahead of the cdclk reprogamming. We do that by adding all crtcs to
> the state.
>
> TODO: the old global state examined during commit may still
> be a problem since it always looks at the _latest_ swapped state
> in dev_priv. Need to add proper old/new state for that too I think.
>
> v2: Remeber to serialize the commits if necessary
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_atomic.c | 44 ++++++++
> drivers/gpu/drm/i915/display/intel_atomic.h | 5 +
> drivers/gpu/drm/i915/display/intel_audio.c | 10 +-
> drivers/gpu/drm/i915/display/intel_cdclk.c | 102 ++++++++++----
> ----
> drivers/gpu/drm/i915/display/intel_display.c | 29 ++++-
> .../drm/i915/display/intel_display_types.h | 8 ++
> drivers/gpu/drm/i915/i915_drv.h | 11 +-
> 7 files changed, 153 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index c5a552a69752..9cd6d2348a1e 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -429,6 +429,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_pipes = 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 *
> @@ -442,3 +449,40 @@ 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 58065d3161a3..49d5cb1b9e0a 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,8 @@ 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 ed18511befa3..85e6b2bbb34f 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_display_types.h"
> #include "intel_lpe_audio.h"
> @@ -818,13 +819,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 6eaebc38ee01..6c17a3bbf866 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2007,11 +2007,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;
> @@ -2034,7 +2043,7 @@ static int intel_compute_min_cdclk(struct
> intel_atomic_state *state)
> * future platforms this code will need to be
> * adjusted.
> */
> -static u8 bxt_compute_min_voltage_level(struct intel_atomic_state
> *state)
> +static int bxt_compute_min_voltage_level(struct intel_atomic_state
> *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct intel_crtc *crtc;
> @@ -2047,11 +2056,21 @@ static u8
> 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->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;
> @@ -2195,20 +2214,24 @@ static int skl_modeset_calc_cdclk(struct
> intel_atomic_state *state)
> static int bxt_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 = bxt_compute_min_voltage_level(state);
> + if (min_voltage_level < 0)
> + return min_voltage_level;
> +
> cdclk = bxt_calc_cdclk(dev_priv, min_cdclk);
> vco = bxt_calc_cdclk_pll_vco(dev_priv, cdclk);
>
> state->cdclk.logical.vco = vco;
> state->cdclk.logical.cdclk = cdclk;
> state->cdclk.logical.voltage_level =
> - max(dev_priv->display.calc_voltage_level(cdclk),
> - bxt_compute_min_voltage_level(state));
> + max_t(int, min_voltage_level,
> + dev_priv->display.calc_voltage_level(cdclk));
>
> if (!state->active_pipes) {
> cdclk = bxt_calc_cdclk(dev_priv, state-
> >cdclk.force_min_cdclk);
> @@ -2225,23 +2248,6 @@ static int bxt_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);
> @@ -2308,46 +2314,56 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
> return ret;
>
> /*
> - * 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)
> + 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 {
> + return 0;
> }
>
> - if (is_power_of_2(state->active_pipes)) {
> + if (is_power_of_2(state->active_pipes) &&
> + 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_pipes);
> 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 7d7d1859775a..a8444d9841c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12191,6 +12191,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
> @@ -13463,7 +13469,6 @@ static int intel_modeset_checks(struct
> intel_atomic_state *state)
> state->active_pipes = dev_priv->active_pipes;
> 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) {
> @@ -13476,6 +13481,12 @@ static int intel_modeset_checks(struct
> intel_atomic_state *state)
> 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);
> if (ret)
> return ret;
> @@ -13577,7 +13588,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,
> @@ -13615,6 +13626,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)
> @@ -14234,6 +14247,14 @@ static void intel_atomic_track_fbs(struct
> intel_atomic_state *state)
> 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);
> +}
> +
> static int intel_atomic_commit(struct drm_device *dev,
> struct drm_atomic_state *_state,
> bool nonblock)
> @@ -14299,7 +14320,9 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> intel_shared_dpll_swap_state(state);
> intel_atomic_track_fbs(state);
>
> - if (state->modeset) {
> + if (state->global_state_changed) {
> + assert_global_state_locked(dev_priv);
> +
> memcpy(dev_priv->min_cdclk, state->min_cdclk,
> sizeof(state->min_cdclk));
> memcpy(dev_priv->min_voltage_level, state-
> >min_voltage_level,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 40390d855815..01fed8957ade 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -506,6 +506,14 @@ 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/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index c46b339064c0..d3e61152d924 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1105,13 +1105,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;
> /*
> @@ -1181,6 +1182,10 @@ struct drm_i915_private {
> */
> struct mutex dpll_lock;
>
> + /*
> + * For reading active_pipes, min_cdclk, min_voltage_level
> holding
> + * any crtc lock is sufficient, for writing must hold all of
> them.
> + */
> u8 active_pipes;
> /* minimum acceptable cdclk for each pipe */
> int min_cdclk[I915_MAX_PIPES];
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
