public inbox for igt-dev@lists.freedesktop.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
@ 2018-10-22 21:41 Tarun Vyas
  2018-10-22 23:09 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Tarun Vyas @ 2018-10-22 21:41 UTC (permalink / raw)
  To: igt-dev; +Cc: dhinakaran.pandiyan, Rodrigo Vivi

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);
+	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);
+
+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);
+
+			/* Dummy read to check if dpcd is available */
+			ret = read(dev_fd, &discard, 1);
+			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);
+	snprintf(sys_name, 16, "drm_dp_aux%d", dpcd.devid);
+
 
 	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);
+
 	switch (dpcd.cmd) {
 	case READ:
 		ret = dpcd_read(fd, dpcd.rw.offset, dpcd.rw.count);
-- 
2.14.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for tools/dpcd_reg: Add dump all functionality
  2018-10-22 21:41 [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality Tarun Vyas
@ 2018-10-22 23:09 ` Patchwork
  2018-10-23  1:03 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-10-22 23:09 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

Series: tools/dpcd_reg: Add dump all functionality
URL   : https://patchwork.freedesktop.org/series/51348/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5013 -> IGTPW_1975 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51348/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1975 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_execlists:
      fi-apl-guc:         PASS -> INCOMPLETE (fdo#106693)

    igt@gem_exec_store@basic-all:
      fi-icl-u:           NOTRUN -> DMESG-WARN (fdo#107732) +7

    igt@gem_exec_suspend@basic:
      fi-icl-u:           NOTRUN -> DMESG-WARN (fdo#107724) +24

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107774, fdo#107859, fdo#107556)
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)
      fi-icl-u:           NOTRUN -> DMESG-WARN (fdo#108512)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#108070, fdo#107732) +1

    igt@gem_flink_basic@flink-lifetime:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#107732) +8

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106693 https://bugs.freedesktop.org/show_bug.cgi?id=106693
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#107732 https://bugs.freedesktop.org/show_bug.cgi?id=107732
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
  fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
  fdo#108070 https://bugs.freedesktop.org/show_bug.cgi?id=108070
  fdo#108512 https://bugs.freedesktop.org/show_bug.cgi?id=108512


== Participating hosts (41 -> 46) ==

  Additional (10): fi-kbl-soraka fi-icl-u fi-skl-guc fi-bdw-gvtdvm fi-glk-dsi fi-gdg-551 fi-ivb-3520m fi-kbl-7560u fi-byt-n2820 fi-snb-2600 
  Missing    (5): fi-ilk-m540 fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-ivb-3770 


== Build changes ==

    * IGT: IGT_4684 -> IGTPW_1975

  CI_DRM_5013: 0008b23799728680da3d6d871d46f746a3f69c35 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1975: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1975/
  IGT_4684: 6f27fddc6dd79c0486181b64201c6773c5c42a24 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1975/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for tools/dpcd_reg: Add dump all functionality
  2018-10-22 21:41 [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality Tarun Vyas
  2018-10-22 23:09 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-10-23  1:03 ` Patchwork
  2018-10-23  8:50 ` [igt-dev] [PATCH i-g-t] " Jani Nikula
  2018-10-30  3:57 ` [igt-dev] ✗ Fi.CI.BAT: failure for tools/dpcd_reg: Add dump all functionality (rev2) Patchwork
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-10-23  1:03 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

Series: tools/dpcd_reg: Add dump all functionality
URL   : https://patchwork.freedesktop.org/series/51348/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4684_full -> IGTPW_1975_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1975_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1975_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51348/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1975_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in IGTPW_1975_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-glk:          NOTRUN -> FAIL (fdo#103158)
      shard-apl:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-hsw:          NOTRUN -> FAIL (fdo#106641)

    igt@kms_busy@extended-modeset-hang-newfb-render-c:
      shard-glk:          NOTRUN -> DMESG-WARN (fdo#107956) +1
      shard-hsw:          NOTRUN -> DMESG-WARN (fdo#107956) +1
      shard-kbl:          NOTRUN -> DMESG-WARN (fdo#107956) +1

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
      shard-apl:          NOTRUN -> DMESG-WARN (fdo#107956) +1

    igt@kms_busy@extended-pageflip-hang-newfb-render-a:
      shard-glk:          PASS -> DMESG-WARN (fdo#107956)
      shard-apl:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-apl:          PASS -> FAIL (fdo#103191, fdo#103232) +1

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          PASS -> FAIL (fdo#103232) +3

    igt@kms_cursor_crc@cursor-256x85-sliding:
      shard-glk:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-64x21-sliding:
      shard-apl:          PASS -> FAIL (fdo#103232) +3

    igt@kms_cursor_crc@cursor-64x64-onscreen:
      shard-kbl:          PASS -> FAIL (fdo#103232) +2

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-kbl:          PASS -> FAIL (fdo#103191, fdo#103232)

    igt@kms_cursor_crc@cursor-size-change:
      shard-apl:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
      shard-apl:          PASS -> FAIL (fdo#103167) +4

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
      shard-kbl:          PASS -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
      shard-glk:          PASS -> FAIL (fdo#103167) +3

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
      shard-apl:          NOTRUN -> FAIL (fdo#108146)
      shard-kbl:          NOTRUN -> FAIL (fdo#108146)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-kbl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
      shard-glk:          PASS -> FAIL (fdo#103166)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    igt@syncobj_wait@wait-all-for-submit-delayed-submit:
      shard-glk:          NOTRUN -> INCOMPLETE (fdo#103359, k.org#198133)

    
    ==== Possible fixes ====

    igt@kms_atomic_interruptible@legacy-dpms:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_color@pipe-c-degamma:
      shard-apl:          FAIL (fdo#104782) -> PASS

    igt@kms_cursor_crc@cursor-128x42-sliding:
      shard-glk:          FAIL (fdo#103232) -> PASS +1

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-kbl:          FAIL (fdo#103191, fdo#103232) -> PASS
      shard-apl:          FAIL (fdo#103191, fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-256x85-onscreen:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
      shard-glk:          FAIL (fdo#103167) -> PASS +4
      shard-apl:          FAIL (fdo#103167) -> PASS

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-glk:          FAIL (fdo#103166) -> PASS +2

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-skl 


== Build changes ==

    * IGT: IGT_4684 -> IGTPW_1975
    * Linux: CI_DRM_5010 -> CI_DRM_5013

  CI_DRM_5010: 27a4f334d3ec8882d50227c26ae4e393d7d1f4a1 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_5013: 0008b23799728680da3d6d871d46f746a3f69c35 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1975: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1975/
  IGT_4684: 6f27fddc6dd79c0486181b64201c6773c5c42a24 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1975/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
  2018-10-22 21:41 [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality Tarun Vyas
  2018-10-22 23:09 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
  2018-10-23  1:03 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2018-10-23  8:50 ` Jani Nikula
  2018-10-23 12:13   ` Tarun Vyas
  2018-10-30  3:57 ` [igt-dev] ✗ Fi.CI.BAT: failure for tools/dpcd_reg: Add dump all functionality (rev2) Patchwork
  3 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2018-10-23  8:50 UTC (permalink / raw)
  To: Tarun Vyas, igt-dev; +Cc: dhinakaran.pandiyan, Rodrigo Vivi

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
  2018-10-23  8:50 ` [igt-dev] [PATCH i-g-t] " Jani Nikula
@ 2018-10-23 12:13   ` Tarun Vyas
  2018-10-29 23:18     ` Tarun Vyas
  0 siblings, 1 reply; 8+ messages in thread
From: Tarun Vyas @ 2018-10-23 12:13 UTC (permalink / raw)
  To: Jani Nikula; +Cc: igt-dev, dhinakaran.pandiyan, Rodrigo Vivi

On Tue, Oct 23, 2018 at 11:50:47AM +0300, Jani Nikula wrote:
> 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.
>
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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
  2018-10-23 12:13   ` Tarun Vyas
@ 2018-10-29 23:18     ` Tarun Vyas
  2019-02-08 18:13       ` Rodrigo Vivi
  0 siblings, 1 reply; 8+ messages in thread
From: Tarun Vyas @ 2018-10-29 23:18 UTC (permalink / raw)
  To: Jani Nikula; +Cc: igt-dev, dhinakaran.pandiyan, Rodrigo Vivi

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@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.
> >
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [igt-dev] ✗ Fi.CI.BAT: failure for tools/dpcd_reg: Add dump all functionality (rev2)
  2018-10-22 21:41 [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality Tarun Vyas
                   ` (2 preceding siblings ...)
  2018-10-23  8:50 ` [igt-dev] [PATCH i-g-t] " Jani Nikula
@ 2018-10-30  3:57 ` Patchwork
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-10-30  3:57 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

Series: tools/dpcd_reg: Add dump all functionality (rev2)
URL   : https://patchwork.freedesktop.org/series/51348/
State : failure

== Summary ==

Applying: tools/dpcd_reg: Add dump all functionality
Using index info to reconstruct a base tree...
M	tools/dpcd_reg.c
Patch failed at 0001 tools/dpcd_reg: Add dump all functionality
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality
  2018-10-29 23:18     ` Tarun Vyas
@ 2019-02-08 18:13       ` Rodrigo Vivi
  0 siblings, 0 replies; 8+ messages in thread
From: Rodrigo Vivi @ 2019-02-08 18:13 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev, dhinakaran.pandiyan

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@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.
> > >
> > 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-02-08 18:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-22 21:41 [igt-dev] [PATCH i-g-t] tools/dpcd_reg: Add dump all functionality Tarun Vyas
2018-10-22 23:09 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2018-10-23  1:03 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-10-23  8:50 ` [igt-dev] [PATCH i-g-t] " Jani Nikula
2018-10-23 12:13   ` Tarun Vyas
2018-10-29 23:18     ` Tarun Vyas
2019-02-08 18:13       ` Rodrigo Vivi
2018-10-30  3:57 ` [igt-dev] ✗ Fi.CI.BAT: failure for tools/dpcd_reg: Add dump all functionality (rev2) Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox