drm/gem: handlecount isn't really a kref so don't make it one.

There were lots of places being inconsistent since handle count
looked like a kref but it really wasn't.

Fix this my just making handle count an atomic on the object,
and have it increase the normal object kref.

Now i915/radeon/nouveau drivers can drop the normal reference on
userspace object creation, and have the handle hold it.

This patch fixes a memory leak or corruption on unload, because
the driver had no way of knowing if a handle had been actually
added for this object, and the fbcon object needed to know this
to clean itself up properly.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Dave Airlie <airlied@redhat.com>

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 6fe2cd2..f7e61be 100644 (file)
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -148,7 +148,7 @@ int drm_gem_object_init(struct drm_device *dev,
                return -ENOMEM;
 
        kref_init(&obj->refcount);
-       kref_init(&obj->handlecount);
+       atomic_set(&obj->handle_count, 0);
        obj->size = size;
 
        atomic_inc(&dev->object_count);
@@ -496,12 +496,8 @@ static void drm_gem_object_ref_bug(struct kref *list_kref)
  * called before drm_gem_object_free or we'll be touching
  * freed memory
  */
-void
-drm_gem_object_handle_free(struct kref *kref)
+void drm_gem_object_handle_free(struct drm_gem_object *obj)
 {
-       struct drm_gem_object *obj = container_of(kref,
-                                                 struct drm_gem_object,
-                                                 handlecount);
        struct drm_device *dev = obj->dev;
 
        /* Remove any name for this object */

diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 2ef2c78..974e970 100644 (file)
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -255,7 +255,7 @@ int drm_gem_one_name_info(int id, void *ptr, void *data)
 
        seq_printf(m, "%6d %8zd %7d %8d\n",
                   obj->name, obj->size,
-                  atomic_read(&obj->handlecount.refcount),
+                  atomic_read(&obj->handle_count),
                   atomic_read(&obj->refcount.refcount));
        return 0;
 }

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cf4ffbe..4cdf742 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -136,14 +136,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
                return -ENOMEM;
 
        ret = drm_gem_handle_create(file_priv, obj, &handle);
+       /* drop reference from allocate - handle holds it now */
+       drm_gem_object_unreference_unlocked(obj);
        if (ret) {
-               drm_gem_object_unreference_unlocked(obj);
                return ret;
        }
 
-       /* Sink the floating reference from kref_init(handlecount) */
-       drm_gem_object_handle_unreference_unlocked(obj);
-
        args->handle = handle;
        return 0;
 }

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 7bdc962..56ad9df 100644 (file)
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -237,8 +237,10 @@ int intel_fbdev_destroy(struct drm_device *dev,
        drm_fb_helper_fini(&ifbdev->helper);
 
        drm_framebuffer_cleanup(&ifb->base);
-       if (ifb->obj)
+       if (ifb->obj) {
+               drm_gem_object_handle_unreference(ifb->obj);
                drm_gem_object_unreference(ifb->obj);
+       }
 
        return 0;
 }

diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index dbd30b2..d204771 100644 (file)
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -352,6 +352,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *nfbdev)
 
        if (nouveau_fb->nvbo) {
                nouveau_bo_unmap(nouveau_fb->nvbo);
+               drm_gem_object_handle_unreference_unlocked(nouveau_fb->nvbo->gem);
                drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem);
                nouveau_fb->nvbo = NULL;
        }

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index ead7b8f..19620a6 100644 (file)
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -167,11 +167,9 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data,
                goto out;
 
        ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle);
+       /* drop reference from allocate - handle holds it now */
+       drm_gem_object_unreference_unlocked(nvbo->gem);
 out:
-       drm_gem_object_handle_unreference_unlocked(nvbo->gem);
-
-       if (ret)
-               drm_gem_object_unreference_unlocked(nvbo->gem);
        return ret;
 }
 

diff --git a/drivers/gpu/drm/nouveau/nouveau_notifier.c b/drivers/gpu/drm/nouveau/nouveau_notifier.c
index 3ec181f..3c9964a 100644 (file)
--- a/drivers/gpu/drm/nouveau/nouveau_notifier.c
+++ b/drivers/gpu/drm/nouveau/nouveau_notifier.c
@@ -79,6 +79,7 @@ nouveau_notifier_takedown_channel(struct nouveau_channel *chan)
        mutex_lock(&dev->struct_mutex);
        nouveau_bo_unpin(chan->notifier_bo);
        mutex_unlock(&dev->struct_mutex);
+       drm_gem_object_handle_unreference_unlocked(chan->notifier_bo->gem);
        drm_gem_object_unreference_unlocked(chan->notifier_bo->gem);
        drm_mm_takedown(&chan->notifier_heap);
 }

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 7422f27..b92d2f2 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -843,8 +843,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
        struct radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb);
 
