[igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
Tarun Vyas
tarun.vyas at intel.com
Tue Oct 23 12:13:34 UTC 2018
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.
>
> BR,
> Jani.
>
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