diff mbox

tests/gen7_forcewake_mt: Don't set the GGTT bit in SRM command

Message ID 1396992136-6726-1-git-send-email-bradley.d.volkin@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

bradley.d.volkin@intel.com April 8, 2014, 9:22 p.m. UTC 
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>
---
 tests/gen7_forcewake_mt.c | 55 +++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

Comments

Chris Wilson April 9, 2014, 6:20 a.m. UTC | #1 
On Tue, Apr 08, 2014 at 02:22:16PM -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 was written how I did in the ddx...
-Chris
Daniel Vetter April 9, 2014, 1:32 p.m. UTC | #2 
On Wed, Apr 09, 2014 at 07:20:30AM +0100, Chris Wilson wrote:
> On Tue, Apr 08, 2014 at 02:22:16PM -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 was written how I did in the ddx...

Oops, sounds like the cmd parser needs to transparently set this when
copying to the hidden batch ... We could of course do some magic in the
kernel and let it pass, but my current plan is that we'll enable the cmd
parser for secure batches only (to avoid the heft perf hit and that's
essentially no worse than what we have already), so that Xorg and mesa
don't need to be run as root on hsw.

But that means for at least sensible test coverage we should still
scan/copy even root batches since until fc22 (or whatever it was) ships we
won't have many systems with non-root X out there yet for testing.
-Daniel
bradley.d.volkin@intel.com April 9, 2014, 3:12 p.m. UTC | #3 
On Tue, Apr 08, 2014 at 11:20:30PM -0700, Chris Wilson wrote:
> On Tue, Apr 08, 2014 at 02:22:16PM -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 was written how I did in the ddx...

Oh, I thought you had said that the ddx didn't use MI_STORE_REGISTER_MEM, and
I didn't see it used in the current ddx code, so I thought that part of the
test wasn't relevant to the actual workaround. It looked like it was just a
write so we could see the values and check that they were updated. But if it
is, then yeah, I don't want to change the test behavior.

Brad

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Daniel Vetter April 9, 2014, 4:47 p.m. UTC | #4 
On Wed, Apr 09, 2014 at 08:12:28AM -0700, Volkin, Bradley D wrote:
> On Tue, Apr 08, 2014 at 11:20:30PM -0700, Chris Wilson wrote:
> > On Tue, Apr 08, 2014 at 02:22:16PM -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 was written how I did in the ddx...
> 
> Oh, I thought you had said that the ddx didn't use MI_STORE_REGISTER_MEM, and
> I didn't see it used in the current ddx code, so I thought that part of the
> test wasn't relevant to the actual workaround. It looked like it was just a
> write so we could see the values and check that they were updated. But if it
> is, then yeah, I don't want to change the test behavior.

Hm right I think Chris said that in the ended he never released a ddx
version with this code. Now I'm indeed rather confused what's going on. If
we don't need it I obviously prefer less complexity in the kernel cmd
parser.
-Daniel
bradley.d.volkin@intel.com April 22, 2014, 4:45 p.m. UTC | #5 
On Wed, Apr 09, 2014 at 09:47:57AM -0700, Daniel Vetter wrote:
> On Wed, Apr 09, 2014 at 08:12:28AM -0700, Volkin, Bradley D wrote:
> > On Tue, Apr 08, 2014 at 11:20:30PM -0700, Chris Wilson wrote:
> > > On Tue, Apr 08, 2014 at 02:22:16PM -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 was written how I did in the ddx...
> > 
> > Oh, I thought you had said that the ddx didn't use MI_STORE_REGISTER_MEM, and
> > I didn't see it used in the current ddx code, so I thought that part of the
> > test wasn't relevant to the actual workaround. It looked like it was just a
> > write so we could see the values and check that they were updated. But if it
> > is, then yeah, I don't want to change the test behavior.
> 
> Hm right I think Chris said that in the ended he never released a ddx
> version with this code. Now I'm indeed rather confused what's going on. If
> we don't need it I obviously prefer less complexity in the kernel cmd
> parser.

Chris, can you help clarify this?

Thanks,
Brad

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

Patch

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);
 	}