[Intel-gfx] [PATCH 4/6] drm/i915: Free batch pool when idle

[Chris Wilson]

On Thu, Mar 19, 2015 at 05:03:56PM +0000, Tvrtko Ursulin wrote:
> 
> On 03/09/2015 09:55 AM, Chris Wilson wrote:
> >At runtime, this helps ensure that the batch pools are kept trim and
> >fast. Then at suspend, this releases memory that we do not need to
> >restore. It also ties into the oom-notifier to ensure that we recover as
> >much kernel memory as possible during OOM.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 9f33005bdfd1..efb5545251c7 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2849,8 +2849,19 @@ i915_gem_idle_work_handler(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> >  		container_of(work, typeof(*dev_priv), mm.idle_work.work);
> >+	struct drm_device *dev = dev_priv->dev;
> >+
> >+	intel_mark_idle(dev);
> >
> >-	intel_mark_idle(dev_priv->dev);
> >+	if (mutex_trylock(&dev->struct_mutex)) {
> >+		struct intel_engine_cs *ring;
> >+		int i;
> >+
> >+		for_each_ring(ring, dev_priv, i)
> >+			i915_gem_batch_pool_fini(&ring->batch_pool);
> 
> This is sooo bad... destructor in the idle handler, what message
> does that send? :D

That I noticed the function could also be used to cleanup the lists at
runtime, and I wasn't a fan of say i915_gem_batch_pool_cleanup().

#define i915_gem_batch_pool_reset i915_gem_batch_pool_fini ?

> >+
> >+		mutex_unlock(&dev->struct_mutex);
> >+	}
> 
> Would it be worth self-re-arming if trylock fails?

Possibly. I wasn't too concerned, holding the struct_mutex generally
involves work being emitted shortly afterwards. And I regarded this as
a performance optimisation rather than preventing leaks.

> I couldn't
> immediately figure out if last retirement can somehow race with a
> struct_mutex holder somewhere.

On suspend, we drop the struct mutex before flushing the idle work. Then
we explicitly free the lists anyway on shutdown.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre