Re: [Intel-gfx] [PATCH v4 61/61] drm/i915: Keep userpointer bindings if seqcount is unchanged, v2.

["Thomas Hellström (Intel)" <thomas_os@shipmail.org>]

On 10/16/20 12:44 PM, Maarten Lankhorst wrote:
> Instead of force unbinding and rebinding every time, we try to check
> if our notifier seqcount is still correct when pages are bound. This
> way we only rebind userptr when we need to, and prevent stalls.
>
> Changes since v1:
> - Missing mutex_unlock, reported by kbuild.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 27 ++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 8f05b6d90d54..b3fd5eecf0a1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -268,12 +268,33 @@ int i915_gem_object_userptr_submit_init(struct drm_i915_gem_object *obj)
>   	if (ret)
>   		return ret;
>   
> -	/* Make sure userptr is unbound for next attempt, so we don't use stale pages. */
> -	ret = i915_gem_object_userptr_unbind(obj, false);
> +	/* optimistically try to preserve current pages while unlocked */
> +	if (i915_gem_object_has_pages(obj) &&
> +	    !mmu_interval_check_retry(&obj->userptr.notifier,
> +				      obj->userptr.notifier_seq)) {
> +		spin_lock(&i915->mm.notifier_lock);
> +		if (obj->userptr.pvec &&
> +		    !mmu_interval_read_retry(&obj->userptr.notifier,
> +					     obj->userptr.notifier_seq)) {

In theory, although extremely unlikely, on 32-bit the internal seqno may 
wrap giving a false negative above, That could be remedied by clearing 
obj->userptr.notifier_seq in the notifier. (Even seqnos will always be 
considered invalid).

> +			obj->userptr.page_ref++;
> +
> +			/* We can keep using the current binding, this is the fastpath */
> +			ret = 1;
> +		}
> +		spin_unlock(&i915->mm.notifier_lock);
> +	}
> +
> +	if (!ret) {
> +		/* Make sure userptr is unbound for next attempt, so we don't use stale pages. */
> +		ret = i915_gem_object_userptr_unbind(obj, false);
> +	}
>   	i915_gem_object_unlock(obj);
> -	if (ret)
> +	if (ret < 0)
>   		return ret;
>   
> +	if (ret > 0)
> +		return 0;
> +
>   	notifier_seq = mmu_interval_read_begin(&obj->userptr.notifier);

This function will sleep until the core is done with the last 
invalidation, during which time all validation checks above will return 
false. We should place this call before any other checks in the function.

Furthermore the sleep is uninterruptible. We probably need a core change 
to get this right.

>   
>   	pvec = kvmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx