Re: [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 22 Oct 2018, Tarun Vyas <tarun.vyas@intel.com> wrote:
> From: Rodrigo Vivi <rodrigo.vivi@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@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Tarun Vyas <tarun.vyas@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev