[PATCH 1/3] drm/gma500: Rework pin/unpin to prevent memory leak

[Patrik Jakobsson]

We had an unmatching pin/unpin count because psb_intel_pipe_set_base was
pinning the framebuffer before use. This caused psb_gtt_free_range to
leak memory and trigger a warning (see bug reports).

Pinning / Unpinning is now done at dumb buffer alloc / destroy and at
vm fault time if needed by non-dumb non-stolen buffers (no use case yet)

Now whenever we call psb_gtt_free_range() it is assumed that the buffer
is fully unpinned. It is also assumed that a framebuffer used when
setting a pipe base is already pinned.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511
Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113
Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
---
 drivers/gpu/drm/gma500/gem.c               | 44 ++++++++++++++++++++++++++----
 drivers/gpu/drm/gma500/gtt.c               |  5 ----
 drivers/gpu/drm/gma500/psb_intel_display.c | 17 ++++--------
 3 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index eefd6cc..d2d11bd 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -159,9 +159,32 @@ static int psb_gem_create(struct drm_file *file,
 int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			struct drm_mode_create_dumb *args)
 {
+	struct drm_gem_object *obj = NULL;
+	struct gtt_range *gt;
+	int ret;
+
 	args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 64);
 	args->size = args->pitch * args->height;
-	return psb_gem_create(file, dev, args->size, &args->handle);
+	ret = psb_gem_create(file, dev, args->size, &args->handle);
+
+	/* Pin all dumb allocated buffers since they're all supposed to be used
+	   for scanout anyways */
+	if (ret == 0) {
+		obj = drm_gem_object_lookup(dev, file, args->handle);
+		if (obj == NULL)
+			return -ENOENT;
+
+		gt = container_of(obj, struct gtt_range, gem);
+		if (psb_gtt_pin(gt) < 0) {
+			ret = -ENOMEM;
+			goto unref;
+		}
+	}
+
+unref:
+	if (obj)
+		drm_gem_object_unreference(obj);
+	return ret;
 }
 
 /**
@@ -177,6 +200,17 @@ int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 int psb_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
 			uint32_t handle)
 {
+	struct drm_gem_object *obj;
+	struct gtt_range *gt;
+
+	obj = drm_gem_object_lookup(dev, file, handle);
+	if (obj == NULL)
+		return -ENOENT;
+
+	gt = container_of(obj, struct gtt_range, gem);
+	psb_gtt_unpin(gt);
+	drm_gem_object_unreference(obj);
+
 	/* No special work needed, drop the reference and see what falls out */
 	return drm_gem_handle_delete(file, handle);
 }
@@ -218,16 +252,16 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	   something from beneath our feet */
 	mutex_lock(&dev->struct_mutex);
 
-	/* For now the mmap pins the object and it stays pinned. As things
-	   stand that will do us no harm */
-	if (r->mmapping == 0) {
+	/* Pin the object if it's not already in gart. Dumb buffers should
+	   already be pinned at this point */
+	if (r->in_gart == 0) {
 		ret = psb_gtt_pin(r);
 		if (ret < 0) {
 			dev_err(dev->dev, "gma500: pin failed: %d\n", ret);
 			goto fail;
 		}
-		r->mmapping = 1;
 	}
+	r->mmapping = 1;
 
 	/* Page relative to the VMA start - we must calculate this ourselves
 	   because vmf->pgoff is the fake GEM offset */
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 1f82183..0d62cd7 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -378,11 +378,6 @@ struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
  */
 void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
 {
-	/* Undo the mmap pin if we are destroying the object */
-	if (gt->mmapping) {
-		psb_gtt_unpin(gt);
-		gt->mmapping = 0;
-	}
 	WARN_ON(gt->in_gart && !gt->stolen);
 	release_resource(&gt->resource);
 	kfree(gt);
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index 6e8f42b..a768c98 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -237,6 +237,7 @@ void psb_intel_wait_for_vblank(struct drm_device *dev)
 	mdelay(20);
 }
 
+/* Framebuffer must be pinned before setting it as pipe base */
 static int psb_intel_pipe_set_base(struct drm_crtc *crtc,
 			    int x, int y, struct drm_framebuffer *old_fb)
 {
@@ -256,14 +257,14 @@ static int psb_intel_pipe_set_base(struct drm_crtc *crtc,
 	/* no fb bound */
 	if (!crtc->fb) {
 		dev_dbg(dev->dev, "No FB bound\n");
-		goto psb_intel_pipe_cleaner;
+		goto psb_intel_pipe_set_base_exit;
 	}
 
-	/* We are displaying this buffer, make sure it is actually loaded
-	   into the GTT */
-	ret = psb_gtt_pin(psbfb->gtt);
-	if (ret < 0)
+	if (psbfb->gtt->in_gart < 1) {
+		WARN_ON(1);
 		goto psb_intel_pipe_set_base_exit;
+	}
+
 	start = psbfb->gtt->offset;
 
 	offset = y * crtc->fb->pitches[0] + x * (crtc->fb->bits_per_pixel / 8);
@@ -290,7 +291,6 @@ static int psb_intel_pipe_set_base(struct drm_crtc *crtc,
 	default:
 		dev_err(dev->dev, "Unknown color depth\n");
 		ret = -EINVAL;
-		psb_gtt_unpin(psbfb->gtt);
 		goto psb_intel_pipe_set_base_exit;
 	}
 	REG_WRITE(map->cntr, dspcntr);
@@ -298,11 +298,6 @@ static int psb_intel_pipe_set_base(struct drm_crtc *crtc,
 	REG_WRITE(map->base, start + offset);
 	REG_READ(map->base);
 
-psb_intel_pipe_cleaner:
-	/* If there was a previous display we can now unpin it */
-	if (old_fb)
-		psb_gtt_unpin(to_psb_fb(old_fb)->gtt);
-
 psb_intel_pipe_set_base_exit:
 	gma_power_end(dev);
 	return ret;
-- 
1.8.1.2