[Intel-gfx] [PATCH v3] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michel Thierry]

On 12/11/2015 2:13 PM, Michał Winiarski wrote:
> According to bspec, some parts of HW require the addresses to be in
> a canonical form, where bits [63:48] == [47]. Let's convert addresses to
> canonical form prior to relocating and return converted offsets to
> userspace. We also need to make sure that userspace is using addresses
> in canonical form in case of softpin.
>
> v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville)
> v3: Rebase on top of softpin, fix a hole in relocate_entry,
>      s/expect/require (Chris)
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>

With updated gem_softpin 
[http://patchwork.freedesktop.org/patch/msgid/1449843255-32640-1-git-send-email-michel.thierry@intel.com]

Tested-by: Michel Thierry <michel.thierry at intel.com>

> ---
>   drivers/gpu/drm/i915/i915_gem.c            |  9 +++++++--
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 +++++++++++++++------
>   drivers/gpu/drm/i915/i915_gem_gtt.h        | 12 ++++++++++++
>   3 files changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8e2acde..b83207b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3482,12 +3482,17 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>
>   	if (flags & PIN_OFFSET_FIXED) {
>   		uint64_t offset = flags & PIN_OFFSET_MASK;
> +		uint64_t noncanonical_offset = offset & ((1ULL << 48) - 1);
>
> -		if (offset & (alignment - 1) || offset + size > end) {
> +		if (offset & (alignment - 1) ||
> +		    noncanonical_offset + size > end ||
> +		    offset != gen8_canonical_addr(offset)) {
>   			ret = -EINVAL;
>   			goto err_free_vma;
>   		}
> -		vma->node.start = offset;
> +		/* While userspace is using addresses in canonical form, our
> +		 * allocator is unaware of this */
> +		vma->node.start = noncanonical_offset;
>   		vma->node.size = size;
>   		vma->node.color = obj->cache_level;
>   		ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 48ec484..445ccc7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -249,6 +249,13 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
>   		obj->cache_level != I915_CACHE_NONE);
>   }
>
> +static inline uint64_t
> +relocation_target(struct drm_i915_gem_relocation_entry *reloc,
> +		  uint64_t target_offset)
> +{
> +	return gen8_canonical_addr((int)reloc->delta + target_offset);
> +}
> +
>   static int
>   relocate_entry_cpu(struct drm_i915_gem_object *obj,
>   		   struct drm_i915_gem_relocation_entry *reloc,
> @@ -256,7 +263,7 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj,
>   {
>   	struct drm_device *dev = obj->base.dev;
>   	uint32_t page_offset = offset_in_page(reloc->offset);
> -	uint64_t delta = reloc->delta + target_offset;
> +	uint64_t delta = relocation_target(reloc, target_offset);
>   	char *vaddr;
>   	int ret;
>
> @@ -292,7 +299,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
>   {
>   	struct drm_device *dev = obj->base.dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint64_t delta = reloc->delta + target_offset;
> +	uint64_t delta = relocation_target(reloc, target_offset);
>   	uint64_t offset;
>   	void __iomem *reloc_page;
>   	int ret;
> @@ -347,7 +354,7 @@ relocate_entry_clflush(struct drm_i915_gem_object *obj,
>   {
>   	struct drm_device *dev = obj->base.dev;
>   	uint32_t page_offset = offset_in_page(reloc->offset);
> -	uint64_t delta = (int)reloc->delta + target_offset;
> +	uint64_t delta = relocation_target(reloc, target_offset);
>   	char *vaddr;
>   	int ret;
>
> @@ -395,7 +402,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
>   	target_i915_obj = target_vma->obj;
>   	target_obj = &target_vma->obj->base;
>
> -	target_offset = target_vma->node.start;
> +	target_offset = gen8_canonical_addr(target_vma->node.start);
>
>   	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
>   	 * pipe_control writes because the gpu doesn't properly redirect them
> @@ -583,6 +590,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>   	struct drm_i915_gem_object *obj = vma->obj;
>   	struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
>   	uint64_t flags;
> +	uint64_t offset;
>   	int ret;
>
>   	flags = PIN_USER;
> @@ -625,8 +633,9 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>   			entry->flags |= __EXEC_OBJECT_HAS_FENCE;
>   	}
>
> -	if (entry->offset != vma->node.start) {
> -		entry->offset = vma->node.start;
> +	offset = gen8_canonical_addr(vma->node.start);
> +	if (entry->offset != offset) {
> +		entry->offset = offset;
>   		*need_reloc = true;
>   	}
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index b448ad8..fa1f3ab 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -504,6 +504,18 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
>   	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
>   }
>
> +/* Used to convert any address to canonical form.
> + * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
> + * MI_LOAD_REGISTER_MEM and others, see Broadwell PRM Vol2a) require the
> + * addresses to be in a canonical form:
> + * "GraphicsAddress[63:48] are ignored by the HW and assumed to be in correct
> + * canonical form [63:48] == [47]."
> + */
> +static inline uint64_t gen8_canonical_addr(uint64_t address)
> +{
> +	return ((int64_t)address << 16) >> 16;
> +}
> +
>   static inline dma_addr_t
>   i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
>   {
>