[Intel-xe] [PATCH 3/5] drm/xe: Fix extobj dropping issue.

[Thomas Hellström]

On 5/26/23 14:10, Maarten Lankhorst wrote:
> I believe originally we did the right thing, but now we hit an issue.
>
> Presumably, this was changed when converting from an array to a list
> in a squashed commit:
> "drm/xe: Use a list for the vm's external objects"
>
> When deleting a vma with an extobj in the list, ensure if it's freed
> but still listed on the vm, we take a different vma and add that to
> the extobj list instead.
>
> I'll add an igt, and make sure it doesn't happen again.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/194

Lgtm. Perhaps add a Fixes: or a !fixup to that broken commit?

Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>


> ---
>   drivers/gpu/drm/xe/xe_vm.c | 95 +++++++++++++++++++++++---------------
>   1 file changed, 58 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f73e08b60670..dd1335e12d4c 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -909,12 +909,14 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>   	return vma;
>   }
>   
> -static void vm_remove_extobj(struct xe_vma *vma)
> +static bool vm_remove_extobj(struct xe_vma *vma)
>   {
>   	if (!list_empty(&vma->extobj.link)) {
>   		vma->vm->extobj.entries--;
>   		list_del_init(&vma->extobj.link);
> +		return true;
>   	}
> +	return false;
>   }
>   
>   static void xe_vma_destroy_late(struct xe_vma *vma)
> @@ -955,6 +957,51 @@ static void vma_destroy_work_func(struct work_struct *w)
>   	xe_vma_destroy_late(vma);
>   }
>   
> +static struct xe_vma *
> +bo_has_vm_references_locked(struct xe_bo *bo, struct xe_vm *vm,
> +			    struct xe_vma *ignore)
> +{
> +	struct xe_vma *vma;
> +
> +	list_for_each_entry(vma, &bo->vmas, bo_link) {
> +		if (vma != ignore && vma->vm == vm && !vma->destroyed)
> +			return vma;
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool bo_has_vm_references(struct xe_bo *bo, struct xe_vm *vm,
> +				 struct xe_vma *ignore)
> +{
> +	struct ww_acquire_ctx ww;
> +	bool ret;
> +
> +	xe_bo_lock(bo, &ww, 0, false);
> +	ret = !!bo_has_vm_references_locked(bo, vm, ignore);
> +	xe_bo_unlock(bo, &ww);
> +
> +	return ret;
> +}
> +
> +static void __vm_insert_extobj(struct xe_vm *vm, struct xe_vma *vma)
> +{
> +	list_add(&vma->extobj.link, &vm->extobj.list);
> +	vm->extobj.entries++;
> +}
> +
> +static void vm_insert_extobj(struct xe_vm *vm, struct xe_vma *vma)
> +{
> +	struct xe_bo *bo = vma->bo;
> +
> +	lockdep_assert_held_write(&vm->lock);
> +
> +	if (bo_has_vm_references(bo, vm, vma))
> +		return;
> +
> +	__vm_insert_extobj(vm, vma);
> +}
> +
>   static void vma_destroy_cb(struct dma_fence *fence,
>   			   struct dma_fence_cb *cb)
>   {
> @@ -980,11 +1027,19 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence)
>   	} else {
>   		xe_bo_assert_held(vma->bo);
>   		list_del(&vma->bo_link);
> +
>   		spin_lock(&vm->notifier.list_lock);
>   		list_del(&vma->notifier.rebind_link);
>   		spin_unlock(&vm->notifier.list_lock);
> -		if (!vma->bo->vm)
> -			vm_remove_extobj(vma);
> +
> +		if (!vma->bo->vm && vm_remove_extobj(vma)) {
> +			struct xe_vma *other;
> +
> +			other = bo_has_vm_references_locked(vma->bo, vm, NULL);
> +
> +			if (other)
> +				__vm_insert_extobj(vm, other);
> +		}
>   	}
>   
>   	xe_vm_assert_held(vm);
> @@ -2481,40 +2536,6 @@ static int vm_bind_ioctl_async(struct xe_vm *vm, struct xe_vma *vma,
>   	return err;
>   }
>   
> -static bool bo_has_vm_references(struct xe_bo *bo, struct xe_vm *vm,
> -				 struct xe_vma *ignore)
> -{
> -	struct ww_acquire_ctx ww;
> -	struct xe_vma *vma;
> -	bool ret = false;
> -
> -	xe_bo_lock(bo, &ww, 0, false);
> -	list_for_each_entry(vma, &bo->vmas, bo_link) {
> -		if (vma != ignore && vma->vm == vm && !vma->destroyed) {
> -			ret = true;
> -			break;
> -		}
> -	}
> -	xe_bo_unlock(bo, &ww);
> -
> -	return ret;
> -}
> -
> -static int vm_insert_extobj(struct xe_vm *vm, struct xe_vma *vma)
> -{
> -	struct xe_bo *bo = vma->bo;
> -
> -	lockdep_assert_held_write(&vm->lock);
> -
> -	if (bo_has_vm_references(bo, vm, vma))
> -		return 0;
> -
> -	list_add(&vma->extobj.link, &vm->extobj.list);
> -	vm->extobj.entries++;
> -
> -	return 0;
> -}
> -
>   static int __vm_bind_ioctl_lookup_vma(struct xe_vm *vm, struct xe_bo *bo,
>   				      u64 addr, u64 range, u32 op)
>   {