[Intel-gfx] [PATCH 3/7] drm/i915: Convert fences to use a GGTT lock rather than struct_mutex

[Chris Wilson]

Quoting Daniel Vetter (2018-07-11 10:08:46)
> On Wed, Jul 11, 2018 at 08:36:04AM +0100, Chris Wilson wrote:
> > Introduce a new mutex to guard all of the vma operations within a vm (as
> > opposed to the BKL struct_mutex) and start by using it to guard the
> > fence operations for a GGTT VMA.
> 
> Commit message is a bit confusing, since you've already introduce this new
> mutex in an earlier patch. Please change to "Switch from dev->struct_mutex
> to ggtt.vm->mutex" or similar ...
> 
> For the reviewers benefit it would also be good to explain how this new
> vm.mutex nests with others stuff here (dev->struct_mutex and rpm come to
> mind, looking from the patch). Probably best here than in docs since it's
> likely going to get outdated.
> 
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c        |  9 ++-
> >  drivers/gpu/drm/i915/i915_gem.c            | 11 +++-
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 +-
> >  drivers/gpu/drm/i915/i915_gem_fence_reg.c  | 68 +++++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_vma.c            | 12 ++--
> >  drivers/gpu/drm/i915/i915_vma.h            | 23 +++++++-
> >  6 files changed, 96 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 75ffed6a3f31..e2ba298a5d88 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -80,7 +80,7 @@ static char get_tiling_flag(struct drm_i915_gem_object *obj)
> >  
> >  static char get_global_flag(struct drm_i915_gem_object *obj)
> >  {
> > -     return obj->userfault_count ? 'g' : ' ';
> > +     return READ_ONCE(obj->userfault_count) ? 'g' : ' ';
> 
> The usefault_count here (and below) look like misplaced hunks?
> 
> >  }
> >  
> >  static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)
> > @@ -914,11 +914,10 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
> >  
> >  static int i915_gem_fence_regs_info(struct seq_file *m, void *data)
> >  {
> > -     struct drm_i915_private *i915 = node_to_i915(m->private);
> > -     const struct i915_ggtt *ggtt = &i915->ggtt;
> > +     struct i915_ggtt *ggtt = &node_to_i915(m->private)->ggtt;
> >       int i, ret;
> >  
> > -     ret = mutex_lock_interruptible(&i915->drm.struct_mutex);
> > +     ret = mutex_lock_interruptible(&ggtt->vm.mutex);
> >       if (ret)
> >               return ret;
> >  
> > @@ -935,7 +934,7 @@ static int i915_gem_fence_regs_info(struct seq_file *m, void *data)
> >               seq_putc(m, '\n');
> >       }
> >  
> > -     mutex_unlock(&i915->drm.struct_mutex);
> > +     mutex_unlock(&ggtt->vm.mutex);
> >       return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 356c86071ccc..cbcba613b175 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2193,8 +2193,8 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> >        * requirement that operations to the GGTT be made holding the RPM
> >        * wakeref.
> >        */
> > -     lockdep_assert_held(&i915->drm.struct_mutex);
> >       intel_runtime_pm_get(i915);
> > +     mutex_lock(&i915->ggtt.vm.mutex);
> >  
> >       if (!obj->userfault_count)
> >               goto out;
> > @@ -2211,6 +2211,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> >       wmb();
> >  
> >  out:
> > +     mutex_unlock(&i915->ggtt.vm.mutex);
> >       intel_runtime_pm_put(i915);
> >  }
> >  
> > @@ -2223,10 +2224,12 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
> >       /*
> >        * Only called during RPM suspend. All users of the userfault_list
> >        * must be holding an RPM wakeref to ensure that this can not
> > -      * run concurrently with themselves (and use the struct_mutex for
> > +      * run concurrently with themselves (and use the ggtt->mutex for
> >        * protection between themselves).
> >        */
> 
> I think the above change isn't correct, at least not yet at this stage:
> All users of the userfault_list still use dev->struct_mutex, not vm.mutex.
> I guess we could move that over to the ggtt.vm.mutex eventually, but this
> patch doesn't do that.

It does, all those misplaced hunks are not so misplaced.
-Chris