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.
>
> + mutex_lock(&i915->ggtt.vm.mutex);
I also don't think you need to take the lock just yet. For rpm we keep the
fences around (we restore them in rpm_resume after all), we just nuke the
mmap ptes.
> +
> list_for_each_entry_safe(obj, on,
> &i915->mm.userfault_list, userfault_link)
> __i915_gem_object_release_mmap(obj);
> @@ -2255,6 +2258,8 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
> GEM_BUG_ON(i915_vma_has_userfault(reg->vma));
> reg->dirty = true;
> }
> +
> + mutex_unlock(&i915->ggtt.vm.mutex);
> }
>
> static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
> @@ -4861,7 +4866,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> mutex_unlock(&i915->drm.struct_mutex);
>
> GEM_BUG_ON(obj->bind_count);
> - GEM_BUG_ON(obj->userfault_count);
> + GEM_BUG_ON(READ_ONCE(obj->userfault_count));
Another misplaced hunk?
> GEM_BUG_ON(atomic_read(&obj->frontbuffer_bits));
> GEM_BUG_ON(!list_empty(&obj->lut_list));
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 3f0c612d42e7..e1d65b165bf1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -426,8 +426,11 @@ static inline void __eb_unreserve_vma(struct i915_vma *vma, unsigned int flags)
> {
> GEM_BUG_ON(!(flags & __EXEC_OBJECT_HAS_PIN));
>
> - if (unlikely(flags & __EXEC_OBJECT_HAS_FENCE))
> + if (unlikely(flags & __EXEC_OBJECT_HAS_FENCE)) {
> + mutex_lock(&vma->vm->mutex);
> __i915_vma_unpin_fence(vma);
> + mutex_unlock(&vma->vm->mutex);
> + }
>
> __i915_vma_unpin(vma);
> }
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> index 60fa5a8276cb..9313a8e675c8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> @@ -188,6 +188,8 @@ static void i830_write_fence_reg(struct drm_i915_fence_reg *fence,
> static void fence_write(struct drm_i915_fence_reg *fence,
> struct i915_vma *vma)
> {
> + lockdep_assert_held(&fence->ggtt->vm.mutex);
> +
> /* Previous access through the fence register is marshalled by
> * the mb() inside the fault handlers (i915_gem_release_mmaps)
> * and explicitly managed for internal users.
> @@ -213,6 +215,8 @@ static int fence_update(struct drm_i915_fence_reg *fence,
> struct i915_ggtt *ggtt = fence->ggtt;
> int ret;
>
> + lockdep_assert_held(&ggtt->vm.mutex);
> +
> if (vma) {
> if (!i915_vma_is_map_and_fenceable(vma))
> return -EINVAL;
> @@ -289,14 +293,39 @@ static int fence_update(struct drm_i915_fence_reg *fence,
> int i915_vma_put_fence(struct i915_vma *vma)
> {
> struct drm_i915_fence_reg *fence = vma->fence;
> + int err;
>
> if (!fence)
> return 0;
>
> - if (fence->pin_count)
> - return -EBUSY;
> + mutex_lock(&vma->vm->mutex);
> + if (!fence->pin_count)
> + err = fence_update(fence, NULL);
> + else
> + err = -EBUSY;
> + mutex_unlock(&vma->vm->mutex);
>
> - return fence_update(fence, NULL);
> + return err;
> +}
> +
> +void i915_vma_revoke_fence(struct i915_vma *vma)
> +{
> + struct drm_i915_fence_reg *fence;
> +
> + GEM_BUG_ON(!i915_vma_is_ggtt(vma));
> + lockdep_assert_held(&vma->vm->mutex);
> +
> + fence = vma->fence;
> + if (!fence)
> + return;
> +
> + GEM_BUG_ON(fence->pin_count);
> +
> + list_move(&fence->link, &i915_vm_to_ggtt(vma->vm)->fence_list);
> + vma->fence = NULL;
> +
> + fence_write(fence, NULL);
> + fence->vma = NULL;
fence_update(fence, NULL); here please instead of open-coding it. Or I'm
kinda not following why you're doing this?
> }
>
> static struct drm_i915_fence_reg *fence_find(struct i915_ggtt *ggtt)
> @@ -337,8 +366,7 @@ static struct drm_i915_fence_reg *fence_find(struct i915_ggtt *ggtt)
> *
> * 0 on success, negative error code on failure.
> */
> -int
> -i915_vma_pin_fence(struct i915_vma *vma)
> +int __i915_vma_pin_fence(struct i915_vma *vma)
I don't (yet) see a caller of this new __ version ...
> {
> struct i915_ggtt *ggtt = i915_vm_to_ggtt(vma->vm);
> struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL;
> @@ -349,6 +377,7 @@ i915_vma_pin_fence(struct i915_vma *vma)
> * must keep the device awake whilst using the fence.
> */
> assert_rpm_wakelock_held(ggtt->vm.i915);
> + lockdep_assert_held(&ggtt->vm.mutex);
>
> /* Just update our place in the LRU if our fence is getting reused. */
> if (vma->fence) {
> @@ -399,27 +428,34 @@ i915_reserve_fence(struct drm_i915_private *i915)
> int count;
> int ret;
>
> - lockdep_assert_held(&i915->drm.struct_mutex);
> + mutex_lock(&i915->ggtt.vm.mutex);
>
> /* Keep at least one fence available for the display engine. */
> count = 0;
> list_for_each_entry(fence, &ggtt->fence_list, link)
> count += !fence->pin_count;
> - if (count <= 1)
> - return ERR_PTR(-ENOSPC);
> + if (count <= 1) {
> + fence = ERR_PTR(-ENOSPC);
> + goto out_unlock;
> + }
>
> fence = fence_find(ggtt);
> if (IS_ERR(fence))
> - return fence;
> + goto out_unlock;
>
> if (fence->vma) {
> /* Force-remove fence from VMA */
> ret = fence_update(fence, NULL);
> - if (ret)
> - return ERR_PTR(ret);
> + if (ret) {
> + fence = ERR_PTR(ret);
> + goto out_unlock;
> + }
> }
>
> list_del(&fence->link);
> +
> +out_unlock:
> + mutex_unlock(&i915->ggtt.vm.mutex);
> return fence;
> }
>
> @@ -431,9 +467,9 @@ i915_reserve_fence(struct drm_i915_private *i915)
> */
> void i915_unreserve_fence(struct drm_i915_fence_reg *fence)
> {
> - lockdep_assert_held(&fence->ggtt->vm.i915->drm.struct_mutex);
> -
> + mutex_lock(&fence->ggtt->vm.mutex);
> list_add(&fence->link, &fence->ggtt->fence_list);
> + mutex_unlock(&fence->ggtt->vm.mutex);
> }
>
> /**
> @@ -451,8 +487,7 @@ void i915_gem_revoke_fences(struct drm_i915_private *i915)
> struct i915_ggtt *ggtt = &i915->ggtt;
> int i;
>
> - lockdep_assert_held(&i915->drm.struct_mutex);
> -
> + mutex_lock(&ggtt->vm.mutex);
> for (i = 0; i < ggtt->num_fence_regs; i++) {
> struct drm_i915_fence_reg *fence = &ggtt->fence_regs[i];
>
> @@ -461,6 +496,7 @@ void i915_gem_revoke_fences(struct drm_i915_private *i915)
> if (fence->vma)
> i915_vma_revoke_mmap(fence->vma);
> }
> + mutex_unlock(&ggtt->vm.mutex);
> }
>
> /**
> @@ -476,6 +512,7 @@ void i915_gem_restore_fences(struct drm_i915_private *i915)
> struct i915_ggtt *ggtt = &i915->ggtt;
> int i;
>
> + mutex_lock(&ggtt->vm.mutex);
> for (i = 0; i < ggtt->num_fence_regs; i++) {
> struct drm_i915_fence_reg *reg = &ggtt->fence_regs[i];
> struct i915_vma *vma = reg->vma;
> @@ -498,6 +535,7 @@ void i915_gem_restore_fences(struct drm_i915_private *i915)
> fence_write(reg, vma);
> reg->vma = vma;
> }
> + mutex_unlock(&ggtt->vm.mutex);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index ed4e0fb558f7..045b75d79f60 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -860,7 +860,7 @@ void i915_vma_revoke_mmap(struct i915_vma *vma)
> struct drm_vma_offset_node *node = &vma->obj->base.vma_node;
> u64 vma_offset;
>
> - lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> + lockdep_assert_held(&vma->vm->mutex);
>
> if (!i915_vma_has_userfault(vma))
> return;
> @@ -1082,6 +1082,8 @@ int i915_vma_unbind(struct i915_vma *vma)
> return 0;
>
> if (i915_vma_is_map_and_fenceable(vma)) {
> + mutex_lock(&vma->vm->mutex);
> +
> /*
> * Check that we have flushed all writes through the GGTT
> * before the unbind, other due to non-strict nature of those
> @@ -1091,16 +1093,14 @@ int i915_vma_unbind(struct i915_vma *vma)
> i915_vma_flush_writes(vma);
> GEM_BUG_ON(i915_vma_has_ggtt_write(vma));
>
> - /* release the fence reg _after_ flushing */
> - ret = i915_vma_put_fence(vma);
> - if (ret)
> - return ret;
> -
> /* Force a pagefault for domain tracking on next user access */
> i915_vma_revoke_mmap(vma);
> + i915_vma_revoke_fence(vma);
>
> __i915_vma_iounmap(vma);
> vma->flags &= ~I915_VMA_CAN_FENCE;
> +
> + mutex_unlock(&vma->vm->mutex);
> }
> GEM_BUG_ON(vma->fence);
> GEM_BUG_ON(i915_vma_has_userfault(vma));
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index f06d66377107..422d90c686b5 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -190,6 +190,7 @@ static inline bool i915_vma_set_userfault(struct i915_vma *vma)
>
> static inline void i915_vma_unset_userfault(struct i915_vma *vma)
> {
> + lockdep_assert_held(&vma->vm->mutex);
> return __clear_bit(I915_VMA_USERFAULT_BIT, &vma->flags);
> }
>
> @@ -378,11 +379,26 @@ static inline struct page *i915_vma_first_page(struct i915_vma *vma)
> *
> * True if the vma has a fence, false otherwise.
> */
> -int i915_vma_pin_fence(struct i915_vma *vma);
> +int __i915_vma_pin_fence(struct i915_vma *vma);
> +static inline int i915_vma_pin_fence(struct i915_vma *vma)
> +{
> + int err;
> +
> + mutex_lock(&vma->vm->mutex);
> + err = __i915_vma_pin_fence(vma);
> + mutex_unlock(&vma->vm->mutex);
> +
> + return err;
> +}
> +
> int __must_check i915_vma_put_fence(struct i915_vma *vma);
> +void i915_vma_revoke_fence(struct i915_vma *vma);
>
> static inline void __i915_vma_unpin_fence(struct i915_vma *vma)
> {
> + lockdep_assert_held(&vma->vm->mutex);
> + GEM_BUG_ON(!i915_vma_is_ggtt(vma));
> +
> GEM_BUG_ON(vma->fence->pin_count <= 0);
> vma->fence->pin_count--;
> }
> @@ -399,8 +415,11 @@ static inline void
> i915_vma_unpin_fence(struct i915_vma *vma)
> {
> /* lockdep_assert_held(&vma->vm->i915->drm.struct_mutex); */
> - if (vma->fence)
> + if (vma->fence) {
I think you need the lock here outside of vma->fence. At least I
understood the fancy new locking rules to mean that vma->fence is
protected by vma->vm.mutex. Since vma's can't move between vm this is
safe.
Would be good to update the kerneldoc/comments for that accordingly too.
> + mutex_lock(&vma->vm->mutex);
> __i915_vma_unpin_fence(vma);
> + mutex_unlock(&vma->vm->mutex);
> + }
> }
>
> void i915_vma_parked(struct drm_i915_private *i915);
Ok the locking changes in i915_gem_fence_reg.c look good, but git grep
says there's more:
- describe_obj in i915_debugfs.c only wants dev->struct_mutex, but really
wants vm.mutex on top too now. At least for ggtt.
- i915_gem_fence_regs_info in i915_debugfs.c is already fixed in this
patch.
- I've ignored i915_gpu_error.c as usual :-)
- intel_display.c is annoying, since the way it tries to figure out
whether it got a fence or not is racy. That one probably needs the
__i915_vma_pin_fence version that I didn't find ...
- intel_fbc.c looks safe due to the PLANE_HAS_FENCE flags (minus the issue
in intel_display.c)
- I found one more access to vma->fence->id in the selftests, that's
really the same as we do in i915_gpu_error.c. If you feel like it, a
i915_vma_fence_id() static inline that does the correct READ_ONCE dance
on vma->fence could be useful for the paranoid.
Absolutely loved the naming collision with dma_fence, and my grep fu isn't
good enough to avoid these. So probably missed some (but tried rather hard
not to).
Besides these small nits and questions looks all good.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
