[v9,63/70] drm/i915: Move gt_revoke() slightly

Commit Message

Maarten Lankhorst March 23, 2021, 3:50 p.m. UTC

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 <[e-mail]>
Reviewed-by: Thomas Hellström <[e-mail]>
---
 drivers/gpu/drm/i915/gt/intel_reset.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Daniel Vetter March 24, 2021, 5 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Thomas Hellström <[e-mail]>
> ---
>  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, &gt->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 <[e-mail]>
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(&gt->reset.mutex);
>  
>  	/* Clear any previous failed attempts at recovery. Time to try again. */
> -- 
> 2.31.0
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjala March 24, 2021, 5:15 p.m. UTC | #2

On Wed, Mar 24, 2021 at 06:00:12PM +0100, Daniel Vetter wrote:
> 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 <[e-mail]>
> > Reviewed-by: Thomas Hellström <[e-mail]>
> > ---
> >  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, &gt->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 <[e-mail]>
> 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.

My recollection is that GPU reset doesn't actually clobber the fence 
registers. Though not 100% sure I can trust my brain on this. Also
dunno if it actually matter here or not, but figured I'd point it out.

Daniel Vetter March 24, 2021, 5:16 p.m. UTC | #3

On Wed, Mar 24, 2021 at 07:15:36PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 24, 2021 at 06:00:12PM +0100, Daniel Vetter wrote:
> > 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 <[e-mail]>
> > > Reviewed-by: Thomas Hellström <[e-mail]>
> > > ---
> > >  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, &gt->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 <[e-mail]>
> > 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.
> 
> My recollection is that GPU reset doesn't actually clobber the fence 
> registers. Though not 100% sure I can trust my brain on this. Also
> dunno if it actually matter here or not, but figured I'd point it out.

I think on gen2/3 it does, because there everything goes over. But yeah
maybe on gen4+ it's all fine, would be worth to check that.
-Daniel

Ville Syrjala March 24, 2021, 5:58 p.m. UTC | #4

On Wed, Mar 24, 2021 at 06:16:44PM +0100, Daniel Vetter wrote:
> On Wed, Mar 24, 2021 at 07:15:36PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 24, 2021 at 06:00:12PM +0100, Daniel Vetter wrote:
> > > 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 <[e-mail]>
> > > > Reviewed-by: Thomas Hellström <[e-mail]>
> > > > ---
> > > >  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, &gt->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 <[e-mail]>
> > > 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.
> > 
> > My recollection is that GPU reset doesn't actually clobber the fence 
> > registers. Though not 100% sure I can trust my brain on this. Also
> > dunno if it actually matter here or not, but figured I'd point it out.
> 
> I think on gen2/3 it does, because there everything goes over. But yeah
> maybe on gen4+ it's all fine, would be worth to check that.

Right you are. Gave it a quick test on my 945gm and the fence
registers did get zeroed out. I guess it was snb+ where it didn't
happen. Well, could be some of the earlier platforms too I guess.

Patch

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, &gt->reset.flags));
+
+	gt_revoke(gt);
+
 	mutex_lock(&gt->reset.mutex);
 
 	/* Clear any previous failed attempts at recovery. Time to try again. */