Re: [PATCH] fixup! drm/xe/eudebug: implement userptr_vma access

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jul 31, 2024 at 01:11:12PM +0200, Andrzej Hajda wrote:
> This fixup pulls call to xe_vma_userptr_pin_pages out of
> vm->userptr.notifier_lock.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
> Hi Matthew,
> 
> I hope this fixup should answer your concerns.
> It was tested on LNL with lockdep turned on and with your
> two patches mentioned earlier.
> 
> Posted in form of fixup to underline the difference,
> let me know if you insist on full patch.
> 

I think you change will work but I think this can be written cleaner and
also in a way in which the EU debug layer doesn't have to call into mmu
notifier functions (eventually we want to move all notifier interaction
to a DRM layer too).

How about:

retry:
	down_read(&vm->userptr.notifier_lock)

	/* re-pin if necessary */
	if (xe_vma_userptr_check_repin(uvma)) {
		up_read(&vm->userptr.notifier_lock)
		
		spin_lock(&vm->userptr.invalidated_lock);
		list_del_init(&uvma->userptr.invalidate_link);
		spin_unlock(&vm->userptr.invalidated_lock);

		ret = xe_vma_userptr_pin_pages(uvma);
		if (ret)
			return err;

		goto retry;
	}

	/* Do copy */

Also while you are here, maybe move the implementation for this function
in xe_vm.c as that seems to be a better place for this as this all
userptr specific code.

Matt

> Regards
> Andrzej
> ---
>  drivers/gpu/drm/xe/xe_eudebug.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> index 472d9314505a..80d6f886969a 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
> @@ -3343,6 +3343,7 @@ static int xe_eudebug_uvma_access(struct xe_userptr_vma *uvma, u64 offset,
>  	struct xe_res_cursor cur = {};
>  	int cur_len, ret = 0;
>  
> +lock_notifier:
>  	/* lock notifier in non-invalidation state */
>  	for (unsigned long nseq = uvma->userptr.notifier_seq; true;
>  	     nseq = mmu_interval_read_begin(&uvma->userptr.notifier)) {
> @@ -3358,9 +3359,11 @@ static int xe_eudebug_uvma_access(struct xe_userptr_vma *uvma, u64 offset,
>  		list_del_init(&uvma->userptr.invalidate_link);
>  		spin_unlock(&vm->userptr.invalidated_lock);
>  
> +		up_read(&vm->userptr.notifier_lock);
>  		ret = xe_vma_userptr_pin_pages(uvma);
>  		if (ret)
> -			goto out_unlock_notifier;
> +			return ret;
> +		goto lock_notifier;
>  	}
>  
>  	if (!up->sg) {
> -- 
> 2.34.1
>