Re: [Intel-gfx] [PATCH] drm/i915: Watch out for idling during i915_gem_evict_something

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> i915_gem_evict_something() is charged with finding a slot within the GTT
> that we may reuse. Since our goal is not to stall, we first look for a
> slot that only overlaps idle vma. To this end, on the first pass we move
> any active vma to the end of the search list. However, we only stopped
> moving active vma after we see the first active vma twice. If during the
> search, that first active vma completed, we would not notice and keep on
> extending the search list.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1746
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: <stable@vger.kernel.org> # v5.5+

Only thing I would change is tune up the subject line.
It fixes a possible busy loop in eviction so I feel 'watch out' is not
strong enough for my liking.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_evict.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 0ba7b1e881c0..6501939929d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -128,6 +128,13 @@ i915_gem_evict_something(struct i915_address_space *vm,
>  	active = NULL;
>  	INIT_LIST_HEAD(&eviction_list);
>  	list_for_each_entry_safe(vma, next, &vm->bound_list, vm_link) {
> +		if (vma == active) { /* now seen this vma twice */
> +			if (flags & PIN_NONBLOCK)
> +				break;
> +
> +			active = ERR_PTR(-EAGAIN);
> +		}
> +
>  		/*
>  		 * We keep this list in a rough least-recently scanned order
>  		 * of active elements (inactive elements are cheap to reap).
> @@ -143,21 +150,12 @@ i915_gem_evict_something(struct i915_address_space *vm,
>  		 * To notice when we complete one full cycle, we record the
>  		 * first active element seen, before moving it to the tail.
>  		 */
> -		if (i915_vma_is_active(vma)) {
> -			if (vma == active) {
> -				if (flags & PIN_NONBLOCK)
> -					break;
> -
> -				active = ERR_PTR(-EAGAIN);
> -			}
> -
> -			if (active != ERR_PTR(-EAGAIN)) {
> -				if (!active)
> -					active = vma;
> +		if (active != ERR_PTR(-EAGAIN) && i915_vma_is_active(vma)) {
> +			if (!active)
> +				active = vma;
>  
> -				list_move_tail(&vma->vm_link, &vm->bound_list);
> -				continue;
> -			}
> +			list_move_tail(&vma->vm_link, &vm->bound_list);
> +			continue;
>  		}
>  
>  		if (mark_free(&scan, vma, flags, &eviction_list))
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx