Re: [PATCH] drm/i915/selftests: Canonicalise gen8 addresses

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> For igt_vm_isolation, we write into the whole 48b address space, and not
> just our usual low 4G of global GTT. For these MI operations, play safe
> and ensure we use the canonical address form.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c    | 19 -----------------
>  drivers/gpu/drm/i915/intel_gpu_commands.h     | 21 +++++++++++++++++++
>  .../gpu/drm/i915/selftests/i915_gem_context.c |  2 ++
>  3 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 943a221acb21..c335c0a4099a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -289,25 +289,6 @@ struct i915_execbuffer {
>  
>  #define exec_entry(EB, VMA) (&(EB)->exec[(VMA)->exec_flags - (EB)->flags])
>  
> -/*
> - * 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]."
> - */
> -#define GEN8_HIGH_ADDRESS_BIT 47
> -static inline u64 gen8_canonical_addr(u64 address)
> -{
> -	return sign_extend64(address, GEN8_HIGH_ADDRESS_BIT);
> -}
> -
> -static inline u64 gen8_noncanonical_addr(u64 address)
> -{
> -	return address & GENMASK_ULL(GEN8_HIGH_ADDRESS_BIT, 0);
> -}
> -
>  static inline bool eb_use_cmdparser(const struct i915_execbuffer *eb)
>  {
>  	return intel_engine_needs_cmd_parser(eb->engine) && eb->batch_len;
> diff --git a/drivers/gpu/drm/i915/intel_gpu_commands.h b/drivers/gpu/drm/i915/intel_gpu_commands.h
> index a34ece53a771..b017bc928d00 100644
> --- a/drivers/gpu/drm/i915/intel_gpu_commands.h
> +++ b/drivers/gpu/drm/i915/intel_gpu_commands.h
> @@ -7,6 +7,8 @@
>  #ifndef _INTEL_GPU_COMMANDS_H_
>  #define _INTEL_GPU_COMMANDS_H_
>  
> +#include <linux/bitops.h>
> +
>  /*
>   * Instruction field definitions used by the command parser
>   */
> @@ -275,4 +277,23 @@
>  #define COLOR_BLT     ((0x2<<29)|(0x40<<22))
>  #define SRC_COPY_BLT  ((0x2<<29)|(0x43<<22))
>  
> +/*
> + * 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]."
> + */
> +#define GEN8_HIGH_ADDRESS_BIT 47
> +static inline u64 gen8_canonical_addr(u64 address)
> +{
> +	return sign_extend64(address, GEN8_HIGH_ADDRESS_BIT);
> +}
> +
> +static inline u64 gen8_noncanonical_addr(u64 address)
> +{
> +	return address & GENMASK_ULL(GEN8_HIGH_ADDRESS_BIT, 0);
> +}
> +
>  #endif /* _INTEL_GPU_COMMANDS_H_ */
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 0346ff224d5d..34a8c15273f4 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -1194,6 +1194,7 @@ static int write_to_scratch(struct i915_gem_context *ctx,
>  
>  	*cmd++ = MI_STORE_DWORD_IMM_GEN4;
>  	if (INTEL_GEN(i915) >= 8) {
> +		offset = gen8_canonical_addr(offset);
>  		*cmd++ = lower_32_bits(offset);
>  		*cmd++ = upper_32_bits(offset);
>  	} else {
> @@ -1284,6 +1285,7 @@ static int read_from_scratch(struct i915_gem_context *ctx,
>  	if (INTEL_GEN(i915) >= 8) {
>  		*cmd++ = MI_LOAD_REGISTER_MEM_GEN8;
>  		*cmd++ = RCS_GPR0;
> +		offset = gen8_canonical_addr(offset);

Yes. Now, I need to go and really convince myself that
MI_STORE_DWORD_IMM is not part of this club.

-Mika


>  		*cmd++ = lower_32_bits(offset);
>  		*cmd++ = upper_32_bits(offset);
>  		*cmd++ = MI_STORE_REGISTER_MEM_GEN8;
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx