On 2017-02-23 09:46 AM, Tom St Denis wrote:
To simplify scripting you can now use * for ipnames as well.

Thanks for the patch, this change is very welcome.


Signed-off-by: Tom St Denis <[email protected]>
---
 doc/umr.1         |   6 ++-
 src/app/main.c    |   5 ++-
 src/app/scan.c    |  21 ++++++++++
 src/app/set_bit.c | 117 +++++++++++++++++++++++++++---------------------------
 src/app/set_reg.c |  91 +++++++++++++++++++++---------------------
 5 files changed, 131 insertions(+), 109 deletions(-)

diff --git a/doc/umr.1 b/doc/umr.1
index 5ed2c3bb93ae..a12dfb0a00ff 100644
--- a/doc/umr.1
+++ b/doc/umr.1
@@ -34,7 +34,7 @@ Look up an MMIO register by address and bitfield decode the 
value specified.
 Write a value specified in hex to a register specified with a complete
 register path in the form <
 .B asicname.ipname.regname
->.  For example, fiji.uvd6.mmUVD_CGC_GATE.  The value of asicname can be
+>.  For example, fiji.uvd6.mmUVD_CGC_GATE.  The value of asicname and/or 
ipname can be
 .B *
 to simplify scripting.  This command can be used multiple times to
 write to multiple registers in a single invocation.
@@ -52,7 +52,9 @@ command but also allows
 for the regname field.
 .IP "--scan, -s <string>"
 Scan and print an IP block by name, for example,
-.B uvd5.
+.B uvd6
+or
+.B carrizo.uvd6.
 Can be used multiple times in a single invocation.
 .IP "--ring, -R <string>(from:to)"
 Read the contents of a ring named by the string without the
diff --git a/src/app/main.c b/src/app/main.c
index 8364d54c5227..99e014888571 100644
--- a/src/app/main.c
+++ b/src/app/main.c
@@ -364,12 +364,13 @@ int main(int argc, char **argv)
 "\n\t--write, -w <address> <number>\n\t\tWrite a value in hex to a register 
specified as a register path in the"
        "\n\t\tform <asicname.ipname.regname>.  For instance 
\"tonga.uvd5.mmUVD_SOFT_RESET\"."
        "\n\t\tCan be used multiple times to set multiple registers.  You can"
-       "\n\t\tspecify * for asicname to simplify scripts.\n"
+       "\n\t\tspecify * for asicname and/or ipname to simplify scripts.\n"
 "\n\t--writebit, -wb <string> <number>\n\t\tWrite a value in hex to a register 
bitfield specified as in --write but"
        "\n\t\tthe addition of the bitfield name.  For instance: 
\"*.gfx80.mmRLC_PG_CNTL.PG_OVERRIDE\"\n"
 "\n\t--read, -r <string>\n\t\tRead a value from a register and print it to stdout.  
This command"
        "\n\t\tuses the same path notation as --write.  It also accepts * for 
regname.\n"
-"\n\t--scan, -s <string>\n\t\tScan and print an ip block by name, e.g. \"uvd5\".  
Can be used multiple times.\n"
+"\n\t--scan, -s <string>\n\t\tScan and print an ip block by name, e.g. \"uvd6\" or 
\"carrizo.uvd6\"."
+       "\n\t\tCan be used multiple times.\n"
 "\n\t--ring, -R <string>([from:to])\n\t\tRead the contents of a ring named by the 
string without the amdgpu_ring_ prefix. "
        "\n\t\tBy default it will read and display the entire ring.  A starting and 
ending "
        "\n\t\taddress can be specified in decimal or a '.' can be used to indicate 
relative "
diff --git a/src/app/scan.c b/src/app/scan.c
index 19c97fe1499d..fbb8f5838069 100644
--- a/src/app/scan.c
+++ b/src/app/scan.c
@@ -31,6 +31,27 @@ int umr_scan_asic(struct umr_asic *asic, char *asicname, 
char *ipname, char *reg
        uint64_t addr, scale;
        char buf[256];

+       // if ipname == '*' scan for a match and stop on first one
+       if (ipname[0] == '*' && regname[0] != '*') {
+               for (i = 0; i < asic->no_blocks; i++) {
+                       for (j = 0; j < asic->blocks[i]->no_regs; j++) {
+                               if (!strcmp(regname, 
asic->blocks[i]->regs[j].regname) ||
+                                   (options.many && 
strstr(asic->blocks[i]->regs[j].regname, regname))) {
+                                       ipname = asic->blocks[i]->ipname;
+                                       goto ip_found;

Since the star globs usually imply a wildcard match, I think it is a little weird to truncate the output to only the first match.

If a star glob is requested by the user, I think it should be okay to loop and print multiple outputs as usually happens when options.many is enabled.

Alternative approach would be to print an 'ambiguous argument' error. That way the user will at least have some feedback. Although I think the behaviour of printing multiple results is preferable.

Cheers,
Andres

+                               }
+                       }
+               }
+       }
+ip_found:
+       if (ipname[0] == '*') {
+               if (regname[0] == '*')
+                       fprintf(stderr, "[ERROR]:  You cannot specify * for both 
ipname and regname\n");
+               else
+                       fprintf(stderr, "[ERROR]:  Register name <%s> was not found 
with ANY ip name\n", regname);
+               return -1;
+       }
+
        /* scan them all in order */
        if (!asicname[0] || !strcmp(asicname, "*") || !strcmp(asicname, 
asic->asicname)) {
                for (i = 0; i < asic->no_blocks; i++) {
diff --git a/src/app/set_bit.c b/src/app/set_bit.c
index d9ee7d8f3a55..bed7ee858b6a 100644
--- a/src/app/set_bit.c
+++ b/src/app/set_bit.c
@@ -31,79 +31,78 @@
  */
 int umr_set_register_bit(struct umr_asic *asic, char *regpath, char *regvalue)
 {
-       char rpath[512];
+       char asicname[128], ipname[128], regname[128], bitname[128];
        int i, j, k, fd;
        uint32_t value, mask, copy;
        uint64_t addr, scale;

-       /* scan until we compare with regpath... */
-       for (i = 0; i < asic->no_blocks; i++) {
-               if (!memcmp(regpath, "*.", 2))
-                       snprintf(rpath, sizeof(rpath)-1, "*.%s.", 
asic->blocks[i]->ipname);
-               else
-                       snprintf(rpath, sizeof(rpath)-1, "%s.%s.", asic->asicname, 
asic->blocks[i]->ipname);
-               if (memcmp(regpath, rpath, strlen(rpath)) == 0) {
-                       for (j = 0; j < asic->blocks[i]->no_regs; j++) {
-                               if (asic->blocks[i]->regs[j].bits) {
-                                       for (k = 0; k < 
asic->blocks[i]->regs[j].no_bits; k++) {
-                                               if (!memcmp(regpath, "*.", 2))
-                                                       snprintf(rpath, sizeof(rpath)-1, 
"*.%s.%s.%s", asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname, 
asic->blocks[i]->regs[j].bits[k].regname);
-                                               else
-                                                       snprintf(rpath, sizeof(rpath)-1, 
"%s.%s.%s.%s", asic->asicname, asic->blocks[i]->ipname, 
asic->blocks[i]->regs[j].regname, asic->blocks[i]->regs[j].bits[k].regname);
-                                               if (!strcmp(regpath, rpath)) {
-                                                       sscanf(regvalue, "%"SCNx32, 
&value);
+       if (sscanf(regpath, "%[^.].%[^.].%[^.].%[^.]", asicname, ipname, 
regname, bitname) != 4) {
+               fprintf(stderr, "[ERROR]: Invalid regpath for bit write\n");
+               return -1;
+       }

-                                                       mask = 
(1UL<<((1+asic->blocks[i]->regs[j].bits[k].stop)-asic->blocks[i]->regs[j].bits[k].start))-1;
-                                                       mask <<= 
asic->blocks[i]->regs[j].bits[k].start;
-                                                       if (asic->pci.mem == 
NULL) {
-                                                               // set this 
register
-                                                               switch 
(asic->blocks[i]->regs[j].type){
-                                                               case REG_MMIO: fd 
= asic->fd.mmio; scale = 4; break;
-                                                               case REG_DIDT: fd 
= asic->fd.didt; scale = 1; break;
-                                                               case REG_PCIE: fd 
= asic->fd.pcie; scale = 1; break;
-                                                               case REG_SMC:  fd 
= asic->fd.smc;  scale = 1; break;
-                                                               default: return 
-1;
-                                                               }
-                                                               if 
(asic->blocks[i]->grant) {
-                                                                       if 
(asic->blocks[i]->grant(asic)) {
-                                                                               
fprintf(stderr, "[ERROR] Must specify at least one 'risky' before writing to this 
block.\n");
-                                                                               
return -1;
+       if (asicname[0] == '*' || !strcmp(asicname, asic->asicname)) {
+               /* scan until we compare with regpath... */
+               for (i = 0; i < asic->no_blocks; i++) {
+                       if (ipname[0] == '*' || !strcmp(ipname, 
asic->blocks[i]->ipname)) {
+                               for (j = 0; j < asic->blocks[i]->no_regs; j++) {
+                                       if (!strcmp(regname, 
asic->blocks[i]->regs[j].regname) && asic->blocks[i]->regs[j].bits) {
+                                               for (k = 0; k < 
asic->blocks[i]->regs[j].no_bits; k++) {
+                                                       if (!strcmp(bitname, 
asic->blocks[i]->regs[j].bits[k].regname)) {
+                                                               sscanf(regvalue, 
"%"SCNx32, &value);
+
+                                                               mask = 
(1UL<<((1+asic->blocks[i]->regs[j].bits[k].stop)-asic->blocks[i]->regs[j].bits[k].start))-1;
+                                                               mask <<= 
asic->blocks[i]->regs[j].bits[k].start;
+                                                               if 
(asic->pci.mem == NULL) {
+                                                                       // set 
this register
+                                                                       switch 
(asic->blocks[i]->regs[j].type){
+                                                                       case 
REG_MMIO: fd = asic->fd.mmio; scale = 4; break;
+                                                                       case 
REG_DIDT: fd = asic->fd.didt; scale = 1; break;
+                                                                       case 
REG_PCIE: fd = asic->fd.pcie; scale = 1; break;
+                                                                       case 
REG_SMC:  fd = asic->fd.smc;  scale = 1; break;
+                                                                       
default: return -1;
+                                                                       }
+                                                                       if 
(asic->blocks[i]->grant) {
+                                                                               if 
(asic->blocks[i]->grant(asic)) {
+                                                                                       
fprintf(stderr, "[ERROR] Must specify at least one 'risky' before writing to this 
block.\n");
+                                                                               
        return -1;
+                                                                               
}
                                                                        }
-                                                               }

-                                                               if (options.use_bank 
&& asic->blocks[i]->regs[j].type == REG_MMIO)
-                                                                       addr =
-                                                                               (1ULL 
<< 62) |
-                                                                               
(((uint64_t)options.se_bank) << 24) |
-                                                                               
(((uint64_t)options.sh_bank) << 34) |
-                                                                               
(((uint64_t)options.instance_bank) << 44);
-                                                               else
-                                                                       addr = 
0;
+                                                                       if (options.use_bank 
&& asic->blocks[i]->regs[j].type == REG_MMIO)
+                                                                               
addr =
+                                                                                     
  (1ULL << 62) |
+                                                                                     
  (((uint64_t)options.se_bank) << 24) |
+                                                                                     
  (((uint64_t)options.sh_bank) << 34) |
+                                                                                     
  (((uint64_t)options.instance_bank) << 44);
+                                                                       else
+                                                                               
addr = 0;

-                                                               lseek(fd, addr | 
(asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
-                                                               read(fd, &copy, 
4);
+                                                                       lseek(fd, 
addr | (asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
+                                                                       read(fd, 
&copy, 4);

-                                                               // 
read-modify-write value back
-                                                               copy &= ~mask;
-                                                               value = (value << 
asic->blocks[i]->regs[j].bits[k].start) & mask;
-                                                               copy |= value;
+                                                                       // 
read-modify-write value back
+                                                                       copy &= 
~mask;
+                                                                       value = (value << 
asic->blocks[i]->regs[j].bits[k].start) & mask;
+                                                                       copy |= 
value;

-                                                               lseek(fd, addr | 
(asic->blocks[i]->regs[j].addr<<2), SEEK_SET);
-                                                               write(fd, 
&copy, 4);
-                                                               if (!options.quiet) 
printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
+                                                                       lseek(fd, addr | 
(asic->blocks[i]->regs[j].addr<<2), SEEK_SET);
+                                                                       write(fd, 
&copy, 4);
+                                                                       if (!options.quiet) 
printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);

-                                                               if 
(asic->blocks[i]->release) {
-                                                                       if 
(asic->blocks[i]->release(asic)) {
-                                                                               
return -1;
+                                                                       if 
(asic->blocks[i]->release) {
+                                                                               if 
(asic->blocks[i]->release(asic)) {
+                                                                               
        return -1;
+                                                                               
}
                                                                        }
+                                                               } else if 
(asic->blocks[i]->regs[j].type == REG_MMIO) {
+                                                                       copy = 
asic->pci.mem[asic->blocks[i]->regs[j].addr] & ~mask;
+                                                                       copy |= (value << 
asic->blocks[i]->regs[j].bits[k].start) & mask;
+                                                                       
asic->pci.mem[asic->blocks[i]->regs[j].addr] = copy;
+                                                                       if (!options.quiet) 
printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
                                                                }
-                                                       } else if 
(asic->blocks[i]->regs[j].type == REG_MMIO) {
-                                                               copy = 
asic->pci.mem[asic->blocks[i]->regs[j].addr] & ~mask;
-                                                               copy |= (value << 
asic->blocks[i]->regs[j].bits[k].start) & mask;
-                                                               
asic->pci.mem[asic->blocks[i]->regs[j].addr] = copy;
-                                                               if (!options.quiet) 
printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
+                                                               return 0;
                                                        }
-                                                       return 0;
                                                }
                                        }
                                }
diff --git a/src/app/set_reg.c b/src/app/set_reg.c
index 9861170d55c3..ed8f708e977f 100644
--- a/src/app/set_reg.c
+++ b/src/app/set_reg.c
@@ -31,64 +31,63 @@
  */
 int umr_set_register(struct umr_asic *asic, char *regpath, char *regvalue)
 {
-       char rpath[512];
+       char asicname[128], ipname[128], regname[128];
        int i, j, fd;
        uint32_t value;
        uint64_t addr, scale;

-       /* scan until we compare with regpath... */
-       for (i = 0; i < asic->no_blocks; i++) {
-               if (!memcmp(regpath, "*.", 2))
-                       snprintf(rpath, sizeof(rpath)-1, "*.%s.", 
asic->blocks[i]->ipname);
-               else
-                       snprintf(rpath, sizeof(rpath)-1, "%s.%s.", asic->asicname, 
asic->blocks[i]->ipname);
-               if (memcmp(regpath, rpath, strlen(rpath)) == 0) {
-                       for (j = 0; j < asic->blocks[i]->no_regs; j++) {
-                               if (!memcmp(regpath, "*.", 2))
-                                       snprintf(rpath, sizeof(rpath)-1, "*.%s.%s", 
asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname);
-                               else
-                                       snprintf(rpath, sizeof(rpath)-1, "%s.%s.%s", 
asic->asicname, asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname);
-                               if (!strcmp(regpath, rpath)) {
-                                       sscanf(regvalue, "%"SCNx32, &value);
+       if (sscanf(regpath, "%[^.].%[^.].%[^.]", asicname, ipname, regname) != 
3) {
+               fprintf(stderr, "[ERROR]: Invalid regpath for write\n");
+               return -1;
+       }

-                                       if (asic->pci.mem == NULL) {
-                                               // set this register
-                                               switch 
(asic->blocks[i]->regs[j].type){
-                                               case REG_MMIO: fd = 
asic->fd.mmio; scale = 4; break;
-                                               case REG_DIDT: fd = 
asic->fd.didt; scale = 1; break;
-                                               case REG_PCIE: fd = 
asic->fd.pcie; scale = 1; break;
-                                               case REG_SMC:  fd = 
asic->fd.smc; scale = 1; break;
-                                               default: return -1;
-                                               }
-                                               
-                                               if (asic->blocks[i]->grant) {
-                                                       if 
(asic->blocks[i]->grant(asic)) {
-                                                               fprintf(stderr, 
"[ERROR] Must specify at least one 'risky' before writing to this block.\n");
-                                                               return -1;
+       if (asicname[0] == '*' || !strcmp(asicname, asic->asicname)) {
+               // scan all ip blocks for matching entry
+               for (i = 0; i < asic->no_blocks; i++) {
+                       if (ipname[0] == '*' || !strcmp(ipname, 
asic->blocks[i]->ipname)) {
+                               for (j = 0; j < asic->blocks[i]->no_regs; j++) {
+                                       if (!strcmp(regname, 
asic->blocks[i]->regs[j].regname)) {
+                                               sscanf(regvalue, "%"SCNx32, 
&value);
+
+                                               if (asic->pci.mem == NULL) {
+                                                       // set this register
+                                                       switch 
(asic->blocks[i]->regs[j].type){
+                                                       case REG_MMIO: fd = 
asic->fd.mmio; scale = 4; break;
+                                                       case REG_DIDT: fd = 
asic->fd.didt; scale = 1; break;
+                                                       case REG_PCIE: fd = 
asic->fd.pcie; scale = 1; break;
+                                                       case REG_SMC:  fd = 
asic->fd.smc; scale = 1; break;
+                                                       default: return -1;
+                                                       }
+
+                                                       if 
(asic->blocks[i]->grant) {
+                                                               if 
(asic->blocks[i]->grant(asic)) {
+                                                                       fprintf(stderr, 
"[ERROR] Must specify at least one 'risky' before writing to this block.\n");
+                                                                       return 
-1;
+                                                               }
                                                        }
-                                               }

-                                               if (options.use_bank && 
asic->blocks[i]->regs[j].type == REG_MMIO)
-                                                       addr =
-                                                               (1ULL << 62) |
-                                                               
(((uint64_t)options.se_bank) << 24) |
-                                                               
(((uint64_t)options.sh_bank) << 34) |
-                                                               
(((uint64_t)options.instance_bank) << 44);
-                                               else
-                                                       addr = 0;
+                                                       if (options.use_bank && 
asic->blocks[i]->regs[j].type == REG_MMIO)
+                                                               addr =
+                                                                       (1ULL 
<< 62) |
+                                                                       
(((uint64_t)options.se_bank) << 24) |
+                                                                       
(((uint64_t)options.sh_bank) << 34) |
+                                                                       
(((uint64_t)options.instance_bank) << 44);
+                                                       else
+                                                               addr = 0;

-                                               lseek(fd, addr | 
(asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
-                                               write(fd, &value, 4);
+                                                       lseek(fd, addr | 
(asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
+                                                       write(fd, &value, 4);

-                                               if (asic->blocks[i]->release) {
-                                                       if 
(asic->blocks[i]->release(asic)) {
-                                                               return -1;
+                                                       if 
(asic->blocks[i]->release) {
+                                                               if 
(asic->blocks[i]->release(asic)) {
+                                                                       return 
-1;
+                                                               }
                                                        }
+                                               } else if 
(asic->blocks[i]->regs[j].type == REG_MMIO) {
+                                                       
asic->pci.mem[asic->blocks[i]->regs[j].addr] = value;
                                                }
-                                       } else if 
(asic->blocks[i]->regs[j].type == REG_MMIO) {
-                                               
asic->pci.mem[asic->blocks[i]->regs[j].addr] = value;
+                                               return 0;
                                        }
-                                       return 0;
                                }
                        }
                }

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to