* [PATCH] drm/i915: During shrink_all we only need to idle the GPU
@ 2015-06-28 12:06 Chris Wilson
2015-06-29 7:13 ` Daniel Vetter
2015-06-29 15:49 ` shuang.he
0 siblings, 2 replies; 4+ messages in thread
From: Chris Wilson @ 2015-06-28 12:06 UTC (permalink / raw)
To: intel-gfx
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.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem_shrinker.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index c41ddf92e404..2d8c79b8c378 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -158,9 +158,16 @@ 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,
- I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
+ unsigned long count;
+
+ count = i915_gem_shrink(dev_priv, LONG_MAX,
+ I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
+
+ /* Use a double call to retire to flush any staged frees */
+ i915_gem_retire_requests(dev_priv->dev);
+ i915_gem_retire_requests(dev_priv->dev);
+
+ return count;
}
static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: During shrink_all we only need to idle the GPU
2015-06-28 12:06 [PATCH] drm/i915: During shrink_all we only need to idle the GPU Chris Wilson
@ 2015-06-29 7:13 ` Daniel Vetter
2015-06-29 7:30 ` Chris Wilson
2015-06-29 15:49 ` shuang.he
1 sibling, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2015-06-29 7:13 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sun, Jun 28, 2015 at 01:06:39PM +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.
Has this already blown up in some bugzilla somewhere? Should be a fairly
recent regression with the olr removal.
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index c41ddf92e404..2d8c79b8c378 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -158,9 +158,16 @@ 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,
> - I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
> + unsigned long count;
> +
> + count = i915_gem_shrink(dev_priv, LONG_MAX,
> + I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
> +
> + /* Use a double call to retire to flush any staged frees */
> + i915_gem_retire_requests(dev_priv->dev);
> + i915_gem_retire_requests(dev_priv->dev);
I'm lost - where's that staged free?
-Daniel
> +
> + return count;
> }
>
> static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
> --
> 2.1.4
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: During shrink_all we only need to idle the GPU
2015-06-29 7:13 ` Daniel Vetter
@ 2015-06-29 7:30 ` Chris Wilson
0 siblings, 0 replies; 4+ messages in thread
From: Chris Wilson @ 2015-06-29 7:30 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, Jun 29, 2015 at 09:13:31AM +0200, Daniel Vetter wrote:
> On Sun, Jun 28, 2015 at 01:06:39PM +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.
>
> Has this already blown up in some bugzilla somewhere? Should be a fairly
> recent regression with the olr removal.
It's damn tricky to catch in an igt, since it requires an oom pass
inside the submit callback. Running out of memory is easy, running out
of memory with outstanding rendering is easy, triggering the recursion
is hard. (Which of course meant it happened almost immediately when
running benchmarks on resource limited machines.) Fault injection is the
only tractible solution. (And don't think what happens if we cancel a
request after doing a switch_context, stale requests galore.)
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_gem_shrinker.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > index c41ddf92e404..2d8c79b8c378 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > @@ -158,9 +158,16 @@ 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,
> > - I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
> > + unsigned long count;
> > +
> > + count = i915_gem_shrink(dev_priv, LONG_MAX,
> > + I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
> > +
> > + /* Use a double call to retire to flush any staged frees */
> > + i915_gem_retire_requests(dev_priv->dev);
> > + i915_gem_retire_requests(dev_priv->dev);
>
> I'm lost - where's that staged free?
Execlists has one, and I'm adding another.
-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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: During shrink_all we only need to idle the GPU
2015-06-28 12:06 [PATCH] drm/i915: During shrink_all we only need to idle the GPU Chris Wilson
2015-06-29 7:13 ` Daniel Vetter
@ 2015-06-29 15:49 ` shuang.he
1 sibling, 0 replies; 4+ messages in thread
From: shuang.he @ 2015-06-29 15:49 UTC (permalink / raw)
To: shuang.he, lei.a.liu, intel-gfx, chris
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6655
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -2 287/287 285/287
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads PASS(1) FAIL(1)
*BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-29 15:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-28 12:06 [PATCH] drm/i915: During shrink_all we only need to idle the GPU Chris Wilson
2015-06-29 7:13 ` Daniel Vetter
2015-06-29 7:30 ` Chris Wilson
2015-06-29 15:49 ` shuang.he
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.