| Message ID | 1460558878-14613-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chris,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160413]
[cannot apply to v4.6-rc3]
[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/20160413-225200
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201615 (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_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3643:9: error: too many arguments to function 'io_mapping_map_wc'
ptr = io_mapping_map_wc(ggtt->mappable,
^
In file included from drivers/gpu/drm/i915/i915_gem_gtt.h:37:0,
from drivers/gpu/drm/i915/i915_drv.h:42,
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)
^
vim +/io_mapping_map_wc +3643 drivers/gpu/drm/i915/i915_gem_gtt.c
3637
3638 if (vma->iomap == NULL) {
3639 struct i915_ggtt *ggtt =
3640 container_of(vma->vm, struct i915_ggtt, base);
3641 void *ptr;
3642
> 3643 ptr = io_mapping_map_wc(ggtt->mappable,
3644 vma->node.start,
3645 vma->node.size);
3646 if (ptr == NULL)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: > + /* 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) > + if (vma->iomap && i915_vma_unbind(vma) == 0) > + freed_pages += vma->node.size >> PAGE_SHIFT; Use after free. I need to store the page count in a local before calling unbind. -Chris
On 13/04/16 16:12, Chris Wilson wrote: > On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: >> + /* 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) >> + if (vma->iomap && i915_vma_unbind(vma) == 0) >> + freed_pages += vma->node.size >> PAGE_SHIFT; > > Use after free. I need to store the page count in a local before calling > unbind. Waiting for respin. :) Also, shouldn't the patch which adds the size argument to io_mapping_map_wc be in this series? Regards, Tvrtko
On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote: > > On 13/04/16 16:12, Chris Wilson wrote: > >On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: > >>+ /* 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) > >>+ if (vma->iomap && i915_vma_unbind(vma) == 0) > >>+ freed_pages += vma->node.size >> PAGE_SHIFT; > > > >Use after free. I need to store the page count in a local before calling > >unbind. > > Waiting for respin. :) > > Also, shouldn't the patch which adds the size argument to > io_mapping_map_wc be in this series? It was, this was just an update to patch 2. The delta here is just unsigned long count = vma->node.size >> PAGE_SHIFT; if (vma->iomap && i915_vma_unbind(vma) == 0) freed_pages += count; -Chris
On 15/04/16 11:00, Chris Wilson wrote: > On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote: >> >> On 13/04/16 16:12, Chris Wilson wrote: >>> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: >>>> + /* 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) >>>> + if (vma->iomap && i915_vma_unbind(vma) == 0) >>>> + freed_pages += vma->node.size >> PAGE_SHIFT; >>> >>> Use after free. I need to store the page count in a local before calling >>> unbind. >> >> Waiting for respin. :) >> >> Also, shouldn't the patch which adds the size argument to >> io_mapping_map_wc be in this series? > > It was, this was just an update to patch 2. The delta here is just > unsigned long count = vma->node.size >> PAGE_SHIFT; > if (vma->iomap && i915_vma_unbind(vma) == 0) > freed_pages += count; My bad, I got lost in the threads.. :( Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs now that is in? Maybe also add a quick return at the top, as a micro-opt: if (vma->iomap) reutrn vma->iomap; Followed by WARNs, GEM_BUG_ONs and rest? Shrinker fix mandatory and with or without my above comments r-b from me. Regards, Tvrtko
On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote: > On 15/04/16 11:00, Chris Wilson wrote: > >On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote: > >> > >>On 13/04/16 16:12, Chris Wilson wrote: > >>>On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: > >>>>+ /* 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) > >>>>+ if (vma->iomap && i915_vma_unbind(vma) == 0) > >>>>+ freed_pages += vma->node.size >> PAGE_SHIFT; > >>> > >>>Use after free. I need to store the page count in a local before calling > >>>unbind. > >> > >>Waiting for respin. :) > >> > >>Also, shouldn't the patch which adds the size argument to > >>io_mapping_map_wc be in this series? > > > >It was, this was just an update to patch 2. The delta here is just > >unsigned long count = vma->node.size >> PAGE_SHIFT; > >if (vma->iomap && i915_vma_unbind(vma) == 0) > > freed_pages += count; > > My bad, I got lost in the threads.. :( > > Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs > now that is in? > > Maybe also add a quick return at the top, as a micro-opt: > > if (vma->iomap) > reutrn vma->iomap; > > Followed by WARNs, GEM_BUG_ONs and rest? Like so? void *i915_vma_iomap(struct i915_vma *vma) { struct i915_ggtt *ggtt; void *ptr; if (vma->iomap) return vma->iomap; 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); ggtt = container_of(vma->vm, struct i915_ggtt, base); ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size); if (ptr == NULL) return ERR_PTR(-ENOMEM); vma->iomap = ptr; return ptr; } -Chris
On 15/04/16 11:38, Chris Wilson wrote: > On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote: >> On 15/04/16 11:00, Chris Wilson wrote: >>> On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 13/04/16 16:12, Chris Wilson wrote: >>>>> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: >>>>>> + /* 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) >>>>>> + if (vma->iomap && i915_vma_unbind(vma) == 0) >>>>>> + freed_pages += vma->node.size >> PAGE_SHIFT; >>>>> >>>>> Use after free. I need to store the page count in a local before calling >>>>> unbind. >>>> >>>> Waiting for respin. :) >>>> >>>> Also, shouldn't the patch which adds the size argument to >>>> io_mapping_map_wc be in this series? >>> >>> It was, this was just an update to patch 2. The delta here is just >>> unsigned long count = vma->node.size >> PAGE_SHIFT; >>> if (vma->iomap && i915_vma_unbind(vma) == 0) >>> freed_pages += count; >> >> My bad, I got lost in the threads.. :( >> >> Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs >> now that is in? >> >> Maybe also add a quick return at the top, as a micro-opt: >> >> if (vma->iomap) >> reutrn vma->iomap; >> >> Followed by WARNs, GEM_BUG_ONs and rest? > > Like so? > > void *i915_vma_iomap(struct i915_vma *vma) > { > struct i915_ggtt *ggtt; > void *ptr; > > if (vma->iomap) > return vma->iomap; > > 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); > > ggtt = container_of(vma->vm, struct i915_ggtt, base); > ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size); > if (ptr == NULL) > return ERR_PTR(-ENOMEM); > > vma->iomap = ptr; > return ptr; > } Yes, that's what I had in mind. You can put my r-b in the repost. Regards, Tvrtko
On pe, 2016-04-15 at 11:38 +0100, Chris Wilson wrote: > On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote: > > > > On 15/04/16 11:00, Chris Wilson wrote: > > > > > > On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote: > > > > > > > > > > > > On 13/04/16 16:12, Chris Wilson wrote: > > > > > > > > > > On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote: > > > > > > > > > > > > + /* 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) > > > > > > + if (vma->iomap && i915_vma_unbind(vma) == 0) > > > > > > + freed_pages += vma->node.size >> PAGE_SHIFT; > > > > > Use after free. I need to store the page count in a local before calling > > > > > unbind. > > > > Waiting for respin. :) > > > > > > > > Also, shouldn't the patch which adds the size argument to > > > > io_mapping_map_wc be in this series? > > > It was, this was just an update to patch 2. The delta here is just > > > unsigned long count = vma->node.size >> PAGE_SHIFT; > > > if (vma->iomap && i915_vma_unbind(vma) == 0) > > > freed_pages += count; > > My bad, I got lost in the threads.. :( > > > > Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs > > now that is in? > > > > Maybe also add a quick return at the top, as a micro-opt: > > > > if (vma->iomap) > > reutrn vma->iomap; > > > > Followed by WARNs, GEM_BUG_ONs and rest? > Like so? > > void *i915_vma_iomap(struct i915_vma *vma) > { > struct i915_ggtt *ggtt; > void *ptr; > > if (vma->iomap) > return vma->iomap; > > 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); > > ggtt = container_of(vma->vm, struct i915_ggtt, base); We have: static inline struct i915_hw_ppgtt * i915_vm_to_ppgtt(struct i915_address_space *vm) So rather make a function just like it. > ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size); > if (ptr == NULL) > return ERR_PTR(-ENOMEM); > > vma->iomap = ptr; > return ptr; > } > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b37ffea8b458..6a485630595e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3393,6 +3393,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 c5cb04907525..53e55aead512 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3626,3 +3626,28 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, return obj->base.size; } } + +void *i915_vma_iomap(struct i915_vma *vma) +{ + if (WARN_ON(!vma->obj->map_and_fenceable)) + return ERR_PTR(-ENODEV); + + BUG_ON(!vma->is_ggtt); + BUG_ON((vma->bound & GLOBAL_BIND) == 0); + + if (vma->iomap == NULL) { + struct i915_ggtt *ggtt = + container_of(vma->vm, struct i915_ggtt, base); + void *ptr; + + ptr = io_mapping_map_wc(ggtt->mappable, + vma->node.start, + vma->node.size); + if (ptr == NULL) + return ERR_PTR(-ENOMEM); + + vma->iomap = ptr; + } + + return vma->iomap; +} diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index d7dd3d8a8758..d95190ddf2d6 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,39 @@ size_t i915_ggtt_view_size(struct drm_i915_gem_object *obj, const struct i915_ggtt_view *view); +/** + * i915_vma_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. Or in + * other words callers are responsible for managing the VMA pinned lifetime and + * ensuring it covers the use of the returned mapping. + * + * Callers must hold the struct_mutex. + * + * Returns a valid iomapped pointer or ERR_PTR. + */ +void *i915_vma_iomap(struct i915_vma *vma); + +/** + * i915_vma_iounmap - unmaps the mapping returned from i915_vma_iomap + * @dev_priv: i915 private pointer + * @vma: VMA to unmap + * + * Unmaps the previously iomapped VMA using iounmap. + * + * Users of i915_vma_iomap should not manually unmap by calling this function + * if they want to take advantage of the mapping getting cached in the VMA. + * + * Callers must hold the struct_mutex. + */ +static inline void i915_vma_iounmap(struct i915_vma *vma) +{ + if (vma->iomap == NULL) + return; + + io_mapping_unmap(vma->iomap); + vma->iomap = NULL; +} + #endif diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index d46388f25e04..908c083a39f1 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -387,17 +387,31 @@ 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) + if (vma->iomap && i915_vma_unbind(vma) == 0) + freed_pages += vma->node.size >> PAGE_SHIFT; + +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..3f3c97a30418 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_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;
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. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ drivers/gpu/drm/i915/i915_gem_gtt.c | 25 +++++++++++++++++++++ drivers/gpu/drm/i915/i915_gem_gtt.h | 38 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++++++++++++++++----- drivers/gpu/drm/i915/intel_fbdev.c | 22 +++++++++--------- 5 files changed, 97 insertions(+), 16 deletions(-)