On Thu, Jun 29, 2017 at 04:49:48PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > 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. How does this solve the deadlock? Afaiui the deadlock is that the gpu reset stopped unconditionally completing all requests before it did anything else, including anything with the hw or taking modeset locks. This ensured that any outstanding flips (we only had pageflips, no atomic ones back then) could complete (although maybe displaying garbage). The only thing we had to do was grab the locks (to avoid concurrent modesets) and then we could happily nuke the hw (since the flips where lost in the CS anyway), and restore it afterwards. Then the TDR rewrite came around and broke that, which now makes atomic time out waiting for the gpu to complete (because the gpu reset waits for the modeset to complete first). Which means if you want to fix this without breaking TDR, then you need to cancel the pending atomic commits. That seems somewhat what you're attempting here with trying to figure out what the latest hw-committed step is (but that function gets it wrong), and lots more locking and stuff on top. Why exactly can't we just go back to the old world of force-completing dead requests on gen4 and earlier? That would be tons simpler imo instead of throwing a pile of hacks (which really needs a complete rewrite of the atomic code in i915) in as a work-around. We didn't have TDR on gen4 and earlier for years, going back to that isn't going to hurt anyone. Making working gen4 gpu reset contigent on cancellable atomic commits and the full commit queue is imo nuts. -Daniel > > 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) > > Cc: stable@xxxxxxxxxxxxxxx # for the brave > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_irq.c | 44 +------- > drivers/gpu/drm/i915/intel_display.c | 199 ++++++++++++++++++++++++----------- > 3 files changed, 139 insertions(+), 106 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/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index eb4f1dca2077..9d591f17fda3 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2587,46 +2587,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) > return ret; > } > > -struct wedge_me { > - struct delayed_work work; > - struct drm_i915_private *i915; > - const char *name; > -}; > - > -static void wedge_me(struct work_struct *work) > -{ > - struct wedge_me *w = container_of(work, typeof(*w), work.work); > - > - dev_err(w->i915->drm.dev, > - "%s timed out, cancelling all in-flight rendering.\n", > - w->name); > - i915_gem_set_wedged(w->i915); > -} > - > -static void __init_wedge(struct wedge_me *w, > - struct drm_i915_private *i915, > - long timeout, > - const char *name) > -{ > - w->i915 = i915; > - w->name = name; > - > - INIT_DELAYED_WORK_ONSTACK(&w->work, wedge_me); > - schedule_delayed_work(&w->work, timeout); > -} > - > -static void __fini_wedge(struct wedge_me *w) > -{ > - cancel_delayed_work_sync(&w->work); > - destroy_delayed_work_on_stack(&w->work); > - w->i915 = NULL; > -} > - > -#define i915_wedge_on_timeout(W, DEV, TIMEOUT) \ > - for (__init_wedge((W), (DEV), (TIMEOUT), __func__); \ > - (W)->i915; \ > - __fini_wedge((W))) > - > /** > * i915_reset_device - do process context error handling work > * @dev_priv: i915 device private > @@ -2640,15 +2600,13 @@ static void i915_reset_device(struct drm_i915_private *dev_priv) > char *error_event[] = { I915_ERROR_UEVENT "=1", NULL }; > char *reset_event[] = { I915_RESET_UEVENT "=1", NULL }; > char *reset_done_event[] = { I915_ERROR_UEVENT "=0", NULL }; > - struct wedge_me w; > > kobject_uevent_env(kobj, KOBJ_CHANGE, error_event); > > DRM_DEBUG_DRIVER("resetting chip\n"); > kobject_uevent_env(kobj, KOBJ_CHANGE, reset_event); > > - /* Use a watchdog to ensure that our reset completes */ > - i915_wedge_on_timeout(&w, dev_priv, 5*HZ) { > + { > intel_prepare_reset(dev_priv); > > /* Signal that locked waiters should reset the GPU */ > 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); > -- > 2.13.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch