Re: Gem GTT mmaps..
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;
- }
+
Re: Gem GTT mmaps..
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 -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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 e...@anholt.net eric.anh...@intel.com signature.asc Description: This is a digitally signed message part -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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
--
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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 -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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;
+
Re: Gem GTT mmaps..
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 -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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; } -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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;
Re: Gem GTT mmaps..
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 -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Gem GTT mmaps..
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 -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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 -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: Gem GTT mmaps..
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 e...@anholt.net eric.anh...@intel.com signature.asc Description: This is a digitally signed message part -- Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing skills and code to build responsive, highly engaging applications that combine the power of local resources and data with the reach of the web. Download the Adobe AIR SDK and Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
