lkml.org 
[lkml]   [2009]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: Gem GTT mmaps..
    From
    Date
    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



    \
     
     \ /
      Last update: 2009-02-07 02:25    [W:0.058 / U:91.896 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site