[igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
Tarun Vyas
tarun.vyas at intel.com
Mon Oct 29 23:18:31 UTC 2018
On Tue, Oct 23, 2018 at 12:13:34PM +0000, Tarun Vyas wrote:
> On Tue, Oct 23, 2018 at 11:50:47AM +0300, Jani Nikula wrote:
> > On Mon, 22 Oct 2018, Tarun Vyas <tarun.vyas at intel.com> wrote:
> > > From: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >
> > > And make it the default when no operation or device is given.
> > >
> > > So, this will replace i915_dpcd for now but can be expanded
> > > later.
> > >
> > > Current debugfs:
> > >
> > > $ sudo cat /sys/kernel/debug/dri/0/DP-1/i915_dpcd
> > > 0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
> > > 0070: 00 00
> > > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > 0100: 14 84 00 06 06 06 06 00 01 04 00
> > > 0200: 41 00 77 77 01 03 22 22
> > > 0600: 01
> > > 0700: 01
> > > 0701: 00 00 00 00
> > > 0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
> > > 0732: 00 00
> > >
> > > $ sudo cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
> > > 0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
> > > 0070: 01 00
> > > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > 0100: 14 04 00 00 00 00 00 00 00 00 00
> > > 0200: 41 00 00 00 80 00 66 66
> > > 0600: 01
> > > 0700: 02
> > > 0701: 9f 40 00 00
> > > 0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
> > > 0732: 00 00
> > >
> > > new tool:
> > >
> > > $ sudo tools/dpcd_reg
> > >
> > > Dumping DPDDC-B
> > > 0x0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
> > > 0x0070: 00 00
> > > 0x0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > 0x0100: 14 84 00 06 06 06 06 00 01 04 00
> > > 0x0200: 41 00 77 77 01 03 22 22
> > > 0x0600: 01
> > > 0x0700: 01
> > > 0x0701: 00 00 00 00
> > > 0x0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
> > > 0x0732: 00 00
> > > 0x2008: 00
> > >
> > > Dumping DPDDC-A
> > > 0x0000: 14 0a 82 41 00 00 01 c0 02 00 00 00 0f 09 80
> > > 0x0070: 03 1b
> > > 0x0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > 0x0100: 00 82 00 00 00 00 00 00 01 08 00
> > > 0x0200: 01 00 00 00 00 01 00 00
> > > 0x0600: 01
> > > 0x0700: 04
> > > 0x0701: fb ff 00 00
> > > 0x0720: 01 03 ff ff 10 0a 10 00 01 00 01 00 00 c0 00 00
> > > 0x0732: 00 14
> > > 0x2008: 02
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Signed-off-by: Tarun Vyas <tarun.vyas at intel.com>
> > > ---
> > > tools/dpcd_reg.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 91 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > > index 2761168d..51cec85f 100644
> > > --- a/tools/dpcd_reg.c
> > > +++ b/tools/dpcd_reg.c
> > > @@ -35,6 +35,7 @@
> > > #include <unistd.h>
> > > #include <limits.h>
> > > #include <stdbool.h>
> > > +#include <dirent.h>
> > >
> > > #define MAX_DP_OFFSET 0xfffff
> > > #define DRM_AUX_MINORS 256
> > > @@ -109,6 +110,35 @@ static inline bool strtol_err_util(char *endptr, long *val)
> > > return false;
> > > }
> > >
> > > +static void connector_name(char *f_name, char *dpcd_name)
> > > +{
> > > + char sysfs_path[PATH_MAX];
> > > + int sysfs_fd, ret;
> > > +
> > > + snprintf(sysfs_path, PATH_MAX,
> > > + "/sys/class/drm_dp_aux_dev/%s/name",
> > > + f_name);
> > > + sysfs_fd = open(sysfs_path, O_RDONLY);
> > > + if (sysfs_fd < 0) {
> > > + fprintf(stderr,
> > > + "fail to open %s\n", sysfs_path);
> > > + return;
> > > + }
> > > +
> > > + ret = read(sysfs_fd, dpcd_name, sizeof(dpcd_name) - 1);
> >
> > Serious bug #1. The sizeof doesn't do what you intend.
> >
> Yes, that was stupid. The original patch was doing it right where the array was declared. I messed it up while creating a new function out of it.
> > Regular C bug. read() may return a positive result smaller than
> > requested. See readN() in igt_sysfs.c.
> >
> >
> Yes, but, if the read was partial (or zero) , then we would still like to treat it as a failure, in this specific case, right ?
> > > + if (ret < 0) {
> > > + fprintf(stderr,
> > > + "fail to read dpcd name from %s\n\n", sysfs_path);
> >
> > Two newlines is unnecessary.
> >
> Ok.
> > > + goto cleanup;
> > > + }
> > > +
> > > + dpcd_name[ret] = '\0';
> > > + printf("%s\n", dpcd_name);
> >
> > Why does this function both retrieve the name to the caller and print
> > it? Those are two things that should stay orthogonal.
> >
> I kept them separate originally...will work on it in the next version.
> > > +
> > > +cleanup:
> > > + close(sysfs_fd);
> > > +}
> > > +
> > > static int parse_opts(struct dpcd_data *dpcd, int argc, char **argv)
> > > {
> > > int ret, vflag = 0;
> > > @@ -280,13 +310,61 @@ static int dpcd_dump(int fd)
> > > return ret;
> > > }
> > >
> > > +static int dpcd_dump_all(void)
> > > +{
> > > + struct dirent *dirent;
> > > + DIR *dir;
> > > + char dev_path[PATH_MAX];
> > > + int dev_fd, ret = 0;
> > > + char dpcd_name[8];
> > > + char discard;
> > > +
> > > + dir = opendir("/sys/class/drm_dp_aux_dev/");
> > > + if (!dir) {
> > > + fprintf(stderr, "fail to read /dev\n");
> > > + return ENOENT;
> > > + }
> > > +
> > > + while ((dirent = readdir(dir))) {
> > > + if (strncmp(dirent->d_name, "drm_dp_aux", 10) == 0) {
> > > + snprintf(dev_path, PATH_MAX,
> > > + "/dev/%s", dirent->d_name);
> > > +
> > > + dev_fd = open(dev_path, O_RDONLY);
> > > + if (dev_fd < 0) {
> > > + fprintf(stderr,
> > > + "fail to open %s\n", dev_path);
> > > + continue;
> > > + }
> > > +
> > > + printf("\nDumping ");
> > > + connector_name(dirent->d_name, dpcd_name);
> >
> > And what if connector_name() fails?
> >
> I had connector_name() return an int but then changed it to void b/c I was like,
> I'd still want to dump the registers regardless of connector_name.
> Do we wanna skip it all if we cant get the connector_name ?
> > > +
> > > + /* Dummy read to check if dpcd is available */
> > > + ret = read(dev_fd, &discard, 1);
> >
> > Serious bug #2. This read botches up the whole dump.
> >
> Hmm. we should skip the dump only if the read return a -ve...
> > > + if (ret != 1) {
> > > + printf("DPCD %s seems not available. skipping...\n",
> > > + dpcd_name);
> > > + continue;
> > > + }
> > > +
> > > + dpcd_dump(dev_fd);
> > > + close(dev_fd);
> > > + }
> > > + }
> > > +
> > > + closedir(dir);
> > > +
> > > + return EXIT_SUCCESS;
> > > +}
> > > int main(int argc, char **argv)
> > > {
> > > char dev_name[20];
> > > + char sys_name[16], dpcd_name[8];
> > > int ret, fd;
> > >
> > > struct dpcd_data dpcd = {
> > > - .devid = 0,
> > > + .devid = -1,
> > > .file_op = O_RDONLY,
> > > .rw.offset = 0x0,
> > > .rw.count = 1,
> > > @@ -297,7 +375,16 @@ int main(int argc, char **argv)
> > > if (ret != EXIT_SUCCESS)
> > > return ret;
> > >
> > > + if (dpcd.devid < 0) {
> > > + if (dpcd.cmd == DUMP)
> > > + return dpcd_dump_all();
> > > +
> > > + dpcd.devid = 0;
> > > + }
> > > +
> > > snprintf(dev_name, strlen(aux_dev) + 4, "%s%d", aux_dev, dpcd.devid);
> >
> > That's not part of this patch, but what on earth does strlen(aux_dev)
> > have to do with how much fits into dev_name?!?
> >
> Yes. Will fix it.
> > > + snprintf(sys_name, 16, "drm_dp_aux%d", dpcd.devid);
> >
> > sizeof.
> >
> Right
> > > +
> >
> > Superfluous newline.
> >
> Ok
> > >
> > > fd = open(dev_name, dpcd.file_op);
> > > if (fd < 0) {
> > > @@ -307,6 +394,9 @@ int main(int argc, char **argv)
> > > return errno;
> > > }
> > >
> > > + printf("\n");
> > > + connector_name(sys_name, dpcd_name);
> > > +
> >
> > Why is the output different for one vs. all devices dumping? Dumping one
> > specified device should look exactly the same as dumping all devices
> > when there's just one available device. Maybe this shouldn't be
> > duplicated. Maybe the dump path shouldn't be in *any* way different for
> > multiple vs one device. Maybe you could write a dump wrapper that takes
> > the aux dev name pattern as input, and in the specified case it should
> > only match one. Would make the whole thing much neater.
c> >
> > BR,
> > Jani.
> >
Another thing is that dpcd_dump_all() iterates over all of the available devices,
whereas for a single device dump, we specify the device id itself.
Here is the diff for unification of the dump path:
diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
index 51cec85f..fa7f077a 100644
--- a/tools/dpcd_reg.c
+++ b/tools/dpcd_reg.c
@@ -110,7 +110,7 @@ static inline bool strtol_err_util(char *endptr, long *val)
return false;
}
-static void connector_name(char *f_name, char *dpcd_name)
+static void connector_name(char *f_name, char *dpcd_name, size_t size)
{
char sysfs_path[PATH_MAX];
int sysfs_fd, ret;
@@ -125,15 +125,14 @@ static void connector_name(char *f_name, char *dpcd_name)
return;
}
- ret = read(sysfs_fd, dpcd_name, sizeof(dpcd_name) - 1);
+ ret = read(sysfs_fd, dpcd_name, size - 1);
if (ret < 0) {
fprintf(stderr,
"fail to read dpcd name from %s\n\n", sysfs_path);
goto cleanup;
}
- dpcd_name[ret] = '\0';
- printf("%s\n", dpcd_name);
+ dpcd_name[size - 1] = '\0';
cleanup:
close(sysfs_fd);
@@ -310,15 +309,21 @@ static int dpcd_dump(int fd)
return ret;
}
-static int dpcd_dump_all(void)
+static int dpcd_dump_all(int devid)
{
struct dirent *dirent;
DIR *dir;
char dev_path[PATH_MAX];
int dev_fd, ret = 0;
char dpcd_name[8];
+ char dev_pattern[16];
char discard;
+ if (devid < 0)
+ strcpy(dev_pattern, "drm_dp_aux");
+ else
+ snprintf(dev_pattern, sizeof(dev_pattern), "drm_dp_aux%d", devid);
+
dir = opendir("/sys/class/drm_dp_aux_dev/");
if (!dir) {
fprintf(stderr, "fail to read /dev\n");
@@ -326,7 +331,7 @@ static int dpcd_dump_all(void)
}
while ((dirent = readdir(dir))) {
- if (strncmp(dirent->d_name, "drm_dp_aux", 10) == 0) {
+ if (strncmp(dirent->d_name, dev_pattern, strlen(dev_pattern)) == 0) {
snprintf(dev_path, PATH_MAX,
"/dev/%s", dirent->d_name);
@@ -337,17 +342,18 @@ static int dpcd_dump_all(void)
continue;
}
- printf("\nDumping ");
- connector_name(dirent->d_name, dpcd_name);
+ connector_name(dirent->d_name, dpcd_name,
+ sizeof(dpcd_name));
/* Dummy read to check if dpcd is available */
ret = read(dev_fd, &discard, 1);
- if (ret != 1) {
+ if (ret < 0) {
printf("DPCD %s seems not available. skipping...\n",
dpcd_name);
continue;
}
+ printf("\nDumping %s\n", dpcd_name);
dpcd_dump(dev_fd);
close(dev_fd);
}
@@ -375,15 +381,14 @@ int main(int argc, char **argv)
if (ret != EXIT_SUCCESS)
return ret;
- if (dpcd.devid < 0) {
- if (dpcd.cmd == DUMP)
- return dpcd_dump_all();
-
+ if (dpcd.cmd == DUMP)
+ return dpcd_dump_all(dpcd.devid);
+ else if (dpcd.devid < 0)
dpcd.devid = 0;
- }
- snprintf(dev_name, strlen(aux_dev) + 4, "%s%d", aux_dev, dpcd.devid);
- snprintf(sys_name, 16, "drm_dp_aux%d", dpcd.devid);
+
+ snprintf(dev_name, sizeof(dev_name), "%s%d", aux_dev, dpcd.devid);
+ snprintf(sys_name, sizeof(sys_name), "drm_dp_aux%d", dpcd.devid);
fd = open(dev_name, dpcd.file_op);
@@ -394,8 +399,8 @@ int main(int argc, char **argv)
return errno;
}
- printf("\n");
- connector_name(sys_name, dpcd_name);
+ connector_name(sys_name, dpcd_name, sizeof(dpcd_name));
+ printf("\n%s\n", dpcd_name);
switch (dpcd.cmd) {
case READ:
@@ -404,9 +409,8 @@ int main(int argc, char **argv)
case WRITE:
ret = dpcd_write(fd, dpcd.rw.offset, dpcd.val);
break;
- case DUMP:
default:
- ret = dpcd_dump(fd);
+ /* No-op to make the compiler happy */
break;
}
Thanks,
Tarun
> My thought was that at this point, it wont be a dump anymore.
> I meant this specifically for reads/writes of byte(s), to a specific device.
> That's why I changed the connector_name() to include printing the dpcd_name
> (but that's just not right). But, yea, the idea was to make this look different
> than a regular dump of all registers, b/c here we just want to print the connector
> name for the device we are looking at.
>
> Thanks for the review Jani.
>
> - Tarun
> > > switch (dpcd.cmd) {
> > > case READ:
> > > ret = dpcd_read(fd, dpcd.rw.offset, dpcd.rw.count);
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
More information about the igt-dev
mailing list