From: Ville Syrjälä ville.syrjala@linux.intel.com
Introduce an rw_semaphore to protect the display commits. All normal commits use down_read() and hence can proceed in parallel, but GPU reset will use down_write() making sure no other commits are in progress when we have to pull the plug on the display engine on pre-g4x platforms. There are no modeset/gem locks taken inside __intel_atomic_commit_tail() itself, and we wait for all dependencies before the down_read(), and thus there is no chance of deadlocks with this scheme.
During reset we should be recommiting the state that was committed last. But for now we'll settle for recommiting the last state for each object. Hence we may commit things a bit too soon when a GPU reset occurs. The rw_semaphore should guarantee that whatever state we observe in obj->state during reset sticks around while we do the commit. The obj->state pointer might change for some objects if another swap_state() occurs while we do our thing, so there migth be some theoretical mismatch which I tink we could avoid by grabbing the rw_semaphore also around the swap_state(), but for now I didn't do that.
Another source of mismatch can happen because we sometimes use the intel_crtc->config during the actual commit, and that only gets updated when we do the commuit. Hence we may get some state via ->state, some via the duplicated ->state, and some via ->config. We'll want to fix this all by tracking the commited state properly, but that will some bigger refactroing so for now we'll just choose to accept these potential mismatches.
I left out the state readout from the post-reset display reinitialization as that still likes to clobber crtc->state etc. If we make it use a free standing atomic state and mke sure it doesn't need any locks we could reintroduce it. For now I just mark the post-reset display state as dirty as possible to make sure we reinitialize everything.
One other potential issue remains in the form of display detection. Either we need to protect that with the same rw_semaphore as well, or perhaps it would be enough to force gmbus into bitbanging mode while the reset is happening and we don't have interrupts, and just across the actual hardware GPU reset we could hold the gmbus mutex.
v2: Keep intel_prepare/finish_reset() outside struct_mutex (Chris) v3: Drop all the committed_state refactoring to make this less obnoxious to backport (Daniel) v4: Preserve the wedge timeout mechanism (Chris)
Cc: stable@vger.kernel.org # for the brave Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Chris Wilson chris@chris-wilson.co.uk Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101597 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99093 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101600 Fixes: 4680816be336 ("drm/i915: Wait first for submission, before waiting for request completion") Fixes: 221fe7994554 ("drm/i915: Perform a direct reset of the GPU from the waiter") Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/intel_display.c | 199 ++++++++++++++++++++++++----------- 2 files changed, 138 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index effbe4f72a64..88ddd27f61b0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2237,6 +2237,8 @@ struct drm_i915_private { struct drm_atomic_state *modeset_restore_state; struct drm_modeset_acquire_ctx reset_ctx;
+ struct rw_semaphore commit_sem; + struct list_head vm_list; /* Global list of all address spaces */ struct i915_ggtt ggtt; /* VM representing the global address space */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7cdd6ec97f80..f13c7d81d4a9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -123,6 +123,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc); static void intel_modeset_setup_hw_state(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc); +static void __intel_atomic_commit_tail(struct drm_atomic_state *state, bool is_reset);
struct intel_limit { struct { @@ -3491,27 +3492,85 @@ static bool gpu_reset_clobbers_display(struct drm_i915_private *dev_priv) INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv); }
-void intel_prepare_reset(struct drm_i915_private *dev_priv) +static void init_intel_state(struct intel_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; + int i; + + state->modeset = true; + + for_each_oldnew_crtc_in_state(&state->base, crtc, + old_crtc_state, new_crtc_state, i) { + if (new_crtc_state->active) + state->active_crtcs |= 1 << i; + else + state->active_crtcs &= ~(1 << i); + + if (old_crtc_state->active != new_crtc_state->active) + state->active_pipe_changes |= drm_crtc_mask(crtc); + } +} + +static struct drm_atomic_state * +intel_duplicate_committed_state(struct drm_i915_private *dev_priv) { - struct drm_device *dev = &dev_priv->drm; - struct drm_modeset_acquire_ctx *ctx = &dev_priv->reset_ctx; struct drm_atomic_state *state; - int ret; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + int i;
- /* - * Need mode_config.mutex so that we don't - * trample ongoing ->detect() and whatnot. - */ - mutex_lock(&dev->mode_config.mutex); - drm_modeset_acquire_init(ctx, 0); - while (1) { - ret = drm_modeset_lock_all_ctx(dev, ctx); - if (ret != -EDEADLK) - break; + state = drm_atomic_helper_duplicate_committed_state(&dev_priv->drm); + if (IS_ERR(state)) { + DRM_ERROR("Duplicating state failed with %ld\n", + PTR_ERR(state)); + return NULL; + } + + to_intel_atomic_state(state)->active_crtcs = 0; + to_intel_atomic_state(state)->cdclk.logical = dev_priv->cdclk.hw; + to_intel_atomic_state(state)->cdclk.actual = dev_priv->cdclk.hw; + + init_intel_state(to_intel_atomic_state(state)); + + /* force a full update */ + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { + struct intel_crtc_state *intel_crtc_state = + to_intel_crtc_state(crtc_state); + + if (!crtc_state->active) + continue;
- drm_modeset_backoff(ctx); + crtc_state->mode_changed = true; + crtc_state->active_changed = true; + crtc_state->planes_changed = true; + crtc_state->connectors_changed = true; + crtc_state->color_mgmt_changed = true; + crtc_state->zpos_changed = true; + + intel_crtc_state->update_pipe = true; + intel_crtc_state->disable_lp_wm = true; + intel_crtc_state->disable_cxsr = true; + intel_crtc_state->update_wm_post = true; + intel_crtc_state->fb_changed = true; + intel_crtc_state->fifo_changed = true; + intel_crtc_state->wm.need_postvbl_update = true; }
+ return state; +} + +void intel_prepare_reset(struct drm_i915_private *dev_priv) +{ + struct drm_atomic_state *disable_state, *restore_state = NULL; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_plane *plane; + struct drm_plane_state *plane_state; + int i; + + down_write(&dev_priv->commit_sem); + /* reset doesn't touch the display, but flips might get nuked anyway, */ if (!i915.force_reset_modeset_test && !gpu_reset_clobbers_display(dev_priv)) @@ -3521,30 +3580,40 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv) * Disabling the crtcs gracefully seems nicer. Also the * g33 docs say we should at least disable all the planes. */ - state = drm_atomic_helper_duplicate_state(dev, ctx); - if (IS_ERR(state)) { - ret = PTR_ERR(state); - DRM_ERROR("Duplicating state failed with %i\n", ret); - return; - } + disable_state = intel_duplicate_committed_state(dev_priv); + if (IS_ERR(disable_state)) + goto out;
- ret = drm_atomic_helper_disable_all(dev, ctx); - if (ret) { - DRM_ERROR("Suspending crtc's failed with %i\n", ret); - drm_atomic_state_put(state); - return; - } + to_intel_atomic_state(disable_state)->active_crtcs = 0;
- dev_priv->modeset_restore_state = state; - state->acquire_ctx = ctx; + for_each_new_crtc_in_state(disable_state, crtc, crtc_state, i) + crtc_state->active = false; + for_each_new_plane_in_state(disable_state, plane, plane_state, i) + plane_state->visible = false; + + __intel_atomic_commit_tail(disable_state, true); + + drm_atomic_helper_clean_committed_state(disable_state); + drm_atomic_state_put(disable_state); + + restore_state = intel_duplicate_committed_state(dev_priv); + if (IS_ERR(restore_state)) + restore_state = NULL; + + for_each_old_crtc_in_state(restore_state, crtc, crtc_state, i) + crtc_state->active = false; + for_each_old_plane_in_state(restore_state, plane, plane_state, i) + plane_state->visible = false; + +out: + dev_priv->modeset_restore_state = restore_state; }
void intel_finish_reset(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm; - struct drm_modeset_acquire_ctx *ctx = &dev_priv->reset_ctx; - struct drm_atomic_state *state = dev_priv->modeset_restore_state; - int ret; + struct drm_atomic_state *restore_state = + dev_priv->modeset_restore_state;
/* * Flips in the rings will be nuked by the reset, @@ -3557,7 +3626,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
/* reset doesn't touch the display */ if (!gpu_reset_clobbers_display(dev_priv)) { - if (!state) { + if (!restore_state) { /* * Flips in the rings have been nuked by the reset, * so update the base address of all primary @@ -3569,11 +3638,11 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) */ intel_update_primary_planes(dev); } else { - ret = __intel_display_resume(dev, state, ctx); - if (ret) - DRM_ERROR("Restoring old state failed with %i\n", ret); + __intel_atomic_commit_tail(restore_state, true); } } else { + i915_redisable_vga(dev_priv); + /* * The display has been reset as well, * so need a full re-initialization. @@ -3589,18 +3658,17 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);
- ret = __intel_display_resume(dev, state, ctx); - if (ret) - DRM_ERROR("Restoring old state failed with %i\n", ret); + __intel_atomic_commit_tail(restore_state, true);
intel_hpd_init(dev_priv); }
- if (state) - drm_atomic_state_put(state); - drm_modeset_drop_locks(ctx); - drm_modeset_acquire_fini(ctx); - mutex_unlock(&dev->mode_config.mutex); + if (restore_state) { + drm_atomic_helper_clean_committed_state(restore_state); + drm_atomic_state_put(restore_state); + } + + up_write(&dev_priv->commit_sem); }
static bool abort_flip_on_reset(struct intel_crtc *crtc) @@ -12592,29 +12660,18 @@ static int intel_modeset_checks(struct drm_atomic_state *state) { struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct drm_i915_private *dev_priv = to_i915(state->dev); - struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state, *new_crtc_state; - int ret = 0, i; + int ret = 0;
if (!check_digital_port_conflicts(state)) { DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); return -EINVAL; }
- intel_state->modeset = true; intel_state->active_crtcs = dev_priv->active_crtcs; intel_state->cdclk.logical = dev_priv->cdclk.logical; intel_state->cdclk.actual = dev_priv->cdclk.actual;
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { - if (new_crtc_state->active) - intel_state->active_crtcs |= 1 << i; - else - intel_state->active_crtcs &= ~(1 << i); - - if (old_crtc_state->active != new_crtc_state->active) - intel_state->active_pipe_changes |= drm_crtc_mask(crtc); - } + init_intel_state(intel_state);
/* * See if the config requires any additional preparation, e.g. @@ -13004,7 +13061,7 @@ static void intel_atomic_helper_free_state_worker(struct work_struct *work) intel_atomic_helper_free_state(dev_priv); }
-static void __intel_atomic_commit_tail(struct drm_atomic_state *state) +static void __intel_atomic_commit_tail(struct drm_atomic_state *state, bool is_reset) { struct drm_device *dev = state->dev; struct intel_atomic_state *intel_state = to_intel_atomic_state(state); @@ -13068,10 +13125,18 @@ static void __intel_atomic_commit_tail(struct drm_atomic_state *state)
/* Only after disabling all output pipelines that will be changed can we * update the the output configuration. */ - intel_modeset_update_crtc_state(state); + if (!is_reset) + intel_modeset_update_crtc_state(state);
if (intel_state->modeset) { - drm_atomic_helper_update_legacy_modeset_state(state->dev, state); + if (!is_reset) { + drm_atomic_helper_update_legacy_modeset_state(state->dev, state); + } else { + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->enable) + drm_calc_timestamping_constants(crtc, &new_crtc_state->adjusted_mode); + } + }
intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
@@ -13082,7 +13147,8 @@ static void __intel_atomic_commit_tail(struct drm_atomic_state *state) if (!intel_can_enable_sagv(state)) intel_disable_sagv(dev_priv);
- intel_modeset_verify_disabled(dev, state); + if (!is_reset) + intel_modeset_verify_disabled(dev, state); }
/* Complete the events for pipes that have now been disabled */ @@ -13135,7 +13201,8 @@ static void __intel_atomic_commit_tail(struct drm_atomic_state *state) if (put_domains[i]) modeset_put_power_domains(dev_priv, put_domains[i]);
- intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state); + if (!is_reset) + intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state); }
if (intel_state->modeset && intel_can_enable_sagv(state)) @@ -13160,10 +13227,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
drm_atomic_helper_wait_for_dependencies(state);
- __intel_atomic_commit_tail(state); + down_read(&dev_priv->commit_sem); + + __intel_atomic_commit_tail(state, false);
drm_atomic_helper_commit_hw_done(state);
+ up_read(&dev_priv->commit_sem); + mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state); mutex_unlock(&dev->struct_mutex); @@ -15022,6 +15093,8 @@ int intel_modeset_init(struct drm_device *dev) INIT_WORK(&dev_priv->atomic_helper.free_work, intel_atomic_helper_free_state_worker);
+ init_rwsem(&dev_priv->commit_sem); + intel_init_quirks(dev);
intel_init_pm(dev_priv);