Commit 29d08b3e authored by Dave Airlie's avatar Dave Airlie

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: default avatarChris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: default avatarDave Airlie <airlied@redhat.com>
parent 130b9851
...@@ -148,7 +148,7 @@ int drm_gem_object_init(struct drm_device *dev, ...@@ -148,7 +148,7 @@ int drm_gem_object_init(struct drm_device *dev,
return -ENOMEM; return -ENOMEM;
kref_init(&obj->refcount); kref_init(&obj->refcount);
kref_init(&obj->handlecount); atomic_set(&obj->handle_count, 0);
obj->size = size; obj->size = size;
atomic_inc(&dev->object_count); atomic_inc(&dev->object_count);
...@@ -496,12 +496,8 @@ static void drm_gem_object_ref_bug(struct kref *list_kref) ...@@ -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 * called before drm_gem_object_free or we'll be touching
* freed memory * freed memory
*/ */
void void drm_gem_object_handle_free(struct drm_gem_object *obj)
drm_gem_object_handle_free(struct kref *kref)
{ {
struct drm_gem_object *obj = container_of(kref,
struct drm_gem_object,
handlecount);
struct drm_device *dev = obj->dev; struct drm_device *dev = obj->dev;
/* Remove any name for this object */ /* Remove any name for this object */
......
...@@ -255,7 +255,7 @@ int drm_gem_one_name_info(int id, void *ptr, void *data) ...@@ -255,7 +255,7 @@ int drm_gem_one_name_info(int id, void *ptr, void *data)
seq_printf(m, "%6d %8zd %7d %8d\n", seq_printf(m, "%6d %8zd %7d %8d\n",
obj->name, obj->size, obj->name, obj->size,
atomic_read(&obj->handlecount.refcount), atomic_read(&obj->handle_count),
atomic_read(&obj->refcount.refcount)); atomic_read(&obj->refcount.refcount));
return 0; return 0;
} }
......
...@@ -136,14 +136,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data, ...@@ -136,14 +136,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
return -ENOMEM; return -ENOMEM;
ret = drm_gem_handle_create(file_priv, obj, &handle); 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) { if (ret) {
drm_gem_object_unreference_unlocked(obj);
return ret; return ret;
} }
/* Sink the floating reference from kref_init(handlecount) */
drm_gem_object_handle_unreference_unlocked(obj);
args->handle = handle; args->handle = handle;
return 0; return 0;
} }
......
...@@ -237,8 +237,10 @@ int intel_fbdev_destroy(struct drm_device *dev, ...@@ -237,8 +237,10 @@ int intel_fbdev_destroy(struct drm_device *dev,
drm_fb_helper_fini(&ifbdev->helper); drm_fb_helper_fini(&ifbdev->helper);
drm_framebuffer_cleanup(&ifb->base); drm_framebuffer_cleanup(&ifb->base);
if (ifb->obj) if (ifb->obj) {
drm_gem_object_handle_unreference(ifb->obj);
drm_gem_object_unreference(ifb->obj); drm_gem_object_unreference(ifb->obj);
}
return 0; return 0;
} }
......
...@@ -352,6 +352,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *nfbdev) ...@@ -352,6 +352,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *nfbdev)
if (nouveau_fb->nvbo) { if (nouveau_fb->nvbo) {
nouveau_bo_unmap(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); drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem);
nouveau_fb->nvbo = NULL; nouveau_fb->nvbo = NULL;
} }
......
...@@ -167,11 +167,9 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data, ...@@ -167,11 +167,9 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data,
goto out; goto out;
ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle); 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: out:
drm_gem_object_handle_unreference_unlocked(nvbo->gem);
if (ret)
drm_gem_object_unreference_unlocked(nvbo->gem);
return ret; return ret;
} }
......
...@@ -79,6 +79,7 @@ nouveau_notifier_takedown_channel(struct nouveau_channel *chan) ...@@ -79,6 +79,7 @@ nouveau_notifier_takedown_channel(struct nouveau_channel *chan)
mutex_lock(&dev->struct_mutex); mutex_lock(&dev->struct_mutex);
nouveau_bo_unpin(chan->notifier_bo); nouveau_bo_unpin(chan->notifier_bo);
mutex_unlock(&dev->struct_mutex); 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_gem_object_unreference_unlocked(chan->notifier_bo->gem);
drm_mm_takedown(&chan->notifier_heap); drm_mm_takedown(&chan->notifier_heap);
} }
......
...@@ -843,8 +843,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb) ...@@ -843,8 +843,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb)
{ {
struct radeon_framebuffer *radeon_fb = to_radeon_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_gem_object_unreference_unlocked(radeon_fb->obj);
}
drm_framebuffer_cleanup(fb); drm_framebuffer_cleanup(fb);
kfree(radeon_fb); kfree(radeon_fb);
} }
......
...@@ -94,8 +94,10 @@ static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj) ...@@ -94,8 +94,10 @@ static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj)
ret = radeon_bo_reserve(rbo, false); ret = radeon_bo_reserve(rbo, false);
if (likely(ret == 0)) { if (likely(ret == 0)) {
radeon_bo_kunmap(rbo); radeon_bo_kunmap(rbo);
radeon_bo_unpin(rbo);
radeon_bo_unreserve(rbo); radeon_bo_unreserve(rbo);
} }
drm_gem_object_handle_unreference(gobj);
drm_gem_object_unreference_unlocked(gobj); drm_gem_object_unreference_unlocked(gobj);
} }
...@@ -325,8 +327,6 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb ...@@ -325,8 +327,6 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
{ {
struct fb_info *info; struct fb_info *info;
struct radeon_framebuffer *rfb = &rfbdev->rfb; struct radeon_framebuffer *rfb = &rfbdev->rfb;
struct radeon_bo *rbo;
int r;
if (rfbdev->helper.fbdev) { if (rfbdev->helper.fbdev) {
info = rfbdev->helper.fbdev; info = rfbdev->helper.fbdev;
...@@ -338,14 +338,8 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb ...@@ -338,14 +338,8 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfb
} }
if (rfb->obj) { if (rfb->obj) {
rbo = rfb->obj->driver_private; radeonfb_destroy_pinned_object(rfb->obj);
r = radeon_bo_reserve(rbo, false); rfb->obj = NULL;
if (likely(r == 0)) {
radeon_bo_kunmap(rbo);
radeon_bo_unpin(rbo);
radeon_bo_unreserve(rbo);
}
drm_gem_object_unreference_unlocked(rfb->obj);
} }
drm_fb_helper_fini(&rfbdev->helper); drm_fb_helper_fini(&rfbdev->helper);
drm_framebuffer_cleanup(&rfb->base); drm_framebuffer_cleanup(&rfb->base);
......
...@@ -201,11 +201,11 @@ int radeon_gem_create_ioctl(struct drm_device *dev, void *data, ...@@ -201,11 +201,11 @@ int radeon_gem_create_ioctl(struct drm_device *dev, void *data,
return r; return r;
} }
r = drm_gem_handle_create(filp, gobj, &handle); r = drm_gem_handle_create(filp, gobj, &handle);
/* drop reference from allocate - handle holds it now */
drm_gem_object_unreference_unlocked(gobj);
if (r) { if (r) {
drm_gem_object_unreference_unlocked(gobj);
return r; return r;
} }
drm_gem_object_handle_unreference_unlocked(gobj);
args->handle = handle; args->handle = handle;
return 0; return 0;
} }
......
...@@ -612,7 +612,7 @@ struct drm_gem_object { ...@@ -612,7 +612,7 @@ struct drm_gem_object {
struct kref refcount; struct kref refcount;
/** Handle count of this object. Each handle also holds a reference */ /** 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 */ /** Related drm device */
struct drm_device *dev; struct drm_device *dev;
...@@ -1461,7 +1461,7 @@ struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev, ...@@ -1461,7 +1461,7 @@ struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev,
size_t size); size_t size);
int drm_gem_object_init(struct drm_device *dev, int drm_gem_object_init(struct drm_device *dev,
struct drm_gem_object *obj, size_t size); 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_open(struct vm_area_struct *vma);
void drm_gem_vm_close(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); int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
...@@ -1496,7 +1496,7 @@ static inline void ...@@ -1496,7 +1496,7 @@ static inline void
drm_gem_object_handle_reference(struct drm_gem_object *obj) drm_gem_object_handle_reference(struct drm_gem_object *obj)
{ {
drm_gem_object_reference(obj); drm_gem_object_reference(obj);
kref_get(&obj->handlecount); atomic_inc(&obj->handle_count);
} }
static inline void static inline void
...@@ -1505,12 +1505,15 @@ drm_gem_object_handle_unreference(struct drm_gem_object *obj) ...@@ -1505,12 +1505,15 @@ drm_gem_object_handle_unreference(struct drm_gem_object *obj)
if (obj == NULL) if (obj == NULL)
return; return;
if (atomic_read(&obj->handle_count) == 0)
return;
/* /*
* Must bump handle count first as this may be the last * Must bump handle count first as this may be the last
* ref, in which case the object would disappear before we * ref, in which case the object would disappear before we
* checked for a name * 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); drm_gem_object_unreference(obj);
} }
...@@ -1520,12 +1523,17 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) ...@@ -1520,12 +1523,17 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
if (obj == NULL) if (obj == NULL)
return; return;
if (atomic_read(&obj->handle_count) == 0)
return;
/* /*
* Must bump handle count first as this may be the last * Must bump handle count first as this may be the last
* ref, in which case the object would disappear before we * ref, in which case the object would disappear before we
* checked for a name * 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); drm_gem_object_unreference_unlocked(obj);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment