From: Chris Wilson <chris@chris-wilson.co.uk>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: "Thomas Hellström" <thomas@shipmail.org>,
DRI <dri-devel@lists.sourceforge.net>,
"Linux Kernel" <linux-kernel@vger.kernel.org>
Subject: Re: Gem GTT mmaps..
Date: Sat, 07 Feb 2009 00:52:21 +0000 [thread overview]
Message-ID: <1233967941.11716.1009.camel@localhost> (raw)
In-Reply-To: <200902061424.02906.jbarnes@virtuousgeek.org>
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
next prev parent reply other threads:[~2009-02-07 1:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1233967941.11716.1009.camel@localhost \
--to=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.sourceforge.net \
--cc=jbarnes@virtuousgeek.org \
--cc=linux-kernel@vger.kernel.org \
--cc=thomas@shipmail.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).