Re: [PATCH 3/3] drm/i915: Move ioremap_wc tracking onto VMA

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx