[Intel-gfx] [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Apr 18 15:08:55 UTC 2016
On 18/04/16 15:54, Chris Wilson wrote:
> By tracking the iomapping on the VMA itself, we can share that area
> between multiple users. Also by only revoking the iomapping upon
> unbinding from the mappable portion of the GGTT, we can keep that iomap
> across multiple invocations (e.g. execlists context pinning).
>
> Note that by moving the iounnmap tracking to the VMA, we actually end up
> fixing a leak of the iomapping in intel_fbdev.
>
> v1.5: Rebase prompted by Tvrtko
> v2: Drop dev_priv parameter, we can recover the i915_ggtt from the vma.
> v3: Move handling of ioremap space exhaustion to vmap_purge and also
> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
> v5: Back to i915_vm_to_ggtt
> v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
> sections and ensure the VMA cannot be reaped whilst mapped.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com> #v4
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 ++
> drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 58 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++----
> drivers/gpu/drm/i915/intel_fbdev.c | 22 ++++++------
> 5 files changed, 120 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..82d0af3f1692 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3378,6 +3378,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
> ret = i915_gem_object_put_fence(obj);
> if (ret)
> return ret;
> +
> + i915_vma_iounmap(vma);
> }
>
> trace_i915_vma_unbind(vma);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b3af2e808b49..e185dff358e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> return obj->base.size;
> }
> }
> +
> +void *i915_vma_pin_iomap(struct i915_vma *vma)
> +{
> + void *ptr;
Just realized we should mark the pointers with __iomem here.
> +
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + if (WARN_ON(!vma->obj->map_and_fenceable))
> + return ERR_PTR(-ENODEV);
> +
> + GEM_BUG_ON(!vma->is_ggtt);
> + GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> +
> + ptr = vma->iomap;
> + if (ptr == NULL) {
> + ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> + vma->node.start,
> + vma->node.size);
> + if (ptr == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + vma->iomap = ptr;
> + }
> +
> + vma->pin_count++;
> + return ptr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..a01d78bc9f24 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -34,6 +34,8 @@
> #ifndef __I915_GEM_GTT_H__
> #define __I915_GEM_GTT_H__
>
> +#include <linux/io-mapping.h>
> +
> struct drm_i915_file_private;
>
> typedef uint32_t gen6_pte_t;
> @@ -175,6 +177,7 @@ struct i915_vma {
> struct drm_mm_node node;
> struct drm_i915_gem_object *obj;
> struct i915_address_space *vm;
> + void *iomap;
>
> /** Flags and address space this VMA is bound to */
> #define GLOBAL_BIND (1<<0)
> @@ -559,4 +562,59 @@ size_t
> i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view);
>
> +/**
> + * i915_vma_pin_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
> + * @vma: VMA to iomap
> + *
> + * The passed in VMA has to be pinned in the global GTT mappable region.
> + * An extra pinning of the VMA is acquired for the return iomapping,
> + * the caller must call i915_vma_unpin_iomap to relinquish the pinning
> + * after the iomapping is no longer required.
> + *
> + * Callers must hold the struct_mutex.
> + *
> + * Returns a valid iomapped pointer or ERR_PTR.
> + */
> +void *i915_vma_pin_iomap(struct i915_vma *vma);
> +
> +/**
> + * i915_vma_iounmap - unmaps the VMA
> + * @vma: VMA to unmap
> + *
> + * Unmaps the previously iomapped VMA using iounmap.
> + *
> + * Users of i915_vma_pin_iomap() should not manually unmap by calling this
> + * function but should call i915_vma_unpin_iomap() instead.
> + *
> + * Callers must hold the struct_mutex.
> + */
> +static inline void i915_vma_iounmap(struct i915_vma *vma)
> +{
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + GEM_BUG_ON(vma->pin_count);
> +
> + if (vma->iomap == NULL)
> + return;
> +
> + io_mapping_unmap(vma->iomap);
> + vma->iomap = NULL;
> +}
It would be best to hide this near the unbind code. Alternatively maybe
put a louder warning in the kerneldoc? Just so it is a bit more
distinguishable from the GEM exported API for this feature.
> +
> +/**
> + * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
> + * @vma: VMA to unpin
> + *
> + * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
> + *
> + * Callers must hold the struct_mutex. This function is only valid to be
> + * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
> + */
> +static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
> +{
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + GEM_BUG_ON(vma->pin_count == 0);
> + GEM_BUG_ON(vma->iomap == NULL);
> + vma->pin_count--;
> +}
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d46388f25e04..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
> struct drm_i915_private *dev_priv =
> container_of(nb, struct drm_i915_private, mm.vmap_notifier);
> struct shrinker_lock_uninterruptible slu;
> - unsigned long freed_pages;
> + struct i915_vma *vma, *next;
> + unsigned long freed_pages = 0;
> + int ret;
>
> if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
> return NOTIFY_DONE;
>
> - freed_pages = i915_gem_shrink(dev_priv, -1UL,
> - I915_SHRINK_BOUND |
> - I915_SHRINK_UNBOUND |
> - I915_SHRINK_ACTIVE |
> - I915_SHRINK_VMAPS);
> + /* Force everything onto the inactive lists */
> + ret = i915_gpu_idle(dev_priv->dev);
> + if (ret)
> + goto out;
> +
> + freed_pages += i915_gem_shrink(dev_priv, -1UL,
> + I915_SHRINK_BOUND |
> + I915_SHRINK_UNBOUND |
> + I915_SHRINK_ACTIVE |
> + I915_SHRINK_VMAPS);
> +
> + /* We also want to clear any cached iomaps as they wrap vmap */
> + list_for_each_entry_safe(vma, next,
> + &dev_priv->ggtt.base.inactive_list, vm_link) {
> + unsigned long count = vma->node.size >> PAGE_SHIFT;
> + if (vma->iomap && i915_vma_unbind(vma) == 0)
> + freed_pages += count;
> + }
>
> +out:
> i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
>
> *(unsigned long *)ptr += freed_pages;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
> struct i915_ggtt *ggtt = &dev_priv->ggtt;
> struct fb_info *info;
> struct drm_framebuffer *fb;
> + struct i915_vma *vma;
> struct drm_i915_gem_object *obj;
> - int size, ret;
> bool prealloc = false;
> + void *vaddr;
> + int ret;
>
> if (intel_fb &&
> (sizes->fb_width > intel_fb->base.width ||
> @@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> }
>
> obj = intel_fb->obj;
> - size = obj->base.size;
>
> mutex_lock(&dev->struct_mutex);
>
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
> info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
> info->fbops = &intelfb_ops;
>
> + vma = i915_gem_obj_to_ggtt(obj);
> +
> /* setup aperture base/size for vesafb takeover */
> info->apertures->ranges[0].base = dev->mode_config.fb_base;
> info->apertures->ranges[0].size = ggtt->mappable_end;
>
> - info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> - info->fix.smem_len = size;
> + info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> + info->fix.smem_len = vma->node.size;
>
> - info->screen_base =
> - ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> - size);
> - if (!info->screen_base) {
> + vaddr = i915_vma_pin_iomap(vma);
> + if (IS_ERR(vaddr)) {
> DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> - ret = -ENOSPC;
> + ret = PTR_ERR(vaddr);
> goto out_destroy_fbi;
> }
> - info->screen_size = size;
> + info->screen_base = vaddr;
> + info->screen_size = vma->node.size;
>
> /* This driver doesn't need a VT switch to restore the mode on resume */
> info->skip_vt_switch = true;
>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list