drm/i915: Watch out for idling during i915_gem_evict_something
diff mbox series

Message ID 20200509115217.26853-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915: Watch out for idling during i915_gem_evict_something
Related show

Commit Message

Chris Wilson May 9, 2020, 11:52 a.m. UTC
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+
---
 drivers/gpu/drm/i915/i915_gem_evict.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Comments

Chris Wilson May 9, 2020, 12:02 p.m. UTC | #1
Quoting Chris Wilson (2020-05-09 12:52:17)
> 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

Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
Fixes: b1e3177bd1d8 ("drm/i915: Coordinate i915_active with its own mutex")

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: <stable@vger.kernel.org> # v5.5+
Mika Kuoppala May 12, 2020, 10:39 a.m. UTC | #2
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
Chris Wilson May 12, 2020, 10:51 a.m. UTC | #3
Quoting Mika Kuoppala (2020-05-12 11:39:31)
> 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.

Duck and cover for the idling is a-coming!
-Chris

Patch
diff mbox series

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))