Re: [PATCH i-g-t] tests/gem_softpin: Use offset addresses in canonical form

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

Hi,

I've seen the RB but unfortunately I think we need another small tweak:

On 11/12/15 14:14, Michel Thierry wrote:
> i915 validates that requested offset is in canonical form, so tests need
> to convert the offsets as required.
>
> Also add test to verify non-canonical 48-bit address will be rejected.
>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>   tests/gem_softpin.c | 66 +++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/tests/gem_softpin.c b/tests/gem_softpin.c
> index 7bee16b..2981b30 100644
> --- a/tests/gem_softpin.c
> +++ b/tests/gem_softpin.c
> @@ -67,7 +67,7 @@ static void *create_mem_buffer(uint64_t size);
>   static int gem_call_userptr_ioctl(int fd, i915_gem_userptr *userptr);
>   static void gem_pin_userptr_test(void);
>   static void gem_pin_bo_test(void);
> -static void gem_pin_invalid_vma_test(bool test_decouple_flags);
> +static void gem_pin_invalid_vma_test(bool test_decouple_flags, bool test_canonical_offset);
>   static void gem_pin_overlap_test(void);
>   static void gem_pin_high_address_test(void);
>
> @@ -198,6 +198,15 @@ static void setup_exec_obj(struct drm_i915_gem_exec_object2 *exec,
>   	exec->offset = offset;
>   }
>
> +/* gen8_canonical_addr
> + * Used to convert any address into canonical form, i.e. [63:48] == [47].
> + * @address - a virtual address
> +*/
> +static uint64_t gen8_canonical_addr(uint64_t address)
> +{
> +	return ((int64_t)address << 16) >> 16;
> +}
> +

This trick relies on implementation defined compiler behaviour and 
kernel patch was modified in the mean time to do it in a safer way. C&P 
of the relevant section:

+/* 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 uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return sign_extend64(address, GEN8_HIGH_ADDRESS_BIT);
+}

Could you update the IGT to do the same?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx