[PATCH 1/5] drm/i915: shrinker_control->nr_to_scan is now unsigned long

[PATCH 1/5] drm/i915: shrinker_control->nr_to_scan is now unsigned long

[Chris Wilson]

As the shrinker_control now passes us unsigned long targets, update our
shrinker functions to match.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h          | 2 +-
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ec731e6db126..6c807c584d59 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3155,7 +3155,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 
 /* i915_gem_shrinker.c */
 unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
-			      long target,
+			      unsigned long target,
 			      unsigned flags);
 #define I915_SHRINK_PURGEABLE 0x1
 #define I915_SHRINK_UNBOUND 0x2
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index f6ecbda2c604..b627d07fad29 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -73,7 +73,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
  */
 unsigned long
 i915_gem_shrink(struct drm_i915_private *dev_priv,
-		long target, unsigned flags)
+		unsigned long target, unsigned flags)
 {
 	const struct {
 		struct list_head *list;
@@ -159,7 +159,7 @@ 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, LONG_MAX,
+	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

[PATCH 2/5] drm/i915: Add a tracepoint for the shrinker

[Chris Wilson]

Often it is very useful to know why we suddenly purge vast tracts of
memory and surprisingly up until now we didn't even have a tracepoint
for when we shrink our memory.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 ++
 drivers/gpu/drm/i915/i915_trace.h        | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index b627d07fad29..88f66a2586ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -85,6 +85,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 	}, *phase;
 	unsigned long count = 0;
 
+	trace_i915_gem_shrink(dev_priv, target, flags);
+
 	/*
 	 * As we may completely rewrite the (un)bound list whilst unbinding
 	 * (due to retiring requests) we have to strictly process only
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index e6b5c7470ba0..ed7f42f2e740 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -107,6 +107,26 @@ TRACE_EVENT(i915_gem_object_create,
 	    TP_printk("obj=%p, size=%u", __entry->obj, __entry->size)
 );
 
+TRACE_EVENT(i915_gem_shrink,
+	    TP_PROTO(struct drm_i915_private *i915, unsigned long target, unsigned flags),
+	    TP_ARGS(i915, target, flags),
+
+	    TP_STRUCT__entry(
+			     __field(int, dev)
+			     __field(unsigned long, target)
+			     __field(unsigned, flags)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->dev = i915->dev->primary->index;
+			   __entry->target = target;
+			   __entry->flags = flags;
+			   ),
+
+	    TP_printk("dev=%d, target=%lu, flags=%x",
+		      __entry->dev, __entry->target, __entry->flags)
+);
+
 TRACE_EVENT(i915_vma_bind,
 	    TP_PROTO(struct i915_vma *vma, unsigned flags),
 	    TP_ARGS(vma, flags),
-- 
2.6.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Add a tracepoint for the shrinker

[Daniel Vetter]

On Thu, Oct 01, 2015 at 12:18:26PM +0100, Chris Wilson wrote:
> Often it is very useful to know why we suddenly purge vast tracts of
> memory and surprisingly up until now we didn't even have a tracepoint
> for when we shrink our memory.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 ++
>  drivers/gpu/drm/i915/i915_trace.h        | 20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index b627d07fad29..88f66a2586ec 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -85,6 +85,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  	}, *phase;
>  	unsigned long count = 0;
>  
> +	trace_i915_gem_shrink(dev_priv, target, flags);

Shouldn't we also dump how many pages we actually managed to shrink, i.e.
count (at the end of the functions).

Also we have a slab_start/end tracepoint already, but that one obviously
doesn't cover the internal calls to i915_gem_shrink. Should imo be
mentioned in the commit message.
-Daniel

> +
>  	/*
>  	 * As we may completely rewrite the (un)bound list whilst unbinding
>  	 * (due to retiring requests) we have to strictly process only
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index e6b5c7470ba0..ed7f42f2e740 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -107,6 +107,26 @@ TRACE_EVENT(i915_gem_object_create,
>  	    TP_printk("obj=%p, size=%u", __entry->obj, __entry->size)
>  );
>  
> +TRACE_EVENT(i915_gem_shrink,
> +	    TP_PROTO(struct drm_i915_private *i915, unsigned long target, unsigned flags),
> +	    TP_ARGS(i915, target, flags),
> +
> +	    TP_STRUCT__entry(
> +			     __field(int, dev)
> +			     __field(unsigned long, target)
> +			     __field(unsigned, flags)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->dev = i915->dev->primary->index;
> +			   __entry->target = target;
> +			   __entry->flags = flags;
> +			   ),
> +
> +	    TP_printk("dev=%d, target=%lu, flags=%x",
> +		      __entry->dev, __entry->target, __entry->flags)
> +);
> +
>  TRACE_EVENT(i915_vma_bind,
>  	    TP_PROTO(struct i915_vma *vma, unsigned flags),
>  	    TP_ARGS(vma, flags),
> -- 
> 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

Re: [PATCH 2/5] drm/i915: Add a tracepoint for the shrinker

[Chris Wilson]

On Tue, Oct 06, 2015 at 02:54:25PM +0200, Daniel Vetter wrote:
> On Thu, Oct 01, 2015 at 12:18:26PM +0100, Chris Wilson wrote:
> > Often it is very useful to know why we suddenly purge vast tracts of
> > memory and surprisingly up until now we didn't even have a tracepoint
> > for when we shrink our memory.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 ++
> >  drivers/gpu/drm/i915/i915_trace.h        | 20 ++++++++++++++++++++
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > index b627d07fad29..88f66a2586ec 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > @@ -85,6 +85,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
> >  	}, *phase;
> >  	unsigned long count = 0;
> >  
> > +	trace_i915_gem_shrink(dev_priv, target, flags);
> 
> Shouldn't we also dump how many pages we actually managed to shrink, i.e.
> count (at the end of the functions).

I didn't because I find the double tracepoints annoying, and you already
have the unbinds following.

I guess shrink_begin, shrink_end (to be consistent with wait_begin/_end
or shrink_start/_end to be consistent with slab).
 
> Also we have a slab_start/end tracepoint already, but that one obviously
> doesn't cover the internal calls to i915_gem_shrink. Should imo be
> mentioned in the commit message.

Sure, I don't usually watch slab, so I don't have a marker for the
thousand unbinds as to what caused them.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Add a tracepoint for the shrinker

[Daniel Vetter]

On Tue, Oct 06, 2015 at 02:16:56PM +0100, Chris Wilson wrote:
> On Tue, Oct 06, 2015 at 02:54:25PM +0200, Daniel Vetter wrote:
> > On Thu, Oct 01, 2015 at 12:18:26PM +0100, Chris Wilson wrote:
> > > Often it is very useful to know why we suddenly purge vast tracts of
> > > memory and surprisingly up until now we didn't even have a tracepoint
> > > for when we shrink our memory.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 ++
> > >  drivers/gpu/drm/i915/i915_trace.h        | 20 ++++++++++++++++++++
> > >  2 files changed, 22 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > > index b627d07fad29..88f66a2586ec 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > > @@ -85,6 +85,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
> > >  	}, *phase;
> > >  	unsigned long count = 0;
> > >  
> > > +	trace_i915_gem_shrink(dev_priv, target, flags);
> > 
> > Shouldn't we also dump how many pages we actually managed to shrink, i.e.
> > count (at the end of the functions).
> 
> I didn't because I find the double tracepoints annoying, and you already
> have the unbinds following.
> 
> I guess shrink_begin, shrink_end (to be consistent with wait_begin/_end
> or shrink_start/_end to be consistent with slab).

I meant moving the tracepoint to the end of the function where we both
know how much core mm asked us to shrink and how much we actually managed
to unshrink. But watching the unbind tracepoints is good enough for that
too.

> > Also we have a slab_start/end tracepoint already, but that one obviously
> > doesn't cover the internal calls to i915_gem_shrink. Should imo be
> > mentioned in the commit message.
> 
> Sure, I don't usually watch slab, so I don't have a marker for the
> thousand unbinds as to what caused them.

I've captured the above two items in a note in the commit message and
applied the patch.

Thanks, Daniel
-- 
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

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

[Chris Wilson]

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

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

[Daniel Vetter]

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

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

[Chris Wilson]

On Tue, Oct 06, 2015 at 03:00:49PM +0200, Daniel Vetter wrote:
> 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?

No. The first one is just a convenience (putting it first just means we
may get more inactive objects during an inactive only shrink, through they
will be at the end and so more likely not to be included by the shrinker's
scan-count).

We need a i915_gem_retire_requests() over and above the usual retirement
because execlists is snafu. The second one is to handle a transient
cache of requests which you haven't seen yet, but execlists needs it
anyway in order to unpin itself (since it is not tied into retirement).
 
> 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).

Interesting. That sounds like a clean way to catch reentrancy, something
to think about.

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

Corner-case performance issue on top of memory pressure. It's so old no
one will have noticed a regression, and it's already on a slow path that
unless you were analysing traces you probably wouldn't even notice the
degradation.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Remove dead i915_gem_evict_everything()

[Chris Wilson]

With UMS gone, we no longer use it during suspend. And with the last
user removed from the shrinker, we can remove the dead code.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h       |  1 -
 drivers/gpu/drm/i915/i915_gem_evict.c | 45 -----------------------------------
 2 files changed, 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6c807c584d59..64b8929acdf2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3128,7 +3128,6 @@ int __must_check i915_gem_evict_something(struct drm_device *dev,
 					  unsigned long end,
 					  unsigned flags);
 int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
-int i915_gem_evict_everything(struct drm_device *dev);
 
 /* belongs in i915_gem_gtt.h */
 static inline void i915_gem_chipset_flush(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index d09e35ed9c9a..d71a133ceff5 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -237,48 +237,3 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 
 	return 0;
 }
-
-/**
- * i915_gem_evict_everything - Try to evict all objects
- * @dev: Device to evict objects for
- *
- * This functions tries to evict all gem objects from all address spaces. Used
- * by the shrinker as a last-ditch effort and for suspend, before releasing the
- * backing storage of all unbound objects.
- */
-int
-i915_gem_evict_everything(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_address_space *vm, *v;
-	bool lists_empty = true;
-	int ret;
-
-	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
-		lists_empty = (list_empty(&vm->inactive_list) &&
-			       list_empty(&vm->active_list));
-		if (!lists_empty)
-			lists_empty = false;
-	}
-
-	if (lists_empty)
-		return -ENOSPC;
-
-	trace_i915_gem_evict_everything(dev);
-
-	/* The gpu_idle will flush everything in the write domain to the
-	 * active list. Then we must move everything off the active list
-	 * with retire requests.
-	 */
-	ret = i915_gpu_idle(dev);
-	if (ret)
-		return ret;
-
-	i915_gem_retire_requests(dev);
-
-	/* Having flushed everything, unbind() should never raise an error */
-	list_for_each_entry_safe(vm, v, &dev_priv->vm_list, global_link)
-		WARN_ON(i915_gem_evict_vm(vm, false));
-
-	return 0;
-}
-- 
2.6.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Avoid GPU stalls from kswapd

[Chris Wilson]

Exclude active GPU pages from the purview of the background shrinker
(kswapd), as these cause uncontrollable GPU stalls. Given that the
shrinker is rerun until the freelists are satisfied, we should have
opportunity in subsequent passes to recover the pages once idle. If the
machine does run out of memory entirely, we have the forced idling in the
oom-notifier as a means of releasing all the pages we can before an oom
is prematurely executed.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 1 +
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 64b8929acdf2..a443310a3598 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3159,6 +3159,7 @@ unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
 #define I915_SHRINK_PURGEABLE 0x1
 #define I915_SHRINK_UNBOUND 0x2
 #define I915_SHRINK_BOUND 0x4
+#define I915_SHRINK_ACTIVE 0x8
 unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
 void i915_gem_shrinker_init(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 2058d162aeb9..b2ccb7346899 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -126,6 +126,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			    obj->madv != I915_MADV_DONTNEED)
 				continue;
 
+			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
+				continue;
+
 			drm_gem_object_reference(&obj->base);
 
 			/* For the unbound phase, this should be a no-op! */
@@ -164,7 +167,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv)
 {
 	return i915_gem_shrink(dev_priv, -1UL,
-			       I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
+			       I915_SHRINK_BOUND |
+			       I915_SHRINK_UNBOUND |
+			       I915_SHRINK_ACTIVE);
 }
 
 static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
