Re: [PATCH 3/5] drm/i915: During shrink_all we only need to idle the GPU

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/5] drm/i915: During shrink_all we only need to idle the GPU
Date: Tue, 6 Oct 2015 15:00:49 +0200	[thread overview]
Message-ID: <20151006130049.GU3383@phenom.ffwll.local> (raw)
In-Reply-To: <1443698309-28038-3-git-send-email-chris@chris-wilson.co.uk>

On Thu, Oct 01, 2015 at 12:18:27PM +0100, Chris Wilson wrote:
> We can forgo an evict-everything here as the shrinker operation itself
> will unbind any vma as required. If we explicitly idle the GPU through a
> switch to the default context, we not only create a request in an
> illegal context (e.g. whilst shrinking during execbuf with a request
> already allocated), but switching to the default context will not free
> up the memory backing the active contexts - unless in the unlikely
> situation that context had already been closed (and just kept arrive by
> being the current context). The saving is near zero and the danger real.
> 
> To compensate for the loss of the forced retire, add a couple of
> retire-requests to i915_gem_shirnk() - this should help free up any
> transitive cache from the requests.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 88f66a2586ec..2058d162aeb9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -86,6 +86,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  	unsigned long count = 0;
>  
>  	trace_i915_gem_shrink(dev_priv, target, flags);
> +	i915_gem_retire_requests(dev_priv->dev);
>  
>  	/*
>  	 * As we may completely rewrite the (un)bound list whilst unbinding
> @@ -141,6 +142,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  		list_splice(&still_in_list, phase->list);
>  	}
>  
> +	i915_gem_retire_requests(dev_priv->dev);

I dont really get the justification for the 2nd retire_requests. Also
isn't the first one only needed for the last patch to not stall in the
normal shrinker on active objects?

Aside for blowing up on requests and nested stuff: We could make
alloc_request/request_submit/cancel a lockdep locking pair. That would
catch bogus nesting and locking inversion through the mm subsystem (since
any malloc function is it's own lockdep critical section to avoid
deadlocks on GFP_NOFS and friends).

Also splitting out evict_everything into that one-line patch might be good
for -fixes if we have bug reports where this blows up.
-Daniel

> +
>  	return count;
>  }
>  
> @@ -160,7 +163,6 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>   */
>  unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv)
>  {
> -	i915_gem_evict_everything(dev_priv->dev);
>  	return i915_gem_shrink(dev_priv, -1UL,
>  			       I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
>  }
> -- 
> 2.6.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx