Re: [PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 04, 2015 at 04:20:43PM +0100, Michał Winiarski wrote:
> According to bspec, some parts of HW expect 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.
> 
> v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Can we igt this? Maybe with softpin or whatever ... For cpu address space
negative addresses are for the kernel, but I think on the gpu we can do
them.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  8 +++++---
>  drivers/gpu/drm/i915/i915_gem_gtt.h        | 12 ++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a4c243c..ceffef9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -395,7 +395,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 +583,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;
> @@ -623,8 +624,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 877c32c..4ea9dab 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -507,6 +507,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) expect 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)
>  {
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx