[PATCH] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

[PATCH] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

[Kenneth Graunke]

Allowing register copies where the source and destination are both
whitelisted should be safe, and is useful.  For example, Mesa uses
this to load the command streamer math registers with data from the
pipeline statistics counters.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 69a1ba8..14f3b44 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -215,7 +215,8 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
 	CMD(  MI_RS_CONTEXT,                    SMI,    F,  1,      S  ),
 	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
 	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
-	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   R  ),
+	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   W,
+	      .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 }    ),
 	CMD(  MI_RS_STORE_DATA_IMM,             SMI,   !F,  0xFF,   S  ),
 	CMD(  MI_LOAD_URB_MEM,                  SMI,   !F,  0xFF,   S  ),
 	CMD(  MI_STORE_URB_MEM,                 SMI,   !F,  0xFF,   S  ),
@@ -1113,6 +1114,12 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 					return false;
 				}
 
+				if (desc->cmd.value == MI_LOAD_REGISTER_REG) {
+					DRM_DEBUG_DRIVER("CMD: Rejected LRR to masked register 0x%08X\n",
+							 reg_addr);
+					return false;
+				}
+
 				if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) &&
 				    (offset + 2 > length ||
 				     (cmd[offset + 1] & reg->mask) != reg->value)) {
@@ -1301,6 +1308,7 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv)
 	 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
 	 * 5. GPGPU dispatch compute indirect registers.
 	 * 6. TIMESTAMP register and Haswell CS GPR registers
+	 * 7. Allow MI_LOAD_REGISTER_REG between whitelisted registers.
 	 */
-	return 6;
+	return 7;
 }
-- 
2.8.2

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

✓ Fi.CI.BAT: success for drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

[Patchwork]

== Series Details ==

Series: drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.
URL   : https://patchwork.freedesktop.org/series/6774/
State : success

== Summary ==

Series 6774v1 drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.
http://patchwork.freedesktop.org/api/1.0/series/6774/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4:
                fail       -> SKIP       (bdw-nuci7-2)

bdw-nuci7-2      total:226  pass:26   dwarn:0   dfail:1   fail:52  skip:147
bsw-nuc-2        total:225  pass:22   dwarn:0   dfail:1   fail:60  skip:142
byt-nuc          total:225  pass:20   dwarn:0   dfail:1   fail:60  skip:144
hsw-brixbox      total:226  pass:22   dwarn:0   dfail:1   fail:60  skip:143
hsw-gt2          total:226  pass:27   dwarn:0   dfail:1   fail:60  skip:138
skl-i7k-2        total:226  pass:22   dwarn:0   dfail:1   fail:60  skip:143
skl-nuci5        total:226  pass:26   dwarn:0   dfail:1   fail:60  skip:139

Results at /archive/results/CI_IGT_test/Patchwork_2141/

e6160ef8b9b3ddfcb1fd382716887e57a2896710 drm-intel-nightly: 2016y-05m-05d-08h-06m-20s UTC integration manifest
579b4fc drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

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

Re: [PATCH] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

[Chris Wilson]

On Thu, May 05, 2016 at 03:06:49AM -0700, Kenneth Graunke wrote:
> Allowing register copies where the source and destination are both
> whitelisted should be safe, and is useful.  For example, Mesa uses
> this to load the command streamer math registers with data from the
> pipeline statistics counters.
> 
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

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

> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 69a1ba8..14f3b44 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -215,7 +215,8 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {

HWS RCS only. ✔

>  	CMD(  MI_RS_CONTEXT,                    SMI,    F,  1,      S  ),
>  	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
>  	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
> -	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   R  ),
> +	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   W,
> +	      .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 }    ),

Bits 22:2 are the address, 0:1 mbz. ✔

offset=1, length=3, step=1 => check both src/dst against whitelist ✔

>  	CMD(  MI_RS_STORE_DATA_IMM,             SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_LOAD_URB_MEM,                  SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_STORE_URB_MEM,                 SMI,   !F,  0xFF,   S  ),
> @@ -1113,6 +1114,12 @@ static bool check_cmd(const struct intel_engine_cs *engine,
>  					return false;
>  				}
>  
> +				if (desc->cmd.value == MI_LOAD_REGISTER_REG) {
> +					DRM_DEBUG_DRIVER("CMD: Rejected LRR to masked register 0x%08X\n",
> +							 reg_addr);
> +					return false;
> +				}

Since we can't inspect the value to see if meets the permitted mask. ✔

The only thing missing on the checklist would be an igt to load, copy,
then store that register back to memory to check this works.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH igt] igt/gem_exec_parse: Simple exercise for MI_LOAD_REGISTER_REG

[Chris Wilson]

Command parser version 7 introduces the ability to copy between
regsiters from the Haswell RCS with MI_LOAD_REGISTER_REG. This provides
a quick smoketest of that ability.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_parse.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c
index aa4ea67..deb93e6 100644
--- a/tests/gem_exec_parse.c
+++ b/tests/gem_exec_parse.c
@@ -317,6 +317,79 @@ int fd;
 
 #define OACONTROL 0x2360
 
+static int command_parser_version(void)
+{
+	int version = -1;
+	drm_i915_getparam_t gp;
+
+	gp.param = I915_PARAM_CMD_PARSER_VERSION;
+	gp.value = &version;
+
+	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == 0)
+		return version;
+
+	return -1;
+}
+
+#define HSW_CS_GPR(n) (0x2600 + 8*(n))
+#define HSW_CS_GPR0 HSW_CS_GPR(0)
+#define HSW_CS_GPR1 HSW_CS_GPR(1)
+
+#define MI_LOAD_REGISTER_REG (0x2a << 23)
+#define MI_STORE_REGISTER_MEM (0x24 << 23)
+static void hsw_load_register_reg(void)
+{
+	uint32_t buf[16] = {
+		MI_LOAD_REGISTER_IMM | (5 - 2),
+		HSW_CS_GPR0,
+		0xabcdabcd,
+		HSW_CS_GPR1,
+		0xdeadbeef,
+
+		MI_LOAD_REGISTER_REG | (3 - 2),
+		HSW_CS_GPR0,
+		HSW_CS_GPR1,
+
+		MI_STORE_REGISTER_MEM | (3 - 2),
+		HSW_CS_GPR1,
+		0, /* address */
+
+		MI_BATCH_BUFFER_END,
+	};
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+
+	igt_require(IS_HASWELL(intel_get_drm_devid(fd)));
+	igt_require(command_parser_version() >= 7);
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = gem_create(fd, 4096);
+	obj[1].handle = gem_create(fd, 4096);
+	gem_write(fd, obj[1].handle, 0, buf, sizeof(buf));
+
+	memset(&reloc, 0, sizeof(reloc));
+	reloc.offset = 10*sizeof(uint32_t);
+	reloc.target_handle = obj[0].handle;
+	reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	obj[1].relocs_ptr = (uintptr_t)&reloc;
+	obj[1].relocation_count = 1;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)obj;
+	execbuf.buffer_count = 2;
+	execbuf.batch_len = sizeof(buf);
+	execbuf.flags = I915_EXEC_RENDER;
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[1].handle);
+
+	gem_read(fd, obj[0].handle, 0, buf, sizeof(buf[0]));
+	gem_close(fd, obj[0].handle);
+
+	igt_assert_eq_u32(buf[0], 0xabcdabcd);
+}
+
 igt_main
 {
 	igt_fixture {
@@ -507,6 +580,9 @@ igt_main
 				   0x12000000);
 	}
 
+	igt_subtest("load-register-reg")
+		hsw_load_register_reg();
+
 	igt_fixture {
 		gem_close(fd, handle);
 
-- 
2.8.1

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

[PATCH igt v2] igt/gem_exec_parse: Simple exercise for MI_LOAD_REGISTER_REG

[Chris Wilson]

Command parser version 7 introduces the ability to copy between
regsiters from the Haswell RCS with MI_LOAD_REGISTER_REG. This provides
a quick smoketest of that ability.

v2: Add some negative tests as well

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_parse.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c
index aa4ea67..0bd8b8b 100644
--- a/tests/gem_exec_parse.c
+++ b/tests/gem_exec_parse.c
@@ -317,6 +317,90 @@ int fd;
 
 #define OACONTROL 0x2360
 
+static int command_parser_version(void)
+{
+	int version = -1;
+	drm_i915_getparam_t gp;
+
+	gp.param = I915_PARAM_CMD_PARSER_VERSION;
+	gp.value = &version;
+
+	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == 0)
+		return version;
+
+	return -1;
+}
+
+#define HSW_CS_GPR(n) (0x2600 + 8*(n))
+#define HSW_CS_GPR0 HSW_CS_GPR(0)
+#define HSW_CS_GPR1 HSW_CS_GPR(1)
+
+#define MI_LOAD_REGISTER_REG (0x2a << 23)
+#define MI_STORE_REGISTER_MEM (0x24 << 23)
+static void hsw_load_register_reg(void)
+{
+	uint32_t buf[16] = {
+		MI_LOAD_REGISTER_IMM | (5 - 2),
+		HSW_CS_GPR0,
+		0xabcdabcd,
+		HSW_CS_GPR1,
+		0xdeadbeef,
+
+		MI_STORE_REGISTER_MEM | (3 - 2),
+		HSW_CS_GPR1,
+		0, /* address0 */
+
+		MI_LOAD_REGISTER_REG | (3 - 2),
+		HSW_CS_GPR0,
+		HSW_CS_GPR1,
+
+		MI_STORE_REGISTER_MEM | (3 - 2),
+		HSW_CS_GPR1,
+		4, /* address1 */
+
+		MI_BATCH_BUFFER_END,
+	};
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc[2];
+
+	igt_require(IS_HASWELL(intel_get_drm_devid(fd)));
+	igt_require(command_parser_version() >= 7);
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = gem_create(fd, 4096);
+	obj[1].handle = gem_create(fd, 4096);
+	gem_write(fd, obj[1].handle, 0, buf, sizeof(buf));
+
+	memset(reloc, 0, sizeof(reloc));
+	reloc[0].offset = 7*sizeof(uint32_t);
+	reloc[0].target_handle = obj[0].handle;
+	reloc[0].delta = 0;
+	reloc[0].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc[0].write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc[1].offset = 13*sizeof(uint32_t);
+	reloc[1].target_handle = obj[0].handle;
+	reloc[1].delta = sizeof(uint32_t);
+	reloc[1].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc[1].write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	obj[1].relocs_ptr = (uintptr_t)&reloc;
+	obj[1].relocation_count = 2;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)obj;
+	execbuf.buffer_count = 2;
+	execbuf.batch_len = sizeof(buf);
+	execbuf.flags = I915_EXEC_RENDER;
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[1].handle);
+
+	gem_read(fd, obj[0].handle, 0, buf, 2*sizeof(buf[0]));
+	gem_close(fd, obj[0].handle);
+
+	igt_assert_eq_u32(buf[0], 0xdeadbeef); /* before copy */
+	igt_assert_eq_u32(buf[1], 0xabcdabcd); /* after copy */
+}
+
 igt_main
 {
 	igt_fixture {
@@ -507,6 +591,9 @@ igt_main
 				   0x12000000);
 	}
 
+	igt_subtest("load-register-reg")
+		hsw_load_register_reg();
+
 	igt_fixture {
 		gem_close(fd, handle);
 
-- 
2.8.1

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

[PATCH igt v3] igt/gem_exec_parse: Simple exercise for MI_LOAD_REGISTER_REG

[Chris Wilson]

Command parser version 7 introduces the ability to copy between
regsiters from the Haswell RCS with MI_LOAD_REGISTER_REG. This provides
a quick smoketest of that ability.

v2: Add some negative tests as well

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_parse.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 116 insertions(+), 3 deletions(-)

diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c
index aa4ea67..8fb9580 100644
--- a/tests/gem_exec_parse.c
+++ b/tests/gem_exec_parse.c
@@ -30,11 +30,123 @@
 
 #include <drm.h>
 
-
 #ifndef I915_PARAM_CMD_PARSER_VERSION
 #define I915_PARAM_CMD_PARSER_VERSION       28
 #endif
 
+#define OACONTROL 0x2360
+#define DERRMR 0x44050
+
+static int command_parser_version(int fd)
+{
+	int version = -1;
+	drm_i915_getparam_t gp;
+
+	gp.param = I915_PARAM_CMD_PARSER_VERSION;
+	gp.value = &version;
+
+	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == 0)
+		return version;
+
+	return -1;
+}
+
+#define HSW_CS_GPR(n) (0x2600 + 8*(n))
+#define HSW_CS_GPR0 HSW_CS_GPR(0)
+#define HSW_CS_GPR1 HSW_CS_GPR(1)
+
+#define MI_LOAD_REGISTER_REG (0x2a << 23)
+#define MI_STORE_REGISTER_MEM (0x24 << 23)
+static void hsw_load_register_reg(int fd)
+{
+	uint32_t buf[16] = {
+		MI_LOAD_REGISTER_IMM | (5 - 2),
+		HSW_CS_GPR0,
+		0xabcdabcd,
+		HSW_CS_GPR1,
+		0xdeadbeef,
+
+		MI_STORE_REGISTER_MEM | (3 - 2),
+		HSW_CS_GPR1,
+		0, /* address0 */
+
+		MI_LOAD_REGISTER_REG | (3 - 2),
+		HSW_CS_GPR0,
+		HSW_CS_GPR1,
+
+		MI_STORE_REGISTER_MEM | (3 - 2),
+		HSW_CS_GPR1,
+		4, /* address1 */
+
+		MI_BATCH_BUFFER_END,
+	};
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc[2];
+
+	igt_require(IS_HASWELL(intel_get_drm_devid(fd)));
+	igt_require(command_parser_version(fd) >= 7);
+
+	memset(obj, 0, sizeof(obj));
+	obj[0].handle = gem_create(fd, 4096);
+	obj[1].handle = gem_create(fd, 4096);
+	gem_write(fd, obj[1].handle, 0, buf, sizeof(buf));
+
+	memset(reloc, 0, sizeof(reloc));
+	reloc[0].offset = 7*sizeof(uint32_t);
+	reloc[0].target_handle = obj[0].handle;
+	reloc[0].delta = 0;
+	reloc[0].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc[0].write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc[1].offset = 13*sizeof(uint32_t);
+	reloc[1].target_handle = obj[0].handle;
+	reloc[1].delta = sizeof(uint32_t);
+	reloc[1].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc[1].write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	obj[1].relocs_ptr = (uintptr_t)&reloc;
+	obj[1].relocation_count = 2;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)obj;
+	execbuf.buffer_count = 2;
+	execbuf.batch_len = sizeof(buf);
+	execbuf.flags = I915_EXEC_RENDER;
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[1].handle);
+
+	gem_read(fd, obj[0].handle, 0, buf, 2*sizeof(buf[0]));
+	gem_close(fd, obj[0].handle);
+
+	igt_assert_eq_u32(buf[0], 0xdeadbeef); /* before copy */
+	igt_assert_eq_u32(buf[1], 0xabcdabcd); /* after copy */
+
+	/* Now a couple of negative tests that should be filtered */
+	obj[0].handle = gem_create(fd, 4096);
+	execbuf.buffer_count = 1;
+	execbuf.batch_len = 4*sizeof(buf[0]);
+
+	buf[0] = MI_LOAD_REGISTER_REG | (3 - 2);
+	buf[1] = HSW_CS_GPR0;
+	buf[2] = 0;
+	buf[3] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[0].handle, 0, buf, execbuf.batch_len);
+	igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
+
+	buf[2] = OACONTROL; /* filtered */
+	gem_write(fd, obj[0].handle, 0, buf, execbuf.batch_len);
+	igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
+
+	buf[2] = DERRMR; /* master only */
+	gem_write(fd, obj[0].handle, 0, buf, execbuf.batch_len);
+	igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
+
+	buf[2] = 0x2038; /* RING_START: invalid */
+	gem_write(fd, obj[0].handle, 0, buf, execbuf.batch_len);
+	igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
+
+	gem_close(fd, obj[0].handle);
+}
+
 static void exec_batch_patched(int fd, uint32_t cmd_bo, uint32_t *cmds,
 			       int size, int patch_offset, uint64_t expected_value)
 {
@@ -315,8 +427,6 @@ int fd;
 #define   PIPE_CONTROL_QW_WRITE	(1<<14)
 #define   PIPE_CONTROL_LRI_POST_OP (1<<23)
 
-#define OACONTROL 0x2360
-
 igt_main
 {
 	igt_fixture {
@@ -507,6 +617,9 @@ igt_main
 				   0x12000000);
 	}
 
+	igt_subtest("load-register-reg")
+		hsw_load_register_reg(fd);
+
 	igt_fixture {
 		gem_close(fd, handle);
 
-- 
2.8.1

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

Re: [PATCH igt v3] igt/gem_exec_parse: Simple exercise for MI_LOAD_REGISTER_REG

[Kenneth Graunke]

[-- Attachment #1.1: Type: text/plain, Size: 435 bytes --]

On Friday, May 6, 2016 8:53:56 AM PDT Chris Wilson wrote:
> Command parser version 7 introduces the ability to copy between
> regsiters from the Haswell RCS with MI_LOAD_REGISTER_REG. This provides
> a quick smoketest of that ability.
> 
> v2: Add some negative tests as well
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Much more thorough than my test.  Thanks :)

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH v2] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

[Chris Wilson]

From: Kenneth Graunke <kenneth@whitecape.org>

Allowing register copies where the source and destination are both
whitelisted should be safe, and is useful.  For example, Mesa uses
this to load the command streamer math registers with data from the
pipeline statistics counters.

v2: Reject writes to OACONTROL (and reads as well :(

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 69a1ba8ebdfb..c3a760375905 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -215,7 +215,8 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
 	CMD(  MI_RS_CONTEXT,                    SMI,    F,  1,      S  ),
 	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
 	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
-	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   R  ),
+	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   W,
+	      .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 }    ),
 	CMD(  MI_RS_STORE_DATA_IMM,             SMI,   !F,  0xFF,   S  ),
 	CMD(  MI_LOAD_URB_MEM,                  SMI,   !F,  0xFF,   S  ),
 	CMD(  MI_STORE_URB_MEM,                 SMI,   !F,  0xFF,   S  ),
@@ -1098,6 +1099,11 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 					return false;
 				}
 
+				if (desc->cmd.value == MI_LOAD_REGISTER_REG) {
+					DRM_DEBUG_DRIVER("CMD: Rejected LRR to OACONTROL\n");
+					return false;
+				}
+
 				if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
 					*oacontrol_set = (cmd[offset + 1] != 0);
 			}
@@ -1113,6 +1119,12 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 					return false;
 				}
 
+				if (desc->cmd.value == MI_LOAD_REGISTER_REG) {
+					DRM_DEBUG_DRIVER("CMD: Rejected LRR to masked register 0x%08X\n",
+							 reg_addr);
+					return false;
+				}
+
 				if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) &&
 				    (offset + 2 > length ||
 				     (cmd[offset + 1] & reg->mask) != reg->value)) {
@@ -1301,6 +1313,7 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv)
 	 * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3.
 	 * 5. GPGPU dispatch compute indirect registers.
 	 * 6. TIMESTAMP register and Haswell CS GPR registers
+	 * 7. Allow MI_LOAD_REGISTER_REG between whitelisted registers.
 	 */
-	return 6;
+	return 7;
 }
-- 
2.8.1

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

Re: [PATCH v2] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

[Kenneth Graunke]

[-- Attachment #1.1: Type: text/plain, Size: 812 bytes --]

On Friday, May 6, 2016 8:50:14 AM PDT Chris Wilson wrote:
> From: Kenneth Graunke <kenneth@whitecape.org>
> 
> Allowing register copies where the source and destination are both
> whitelisted should be safe, and is useful.  For example, Mesa uses
> this to load the command streamer math registers with data from the
> pipeline statistics counters.
> 
> v2: Reject writes to OACONTROL (and reads as well :(
> 
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

Oh, probably a good call to disallow OACONTROL.  Looks good to me.

Thanks for fixing this up, Chris :)

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

✗ Fi.CI.BAT: failure for drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers. (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers. (rev2)
URL   : https://patchwork.freedesktop.org/series/6774/
State : failure

== Summary ==

Series 6774v2 drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.
http://patchwork.freedesktop.org/api/1.0/series/6774/revisions/2/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                skip       -> FAIL       (bdw-nuci7-2)

bdw-nuci7-2      total:226  pass:26   dwarn:0   dfail:1   fail:51  skip:148
bsw-nuc-2        total:225  pass:22   dwarn:0   dfail:1   fail:60  skip:142
byt-nuc          total:225  pass:20   dwarn:0   dfail:1   fail:60  skip:144
hsw-brixbox      total:226  pass:22   dwarn:0   dfail:1   fail:60  skip:143
hsw-gt2          total:226  pass:27   dwarn:0   dfail:1   fail:60  skip:138
ivb-t430s        total:226  pass:24   dwarn:0   dfail:1   fail:60  skip:141
skl-i7k-2        total:226  pass:22   dwarn:0   dfail:1   fail:60  skip:143
skl-nuci5        total:226  pass:26   dwarn:0   dfail:1   fail:60  skip:139
snb-dellxps      total:226  pass:23   dwarn:0   dfail:1   fail:60  skip:142
snb-x220t        total:226  pass:23   dwarn:0   dfail:1   fail:60  skip:142
bdw-ultra failed to connect after reboot
ilk-hp8440p failed to connect after reboot

Results at /archive/results/CI_IGT_test/Patchwork_2144/

871c895c2d3192a93a701b31d2576184ab070c66 drm-intel-nightly: 2016y-05m-05d-23h-49m-21s UTC integration manifest
436f741 drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers. (rev2)

[Chris Wilson]

On Fri, May 06, 2016 at 08:13:22AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers. (rev2)
> URL   : https://patchwork.freedesktop.org/series/6774/
> State : failure
> 
> == Summary ==
> 
> Series 6774v2 drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.
> http://patchwork.freedesktop.org/api/1.0/series/6774/revisions/2/mbox/
> 
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 skip       -> FAIL       (bdw-nuci7-2)

Impact is minimal, so I've trusted local results and the new test cases.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx