diff mbox series

drm/i915: Differentiate between aliasing-ppgtt and ggtt pinning

Message ID 20200326142727.31962-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series drm/i915: Differentiate between aliasing-ppgtt and ggtt pinning | expand

Commit Message

Chris Wilson March 26, 2020, 2:27 p.m. UTC 
Userptr causes lockdep to complain when we are using the aliasing-ppgtt
(and ggtt, but for that it is rightfully so to complain about) in that
when we revoke the userptr we take a mutex which we also use to revoke
the mmaps. However, we only revoke mmaps for GGTT bindings and we never
allow userptr to create a GGTT binding so the warning should be false
and is simply caused by our conflation of the aliasing-ppgtt with the
ggtt. So lets try treating the binding into the aliasing-ppgtt as a
separate lockclass from the ggtt. The downside is that we are
deliberately suppressing lockdep;s ability to warn us of cycles.

Closes: https://gitlab.freedesktop.org/drm/intel/issues/478
---
 drivers/gpu/drm/i915/i915_vma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Andi Shyti March 27, 2020, 4:27 p.m. UTC | #1 
Hi Chris,

On Thu, Mar 26, 2020 at 02:27:27PM +0000, Chris Wilson wrote:
> Userptr causes lockdep to complain when we are using the aliasing-ppgtt
> (and ggtt, but for that it is rightfully so to complain about) in that
> when we revoke the userptr we take a mutex which we also use to revoke
> the mmaps. However, we only revoke mmaps for GGTT bindings and we never
> allow userptr to create a GGTT binding so the warning should be false
> and is simply caused by our conflation of the aliasing-ppgtt with the
> ggtt. So lets try treating the binding into the aliasing-ppgtt as a
> separate lockclass from the ggtt. The downside is that we are
> deliberately suppressing lockdep;s ability to warn us of cycles.
                                ^^^^
typo

> 
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/478

I'm not a big fan of links in commit messages, I think they would
be forbidden by law, but I'm not being picky on that.

I don't know, thogh, why your S-o-b is missing.

> ---
>  drivers/gpu/drm/i915/i915_vma.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 191577a98390..9f4a31cd54ac 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -914,7 +914,8 @@ int i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  		wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm);
>  
>  	/* No more allocations allowed once we hold vm->mutex */
> -	err = mutex_lock_interruptible(&vma->vm->mutex);
> +	err = mutex_lock_interruptible_nested(&vma->vm->mutex,
> +					      !(flags & PIN_GLOBAL));
>  	if (err)
>  		goto err_fence;
>  
> @@ -1320,7 +1321,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	if (err)
>  		goto out_rpm;
>  
> -	err = mutex_lock_interruptible(&vm->mutex);
> +	err = mutex_lock_interruptible_nested(&vma->vm->mutex, !wakeref);

looks reasonable to me. Thanks!

Are you planning to push it? You have my review for this.

Andi
Chris Wilson March 27, 2020, 4:35 p.m. UTC | #2 
Quoting Andi Shyti (2020-03-27 16:27:27)
> Hi Chris,
> 
> On Thu, Mar 26, 2020 at 02:27:27PM +0000, Chris Wilson wrote:
> > Userptr causes lockdep to complain when we are using the aliasing-ppgtt
> > (and ggtt, but for that it is rightfully so to complain about) in that
> > when we revoke the userptr we take a mutex which we also use to revoke
> > the mmaps. However, we only revoke mmaps for GGTT bindings and we never
> > allow userptr to create a GGTT binding so the warning should be false
> > and is simply caused by our conflation of the aliasing-ppgtt with the
> > ggtt. So lets try treating the binding into the aliasing-ppgtt as a
> > separate lockclass from the ggtt. The downside is that we are
> > deliberately suppressing lockdep;s ability to warn us of cycles.
>                                 ^^^^
> typo
> 
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/issues/478
> 
> I'm not a big fan of links in commit messages, I think they would
> be forbidden by law, but I'm not being picky on that.

I'm lazy, I take clickable links.

> I don't know, thogh, why your S-o-b is missing.

I felt uncomfortable with this hack, but it passes CI (but it may be
suppressing too much -- I think the code is safe at the moment, but we
may lose our sensitivity to future bugs).

> 
> > ---
> >  drivers/gpu/drm/i915/i915_vma.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> > index 191577a98390..9f4a31cd54ac 100644
> > --- a/drivers/gpu/drm/i915/i915_vma.c
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -914,7 +914,8 @@ int i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> >               wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm);
> >  
> >       /* No more allocations allowed once we hold vm->mutex */
> > -     err = mutex_lock_interruptible(&vma->vm->mutex);
> > +     err = mutex_lock_interruptible_nested(&vma->vm->mutex,
> > +                                           !(flags & PIN_GLOBAL));
> >       if (err)
> >               goto err_fence;
> >  
> > @@ -1320,7 +1321,7 @@ int i915_vma_unbind(struct i915_vma *vma)
> >       if (err)
> >               goto out_rpm;
> >  
> > -     err = mutex_lock_interruptible(&vm->mutex);
> > +     err = mutex_lock_interruptible_nested(&vma->vm->mutex, !wakeref);
> 
> looks reasonable to me. Thanks!
> 
> Are you planning to push it? You have my review for this.

I'm planning on adding a comment to attempt to justify itself and then
push.
-Chris
Andi Shyti March 27, 2020, 5:53 p.m. UTC | #3 
Hi Chris,

> > On Thu, Mar 26, 2020 at 02:27:27PM +0000, Chris Wilson wrote:
> > > Userptr causes lockdep to complain when we are using the aliasing-ppgtt
> > > (and ggtt, but for that it is rightfully so to complain about) in that
> > > when we revoke the userptr we take a mutex which we also use to revoke
> > > the mmaps. However, we only revoke mmaps for GGTT bindings and we never
> > > allow userptr to create a GGTT binding so the warning should be false
> > > and is simply caused by our conflation of the aliasing-ppgtt with the
> > > ggtt. So lets try treating the binding into the aliasing-ppgtt as a
> > > separate lockclass from the ggtt. The downside is that we are
> > > deliberately suppressing lockdep;s ability to warn us of cycles.
> >                                 ^^^^
> > typo
> > 
> > > 
> > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/478
> > 
> > I'm not a big fan of links in commit messages, I think they would
> > be forbidden by law, but I'm not being picky on that.
> 
> I'm lazy, I take clickable links.
> 
> > I don't know, thogh, why your S-o-b is missing.
> 
> I felt uncomfortable with this hack, but it passes CI (but it may be
> suppressing too much -- I think the code is safe at the moment, but we
> may lose our sensitivity to future bugs).

We can only hope in CI critical cases are covered well enough...
in the worst case we can always revert it.

Andi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 191577a98390..9f4a31cd54ac 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -914,7 +914,8 @@  int i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 		wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm);
 
 	/* No more allocations allowed once we hold vm->mutex */
-	err = mutex_lock_interruptible(&vma->vm->mutex);
+	err = mutex_lock_interruptible_nested(&vma->vm->mutex,
+					      !(flags & PIN_GLOBAL));
 	if (err)
 		goto err_fence;
 
@@ -1320,7 +1321,7 @@  int i915_vma_unbind(struct i915_vma *vma)
 	if (err)
 		goto out_rpm;
 
-	err = mutex_lock_interruptible(&vm->mutex);
+	err = mutex_lock_interruptible_nested(&vma->vm->mutex, !wakeref);
 	if (err)
 		goto out_rpm;