[Intel-gfx] [PATCH] drm/i915: Differentiate between aliasing-ppgtt and ggtt pinning

[Chris Wilson]

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