@@ -217,7 +222,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 			count += obj->base.size >> PAGE_SHIFT;
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		if (obj->pages_pin_count == num_vma_bound(obj))
+		if (!obj->active && obj->pages_pin_count == num_vma_bound(obj))
 			count += obj->base.size >> PAGE_SHIFT;
 	}
 
-- 
2.6.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Avoid GPU stalls from kswapd

[Daniel Vetter]

On Thu, Oct 01, 2015 at 12:18:29PM +0100, Chris Wilson wrote:
> Exclude active GPU pages from the purview of the background shrinker
> (kswapd), as these cause uncontrollable GPU stalls. Given that the
> shrinker is rerun until the freelists are satisfied, we should have
> opportunity in subsequent passes to recover the pages once idle. If the
> machine does run out of memory entirely, we have the forced idling in the
> oom-notifier as a means of releasing all the pages we can before an oom
> is prematurely executed.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

lgtm, but imo we should move the retire_requests from an earlier patch to
this one.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 1 +
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 64b8929acdf2..a443310a3598 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3159,6 +3159,7 @@ unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
>  #define I915_SHRINK_PURGEABLE 0x1
>  #define I915_SHRINK_UNBOUND 0x2
>  #define I915_SHRINK_BOUND 0x4
> +#define I915_SHRINK_ACTIVE 0x8
>  unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
>  void i915_gem_shrinker_init(struct drm_i915_private *dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 2058d162aeb9..b2ccb7346899 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -126,6 +126,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  			    obj->madv != I915_MADV_DONTNEED)
>  				continue;
>  
> +			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
> +				continue;
> +
>  			drm_gem_object_reference(&obj->base);
>  
>  			/* For the unbound phase, this should be a no-op! */
> @@ -164,7 +167,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv)
>  {
>  	return i915_gem_shrink(dev_priv, -1UL,
> -			       I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
> +			       I915_SHRINK_BOUND |
> +			       I915_SHRINK_UNBOUND |
> +			       I915_SHRINK_ACTIVE);
>  }
>  
>  static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
> @@ -217,7 +222,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
>  			count += obj->base.size >> PAGE_SHIFT;
>  
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> -		if (obj->pages_pin_count == num_vma_bound(obj))
> +		if (!obj->active && obj->pages_pin_count == num_vma_bound(obj))
>  			count += obj->base.size >> PAGE_SHIFT;
>  	}
>  
> -- 
> 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

