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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
