[Intel-gfx] [PATCH 3/3] drm/i915: Move ioremap_wc tracking onto VMA
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Apr 13 12:30:39 UTC 2016
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 at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at 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
More information about the Intel-gfx
mailing list