Re: [PATCH] tests/gen7_forcewake_mt: Don't set the GGTT bit in SRM command

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, May 10, 2014 at 02:11:53PM -0700, bradley.d.volkin@intel.com wrote:
> From: Brad Volkin <bradley.d.volkin@intel.com>
> 
> The command parser in newer kernels will reject it and setting this
> bit is not required for the actual test case.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76670
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> ---
> 
> This is a resend of
> http://lists.freedesktop.org/archives/intel-gfx/2014-April/043223.html
> 
> There was initially some discussion as to the fact that the test was
> written to reflect the implementation of a workaround in the ddx and
> whether this patch lead to a deviation between the two. There was no
> real closure on that discussion, however, I don't believe the
> MI_STORE_REGISTER_MEM aspect of the test is relevant to the ddx code,
> so I'd like to move forward with this or get clear direction on the
> preferred solution.

Afaik Chris agreed that we've never shipped with this, so changing it is
ok. If we're proven wrong we can look at this again. Patch merged to igt,
thanks.
-Daniel
> 
>  tests/gen7_forcewake_mt.c | 55 +++++++++++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/gen7_forcewake_mt.c b/tests/gen7_forcewake_mt.c
> index fdc34ce..3afd80a 100644
> --- a/tests/gen7_forcewake_mt.c
> +++ b/tests/gen7_forcewake_mt.c
> @@ -121,7 +121,7 @@ static void *thread(void *arg)
>  }
>  
>  #define MI_LOAD_REGISTER_IMM                    (0x22<<23)
> -#define MI_STORE_REGISTER_MEM                   (0x24<<23| 1<<22)
> +#define MI_STORE_REGISTER_MEM                   (0x24<<23)
>  
>  igt_simple_main
>  {
> @@ -140,8 +140,9 @@ igt_simple_main
>  	sleep(2);
>  
>  	for (i = 0; i < 1000; i++) {
> +		uint32_t *p;
>  		struct drm_i915_gem_execbuffer2 execbuf;
> -		struct drm_i915_gem_exec_object2 exec;
> +		struct drm_i915_gem_exec_object2 exec[2];
>  		struct drm_i915_gem_relocation_entry reloc[2];
>  		uint32_t b[] = {
>  			MI_LOAD_REGISTER_IMM | 1,
> @@ -149,54 +150,56 @@ igt_simple_main
>  			2 << 16 | 2,
>  			MI_STORE_REGISTER_MEM | 1,
>  			FORCEWAKE_MT,
> -			5*sizeof(uint32_t),
> +			0, // to be patched
>  			MI_LOAD_REGISTER_IMM | 1,
>  			FORCEWAKE_MT,
>  			2 << 16,
>  			MI_STORE_REGISTER_MEM | 1,
>  			FORCEWAKE_MT,
> -			11*sizeof(uint32_t),
> +			1 * sizeof(uint32_t), // to be patched
>  			MI_BATCH_BUFFER_END,
>  			0
>  		};
>  
> -		memset(&exec, 0, sizeof(exec));
> -		exec.handle = gem_create(t[0].fd, 4096);
> -		exec.relocation_count = 2;
> -		exec.relocs_ptr = (uintptr_t)reloc;
> -		//exec.flags = EXEC_OBJECT_NEEDS_GTT;
> -		gem_write(t[0].fd, exec.handle, 0, b, sizeof(b));
> +		memset(exec, 0, sizeof(exec));
> +		exec[1].handle = gem_create(t[0].fd, 4096);
> +		exec[1].relocation_count = 2;
> +		exec[1].relocs_ptr = (uintptr_t)reloc;
> +		gem_write(t[0].fd, exec[1].handle, 0, b, sizeof(b));
> +		exec[0].handle = gem_create(t[0].fd, 4096);
>  
>  		reloc[0].offset = 5 * sizeof(uint32_t);
> -		reloc[0].delta = 5 * sizeof(uint32_t);
> -		reloc[0].target_handle = exec.handle;
> -		reloc[0].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> -		reloc[0].write_domain = 0;
> +		reloc[0].delta = 0;
> +		reloc[0].target_handle = exec[0].handle;
> +		reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> +		reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
>  		reloc[0].presumed_offset = 0;
>  
>  		reloc[1].offset = 11 * sizeof(uint32_t);
> -		reloc[1].delta = 11 * sizeof(uint32_t);
> -		reloc[1].target_handle = exec.handle;
> -		reloc[1].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> -		reloc[1].write_domain = 0;
> +		reloc[1].delta = 1 * sizeof(uint32_t);
> +		reloc[1].target_handle = exec[0].handle;
> +		reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
> +		reloc[1].write_domain = I915_GEM_DOMAIN_RENDER;
>  		reloc[1].presumed_offset = 0;
>  
>  		memset(&execbuf, 0, sizeof(execbuf));
>  		execbuf.buffers_ptr = (uintptr_t)&exec;
> -		execbuf.buffer_count = 1;
> +		execbuf.buffer_count = 2;
>  		execbuf.batch_len = sizeof(b);
>  		execbuf.flags = I915_EXEC_SECURE;
>  
>  		gem_execbuf(t[0].fd, &execbuf);
> -		gem_sync(t[0].fd, exec.handle);
> -		gem_read(t[0].fd, exec.handle, 0, b, sizeof(b));
> -		gem_close(t[0].fd, exec.handle);
> +		gem_sync(t[0].fd, exec[1].handle);
>  
> -		printf("[%d]={ %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x }\n",
> -		       i, b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7], b[8], b[9], b[10], b[11], b[12]);
> +		p = gem_mmap(t[0].fd, exec[0].handle, 4096, PROT_READ);
>  
> -		igt_assert(b[5] & 2);
> -		igt_assert((b[11] & 2) == 0);
> +		printf("[%d]={ %08x %08x }\n", i, p[0], p[1]);
> +		igt_assert(p[0] & 2);
> +		igt_assert((p[1] & 2) == 0);
> +
> +		munmap(p, 4096);
> +		gem_close(t[0].fd, exec[0].handle);
> +		gem_close(t[0].fd, exec[1].handle);
>  
>  		usleep(1000);
>  	}
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch