subject:"\[Intel\-gfx\] \[PATCH igt\] tools\/intel_reg\: Add reading and writing registers through engine"

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-31 Thread Mika Kuoppala

Jani Nikula  writes:

> On Mon, 22 Jan 2018, Mika Kuoppala  wrote:
>> Jani Nikula  writes:
>>
>>> On Mon, 08 Jan 2018, Mika Kuoppala  wrote:
 Add option to specify engine for register read/write operation.
 If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
 to write and read register using a batch targeted at that engine.
>>>
>>> Copy-pasting from the man page, we already have the notation:
>>>
>>> "Registers are defined as 
>>> [(PORTNAME|PORTNUM|MMIO-OFFSET):](REGNAME|REGADDR)."
>>>
>>> Why don't we add this as ENGINE:REGNAME or something instead of an extra
>>> --engine parameter? As a "port". Sure, it's more work, but I really like
>>> the current possibility of reading all types of registers at once. Now
>>> you prevent dumps that would contain both mmio and batch based reads.
>>>
>>
>> Are you ok with the latest version? As discussed in irc, there are
>> problems with trying to add engines as ports, due to dynamic nature
>> and also that the existing infra relies on PORTNAME being mmio
>> for symbolic register dumping to work correctly.
>>
>> for the wart of if (reg->engine) in read/write paths
>> we gain the benefits of mmio dumps with engines.
>
> Can't say I would've reviewed all the engine bits and pieces, but
>
> Acked-by: Jani Nikula 

Thanks for ack and review. Pushed.
-Mika
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-31 Thread Jani Nikula

On Mon, 22 Jan 2018, Mika Kuoppala  wrote:
> Jani Nikula  writes:
>
>> On Mon, 08 Jan 2018, Mika Kuoppala  wrote:
>>> Add option to specify engine for register read/write operation.
>>> If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
>>> to write and read register using a batch targeted at that engine.
>>
>> Copy-pasting from the man page, we already have the notation:
>>
>> "Registers are defined as 
>> [(PORTNAME|PORTNUM|MMIO-OFFSET):](REGNAME|REGADDR)."
>>
>> Why don't we add this as ENGINE:REGNAME or something instead of an extra
>> --engine parameter? As a "port". Sure, it's more work, but I really like
>> the current possibility of reading all types of registers at once. Now
>> you prevent dumps that would contain both mmio and batch based reads.
>>
>
> Are you ok with the latest version? As discussed in irc, there are
> problems with trying to add engines as ports, due to dynamic nature
> and also that the existing infra relies on PORTNAME being mmio
> for symbolic register dumping to work correctly.
>
> for the wart of if (reg->engine) in read/write paths
> we gain the benefits of mmio dumps with engines.

Can't say I would've reviewed all the engine bits and pieces, but

Acked-by: Jani Nikula 


>
> -Mika
>
>> BR,
>> Jani.
>>
>>
>>>
>>> v2: no MI_NOOP after BBE (Chris)
>>> v3: use modern engine names (Chris), use global fd
>>>
>>> Cc: Jani Nikula 
>>> Cc: Chris Wilson 
>>> CC: Joonas Lahtinen 
>>> Signed-off-by: Mika Kuoppala 
>>> ---
>>>  tools/intel_reg.c | 156 
>>> +-
>>>  1 file changed, 154 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
>>> index 00d2a4a1..7f3494ef 100644
>>> --- a/tools/intel_reg.c
>>> +++ b/tools/intel_reg.c
>>> @@ -33,6 +33,7 @@
>>>  #include 
>>>  
>>>  #include "igt.h"
>>> +#include "igt_gt.h"
>>>  #include "intel_io.h"
>>>  #include "intel_chipset.h"
>>>  
>>> @@ -73,6 +74,11 @@ struct config {
>>>  
>>> /* register spec */
>>> char *specfile;
>>> +
>>> +   /* engine to use for lri (write) and srm (read) */
>>> +   char *engine;
>>> +   int fd;
>>> +
>>> struct reg *regs;
>>> ssize_t regcount;
>>>  
>>> @@ -236,13 +242,140 @@ static void dump_decode(struct config *config, 
>>> struct reg *reg, uint32_t val)
>>> }
>>>  }
>>>  
>>> +static const struct intel_execution_engine2 *find_engine(const char *name,
>>> +bool *secure)
>>> +{
>>> +   const struct intel_execution_engine2 *e;
>>> +
>>> +   if (strlen(name) < 2)
>>> +   goto out;
>>> +
>>> +   if (name[0] == '-') {
>>> +   *secure = false;
>>> +   name++;
>>> +   } else {
>>> +   *secure = true;
>>> +   }
>>> +
>>> +   for (e = intel_execution_engines2; e->name; e++) {
>>> +   if (!strcmp(e->name, name))
>>> +   return e;
>>> +   }
>>> +
>>> +out:
>>> +   fprintf(stderr, "no such engine as '%s'\n", name);
>>> +
>>> +   fprintf(stderr, "valid engines:");
>>> +   for (e = intel_execution_engines2; e->name; e++)
>>> +   fprintf(stderr, " %s", e->name);
>>> +
>>> +   fprintf(stderr, "\n");
>>> +
>>> +   exit(EXIT_FAILURE);
>>> +}
>>> +
>>> +static int register_srm(struct config *config, struct reg *reg,
>>> +   uint32_t *val_in)
>>> +{
>>> +   const int gen = intel_gen(config->devid);
>>> +   const bool r64b = gen >= 8;
>>> +   const uint32_t ctx = 0;
>>> +   struct drm_i915_gem_exec_object2 obj[2];
>>> +   struct drm_i915_gem_relocation_entry reloc[1];
>>> +   struct drm_i915_gem_execbuffer2 execbuf;
>>> +   uint32_t *batch, *r;
>>> +   const struct intel_execution_engine2 *engine;
>>> +   bool secure;
>>> +   int fd, i;
>>> +   uint32_t val;
>>> +
>>> +   if (config->fd == -1) {
>>> +   config->fd = __drm_open_driver(DRIVER_INTEL);
>>> +   if (config->fd == -1) {
>>> +   fprintf(stderr, "Error opening driver: %s",
>>> +   strerror(errno));
>>> +   exit(EXIT_FAILURE);
>>> +   }
>>> +   }
>>> +
>>> +   fd = config->fd;
>>> +   engine = find_engine(config->engine, );
>>> +
>>> +   memset(obj, 0, sizeof(obj));
>>> +   obj[0].handle = gem_create(fd, 4096);
>>> +   obj[1].handle = gem_create(fd, 4096);
>>> +   obj[1].relocs_ptr = to_user_pointer(reloc);
>>> +   obj[1].relocation_count = 1;
>>> +
>>> +   batch = gem_mmap__cpu(fd, obj[1].handle, 0, 4096, PROT_WRITE);
>>> +   gem_set_domain(fd, obj[1].handle,
>>> +  I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>>> +
>>> +   i = 0;
>>> +   if (val_in) {
>>> +   batch[i++] = MI_NOOP;
>>> +   batch[i++] = MI_NOOP;
>>> +
>>> +   batch[i++] = MI_LOAD_REGISTER_IMM;
>>> +

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-22 Thread Mika Kuoppala

Jani Nikula  writes:

> On Mon, 08 Jan 2018, Mika Kuoppala  wrote:
>> Add option to specify engine for register read/write operation.
>> If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
>> to write and read register using a batch targeted at that engine.
>
> Copy-pasting from the man page, we already have the notation:
>
> "Registers are defined as [(PORTNAME|PORTNUM|MMIO-OFFSET):](REGNAME|REGADDR)."
>
> Why don't we add this as ENGINE:REGNAME or something instead of an extra
> --engine parameter? As a "port". Sure, it's more work, but I really like
> the current possibility of reading all types of registers at once. Now
> you prevent dumps that would contain both mmio and batch based reads.
>

Are you ok with the latest version? As discussed in irc, there are
problems with trying to add engines as ports, due to dynamic nature
and also that the existing infra relies on PORTNAME being mmio
for symbolic register dumping to work correctly.

for the wart of if (reg->engine) in read/write paths
we gain the benefits of mmio dumps with engines.

-Mika

> BR,
> Jani.
>
>
>>
>> v2: no MI_NOOP after BBE (Chris)
>> v3: use modern engine names (Chris), use global fd
>>
>> Cc: Jani Nikula 
>> Cc: Chris Wilson 
>> CC: Joonas Lahtinen 
>> Signed-off-by: Mika Kuoppala 
>> ---
>>  tools/intel_reg.c | 156 
>> +-
>>  1 file changed, 154 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
>> index 00d2a4a1..7f3494ef 100644
>> --- a/tools/intel_reg.c
>> +++ b/tools/intel_reg.c
>> @@ -33,6 +33,7 @@
>>  #include 
>>  
>>  #include "igt.h"
>> +#include "igt_gt.h"
>>  #include "intel_io.h"
>>  #include "intel_chipset.h"
>>  
>> @@ -73,6 +74,11 @@ struct config {
>>  
>>  /* register spec */
>>  char *specfile;
>> +
>> +/* engine to use for lri (write) and srm (read) */
>> +char *engine;
>> +int fd;
>> +
>>  struct reg *regs;
>>  ssize_t regcount;
>>  
>> @@ -236,13 +242,140 @@ static void dump_decode(struct config *config, struct 
>> reg *reg, uint32_t val)
>>  }
>>  }
>>  
>> +static const struct intel_execution_engine2 *find_engine(const char *name,
>> + bool *secure)
>> +{
>> +const struct intel_execution_engine2 *e;
>> +
>> +if (strlen(name) < 2)
>> +goto out;
>> +
>> +if (name[0] == '-') {
>> +*secure = false;
>> +name++;
>> +} else {
>> +*secure = true;
>> +}
>> +
>> +for (e = intel_execution_engines2; e->name; e++) {
>> +if (!strcmp(e->name, name))
>> +return e;
>> +}
>> +
>> +out:
>> +fprintf(stderr, "no such engine as '%s'\n", name);
>> +
>> +fprintf(stderr, "valid engines:");
>> +for (e = intel_execution_engines2; e->name; e++)
>> +fprintf(stderr, " %s", e->name);
>> +
>> +fprintf(stderr, "\n");
>> +
>> +exit(EXIT_FAILURE);
>> +}
>> +
>> +static int register_srm(struct config *config, struct reg *reg,
>> +uint32_t *val_in)
>> +{
>> +const int gen = intel_gen(config->devid);
>> +const bool r64b = gen >= 8;
>> +const uint32_t ctx = 0;
>> +struct drm_i915_gem_exec_object2 obj[2];
>> +struct drm_i915_gem_relocation_entry reloc[1];
>> +struct drm_i915_gem_execbuffer2 execbuf;
>> +uint32_t *batch, *r;
>> +const struct intel_execution_engine2 *engine;
>> +bool secure;
>> +int fd, i;
>> +uint32_t val;
>> +
>> +if (config->fd == -1) {
>> +config->fd = __drm_open_driver(DRIVER_INTEL);
>> +if (config->fd == -1) {
>> +fprintf(stderr, "Error opening driver: %s",
>> +strerror(errno));
>> +exit(EXIT_FAILURE);
>> +}
>> +}
>> +
>> +fd = config->fd;
>> +engine = find_engine(config->engine, );
>> +
>> +memset(obj, 0, sizeof(obj));
>> +obj[0].handle = gem_create(fd, 4096);
>> +obj[1].handle = gem_create(fd, 4096);
>> +obj[1].relocs_ptr = to_user_pointer(reloc);
>> +obj[1].relocation_count = 1;
>> +
>> +batch = gem_mmap__cpu(fd, obj[1].handle, 0, 4096, PROT_WRITE);
>> +gem_set_domain(fd, obj[1].handle,
>> +   I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>> +
>> +i = 0;
>> +if (val_in) {
>> +batch[i++] = MI_NOOP;
>> +batch[i++] = MI_NOOP;
>> +
>> +batch[i++] = MI_LOAD_REGISTER_IMM;
>> +batch[i++] = reg->addr;
>> +batch[i++] = *val_in;
>> +batch[i++] = MI_NOOP;
>> +}
>> +
>> +batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */
>> +batch[i++] = reg->addr;
>> +reloc[0].target_handle = obj[0].handle;
>> +reloc[0].presumed_offset =

[Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-10 Thread Mika Kuoppala

Add option to specify engine for register read/write operation.
If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
to write and read register using a batch targeted at that engine.

v2: no MI_NOOP after BBE (Chris)
v3: use modern engine names (Chris), use global fd
v4: strcasecmp (Chris)
v5: use register definition format for engine (Jani)

Cc: Jani Nikula 
Cc: Chris Wilson 
CC: Joonas Lahtinen 
Signed-off-by: Mika Kuoppala 
Reviewed-by: Chris Wilson  (v4)
---
 man/intel_reg.rst  |   9 ++-
 tools/intel_reg.c  | 169 +++--
 tools/intel_reg_spec.h |   1 +
 3 files changed, 171 insertions(+), 8 deletions(-)

diff --git a/man/intel_reg.rst b/man/intel_reg.rst
index d1af178a..4ec24c4c 100644
--- a/man/intel_reg.rst
+++ b/man/intel_reg.rst
@@ -103,7 +103,7 @@ Display brief help.
 REGISTER REFERENCES
 ===
 
-Registers are defined as [(PORTNAME|PORTNUM|MMIO-OFFSET):](REGNAME|REGADDR).
+Registers are defined as 
[(PORTNAME|PORTNUM|ENGINE|MMIO-OFFSET):](REGNAME|REGADDR).
 
 PORTNAME
 The register access method, most often MMIO, which is the default. The
@@ -120,6 +120,13 @@ PORTNUM
 Numbers above 0xff are automatically interpreted as MMIO offsets, not port
 numbers.
 
+ENGINE
+Instead of cpu based MMIO, specified engine can be used for access method.
+Batchbuffer will be targeted for the engine to do read/write. The list of
+available engines is architecture specific and can be found with
+"intel_reg help". Prefixing engine name with '-' uses non-privileged
+batchbuffer for access.
+
 MMIO-OFFSET
 Use MMIO, and add this offset to the register address.
 
diff --git a/tools/intel_reg.c b/tools/intel_reg.c
index 00d2a4a1..ddff2794 100644
--- a/tools/intel_reg.c
+++ b/tools/intel_reg.c
@@ -33,6 +33,7 @@
 #include 
 
 #include "igt.h"
+#include "igt_gt.h"
 #include "intel_io.h"
 #include "intel_chipset.h"
 
@@ -73,6 +74,10 @@ struct config {
 
/* register spec */
char *specfile;
+
+   /* fd for engine access avoiding reopens */
+   int fd;
+
struct reg *regs;
ssize_t regcount;
 
@@ -236,13 +241,130 @@ static void dump_decode(struct config *config, struct 
reg *reg, uint32_t val)
}
 }
 
+static const struct intel_execution_engine2 *find_engine(const char *name)
+{
+   const struct intel_execution_engine2 *e;
+
+   if (strlen(name) < 2)
+   return NULL;
+
+   if (name[0] == '-')
+   name++;
+
+   for (e = intel_execution_engines2; e->name; e++) {
+   if (!strcasecmp(e->name, name))
+   return e;
+   }
+
+   return NULL;
+}
+
+static int register_srm(struct config *config, struct reg *reg,
+   uint32_t *val_in)
+{
+   const int gen = intel_gen(config->devid);
+   const bool r64b = gen >= 8;
+   const uint32_t ctx = 0;
+   struct drm_i915_gem_exec_object2 obj[2];
+   struct drm_i915_gem_relocation_entry reloc[1];
+   struct drm_i915_gem_execbuffer2 execbuf;
+   uint32_t *batch, *r;
+   const struct intel_execution_engine2 *engine;
+   bool secure;
+   int fd, i;
+   uint32_t val;
+
+   if (config->fd == -1) {
+   config->fd = __drm_open_driver(DRIVER_INTEL);
+   if (config->fd == -1) {
+   fprintf(stderr, "Error opening driver: %s",
+   strerror(errno));
+   exit(EXIT_FAILURE);
+   }
+   }
+
+   fd = config->fd;
+   engine = find_engine(reg->engine);
+   if (engine == NULL)
+   exit(EXIT_FAILURE);
+
+   secure = reg->engine[0] != '-';
+
+   memset(obj, 0, sizeof(obj));
+   obj[0].handle = gem_create(fd, 4096);
+   obj[1].handle = gem_create(fd, 4096);
+   obj[1].relocs_ptr = to_user_pointer(reloc);
+   obj[1].relocation_count = 1;
+
+   batch = gem_mmap__cpu(fd, obj[1].handle, 0, 4096, PROT_WRITE);
+   gem_set_domain(fd, obj[1].handle,
+  I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+
+   i = 0;
+   if (val_in) {
+   batch[i++] = MI_NOOP;
+   batch[i++] = MI_NOOP;
+
+   batch[i++] = MI_LOAD_REGISTER_IMM;
+   batch[i++] = reg->addr;
+   batch[i++] = *val_in;
+   batch[i++] = MI_NOOP;
+   }
+
+   batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */
+   batch[i++] = reg->addr;
+   reloc[0].target_handle = obj[0].handle;
+   reloc[0].presumed_offset = obj[0].offset;
+   reloc[0].offset = i * sizeof(uint32_t);
+   reloc[0].delta = 0;
+   reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
+   reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
+   batch[i++] = reloc[0].delta;
+   if

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-08 Thread Jani Nikula

On Mon, 08 Jan 2018, Mika Kuoppala  wrote:
> Add option to specify engine for register read/write operation.
> If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
> to write and read register using a batch targeted at that engine.

Copy-pasting from the man page, we already have the notation:

"Registers are defined as [(PORTNAME|PORTNUM|MMIO-OFFSET):](REGNAME|REGADDR)."

Why don't we add this as ENGINE:REGNAME or something instead of an extra
--engine parameter? As a "port". Sure, it's more work, but I really like
the current possibility of reading all types of registers at once. Now
you prevent dumps that would contain both mmio and batch based reads.

BR,
Jani.


>
> v2: no MI_NOOP after BBE (Chris)
> v3: use modern engine names (Chris), use global fd
>
> Cc: Jani Nikula 
> Cc: Chris Wilson 
> CC: Joonas Lahtinen 
> Signed-off-by: Mika Kuoppala 
> ---
>  tools/intel_reg.c | 156 
> +-
>  1 file changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 00d2a4a1..7f3494ef 100644
> --- a/tools/intel_reg.c
> +++ b/tools/intel_reg.c
> @@ -33,6 +33,7 @@
>  #include 
>  
>  #include "igt.h"
> +#include "igt_gt.h"
>  #include "intel_io.h"
>  #include "intel_chipset.h"
>  
> @@ -73,6 +74,11 @@ struct config {
>  
>   /* register spec */
>   char *specfile;
> +
> + /* engine to use for lri (write) and srm (read) */
> + char *engine;
> + int fd;
> +
>   struct reg *regs;
>   ssize_t regcount;
>  
> @@ -236,13 +242,140 @@ static void dump_decode(struct config *config, struct 
> reg *reg, uint32_t val)
>   }
>  }
>  
> +static const struct intel_execution_engine2 *find_engine(const char *name,
> +  bool *secure)
> +{
> + const struct intel_execution_engine2 *e;
> +
> + if (strlen(name) < 2)
> + goto out;
> +
> + if (name[0] == '-') {
> + *secure = false;
> + name++;
> + } else {
> + *secure = true;
> + }
> +
> + for (e = intel_execution_engines2; e->name; e++) {
> + if (!strcmp(e->name, name))
> + return e;
> + }
> +
> +out:
> + fprintf(stderr, "no such engine as '%s'\n", name);
> +
> + fprintf(stderr, "valid engines:");
> + for (e = intel_execution_engines2; e->name; e++)
> + fprintf(stderr, " %s", e->name);
> +
> + fprintf(stderr, "\n");
> +
> + exit(EXIT_FAILURE);
> +}
> +
> +static int register_srm(struct config *config, struct reg *reg,
> + uint32_t *val_in)
> +{
> + const int gen = intel_gen(config->devid);
> + const bool r64b = gen >= 8;
> + const uint32_t ctx = 0;
> + struct drm_i915_gem_exec_object2 obj[2];
> + struct drm_i915_gem_relocation_entry reloc[1];
> + struct drm_i915_gem_execbuffer2 execbuf;
> + uint32_t *batch, *r;
> + const struct intel_execution_engine2 *engine;
> + bool secure;
> + int fd, i;
> + uint32_t val;
> +
> + if (config->fd == -1) {
> + config->fd = __drm_open_driver(DRIVER_INTEL);
> + if (config->fd == -1) {
> + fprintf(stderr, "Error opening driver: %s",
> + strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> + }
> +
> + fd = config->fd;
> + engine = find_engine(config->engine, );
> +
> + memset(obj, 0, sizeof(obj));
> + obj[0].handle = gem_create(fd, 4096);
> + obj[1].handle = gem_create(fd, 4096);
> + obj[1].relocs_ptr = to_user_pointer(reloc);
> + obj[1].relocation_count = 1;
> +
> + batch = gem_mmap__cpu(fd, obj[1].handle, 0, 4096, PROT_WRITE);
> + gem_set_domain(fd, obj[1].handle,
> +I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> +
> + i = 0;
> + if (val_in) {
> + batch[i++] = MI_NOOP;
> + batch[i++] = MI_NOOP;
> +
> + batch[i++] = MI_LOAD_REGISTER_IMM;
> + batch[i++] = reg->addr;
> + batch[i++] = *val_in;
> + batch[i++] = MI_NOOP;
> + }
> +
> + batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */
> + batch[i++] = reg->addr;
> + reloc[0].target_handle = obj[0].handle;
> + reloc[0].presumed_offset = obj[0].offset;
> + reloc[0].offset = i * sizeof(uint32_t);
> + reloc[0].delta = 0;
> + reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> + reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> + batch[i++] = reloc[0].delta;
> + if (r64b)
> + batch[i++] = 0;
> +
> + batch[i++] = MI_BATCH_BUFFER_END;
> + munmap(batch, 4096);
> +
> + memset(, 0, sizeof(execbuf));
> + execbuf.buffers_ptr = to_user_pointer(obj);
> + execbuf.buffer_count =

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-08 Thread Chris Wilson

Quoting Mika Kuoppala (2018-01-08 14:12:02)
> Add option to specify engine for register read/write operation.
> If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
> to write and read register using a batch targeted at that engine.
> 
> v2: no MI_NOOP after BBE (Chris)
> v3: use modern engine names (Chris), use global fd
> 
> Cc: Jani Nikula 
> Cc: Chris Wilson 
> CC: Joonas Lahtinen 
> Signed-off-by: Mika Kuoppala 
> ---
> +static const struct intel_execution_engine2 *find_engine(const char *name,
> +bool *secure)
> +{
> +   const struct intel_execution_engine2 *e;
> +
> +   if (strlen(name) < 2)
> +   goto out;
> +
> +   if (name[0] == '-') {
> +   *secure = false;
> +   name++;
> +   } else {
> +   *secure = true;
> +   }
> +
> +   for (e = intel_execution_engines2; e->name; e++) {
> +   if (!strcmp(e->name, name))

strcasecmp() just for ease of use.

Lgtm, 
Reviewed-by: Chris Wilson 
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2018-01-08 Thread Mika Kuoppala

Add option to specify engine for register read/write operation.
If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
to write and read register using a batch targeted at that engine.

v2: no MI_NOOP after BBE (Chris)
v3: use modern engine names (Chris), use global fd

Cc: Jani Nikula 
Cc: Chris Wilson 
CC: Joonas Lahtinen 
Signed-off-by: Mika Kuoppala 
---
 tools/intel_reg.c | 156 +-
 1 file changed, 154 insertions(+), 2 deletions(-)

diff --git a/tools/intel_reg.c b/tools/intel_reg.c
index 00d2a4a1..7f3494ef 100644
--- a/tools/intel_reg.c
+++ b/tools/intel_reg.c
@@ -33,6 +33,7 @@
 #include 
 
 #include "igt.h"
+#include "igt_gt.h"
 #include "intel_io.h"
 #include "intel_chipset.h"
 
@@ -73,6 +74,11 @@ struct config {
 
/* register spec */
char *specfile;
+
+   /* engine to use for lri (write) and srm (read) */
+   char *engine;
+   int fd;
+
struct reg *regs;
ssize_t regcount;
 
@@ -236,13 +242,140 @@ static void dump_decode(struct config *config, struct 
reg *reg, uint32_t val)
}
 }
 
+static const struct intel_execution_engine2 *find_engine(const char *name,
+bool *secure)
+{
+   const struct intel_execution_engine2 *e;
+
+   if (strlen(name) < 2)
+   goto out;
+
+   if (name[0] == '-') {
+   *secure = false;
+   name++;
+   } else {
+   *secure = true;
+   }
+
+   for (e = intel_execution_engines2; e->name; e++) {
+   if (!strcmp(e->name, name))
+   return e;
+   }
+
+out:
+   fprintf(stderr, "no such engine as '%s'\n", name);
+
+   fprintf(stderr, "valid engines:");
+   for (e = intel_execution_engines2; e->name; e++)
+   fprintf(stderr, " %s", e->name);
+
+   fprintf(stderr, "\n");
+
+   exit(EXIT_FAILURE);
+}
+
+static int register_srm(struct config *config, struct reg *reg,
+   uint32_t *val_in)
+{
+   const int gen = intel_gen(config->devid);
+   const bool r64b = gen >= 8;
+   const uint32_t ctx = 0;
+   struct drm_i915_gem_exec_object2 obj[2];
+   struct drm_i915_gem_relocation_entry reloc[1];
+   struct drm_i915_gem_execbuffer2 execbuf;
+   uint32_t *batch, *r;
+   const struct intel_execution_engine2 *engine;
+   bool secure;
+   int fd, i;
+   uint32_t val;
+
+   if (config->fd == -1) {
+   config->fd = __drm_open_driver(DRIVER_INTEL);
+   if (config->fd == -1) {
+   fprintf(stderr, "Error opening driver: %s",
+   strerror(errno));
+   exit(EXIT_FAILURE);
+   }
+   }
+
+   fd = config->fd;
+   engine = find_engine(config->engine, );
+
+   memset(obj, 0, sizeof(obj));
+   obj[0].handle = gem_create(fd, 4096);
+   obj[1].handle = gem_create(fd, 4096);
+   obj[1].relocs_ptr = to_user_pointer(reloc);
+   obj[1].relocation_count = 1;
+
+   batch = gem_mmap__cpu(fd, obj[1].handle, 0, 4096, PROT_WRITE);
+   gem_set_domain(fd, obj[1].handle,
+  I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+
+   i = 0;
+   if (val_in) {
+   batch[i++] = MI_NOOP;
+   batch[i++] = MI_NOOP;
+
+   batch[i++] = MI_LOAD_REGISTER_IMM;
+   batch[i++] = reg->addr;
+   batch[i++] = *val_in;
+   batch[i++] = MI_NOOP;
+   }
+
+   batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */
+   batch[i++] = reg->addr;
+   reloc[0].target_handle = obj[0].handle;
+   reloc[0].presumed_offset = obj[0].offset;
+   reloc[0].offset = i * sizeof(uint32_t);
+   reloc[0].delta = 0;
+   reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
+   reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
+   batch[i++] = reloc[0].delta;
+   if (r64b)
+   batch[i++] = 0;
+
+   batch[i++] = MI_BATCH_BUFFER_END;
+   munmap(batch, 4096);
+
+   memset(, 0, sizeof(execbuf));
+   execbuf.buffers_ptr = to_user_pointer(obj);
+   execbuf.buffer_count = 2;
+   execbuf.flags = gem_class_instance_to_eb_flags(fd,
+  engine->class,
+  engine->instance);
+   if (secure)
+   execbuf.flags |= I915_EXEC_SECURE;
+
+   if (config->verbosity > 0)
+   printf("%s: using %sprivileged batch\n",
+  engine->name,
+  secure ? "" : "non-");
+
+   execbuf.rsvd1 = ctx;
+   gem_execbuf(fd, );
+   gem_close(fd, obj[1].handle);
+
+   r = gem_mmap__cpu(fd, obj[0].handle, 0, 4096, PROT_READ);
+

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2017-12-01 Thread Chris Wilson

Quoting Mika Kuoppala (2017-12-01 14:16:39)
> Add option to specify engine for register read/write operation.
> If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
> to write and read register using a batch targeted at that engine.
> 
> v2: no MI_NOOP after BBE (Chris)
> 
> Cc: Jani Nikula 
> Cc: Chris Wilson 
> CC: Joonas Lahtinen 
> Signed-off-by: Mika Kuoppala 
> ---
>  tools/intel_reg.c | 139 
> +-
>  1 file changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 00d2a4a1..ec45c2c9 100644
> --- a/tools/intel_reg.c
> +++ b/tools/intel_reg.c
> @@ -33,6 +33,7 @@
>  #include 
>  
>  #include "igt.h"
> +#include "igt_gt.h"
>  #include "intel_io.h"
>  #include "intel_chipset.h"
>  
> @@ -73,6 +74,12 @@ struct config {
>  
> /* register spec */
> char *specfile;
> +
> +   /* engine to use for lri (write) and srm (read) */
> +   char *engine;
> +   /* use secure batch */
> +   bool engine_secure_batch;
> +
> struct reg *regs;
> ssize_t regcount;
>  
> @@ -236,13 +243,116 @@ static void dump_decode(struct config *config, struct 
> reg *reg, uint32_t val)
> }
>  }
>  
> +static const struct intel_execution_engine *find_engine(const char *name)
> +{

Being modern, we should be using the "$class$instance" pattern (e.g. vcs1).

> +   const struct intel_execution_engine *e;
> +
> +   for (e = intel_execution_engines; e->name; e++) {
> +   if (!strcmp(e->name, name))
> +   return e;
> +   }
> +
> +   fprintf(stderr, "no such engine as '%s'\n", name);
> +
> +   fprintf(stderr, "valid engines:");
> +   for (e = intel_execution_engines; e->name; e++)
> +   fprintf(stderr, " %s", e->name);
> +
> +   fprintf(stderr, "\n");
> +
> +   exit(EXIT_FAILURE);
> +}
> +
> +static int register_srm(struct config *config, struct reg *reg,
> +   uint32_t *val_in)
> +{
> +   const int gen = intel_gen(intel_get_drm_devid(config->drm_fd));
> +   const bool r64b = gen >= 8;
> +   const uint32_t ctx = 0;
> +   struct drm_i915_gem_exec_object2 obj[2];
> +   struct drm_i915_gem_relocation_entry reloc[1];
> +   struct drm_i915_gem_execbuffer2 execbuf;
> +   uint32_t *batch, *r;
> +   const struct intel_execution_engine *engine;
> +   int i915, i;
> +   uint32_t val;
> +   i915 = config->drm_fd;
> +
> +   engine = find_engine(config->engine);
> +
> +   memset(obj, 0, sizeof(obj));
> +   obj[0].handle = gem_create(i915, 4096);
> +   obj[1].handle = gem_create(i915, 4096);
> +   obj[1].relocs_ptr = to_user_pointer(reloc);
> +   obj[1].relocation_count = 1;
> +
> +   batch = gem_mmap__cpu(i915, obj[1].handle, 0, 4096, PROT_WRITE);
> +   gem_set_domain(i915, obj[1].handle,
> +  I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> +
> +   i = 0;
> +   if (val_in) {
> +   batch[i++] = MI_NOOP;
> +   batch[i++] = MI_NOOP;
> +
> +   batch[i++] = MI_LOAD_REGISTER_IMM;
> +   batch[i++] = reg->addr;
> +   batch[i++] = *val_in;
> +   batch[i++] = MI_NOOP;
> +   }
> +
> +   batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */
> +   batch[i++] = reg->addr;
> +   reloc[0].target_handle = obj[0].handle;
> +   reloc[0].presumed_offset = obj[0].offset;
> +   reloc[0].offset = i * sizeof(uint32_t);
> +   reloc[0].delta = 0;
> +   reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> +   reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> +   batch[i++] = reloc[0].delta;
> +   if (r64b)
> +   batch[i++] = 0;
> +
> +   batch[i++] = MI_BATCH_BUFFER_END;
> +   munmap(batch, 4096);
> +
> +   memset(, 0, sizeof(execbuf));
> +   execbuf.buffers_ptr = to_user_pointer(obj);
> +   execbuf.buffer_count = 2;
> +   execbuf.flags = engine->exec_id;
> +
> +   if (config->engine_secure_batch) {
> +   execbuf.flags |= I915_EXEC_SECURE;
> +
> +   if (config->verbosity > 0)
> +   printf("%s: using priviledged (secure) batch\n",
privileged
Scrap saying (secure) to the users. It's blatantly not at this point if
we allow any old (root) user to write any old register.

> +  engine->name);
> +   }
> +
> +   execbuf.rsvd1 = ctx;
> +   gem_execbuf(i915, );
> +   gem_close(i915, obj[1].handle);
> +
> +   r = gem_mmap__cpu(i915, obj[0].handle, 0, 4096, PROT_READ);
> +   gem_set_domain(i915, obj[0].handle, I915_GEM_DOMAIN_CPU, 0);
> +
> +   val = r[0];
> +
> +   gem_close(i915, obj[0].handle);
> +
> +   return val;
> +}
> +
>  static int read_register(struct

[Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

2017-12-01 Thread Mika Kuoppala

Add option to specify engine for register read/write operation.
If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM
to write and read register using a batch targeted at that engine.

v2: no MI_NOOP after BBE (Chris)

Cc: Jani Nikula 
Cc: Chris Wilson 
CC: Joonas Lahtinen 
Signed-off-by: Mika Kuoppala 
---
 tools/intel_reg.c | 139 +-
 1 file changed, 137 insertions(+), 2 deletions(-)

diff --git a/tools/intel_reg.c b/tools/intel_reg.c
index 00d2a4a1..ec45c2c9 100644
--- a/tools/intel_reg.c
+++ b/tools/intel_reg.c
@@ -33,6 +33,7 @@
 #include 
 
 #include "igt.h"
+#include "igt_gt.h"
 #include "intel_io.h"
 #include "intel_chipset.h"
 
@@ -73,6 +74,12 @@ struct config {
 
/* register spec */
char *specfile;
+
+   /* engine to use for lri (write) and srm (read) */
+   char *engine;
+   /* use secure batch */
+   bool engine_secure_batch;
+
struct reg *regs;
ssize_t regcount;
 
@@ -236,13 +243,116 @@ static void dump_decode(struct config *config, struct 
reg *reg, uint32_t val)
}
 }
 
+static const struct intel_execution_engine *find_engine(const char *name)
+{
+   const struct intel_execution_engine *e;
+
+   for (e = intel_execution_engines; e->name; e++) {
+   if (!strcmp(e->name, name))
+   return e;
+   }
+
+   fprintf(stderr, "no such engine as '%s'\n", name);
+
+   fprintf(stderr, "valid engines:");
+   for (e = intel_execution_engines; e->name; e++)
+   fprintf(stderr, " %s", e->name);
+
+   fprintf(stderr, "\n");
+
+   exit(EXIT_FAILURE);
+}
+
+static int register_srm(struct config *config, struct reg *reg,
+   uint32_t *val_in)
+{
+   const int gen = intel_gen(intel_get_drm_devid(config->drm_fd));
+   const bool r64b = gen >= 8;
+   const uint32_t ctx = 0;
+   struct drm_i915_gem_exec_object2 obj[2];
+   struct drm_i915_gem_relocation_entry reloc[1];
+   struct drm_i915_gem_execbuffer2 execbuf;
+   uint32_t *batch, *r;
+   const struct intel_execution_engine *engine;
+   int i915, i;
+   uint32_t val;
+   i915 = config->drm_fd;
+
+   engine = find_engine(config->engine);
+
+   memset(obj, 0, sizeof(obj));
+   obj[0].handle = gem_create(i915, 4096);
+   obj[1].handle = gem_create(i915, 4096);
+   obj[1].relocs_ptr = to_user_pointer(reloc);
+   obj[1].relocation_count = 1;
+
+   batch = gem_mmap__cpu(i915, obj[1].handle, 0, 4096, PROT_WRITE);
+   gem_set_domain(i915, obj[1].handle,
+  I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+
+   i = 0;
+   if (val_in) {
+   batch[i++] = MI_NOOP;
+   batch[i++] = MI_NOOP;
+
+   batch[i++] = MI_LOAD_REGISTER_IMM;
+   batch[i++] = reg->addr;
+   batch[i++] = *val_in;
+   batch[i++] = MI_NOOP;
+   }
+
+   batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */
+   batch[i++] = reg->addr;
+   reloc[0].target_handle = obj[0].handle;
+   reloc[0].presumed_offset = obj[0].offset;
+   reloc[0].offset = i * sizeof(uint32_t);
+   reloc[0].delta = 0;
+   reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
+   reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
+   batch[i++] = reloc[0].delta;
+   if (r64b)
+   batch[i++] = 0;
+
+   batch[i++] = MI_BATCH_BUFFER_END;
+   munmap(batch, 4096);
+
+   memset(, 0, sizeof(execbuf));
+   execbuf.buffers_ptr = to_user_pointer(obj);
+   execbuf.buffer_count = 2;
+   execbuf.flags = engine->exec_id;
+
+   if (config->engine_secure_batch) {
+   execbuf.flags |= I915_EXEC_SECURE;
+
+   if (config->verbosity > 0)
+   printf("%s: using priviledged (secure) batch\n",
+  engine->name);
+   }
+
+   execbuf.rsvd1 = ctx;
+   gem_execbuf(i915, );
+   gem_close(i915, obj[1].handle);
+
+   r = gem_mmap__cpu(i915, obj[0].handle, 0, 4096, PROT_READ);
+   gem_set_domain(i915, obj[0].handle, I915_GEM_DOMAIN_CPU, 0);
+
+   val = r[0];
+
+   gem_close(i915, obj[0].handle);
+
+   return val;
+}
+
 static int read_register(struct config *config, struct reg *reg, uint32_t 
*valp)
 {
uint32_t val = 0;
 
switch (reg->port_desc.port) {
case PORT_MMIO:
-   val = INREG(reg->mmio_offset + reg->addr);
+   if (config->engine)
+   val = register_srm(config, reg, NULL);
+   else
+   val = INREG(reg->mmio_offset + reg->addr);
break;
case PORT_PORTIO_VGA:
iopl(3);
@@ -299,7 +409,15 @@ static int write_register(struct config *config, struct 
reg *reg,

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

[Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

[Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

Re: [Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

[Intel-gfx] [PATCH igt] tools/intel_reg: Add reading and writing registers through engine

9 matches

Site Navigation

Mail list logo

Footer information