[PATCH] drm/i915: fixup overlay stolen memory leak

[PATCH] drm/i915: fixup overlay stolen memory leak

[Daniel Vetter]

We need to clean up the overlay first, before taking down the
stolen memory allocator.

This regression has been introducec in

commit 8040513870399f1cb032cb8bc805df5042fedcdf
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:29 2012 +0000

    drm/i915: Allocate overlay registers from stolen memory

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ad488f6..532ad39 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1720,9 +1720,9 @@ int i915_driver_unload(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 		i915_gem_cleanup_aliasing_ppgtt(dev);
 		i915_gem_cleanup_stolen(dev);
-		drm_mm_takedown(&dev_priv->mm.stolen);
 
 		intel_cleanup_overlay(dev);
+		drm_mm_takedown(&dev_priv->mm.stolen);
 
 		if (!I915_NEED_GFX_HWS(dev))
 			i915_free_hws(dev);
-- 
1.7.10.4

Re: [PATCH] drm/i915: fixup overlay stolen memory leak

[Chris Wilson]

On Tue, 11 Dec 2012 00:07:45 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We need to clean up the overlay first, before taking down the
> stolen memory allocator.
> 
> This regression has been introducec in
> 
> commit 8040513870399f1cb032cb8bc805df5042fedcdf
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Nov 15 11:32:29 2012 +0000
> 
>     drm/i915: Allocate overlay registers from stolen memory

Time for a philosophical debate; I suggest the bug was latent and that
the cleanup order was wrong originally. Here, we probably want to
cleanup the overlay earlier, before we start tidying up the [pp]gtt etc.
Also you want to move the drm_mm_takedown() into
i915_gem_cleanup_stolen() for symmetry.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: fixup overlay stolen memory leak

[Daniel Vetter]

We need to clean up the overlay first, before taking down the
stolen memory allocator.

This regression has been introducec in

commit 8040513870399f1cb032cb8bc805df5042fedcdf
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:29 2012 +0000

    drm/i915: Allocate overlay registers from stolen memory

Note: This is just a quick hack to shut up a warning in the module
unload code, so that I can check again whether we don't leak any
framebuffers.

v2: Rework the patch a bit as suggested by Chris Wilson:
- move the overlay teardown up, into the modeset cleanup
- move the stolen mm takedown into i915_gem_cleanup_stolen

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c        |    3 ---
 drivers/gpu/drm/i915/i915_gem_stolen.c |    3 +++
 drivers/gpu/drm/i915/intel_display.c   |    2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 14a39e2..27d6af7 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1737,9 +1737,6 @@ int i915_driver_unload(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 		i915_gem_cleanup_aliasing_ppgtt(dev);
 		i915_gem_cleanup_stolen(dev);
-		drm_mm_takedown(&dev_priv->mm.stolen);
-
-		intel_cleanup_overlay(dev);
 
 		if (!I915_NEED_GFX_HWS(dev))
 			i915_free_hws(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index f817b0c..f21ae17 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -173,7 +173,10 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 
 void i915_gem_cleanup_stolen(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	i915_gem_stolen_cleanup_compression(dev);
+	drm_mm_takedown(&dev_priv->mm.stolen);
 }
 
 int i915_gem_init_stolen(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f49fd2a..91c57fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9020,6 +9020,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	flush_scheduled_work();
 
 	drm_mode_config_cleanup(dev);
+
+	intel_cleanup_overlay(dev);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH] drm/i915: fixup overlay stolen memory leak

[Chris Wilson]

On Tue, 18 Dec 2012 15:24:37 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We need to clean up the overlay first, before taking down the
> stolen memory allocator.
> 
> This regression has been introducec in
> 
> commit 8040513870399f1cb032cb8bc805df5042fedcdf
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Nov 15 11:32:29 2012 +0000
> 
>     drm/i915: Allocate overlay registers from stolen memory
> 
> Note: This is just a quick hack to shut up a warning in the module
> unload code, so that I can check again whether we don't leak any
> framebuffers.
> 
> v2: Rework the patch a bit as suggested by Chris Wilson:
> - move the overlay teardown up, into the modeset cleanup
> - move the stolen mm takedown into i915_gem_cleanup_stolen
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

That organisation indeed makes more sense.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: fixup overlay stolen memory leak

[Daniel Vetter]

On Tue, Dec 18, 2012 at 02:48:39PM +0000, Chris Wilson wrote:
> On Tue, 18 Dec 2012 15:24:37 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We need to clean up the overlay first, before taking down the
> > stolen memory allocator.
> > 
> > This regression has been introducec in
> > 
> > commit 8040513870399f1cb032cb8bc805df5042fedcdf
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Thu Nov 15 11:32:29 2012 +0000
> > 
> >     drm/i915: Allocate overlay registers from stolen memory
> > 
> > Note: This is just a quick hack to shut up a warning in the module
> > unload code, so that I can check again whether we don't leak any
> > framebuffers.

Dropped this note, since it's no longer a hack ...

> > v2: Rework the patch a bit as suggested by Chris Wilson:
> > - move the overlay teardown up, into the modeset cleanup
> > - move the stolen mm takedown into i915_gem_cleanup_stolen
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> That organisation indeed makes more sense.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

... and merged it. Thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch