On Tue, Mar 23, 2021 at 04:50:52PM +0100, Maarten Lankhorst wrote:
> We get a lockdep splat when the reset mutex is held, because it can be
> taken from fence_wait. This conflicts with the mmu notifier we have,
> because we recurse between reset mutex and mmap lock -> mmu notifier.
>
> Remove this recursion by calling revoke_mmaps before taking the lock.
>
> The reset code still needs fixing, as taking mmap locks during reset
> is not allowed.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Reviewed-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_reset.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 990cb4adbb9a..447f589750c2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -970,8 +970,6 @@ static int do_reset(struct intel_gt *gt, intel_engine_mask_t stalled_mask)
> {
> int err, i;
>
> - gt_revoke(gt);
> -
> err = __intel_gt_reset(gt, ALL_ENGINES);
> for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
> msleep(10 * (i + 1));
> @@ -1026,6 +1024,9 @@ void intel_gt_reset(struct intel_gt *gt,
>
> might_sleep();
> GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, >->reset.flags));
> +
I've added a FIXME comment here just so we don't totally forget. This will
also blow up again when we wrap the entire reset path into a dma_fence
critical section annotation (at least going forward, we can't do that on
hw that needs display reset with the current code unfortunately).
But I did look at the code which originally added this in
commit 2caffbf1176256cc4f8d4e5c3c524fc689cb9876
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Fri Feb 8 15:37:03 2019 +0000
drm/i915: Revoke mmaps and prevent access to fence registers across reset
and noped right out.
I think this complexity needs to go entirely, and instead we just protect
the fence register state to make sure that after reset they are all good
again:
- add a new mutex for low level fence register state
- hold that mutex around fence register writes (really just the low level
fence writes)
- hold it in the reset path when we restore fence registers
This means that a global reset also thrashes mmaps, but it's a global
reset we're talking about here, everything is thrash anyway. Plus/minus
fenced gtt mmaps really doesn't change the tally.
The real solution is per-engine reset here, and making sure that works as
well as absolutely possible.
Maarten, can you pls take care of this in a follow-up? We have to do this
anyway when we roll out more dma_fence annotations.
Thanks, Daniel
> + gt_revoke(gt);
> +
> mutex_lock(>->reset.mutex);
>
> /* Clear any previous failed attempts at recovery. Time to try again. */
> --
> 2.31.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
