[igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
Jani Nikula
jani.nikula at linux.intel.com
Tue Oct 23 08:50:47 UTC 2018
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.
Regular C bug. read() may return a positive result smaller than
requested. See readN() in igt_sysfs.c.
> + if (ret < 0) {
> + fprintf(stderr,
> + "fail to read dpcd name from %s\n\n", sysfs_path);
Two newlines is unnecessary.
> + 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.
> +
> +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?
> +
> + /* Dummy read to check if dpcd is available */
> + ret = read(dev_fd, &discard, 1);
Serious bug #2. This read botches up the whole dump.
> + 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?!?
> + snprintf(sys_name, 16, "drm_dp_aux%d", dpcd.devid);
sizeof.
> +
Superfluous newline.
>
> 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.
> 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