| Message ID | 1460541160-1955-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/04/16 10:52, 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. > > 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 | 42 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++++++++++++ > drivers/gpu/drm/i915/intel_fbdev.c | 20 +++++++++--------- > 4 files changed, 67 insertions(+), 10 deletions(-) > > 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..e759f8cafd9e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -93,6 +93,12 @@ > * > */ > > +static inline struct i915_ggtt *to_ggtt(struct i915_address_space *vm) > +{ > + BUG_ON(!i915_is_ggtt(vm)); > + return container_of(vm, struct i915_ggtt, base); > +} > + As long as it remains hidden in here, otherwise is a bit heavy and rude (BUG_ON), or weak as API (if converted to GEM_BUG_ON and return pointer undocumented). And if it stays here BUG_ON is redundant. Hm.. leaning towards the direct container_of below to bypass all these considerations. > static int > i915_get_ggtt_vma_pages(struct i915_vma *vma); > > @@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, > return obj->base.size; > } > } > + > +void *i915_vma_iomap(struct i915_vma *vma) Kerneldoc! :) > +{ > + if (WARN_ON(!vma->obj->map_and_fenceable)) > + return ERR_PTR(-ENODEV); > + > + BUG_ON(!vma->is_ggtt); > + BUG_ON((vma->bound & GLOBAL_BIND) == 0); Maybe aggregate the two into a single WARN_ON and return -EINVAL ? Since we easily can sounds better. > + > + if (vma->iomap == NULL) { > + struct i915_ggtt *ggtt = to_ggtt(vma->vm); > + void *ptr; > + > + ptr = io_mapping_map_wc(ggtt->mappable, > + vma->node.start, > + vma->node.size); > + if (ptr == NULL) { > + int ret; > + > + /* Too many areas already allocated? */ > + ret = i915_gem_evict_vm(vma->vm, true); I don't know much about the iomapping bussiness. Can we exhaust the address space similar to vmap and would then interfere with our own (or other?) call sites doing plain io_mapping_map* calls? > + if (ret) > + return ERR_PTR(ret); > + > + 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..4b8ffc1c3d33 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,14 @@ size_t > i915_ggtt_view_size(struct drm_i915_gem_object *obj, > const struct i915_ggtt_view *view); > > +void *i915_vma_iomap(struct i915_vma *vma); > +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/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 79ac202f3870..a01bfc33e3d7 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -186,9 +186,10 @@ 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; > + int ret; > > if (intel_fb && > (sizes->fb_width > intel_fb->base.width || > @@ -214,7 +215,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 +244,22 @@ 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) { > + info->screen_base = i915_vma_iomap(vma); We are slowly establishing that ERR_PTR should not be stored in structure members but I suppose here we can allow it since the structure is freed if it fails. > + if (IS_ERR(info->screen_base)) { > DRM_ERROR("Failed to remap framebuffer into virtual memory\n"); > - ret = -ENOSPC; > + ret = PTR_ERR(info->screen_base); > goto out_destroy_fbi; > } > - info->screen_size = size; > + 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
On Wed, Apr 13, 2016 at 01:30:39PM +0100, Tvrtko Ursulin wrote: > As long as it remains hidden in here, otherwise is a bit heavy and > rude (BUG_ON), or weak as API (if converted to GEM_BUG_ON and return > pointer undocumented). And if it stays here BUG_ON is redundant. > Hm.. leaning towards the direct container_of below to bypass all > these considerations. Reasonable. I do value the shorthand to_ggtt() though. Ok, the shorthand can wait until we have more use cases. > > static int > > i915_get_ggtt_vma_pages(struct i915_vma *vma); > > > >@@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, > > return obj->base.size; > > } > > } > >+ > >+void *i915_vma_iomap(struct i915_vma *vma) > > Kerneldoc! :) Sorry. Should have double checked with your patch. > >+{ > >+ if (WARN_ON(!vma->obj->map_and_fenceable)) > >+ return ERR_PTR(-ENODEV); > >+ > >+ BUG_ON(!vma->is_ggtt); > >+ BUG_ON((vma->bound & GLOBAL_BIND) == 0); > > Maybe aggregate the two into a single WARN_ON and return -EINVAL ? > Since we easily can sounds better. > > >+ > >+ if (vma->iomap == NULL) { > >+ struct i915_ggtt *ggtt = to_ggtt(vma->vm); > >+ void *ptr; > >+ > >+ ptr = io_mapping_map_wc(ggtt->mappable, > >+ vma->node.start, > >+ vma->node.size); > >+ if (ptr == NULL) { > >+ int ret; > >+ > >+ /* Too many areas already allocated? */ > >+ ret = i915_gem_evict_vm(vma->vm, true); > > I don't know much about the iomapping bussiness. Can we exhaust the > address space similar to vmap and would then interfere with our own > (or other?) call sites doing plain io_mapping_map* calls? It's vmap underneath. My thinking here was that if we couldn't allocate a new ioremap_wc, the obvious candidates were to reap everything inside the GGTT i.e. inactive ioremap_wc. But the question is, do we want to reap vma->iomaps upon vmap_purge. Yes, we probably should - simplest will be to do a similar forced GGTT eviction from vmap_purge and refine from there. > >+ 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) { > >+ info->screen_base = i915_vma_iomap(vma); > > We are slowly establishing that ERR_PTR should not be stored in > structure members but I suppose here we can allow it since the > structure is freed if it fails. I know, I tell everyone else off for doing it like this as well. Practice what you preach! -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..e759f8cafd9e 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -93,6 +93,12 @@ * */ +static inline struct i915_ggtt *to_ggtt(struct i915_address_space *vm) +{ + BUG_ON(!i915_is_ggtt(vm)); + return container_of(vm, struct i915_ggtt, base); +} + static int i915_get_ggtt_vma_pages(struct i915_vma *vma); @@ -3626,3 +3632,39 @@ 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 = to_ggtt(vma->vm); + void *ptr; + + ptr = io_mapping_map_wc(ggtt->mappable, + vma->node.start, + vma->node.size); + if (ptr == NULL) { + int ret; + + /* Too many areas already allocated? */ + ret = i915_gem_evict_vm(vma->vm, true); + if (ret) + return ERR_PTR(ret); + + 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..4b8ffc1c3d33 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,14 @@ size_t i915_ggtt_view_size(struct drm_i915_gem_object *obj, const struct i915_ggtt_view *view); +void *i915_vma_iomap(struct i915_vma *vma); +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/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 79ac202f3870..a01bfc33e3d7 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -186,9 +186,10 @@ 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; + int ret; if (intel_fb && (sizes->fb_width > intel_fb->base.width || @@ -214,7 +215,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 +244,22 @@ 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) { + info->screen_base = i915_vma_iomap(vma); + if (IS_ERR(info->screen_base)) { DRM_ERROR("Failed to remap framebuffer into virtual memory\n"); - ret = -ENOSPC; + ret = PTR_ERR(info->screen_base); goto out_destroy_fbi; } - info->screen_size = size; + 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. 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 | 42 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++++++++++++ drivers/gpu/drm/i915/intel_fbdev.c | 20 +++++++++--------- 4 files changed, 67 insertions(+), 10 deletions(-)