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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
