[igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
Rodrigo Vivi
rodrigo.vivi at intel.com
Fri Feb 8 18:13:20 UTC 2019
Hi Tarun,
I lost the track here...
Is this completed and we could remove the kernel debugfs already?
Or do we need to follow up on this dump all?
Thanks,
Rodrigo.
On Mon, Oct 29, 2018 at 11:18:31PM +0000, Tarun Vyas wrote:
> 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
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list