Gem GTT mmaps..

Gem GTT mmaps..

[Thomas Hellström]

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

Re: Gem GTT mmaps..

[Jesse Barnes]

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

Re: Gem GTT mmaps..

[Eric Anholt]

[-- 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 --]

Re: Gem GTT mmaps..

[Jesse Barnes]

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

Re: Gem GTT mmaps..

[Jesse Barnes]

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

Re: Gem GTT mmaps..

[Thomas Hellström]

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,
>   

Re: Gem GTT mmaps..

[Jesse Barnes]

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

Re: Gem GTT mmaps..

[Thomas Hellström]

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

Re: Gem GTT mmaps..

[Jesse Barnes]

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;
 }

Re: Gem GTT mmaps..

[Chris Wilson]

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

Re: Gem GTT mmaps..

[Xavier Bestel]

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

Re: Gem GTT mmaps..

[Eric Anholt]

[-- 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 --]

Re: Gem GTT mmaps..

[Jesse Barnes]

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

Re: Gem GTT mmaps..

[Jesse Barnes]

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