* [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