* Gem GTT mmaps.. @ 2009-02-04 22:32 Thomas Hellström 2009-02-04 23:02 ` Jesse Barnes 0 siblings, 1 reply; 14+ messages in thread From: Thomas Hellström @ 2009-02-04 22:32 UTC (permalink / raw) To: Jesse Barnes; +Cc: DRI, Linux Kernel Jesse, I have some concerns about the GEM GTT mmap functionality. First, a gem object pointer is copied to map->offset and then to the vma->vm_private_data without proper reference counting. This pointer is used in i915_gem_fault() to access the gem object. However if the gem object is destroyed and a process then tries to access data in a vma mapping the (now destroyed) object, it would dereference a stale pointer into kernel space? Shouldn't those pointers be reference counted, and to account for fork(), a vm open and close would be needed to reference count corresponding pointers of newly created and destroyed vmas? Second, the i915_gem_fault method returns VM_FAULT_SIGBUS if vm_insert_pfn() fails with an -EBUSY. I think that's an error, since that would indicate that the pte was already populated by a racing thread. /Thomas ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-04 22:32 Gem GTT mmaps Thomas Hellström @ 2009-02-04 23:02 ` Jesse Barnes 2009-02-04 23:42 ` Eric Anholt 0 siblings, 1 reply; 14+ messages in thread From: Jesse Barnes @ 2009-02-04 23:02 UTC (permalink / raw) To: Thomas Hellström; +Cc: DRI, Linux Kernel On Wednesday, February 4, 2009 2:32 pm Thomas Hellström wrote: > Jesse, > > I have some concerns about the GEM GTT mmap functionality. Thanks for looking it over again; you would know since some of this code came from you in the first place. :) > First, a gem object pointer is copied to map->offset and then to the > vma->vm_private_data without proper reference counting. This pointer is > used in i915_gem_fault() to access the gem object. However if the gem > object is destroyed and a process then tries to access data in a vma > mapping the (now destroyed) object, it would dereference a stale pointer > into kernel space? Shouldn't those pointers be reference counted, and to > account for fork(), a vm open and close would be needed to reference > count corresponding pointers of newly created and destroyed vmas? Yeah looks like we don't protect against vm_private_data pointing at a freed or other object. But rather than refcounting the pointers I wonder if we could make the private data use the GEM object name instead, then do the lookup in the fault handler? > Second, the i915_gem_fault method returns VM_FAULT_SIGBUS if > vm_insert_pfn() fails with an -EBUSY. I think that's an error, since > that would indicate that the pte was already populated by a racing thread. Ah ok that's easy enough to fix up; I didn't see that EBUSY meant "pte already valid". -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-04 23:02 ` Jesse Barnes @ 2009-02-04 23:42 ` Eric Anholt 2009-02-05 18:37 ` Jesse Barnes 0 siblings, 1 reply; 14+ messages in thread From: Eric Anholt @ 2009-02-04 23:42 UTC (permalink / raw) To: Jesse Barnes; +Cc: Thomas Hellström, DRI, Linux Kernel [-- Attachment #1: Type: text/plain, Size: 1383 bytes --] On Wed, 2009-02-04 at 15:02 -0800, Jesse Barnes wrote: > On Wednesday, February 4, 2009 2:32 pm Thomas Hellström wrote: > > Jesse, > > > > I have some concerns about the GEM GTT mmap functionality. > > Thanks for looking it over again; you would know since some of this code came > from you in the first place. :) > > > First, a gem object pointer is copied to map->offset and then to the > > vma->vm_private_data without proper reference counting. This pointer is > > used in i915_gem_fault() to access the gem object. However if the gem > > object is destroyed and a process then tries to access data in a vma > > mapping the (now destroyed) object, it would dereference a stale pointer > > into kernel space? Shouldn't those pointers be reference counted, and to > > account for fork(), a vm open and close would be needed to reference > > count corresponding pointers of newly created and destroyed vmas? > > Yeah looks like we don't protect against vm_private_data pointing at a freed > or other object. But rather than refcounting the pointers I wonder if we > could make the private data use the GEM object name instead, then do the > lookup in the fault handler? The object doesn't necessarily have a public name. You do need to refcount the objects. -- Eric Anholt eric@anholt.net eric.anholt@intel.com [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-04 23:42 ` Eric Anholt @ 2009-02-05 18:37 ` Jesse Barnes 2009-02-06 17:14 ` Jesse Barnes 0 siblings, 1 reply; 14+ messages in thread From: Jesse Barnes @ 2009-02-05 18:37 UTC (permalink / raw) To: Eric Anholt; +Cc: Thomas Hellström, DRI, Linux Kernel On Wednesday, February 4, 2009 3:42 pm Eric Anholt wrote: > On Wed, 2009-02-04 at 15:02 -0800, Jesse Barnes wrote: > > On Wednesday, February 4, 2009 2:32 pm Thomas Hellström wrote: > > > Jesse, > > > > > > I have some concerns about the GEM GTT mmap functionality. > > > > Thanks for looking it over again; you would know since some of this code > > came from you in the first place. :) > > > > > First, a gem object pointer is copied to map->offset and then to the > > > vma->vm_private_data without proper reference counting. This pointer is > > > used in i915_gem_fault() to access the gem object. However if the gem > > > object is destroyed and a process then tries to access data in a vma > > > mapping the (now destroyed) object, it would dereference a stale > > > pointer into kernel space? Shouldn't those pointers be reference > > > counted, and to account for fork(), a vm open and close would be needed > > > to reference count corresponding pointers of newly created and > > > destroyed vmas? > > > > Yeah looks like we don't protect against vm_private_data pointing at a > > freed or other object. But rather than refcounting the pointers I wonder > > if we could make the private data use the GEM object name instead, then > > do the lookup in the fault handler? > > The object doesn't necessarily have a public name. You do need to > refcount the objects. So if we leave the lookup reference around from the GTT mapping ioctl, that would take care of new mappings. And if we added/removed references at VM open/close time, we should be covered for fork. But is it ok to add a new unref in the finish ioctl for GTT mapped objects? I don't think so, because we don't know for sure if the caller was the one that created the new fake offset (which would be one way of detecting whether it was GTT mapped). Seems like we need a new unmap ioctl? Or we could put the mapping ref/unref in libdrm, where it would be tracked on a per-process basis... -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-05 18:37 ` Jesse Barnes @ 2009-02-06 17:14 ` Jesse Barnes 2009-02-06 21:35 ` Thomas Hellström 0 siblings, 1 reply; 14+ messages in thread From: Jesse Barnes @ 2009-02-06 17:14 UTC (permalink / raw) To: Eric Anholt; +Cc: Thomas Hellström, DRI, Linux Kernel On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: > So if we leave the lookup reference around from the GTT mapping ioctl, that > would take care of new mappings. And if we added/removed references at VM > open/close time, we should be covered for fork. But is it ok to add a new > unref in the finish ioctl for GTT mapped objects? I don't think so, > because we don't know for sure if the caller was the one that created the > new fake offset (which would be one way of detecting whether it was GTT > mapped). Seems like we need a new unmap ioctl? Or we could put the mapping > ref/unref in libdrm, where it would be tracked on a per-process basis... Ah but maybe we should just tear down the fake offset at unmap time; then we'd be able to use it as an existence test for the mapping and get the refcounting right. The last thing I thought of was whether we'd be ok in a map_gtt -> crash case. I *think* the vm_close code will deal with that, if we do a deref there? Thanks, -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 17:14 ` Jesse Barnes @ 2009-02-06 21:35 ` Thomas Hellström 2009-02-06 22:24 ` Jesse Barnes 0 siblings, 1 reply; 14+ messages in thread From: Thomas Hellström @ 2009-02-06 21:35 UTC (permalink / raw) To: Jesse Barnes; +Cc: Eric Anholt, DRI, Linux Kernel Jesse Barnes wrote: > On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: > >> So if we leave the lookup reference around from the GTT mapping ioctl, that >> would take care of new mappings. And if we added/removed references at VM >> open/close time, we should be covered for fork. But is it ok to add a new >> unref in the finish ioctl for GTT mapped objects? I don't think so, >> because we don't know for sure if the caller was the one that created the >> new fake offset (which would be one way of detecting whether it was GTT >> mapped). Seems like we need a new unmap ioctl? Or we could put the mapping >> ref/unref in libdrm, where it would be tracked on a per-process basis... >> > > Ah but maybe we should just tear down the fake offset at unmap time; then we'd > be able to use it as an existence test for the mapping and get the > refcounting right. The last thing I thought of was whether we'd be ok in a > map_gtt -> crash case. I *think* the vm_close code will deal with that, if > we do a deref there? > Yes, an mmap() is always paired with a vm_close(), and the vm_close() also happens in a crash situation. /Thomas > Thanks, > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 21:35 ` Thomas Hellström @ 2009-02-06 22:24 ` Jesse Barnes 2009-02-06 22:39 ` Thomas Hellström ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Jesse Barnes @ 2009-02-06 22:24 UTC (permalink / raw) To: Thomas Hellström; +Cc: Eric Anholt, DRI, Linux Kernel On Friday, February 6, 2009 1:35 pm Thomas Hellström wrote: > Jesse Barnes wrote: > > On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: > >> So if we leave the lookup reference around from the GTT mapping ioctl, > >> that would take care of new mappings. And if we added/removed > >> references at VM open/close time, we should be covered for fork. But is > >> it ok to add a new unref in the finish ioctl for GTT mapped objects? I > >> don't think so, because we don't know for sure if the caller was the one > >> that created the new fake offset (which would be one way of detecting > >> whether it was GTT mapped). Seems like we need a new unmap ioctl? Or we > >> could put the mapping ref/unref in libdrm, where it would be tracked on > >> a per-process basis... > > > > Ah but maybe we should just tear down the fake offset at unmap time; then > > we'd be able to use it as an existence test for the mapping and get the > > refcounting right. The last thing I thought of was whether we'd be ok in > > a map_gtt -> crash case. I *think* the vm_close code will deal with > > that, if we do a deref there? > > Yes, an mmap() is always paired with a vm_close(), and the vm_close() > also happens in a crash situation. This one should cover the cases you found. - ref at map time will keep the object around so fault shouldn't fail - additional threads will take their refs in vm_open/close - unmap will unref and remove mmap_offset allowing object to be freed -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6915fb8..2752114 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -460,6 +460,23 @@ drm_gem_object_handle_free(struct kref *kref) } EXPORT_SYMBOL(drm_gem_object_handle_free); +void drm_gem_vm_open(struct vm_area_struct *vma) +{ + struct drm_gem_object *obj = vma->vm_private_data; + + drm_gem_object_reference(obj); +} +EXPORT_SYMBOL(drm_gem_vm_open); + +void drm_gem_vm_close(struct vm_area_struct *vma) +{ + struct drm_gem_object *obj = vma->vm_private_data; + + drm_gem_object_unreference(obj); +} +EXPORT_SYMBOL(drm_gem_vm_close); + + /** * drm_gem_mmap - memory map routine for GEM objects * @filp: DRM file pointer diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index aac12ee..a31cbdb 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -94,6 +94,8 @@ static int i915_resume(struct drm_device *dev) static struct vm_operations_struct i915_gem_vm_ops = { .fault = i915_gem_fault, + .open = drm_gem_vm_open, + .close = drm_gem_vm_close, }; static struct drm_driver driver = { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1441831..a476de0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -52,6 +52,7 @@ static void i915_gem_object_free_page_list(struct drm_gem_object *obj); static int i915_gem_object_wait_rendering(struct drm_gem_object *obj); static int i915_gem_object_bind_to_gtt(struct drm_gem_object *obj, unsigned alignment); +static void i915_gem_free_mmap_offset(struct drm_gem_object *obj); static int i915_gem_object_get_fence_reg(struct drm_gem_object *obj, bool write); static void i915_gem_clear_fence_reg(struct drm_gem_object *obj); static int i915_gem_evict_something(struct drm_device *dev); @@ -497,6 +498,13 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, i915_gem_object_flush_cpu_write_domain(obj); drm_gem_object_unreference(obj); + + /* If it's been GTT mapped, unref it again... */ + if (obj_priv->mmap_offset) { + i915_gem_free_mmap_offset(obj); + drm_gem_object_unreference(obj); + } + mutex_unlock(&dev->struct_mutex); return ret; } @@ -607,8 +615,6 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) case -EAGAIN: return VM_FAULT_OOM; case -EFAULT: - case -EBUSY: - DRM_ERROR("can't insert pfn?? fault or busy...\n"); return VM_FAULT_SIGBUS; default: return VM_FAULT_NOPAGE; @@ -684,6 +690,32 @@ out_free_list: return ret; } +static void +i915_gem_free_mmap_offset(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + struct drm_i915_gem_object *obj_priv = obj->driver_private; + struct drm_gem_mm *mm = dev->mm_private; + struct drm_map_list *list; + struct drm_map *map; + + list = &obj->map_list; + drm_ht_remove_item(&mm->offset_hash, &list->hash); + + if (list->file_offset_node) { + drm_mm_put_block(list->file_offset_node); + list->file_offset_node = NULL; + } + + map = list->map; + if (map) { + drm_free(map, sizeof(*map), DRM_MEM_DRIVER); + list->map = NULL; + } + + obj_priv->mmap_offset = 0; +} + /** * i915_gem_get_gtt_alignment - return required GTT alignment for an object * @obj: object to check @@ -788,7 +820,7 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data, list_add(&obj_priv->list, &dev_priv->mm.inactive_list); } - drm_gem_object_unreference(obj); + /* Leave lookup reference around until unmap time */ mutex_unlock(&dev->struct_mutex); return 0; @@ -2885,9 +2917,6 @@ int i915_gem_init_object(struct drm_gem_object *obj) void i915_gem_free_object(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; - struct drm_gem_mm *mm = dev->mm_private; - struct drm_map_list *list; - struct drm_map *map; struct drm_i915_gem_object *obj_priv = obj->driver_private; while (obj_priv->pin_count > 0) @@ -2898,20 +2927,6 @@ void i915_gem_free_object(struct drm_gem_object *obj) i915_gem_object_unbind(obj); - list = &obj->map_list; - drm_ht_remove_item(&mm->offset_hash, &list->hash); - - if (list->file_offset_node) { - drm_mm_put_block(list->file_offset_node); - list->file_offset_node = NULL; - } - - map = list->map; - if (map) { - drm_free(map, sizeof(*map), DRM_MEM_DRIVER); - list->map = NULL; - } - drm_free(obj_priv->page_cpu_valid, 1, DRM_MEM_DRIVER); drm_free(obj->driver_private, 1, DRM_MEM_DRIVER); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8190b9b..e5f4ae9 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1321,6 +1321,8 @@ void drm_gem_object_free(struct kref *kref); struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev, size_t size); void drm_gem_object_handle_free(struct kref *kref); +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); static inline void ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 22:24 ` Jesse Barnes @ 2009-02-06 22:39 ` Thomas Hellström 2009-02-06 23:22 ` Jesse Barnes 2009-02-07 0:52 ` Chris Wilson ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Thomas Hellström @ 2009-02-06 22:39 UTC (permalink / raw) To: Jesse Barnes; +Cc: Eric Anholt, DRI, Linux Kernel Jesse Barnes wrote: > On Friday, February 6, 2009 1:35 pm Thomas Hellström wrote: > >> Jesse Barnes wrote: >> >>> On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: >>> >>>> So if we leave the lookup reference around from the GTT mapping ioctl, >>>> that would take care of new mappings. And if we added/removed >>>> references at VM open/close time, we should be covered for fork. But is >>>> it ok to add a new unref in the finish ioctl for GTT mapped objects? I >>>> don't think so, because we don't know for sure if the caller was the one >>>> that created the new fake offset (which would be one way of detecting >>>> whether it was GTT mapped). Seems like we need a new unmap ioctl? Or we >>>> could put the mapping ref/unref in libdrm, where it would be tracked on >>>> a per-process basis... >>>> >>> Ah but maybe we should just tear down the fake offset at unmap time; then >>> we'd be able to use it as an existence test for the mapping and get the >>> refcounting right. The last thing I thought of was whether we'd be ok in >>> a map_gtt -> crash case. I *think* the vm_close code will deal with >>> that, if we do a deref there? >>> >> Yes, an mmap() is always paired with a vm_close(), and the vm_close() >> also happens in a crash situation. >> > > This one should cover the cases you found. > - ref at map time will keep the object around so fault shouldn't fail > - additional threads will take their refs in vm_open/close > - unmap will unref and remove mmap_offset allowing object to be freed > > Jesse, Yes, it looks OK to me. A short question, though, when is i915_gem_sw_finish_ioctl called? Is it possible that the client may still think its mmap offset is valid? Thomas ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 22:39 ` Thomas Hellström @ 2009-02-06 23:22 ` Jesse Barnes 0 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2009-02-06 23:22 UTC (permalink / raw) To: Thomas Hellström; +Cc: Eric Anholt, DRI, Linux Kernel On Friday, February 6, 2009 2:39 pm Thomas Hellström wrote: > Jesse Barnes wrote: > > On Friday, February 6, 2009 1:35 pm Thomas Hellström wrote: > >> Jesse Barnes wrote: > >>> On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: > >>>> So if we leave the lookup reference around from the GTT mapping ioctl, > >>>> that would take care of new mappings. And if we added/removed > >>>> references at VM open/close time, we should be covered for fork. But > >>>> is it ok to add a new unref in the finish ioctl for GTT mapped > >>>> objects? I don't think so, because we don't know for sure if the > >>>> caller was the one that created the new fake offset (which would be > >>>> one way of detecting whether it was GTT mapped). Seems like we need a > >>>> new unmap ioctl? Or we could put the mapping ref/unref in libdrm, > >>>> where it would be tracked on a per-process basis... > >>> > >>> Ah but maybe we should just tear down the fake offset at unmap time; > >>> then we'd be able to use it as an existence test for the mapping and > >>> get the refcounting right. The last thing I thought of was whether > >>> we'd be ok in a map_gtt -> crash case. I *think* the vm_close code > >>> will deal with that, if we do a deref there? > >> > >> Yes, an mmap() is always paired with a vm_close(), and the vm_close() > >> also happens in a crash situation. > > > > This one should cover the cases you found. > > - ref at map time will keep the object around so fault shouldn't fail > > - additional threads will take their refs in vm_open/close > > - unmap will unref and remove mmap_offset allowing object to be freed > > Jesse, > > Yes, it looks OK to me. > A short question, though, when is i915_gem_sw_finish_ioctl called? Is it > possible that the client may still think its mmap offset is valid? It's called at drm_bo_unmap time, but only if the swrast bit is set, so we'd need to do that in libdrm as well. So if the program is written properly it shouldn't think the mapping is valid (we should probably set bo->virtual to 0 in unmap as well, to catch those errors). -- Jesse Barnes, Intel Open Source Technology Center diff --git a/libdrm/intel/intel_bufmgr_gem.c b/libdrm/intel/intel_bufmgr_gem.c index f578a67..1ea4761 100644 --- a/libdrm/intel/intel_bufmgr_gem.c +++ b/libdrm/intel/intel_bufmgr_gem.c @@ -670,6 +670,7 @@ drm_intel_gem_bo_map_gtt(drm_intel_bo *bo) } } + bo_gem->swrast = 1; bo->virtual = bo_gem->virtual; DBG("bo_map: %d (%s) -> %p\n", bo_gem->gem_handle, bo_gem->name, @@ -716,6 +717,8 @@ drm_intel_gem_bo_unmap(drm_intel_bo *bo) } while (ret == -1 && errno == EINTR); bo_gem->swrast = 0; } + bo_gem->virtual = NULL; + bo->virtual = NULL; pthread_mutex_unlock(&bufmgr_gem->lock); return 0; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 22:24 ` Jesse Barnes 2009-02-06 22:39 ` Thomas Hellström @ 2009-02-07 0:52 ` Chris Wilson 2009-02-11 22:01 ` Jesse Barnes 2009-02-07 8:06 ` Xavier Bestel 2009-02-10 22:00 ` Eric Anholt 3 siblings, 1 reply; 14+ messages in thread From: Chris Wilson @ 2009-02-07 0:52 UTC (permalink / raw) To: Jesse Barnes; +Cc: Thomas Hellström, DRI, Linux Kernel I tried it, it's not too happy. My only concern is that this now relies on userspace to correctly call SW_FINISH (and not unmap after mmapping the GTT_MMAP) or otherwise the object is leaked? Patch comments inline. On Fri, 2009-02-06 at 14:24 -0800, Jesse Barnes wrote: > This one should cover the cases you found. > - ref at map time will keep the object around so fault shouldn't fail > - additional threads will take their refs in vm_open/close > - unmap will unref and remove mmap_offset allowing object to be freed > > -- > Jesse Barnes, Intel Open Source Technology Center > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6915fb8..2752114 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -460,6 +460,23 @@ drm_gem_object_handle_free(struct kref *kref) > } > EXPORT_SYMBOL(drm_gem_object_handle_free); > > +void drm_gem_vm_open(struct vm_area_struct *vma) > +{ > + struct drm_gem_object *obj = vma->vm_private_data; > + > + drm_gem_object_reference(obj); > +} > +EXPORT_SYMBOL(drm_gem_vm_open); > + > +void drm_gem_vm_close(struct vm_area_struct *vma) > +{ > + struct drm_gem_object *obj = vma->vm_private_data; > + > + drm_gem_object_unreference(obj); Need to take the dev->struct_mutex for this potential free. > +} > +EXPORT_SYMBOL(drm_gem_vm_close); > + > + > /** > * drm_gem_mmap - memory map routine for GEM objects > * @filp: DRM file pointer > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index aac12ee..a31cbdb 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -94,6 +94,8 @@ static int i915_resume(struct drm_device *dev) > > static struct vm_operations_struct i915_gem_vm_ops = { > .fault = i915_gem_fault, > + .open = drm_gem_vm_open, > + .close = drm_gem_vm_close, > }; > > static struct drm_driver driver = { > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 1441831..a476de0 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -52,6 +52,7 @@ static void i915_gem_object_free_page_list(struct drm_gem_object *obj); > static int i915_gem_object_wait_rendering(struct drm_gem_object *obj); > static int i915_gem_object_bind_to_gtt(struct drm_gem_object *obj, > unsigned alignment); > +static void i915_gem_free_mmap_offset(struct drm_gem_object *obj); > static int i915_gem_object_get_fence_reg(struct drm_gem_object *obj, bool write); > static void i915_gem_clear_fence_reg(struct drm_gem_object *obj); > static int i915_gem_evict_something(struct drm_device *dev); > @@ -497,6 +498,13 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > i915_gem_object_flush_cpu_write_domain(obj); > > drm_gem_object_unreference(obj); > + > + /* If it's been GTT mapped, unref it again... */ > + if (obj_priv->mmap_offset) { This test occurs after the potential free above. > + i915_gem_free_mmap_offset(obj); > + drm_gem_object_unreference(obj); > + } > + > mutex_unlock(&dev->struct_mutex); > return ret; > } > @@ -607,8 +615,6 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > case -EAGAIN: > return VM_FAULT_OOM; > case -EFAULT: > - case -EBUSY: > - DRM_ERROR("can't insert pfn?? fault or busy...\n"); > return VM_FAULT_SIGBUS; > default: > return VM_FAULT_NOPAGE; > @@ -684,6 +690,32 @@ out_free_list: > return ret; > } > > +static void > +i915_gem_free_mmap_offset(struct drm_gem_object *obj) > +{ > + struct drm_device *dev = obj->dev; > + struct drm_i915_gem_object *obj_priv = obj->driver_private; > + struct drm_gem_mm *mm = dev->mm_private; > + struct drm_map_list *list; > + struct drm_map *map; > + > + list = &obj->map_list; > + drm_ht_remove_item(&mm->offset_hash, &list->hash); > + > + if (list->file_offset_node) { > + drm_mm_put_block(list->file_offset_node); > + list->file_offset_node = NULL; > + } > + > + map = list->map; > + if (map) { > + drm_free(map, sizeof(*map), DRM_MEM_DRIVER); > + list->map = NULL; > + } > + > + obj_priv->mmap_offset = 0; > +} > + > /** > * i915_gem_get_gtt_alignment - return required GTT alignment for an object > * @obj: object to check > @@ -788,7 +820,7 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data, > list_add(&obj_priv->list, &dev_priv->mm.inactive_list); > } > > - drm_gem_object_unreference(obj); > + /* Leave lookup reference around until unmap time */ > mutex_unlock(&dev->struct_mutex); > > return 0; > @@ -2885,9 +2917,6 @@ int i915_gem_init_object(struct drm_gem_object *obj) > void i915_gem_free_object(struct drm_gem_object *obj) > { > struct drm_device *dev = obj->dev; > - struct drm_gem_mm *mm = dev->mm_private; > - struct drm_map_list *list; > - struct drm_map *map; > struct drm_i915_gem_object *obj_priv = obj->driver_private; > > while (obj_priv->pin_count > 0) > @@ -2898,20 +2927,6 @@ void i915_gem_free_object(struct drm_gem_object *obj) > > i915_gem_object_unbind(obj); > > - list = &obj->map_list; > - drm_ht_remove_item(&mm->offset_hash, &list->hash); > - > - if (list->file_offset_node) { > - drm_mm_put_block(list->file_offset_node); > - list->file_offset_node = NULL; > - } > - > - map = list->map; > - if (map) { > - drm_free(map, sizeof(*map), DRM_MEM_DRIVER); > - list->map = NULL; > - } > - Add a BUG_ON(obj_priv->mmap_offset) for fun and double-faults! > drm_free(obj_priv->page_cpu_valid, 1, DRM_MEM_DRIVER); > drm_free(obj->driver_private, 1, DRM_MEM_DRIVER); > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 8190b9b..e5f4ae9 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1321,6 +1321,8 @@ void drm_gem_object_free(struct kref *kref); > struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev, > size_t size); > void drm_gem_object_handle_free(struct kref *kref); > +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); -ickle ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-07 0:52 ` Chris Wilson @ 2009-02-11 22:01 ` Jesse Barnes 0 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2009-02-11 22:01 UTC (permalink / raw) To: dri-devel, Eric Anholt; +Cc: Chris Wilson, Linux Kernel On Friday, February 6, 2009 4:52 pm Chris Wilson wrote: > I tried it, it's not too happy. My only concern is that this now relies > on userspace to correctly call SW_FINISH (and not unmap after mmapping > the GTT_MMAP) or otherwise the object is leaked? Patch comments inline. This one relies on munmap or exit (as it should be). I think we're leaking a refcount somewhere, but afaict it's not caused by this patch (failing to take a ref at mmap time leads to a panic in unpin). That leak may have been fixed by one of your other patches, haven't updated & tested yet. - ref at map time will keep the object around so fault shouldn't fail - additional threads will take their refs in vm_open/close - unmap will unref and remove mmap_offset allowing object to be freed Tested with modetest while looking at /proc/dri/... -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 6915fb8..393d2db 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -460,6 +460,26 @@ drm_gem_object_handle_free(struct kref *kref) } EXPORT_SYMBOL(drm_gem_object_handle_free); +void drm_gem_vm_open(struct vm_area_struct *vma) +{ + struct drm_gem_object *obj = vma->vm_private_data; + + drm_gem_object_reference(obj); +} +EXPORT_SYMBOL(drm_gem_vm_open); + +void drm_gem_vm_close(struct vm_area_struct *vma) +{ + struct drm_gem_object *obj = vma->vm_private_data; + struct drm_device *dev = obj->dev; + + mutex_lock(&dev->struct_mutex); + drm_gem_object_unreference(obj); + mutex_unlock(&dev->struct_mutex); +} +EXPORT_SYMBOL(drm_gem_vm_close); + + /** * drm_gem_mmap - memory map routine for GEM objects * @filp: DRM file pointer @@ -521,6 +541,12 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) #endif vma->vm_page_prot = __pgprot(prot); + /* Take a ref here so our fault handler always has an object + * to work with. This ref will be dropped when the last + * process does a vm_close (at munmap or exit time). + */ + drm_gem_object_reference(obj); + vma->vm_file = filp; /* Needed for drm_vm_open() */ drm_vm_open_locked(vma); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index aac12ee..a31cbdb 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -94,6 +94,8 @@ static int i915_resume(struct drm_device *dev) static struct vm_operations_struct i915_gem_vm_ops = { .fault = i915_gem_fault, + .open = drm_gem_vm_open, + .close = drm_gem_vm_close, }; static struct drm_driver driver = { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1441831..d1a031b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -607,8 +607,6 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) case -EAGAIN: return VM_FAULT_OOM; case -EFAULT: - case -EBUSY: - DRM_ERROR("can't insert pfn?? fault or busy...\n"); return VM_FAULT_SIGBUS; default: return VM_FAULT_NOPAGE; @@ -684,6 +682,30 @@ out_free_list: return ret; } +static void +i915_gem_free_mmap_offset(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + struct drm_i915_gem_object *obj_priv = obj->driver_private; + struct drm_gem_mm *mm = dev->mm_private; + struct drm_map_list *list; + + list = &obj->map_list; + drm_ht_remove_item(&mm->offset_hash, &list->hash); + + if (list->file_offset_node) { + drm_mm_put_block(list->file_offset_node); + list->file_offset_node = NULL; + } + + if (list->map) { + drm_free(list->map, sizeof(struct drm_map), DRM_MEM_DRIVER); + list->map = NULL; + } + + obj_priv->mmap_offset = 0; +} + /** * i915_gem_get_gtt_alignment - return required GTT alignment for an object * @obj: object to check @@ -2885,9 +2907,6 @@ int i915_gem_init_object(struct drm_gem_object *obj) void i915_gem_free_object(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; - struct drm_gem_mm *mm = dev->mm_private; - struct drm_map_list *list; - struct drm_map *map; struct drm_i915_gem_object *obj_priv = obj->driver_private; while (obj_priv->pin_count > 0) @@ -2898,19 +2917,7 @@ void i915_gem_free_object(struct drm_gem_object *obj) i915_gem_object_unbind(obj); - list = &obj->map_list; - drm_ht_remove_item(&mm->offset_hash, &list->hash); - - if (list->file_offset_node) { - drm_mm_put_block(list->file_offset_node); - list->file_offset_node = NULL; - } - - map = list->map; - if (map) { - drm_free(map, sizeof(*map), DRM_MEM_DRIVER); - list->map = NULL; - } + i915_gem_free_mmap_offset(obj); drm_free(obj_priv->page_cpu_valid, 1, DRM_MEM_DRIVER); drm_free(obj->driver_private, 1, DRM_MEM_DRIVER); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8190b9b..e5f4ae9 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1321,6 +1321,8 @@ void drm_gem_object_free(struct kref *kref); struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev, size_t size); void drm_gem_object_handle_free(struct kref *kref); +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); static inline void ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 22:24 ` Jesse Barnes 2009-02-06 22:39 ` Thomas Hellström 2009-02-07 0:52 ` Chris Wilson @ 2009-02-07 8:06 ` Xavier Bestel 2009-02-10 22:00 ` Eric Anholt 3 siblings, 0 replies; 14+ messages in thread From: Xavier Bestel @ 2009-02-07 8:06 UTC (permalink / raw) To: Jesse Barnes; +Cc: Thomas Hellström, Eric Anholt, DRI, Linux Kernel Le vendredi 06 février 2009 à 14:24 -0800, Jesse Barnes a écrit : > + map = list->map; > + if (map) { > + drm_free(map, sizeof(*map), DRM_MEM_DRIVER); > + list->map = NULL; I would have inverted the two last lines (otherwise why wouldn't you directly drw_free(list->map) ?). Xav ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-06 22:24 ` Jesse Barnes ` (2 preceding siblings ...) 2009-02-07 8:06 ` Xavier Bestel @ 2009-02-10 22:00 ` Eric Anholt 2009-02-10 22:58 ` Jesse Barnes 3 siblings, 1 reply; 14+ messages in thread From: Eric Anholt @ 2009-02-10 22:00 UTC (permalink / raw) To: Jesse Barnes; +Cc: Thomas Hellström, DRI, Linux Kernel [-- Attachment #1: Type: text/plain, Size: 1843 bytes --] On Fri, 2009-02-06 at 14:24 -0800, Jesse Barnes wrote: > On Friday, February 6, 2009 1:35 pm Thomas Hellström wrote: > > Jesse Barnes wrote: > > > On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: > > >> So if we leave the lookup reference around from the GTT mapping ioctl, > > >> that would take care of new mappings. And if we added/removed > > >> references at VM open/close time, we should be covered for fork. But is > > >> it ok to add a new unref in the finish ioctl for GTT mapped objects? I > > >> don't think so, because we don't know for sure if the caller was the one > > >> that created the new fake offset (which would be one way of detecting > > >> whether it was GTT mapped). Seems like we need a new unmap ioctl? Or we > > >> could put the mapping ref/unref in libdrm, where it would be tracked on > > >> a per-process basis... > > > > > > Ah but maybe we should just tear down the fake offset at unmap time; then > > > we'd be able to use it as an existence test for the mapping and get the > > > refcounting right. The last thing I thought of was whether we'd be ok in > > > a map_gtt -> crash case. I *think* the vm_close code will deal with > > > that, if we do a deref there? > > > > Yes, an mmap() is always paired with a vm_close(), and the vm_close() > > also happens in a crash situation. > > This one should cover the cases you found. > - ref at map time will keep the object around so fault shouldn't fail > - additional threads will take their refs in vm_open/close > - unmap will unref and remove mmap_offset allowing object to be freed sw_finish doesn't mean unmap (note that it doesn't actually unmap). If you want to actually unmap, that should be done with munmap. -- Eric Anholt eric@anholt.net eric.anholt@intel.com [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Gem GTT mmaps.. 2009-02-10 22:00 ` Eric Anholt @ 2009-02-10 22:58 ` Jesse Barnes 0 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2009-02-10 22:58 UTC (permalink / raw) To: Eric Anholt; +Cc: Thomas Hellström, DRI, Linux Kernel On Tuesday, February 10, 2009 2:00 pm Eric Anholt wrote: > On Fri, 2009-02-06 at 14:24 -0800, Jesse Barnes wrote: > > On Friday, February 6, 2009 1:35 pm Thomas Hellström wrote: > > > Jesse Barnes wrote: > > > > On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote: > > > >> So if we leave the lookup reference around from the GTT mapping > > > >> ioctl, that would take care of new mappings. And if we > > > >> added/removed references at VM open/close time, we should be covered > > > >> for fork. But is it ok to add a new unref in the finish ioctl for > > > >> GTT mapped objects? I don't think so, because we don't know for > > > >> sure if the caller was the one that created the new fake offset > > > >> (which would be one way of detecting whether it was GTT mapped). > > > >> Seems like we need a new unmap ioctl? Or we could put the mapping > > > >> ref/unref in libdrm, where it would be tracked on a per-process > > > >> basis... > > > > > > > > Ah but maybe we should just tear down the fake offset at unmap time; > > > > then we'd be able to use it as an existence test for the mapping and > > > > get the refcounting right. The last thing I thought of was whether > > > > we'd be ok in a map_gtt -> crash case. I *think* the vm_close code > > > > will deal with that, if we do a deref there? > > > > > > Yes, an mmap() is always paired with a vm_close(), and the vm_close() > > > also happens in a crash situation. > > > > This one should cover the cases you found. > > - ref at map time will keep the object around so fault shouldn't fail > > - additional threads will take their refs in vm_open/close > > - unmap will unref and remove mmap_offset allowing object to be freed > > sw_finish doesn't mean unmap (note that it doesn't actually unmap). > > If you want to actually unmap, that should be done with munmap. Yeah, but it does get called at dri_bo_unmap time... I haven't traced the munmap code to see if it would do what we want... if it ends up in vm_close too then it would be fine (ref at gtt ioctl time, unref at vm_close time, as long as there's not a stray vm_open in there). Jesse -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-02-11 22:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-04 22:32 Gem GTT mmaps Thomas Hellström 2009-02-04 23:02 ` Jesse Barnes 2009-02-04 23:42 ` Eric Anholt 2009-02-05 18:37 ` Jesse Barnes 2009-02-06 17:14 ` Jesse Barnes 2009-02-06 21:35 ` Thomas Hellström 2009-02-06 22:24 ` Jesse Barnes 2009-02-06 22:39 ` Thomas Hellström 2009-02-06 23:22 ` Jesse Barnes 2009-02-07 0:52 ` Chris Wilson 2009-02-11 22:01 ` Jesse Barnes 2009-02-07 8:06 ` Xavier Bestel 2009-02-10 22:00 ` Eric Anholt 2009-02-10 22:58 ` Jesse Barnes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).