| Message ID | 1461063973-25029-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/04/16 12:06, 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. > v7: Move i915_vma_iounmap so that consumers of the API are not tempted, > and add iomem annotations > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4 > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 11 ++++++++++ > drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_gtt.h | 35 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++++++++++------ > drivers/gpu/drm/i915/intel_fbdev.c | 22 +++++++++++--------- > 5 files changed, 106 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6ce2c31b9a81..9ef47329e8ae 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) > old_write_domain); > } > > +static void __i915_vma_iounmap(struct i915_vma *vma) > +{ > + if (vma->iomap == NULL) > + return; > + > + io_mapping_unmap(vma->iomap); > + vma->iomap = NULL; > +} > + > static int __i915_vma_unbind(struct i915_vma *vma, bool wait) > { > struct drm_i915_gem_object *obj = vma->obj; > @@ -3378,6 +3387,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..9d4293f247fc 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 __iomem *i915_vma_pin_iomap(struct i915_vma *vma) > +{ > + void __iomem *ptr; > + > + 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..b0ae6632c01a 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 __iomem *iomap; > > /** Flags and address space this VMA is bound to */ > #define GLOBAL_BIND (1<<0) > @@ -559,4 +562,36 @@ 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 __iomem *i915_vma_pin_iomap(struct i915_vma *vma); > + > +/** > + * 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; > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Hi,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160419]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Move-ioremap_wc-tracking-onto-VMA/20160419-190843
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/i915_gem_gtt.c: In function 'i915_vma_pin_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:3: error: implicit declaration of function 'i915_vm_to_ggtt' [-Werror=implicit-function-declaration]
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:51: error: invalid type argument of '->' (have 'int')
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:9: error: too many arguments to function 'io_mapping_map_wc'
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
In file included from drivers/gpu/drm/i915/i915_drv.h:36:0,
from drivers/gpu/drm/i915/i915_gem_gtt.c:30:
include/linux/io-mapping.h:158:1: note: declared here
io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
^
cc1: some warnings being treated as errors
vim +/i915_vm_to_ggtt +3644 drivers/gpu/drm/i915/i915_gem_gtt.c
3638
3639 GEM_BUG_ON(!vma->is_ggtt);
3640 GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
3641
3642 ptr = vma->iomap;
3643 if (ptr == NULL) {
> 3644 ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
3645 vma->node.start,
3646 vma->node.size);
3647 if (ptr == NULL)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6ce2c31b9a81..9ef47329e8ae 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) old_write_domain); } +static void __i915_vma_iounmap(struct i915_vma *vma) +{ + if (vma->iomap == NULL) + return; + + io_mapping_unmap(vma->iomap); + vma->iomap = NULL; +} + static int __i915_vma_unbind(struct i915_vma *vma, bool wait) { struct drm_i915_gem_object *obj = vma->obj; @@ -3378,6 +3387,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..9d4293f247fc 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 __iomem *i915_vma_pin_iomap(struct i915_vma *vma) +{ + void __iomem *ptr; + + 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..b0ae6632c01a 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 __iomem *iomap; /** Flags and address space this VMA is bound to */ #define GLOBAL_BIND (1<<0) @@ -559,4 +562,36 @@ 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 __iomem *i915_vma_pin_iomap(struct i915_vma *vma); + +/** + * 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;