-       if (radeon_fb->obj)
+       if (radeon_fb->obj) {
                drm_gem_object_unreference_unlocked(radeon_fb->obj);
+       }
        drm_framebuffer_cleanup(fb);
        kfree(radeon_fb);
 }

diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index c74a8b2..9cdf6a3 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -94,8 +94,10 @@ static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj)
        ret = radeon_bo_reserve(rbo, false);
        if (likely(ret == 0)) {
                radeon_bo_kunmap(rbo);
+               radeon_bo_unpin(rbo);
                radeon_bo_unreserve(rbo);
        }
+       drm_gem_object_handle_unreference(gobj);
        drm_gem_object_unreference_unlocked(gobj);
 }
 
@@ -325,8 +327,6 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
 {
        struct fb_info *info;
        struct radeon_framebuffer *rfb = &rfbdev->rfb;
-       struct radeon_bo *rbo;
-       int r;
 
        if (rfbdev->helper.fbdev) {
                info = rfbdev->helper.fbdev;
@@ -338,14 +338,8 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
        }
 
        if (rfb->obj) {
-               rbo = rfb->obj->driver_private;
-               r = radeon_bo_reserve(rbo, false);
-               if (likely(r == 0)) {
-                       radeon_bo_kunmap(rbo);
-                       radeon_bo_unpin(rbo);
-                       radeon_bo_unreserve(rbo);
-               }
-               drm_gem_object_unreference_unlocked(rfb->obj);
+               radeonfb_destroy_pinned_object(rfb->obj);
+               rfb->obj = NULL;
        }
        drm_fb_helper_fini(&rfbdev->helper);
        drm_framebuffer_cleanup(&rfb->base);

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
index c578f26..d1e595d 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -201,11 +201,11 @@ int radeon_gem_create_ioctl(struct drm_device *dev, void *data,
                return r;
        }
        r = drm_gem_handle_create(filp, gobj, &handle);
+       /* drop reference from allocate - handle holds it now */
+       drm_gem_object_unreference_unlocked(gobj);
        if (r) {
-               drm_gem_object_unreference_unlocked(gobj);
                return r;
        }
-       drm_gem_object_handle_unreference_unlocked(gobj);
        args->handle = handle;
        return 0;
 }

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 774e1d4..07e4726 100644 (file)
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -612,7 +612,7 @@ struct drm_gem_object {
        struct kref refcount;
 
        /** Handle count of this object. Each handle also holds a reference */
-       struct kref handlecount;
+       atomic_t handle_count; /* number of handles on this object */
 
        /** Related drm device */
        struct drm_device *dev;
@@ -1461,7 +1461,7 @@ struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev,
                                            size_t size);
 int drm_gem_object_init(struct drm_device *dev,
                        struct drm_gem_object *obj, size_t size);
-void drm_gem_object_handle_free(struct kref *kref);
+void drm_gem_object_handle_free(struct drm_gem_object *obj);
 void drm_gem_vm_open(struct vm_area_struct *vma);
 void drm_gem_vm_close(struct vm_area_struct *vma);
 int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
@@ -1496,7 +1496,7 @@ static inline void
 drm_gem_object_handle_reference(struct drm_gem_object *obj)
 {
        drm_gem_object_reference(obj);
-       kref_get(&obj->handlecount);
+       atomic_inc(&obj->handle_count);
 }
 
 static inline void
@@ -1505,12 +1505,15 @@ drm_gem_object_handle_unreference(struct drm_gem_object *obj)
        if (obj == NULL)
                return;
 
+       if (atomic_read(&obj->handle_count) == 0)
+               return;
        /*
         * Must bump handle count first as this may be the last
         * ref, in which case the object would disappear before we
         * checked for a name
         */
-       kref_put(&obj->handlecount, drm_gem_object_handle_free);
+       if (atomic_dec_and_test(&obj->handle_count))
+               drm_gem_object_handle_free(obj);
        drm_gem_object_unreference(obj);
 }
 
@@ -1520,12 +1523,17 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
        if (obj == NULL)
                return;
 
+       if (atomic_read(&obj->handle_count) == 0)
+               return;
+
        /*
        * Must bump handle count first as this may be the last
        * ref, in which case the object would disappear before we
        * checked for a name
        */
-       kref_put(&obj->handlecount, drm_gem_object_handle_free);
+
+       if (atomic_dec_and_test(&obj->handle_count))
+               drm_gem_object_handle_free(obj);
        drm_gem_object_unreference_unlocked(obj);
 }