Re: [PATCH 5/5] drm/i915: Avoid GPU stalls from kswapd

[Chris Wilson]

On Tue, Oct 06, 2015 at 03:01:45PM +0200, Daniel Vetter wrote:
> On Thu, Oct 01, 2015 at 12:18:29PM +0100, Chris Wilson wrote:
> > Exclude active GPU pages from the purview of the background shrinker
> > (kswapd), as these cause uncontrollable GPU stalls. Given that the
> > shrinker is rerun until the freelists are satisfied, we should have
> > opportunity in subsequent passes to recover the pages once idle. If the
> > machine does run out of memory entirely, we have the forced idling in the
> > oom-notifier as a means of releasing all the pages we can before an oom
> > is prematurely executed.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> lgtm, but imo we should move the retire_requests from an earlier patch to
> this one.

I am not convinced. The retire_requests are there for their own reasons
(to cover up cracks elsewhere) and not because we need them for retiring
active objects.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Avoid GPU stalls from kswapd

[Daniel Vetter]

On Tue, Oct 06, 2015 at 02:18:34PM +0100, Chris Wilson wrote:
> On Tue, Oct 06, 2015 at 03:01:45PM +0200, Daniel Vetter wrote:
> > On Thu, Oct 01, 2015 at 12:18:29PM +0100, Chris Wilson wrote:
> > > Exclude active GPU pages from the purview of the background shrinker
> > > (kswapd), as these cause uncontrollable GPU stalls. Given that the
> > > shrinker is rerun until the freelists are satisfied, we should have
> > > opportunity in subsequent passes to recover the pages once idle. If the
> > > machine does run out of memory entirely, we have the forced idling in the
> > > oom-notifier as a means of releasing all the pages we can before an oom
> > > is prematurely executed.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > 
> > lgtm, but imo we should move the retire_requests from an earlier patch to
> > this one.
> 
> I am not convinced. The retire_requests are there for their own reasons
> (to cover up cracks elsewhere) and not because we need them for retiring
> active objects.

Ok pulled in all of them, with notes added for the patches where I had
questions.
-Daniel
-- 
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

Re: [PATCH 1/5] drm/i915: shrinker_control->nr_to_scan is now unsigned long

[Daniel Vetter]

On Thu, Oct 01, 2015 at 12:18:25PM +0100, Chris Wilson wrote:
> As the shrinker_control now passes us unsigned long targets, update our
> shrinker functions to match.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 2 +-
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ec731e6db126..6c807c584d59 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3155,7 +3155,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  
>  /* i915_gem_shrinker.c */
>  unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
> -			      long target,
> +			      unsigned long target,
>  			      unsigned flags);
>  #define I915_SHRINK_PURGEABLE 0x1
>  #define I915_SHRINK_UNBOUND 0x2
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index f6ecbda2c604..b627d07fad29 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -73,7 +73,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
>   */
>  unsigned long
>  i915_gem_shrink(struct drm_i915_private *dev_priv,
> -		long target, unsigned flags)
> +		unsigned long target, unsigned flags)
>  {
>  	const struct {
>  		struct list_head *list;
> @@ -159,7 +159,7 @@ 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, LONG_MAX,
> +	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