* [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
@ 2018-06-14 7:11 Tarun Vyas
2018-06-14 8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Tarun Vyas @ 2018-06-14 7:11 UTC (permalink / raw)
To: igt-dev; +Cc: dhinakaran.pandiyan
This tool serves as a wrapper around the constructs provided by the
drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
devices created by the kernel module.
It supports reading and writing dpcd registers on the connected aux
channels.
In the follow-up patch, support for decoding these registers will be
added to facilate debugging panel related issues.
Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
tools/Makefile.am | 2 +-
tools/Makefile.sources | 4 +-
tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 223 insertions(+), 2 deletions(-)
create mode 100644 tools/dpcd_reg.c
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 09b6dbcc3ece..b78824dadfdb 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
intel_error_decode_LDFLAGS = -lz
endif
-bin_PROGRAMS += intel_dp_compliance
+bin_PROGRAMS += intel_dp_compliance dpcd_reg
intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
diff --git a/tools/Makefile.sources b/tools/Makefile.sources
index abd23a0f4628..db606b28560f 100644
--- a/tools/Makefile.sources
+++ b/tools/Makefile.sources
@@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
intel_dp_compliance.h \
intel_dp_compliance_hotplug.c \
$(NULL)
-
+dpcd_reg_SOURCES = \
+ dpcd_reg.c \
+ $(NULL)
diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
new file mode 100644
index 000000000000..2848277aa792
--- /dev/null
+++ b/tools/dpcd_reg.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * DPCD register read/decode tool
+ * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
+ * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
+ */
+
+#include "igt_core.h"
+#include <errno.h>
+#include <fcntl.h>
+
+#define INVALID 0xff
+#define RW_SIZE 1
+
+const char aux_dev[] = "/dev/drm_dp_aux";
+
+static void print_usage(char *tool, int help)
+{
+ igt_info("DPCD register rw and decode tool\n\n");
+ igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
+ "to be set in the kernel config.\n");
+ igt_info("Usage %s command [options...]\n", tool);
+ igt_info("Supported commands are:\n"
+ "\tread: Read a dpcd reg at an offset\n"
+ "\twrite: Write a dpcd reg at an offset\n"
+ "\tdecode: Decode the value of a dpcd reg\n");
+ igt_info("Options for the above commands are\n"
+ "\t--device: Aux device id, IS_REQUIRED\n"
+ "\t--help: print the usage\n"
+ "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
+ "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
+ "\t--val: Specify a value, for reg writes\n");
+ exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
+}
+
+static int dpcd_read(char *device, const uint32_t *offset)
+{
+ int fd, ret;
+ void *buf = malloc(sizeof(uint8_t));
+
+ if(NULL != buf)
+ memset(buf, 0, sizeof(uint8_t));
+ else {
+ igt_warn("Can't allocate read buffer\n");
+ ret = EXIT_FAILURE;
+ goto out;
+ }
+
+ fd = open(device, O_RDONLY);
+ if (fd > 0) {
+ ret = pread(fd, buf, RW_SIZE, *offset);
+ close(fd);
+ if (ret < 0) {
+ igt_warn("Failed to read from %s aux device - error %s\n", device, strerror(errno));
+ ret = EXIT_FAILURE;
+ goto free_up;
+ }
+ printf("Value @ offset %x is %x\n", *offset, *((int *)(buf)));
+ }
+ else {
+ igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
+ ret = EXIT_FAILURE;
+ }
+free_up:
+ free(buf);
+out:
+ return ret;
+}
+
+static int dpcd_write(char *device, const uint32_t *offset, const uint8_t *val)
+{
+ int fd, ret;
+
+ fd = open(device, O_RDWR);
+ if (fd > 0) {
+ ret = pwrite(fd, (const void *)val, RW_SIZE, *offset);
+ close(fd);
+ if (ret < 0) {
+ igt_warn("Failed to write to %s aux device - error %s\n", device, strerror(errno));
+ ret = EXIT_FAILURE;
+ goto out;
+ }
+ ret = dpcd_read(device, offset);
+ }
+ else {
+ igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
+ ret = EXIT_FAILURE;
+ }
+out:
+ return ret;
+}
+
+/*TO-DO: Decode dpcd reg value.
+ * By default, the latest DP/eDP spec
+ * versions will be used. To override,
+ * specify the spec version through the
+ * --spec option.
+static void dpcd_decode()
+{
+}
+*/
+
+int main(int argc, char **argv)
+{
+ char dev_name[20];
+ int ret, devid, help_flg;
+ uint32_t offset;
+ uint8_t val;
+
+ enum command {
+ INV = -1,
+ READ = 2,
+ WRITE,
+ DECODE,
+ } cmd = INV;
+
+ struct option longopts [] = {
+ { "device", required_argument, NULL, 'd' },
+ { "help", no_argument, &help_flg, 2 },
+ { "offset", required_argument, NULL, 'o' },
+ { "spec", optional_argument, NULL, 's' },
+ { "val", required_argument, NULL, 'v' },
+ { 0 }
+ };
+
+ offset = val = INVALID;
+ devid = -1;
+
+ while ((ret = getopt_long(argc, argv, "-:d:o:s::v:", longopts, NULL)) != -1) {
+ switch (ret) {
+ case 'd':
+ devid = strtoul(optarg, NULL, 10);
+ break;
+ case 'o':
+ offset = strtoul(optarg, NULL, 16);
+ break;
+ case 's':
+ /* TO-DO: Parse --spec version */
+ break;
+ case 'v':
+ val = strtoul(optarg, NULL, 16);
+ break;
+ /* Fall through for --help */
+ case 0:
+ break;
+ /* Command parsing */
+ case 1:
+ if (strcmp(optarg, "read") == 0)
+ cmd = READ;
+ else if (strcmp(optarg, "write") == 0)
+ cmd = WRITE;
+ else if (strcmp(optarg, "decode") == 0)
+ cmd = DECODE;
+ break;
+ case ':':
+ fprintf(stderr, "The -%c option of %s requires an argument\n", optopt, argv[0]);
+ print_usage(argv[0], 0);
+ case '?':
+ default :
+ printf("%s - option %c is invalid\n", argv[0], optopt);
+ print_usage(argv[0], 0);
+ }
+ }
+
+ if (help_flg == 2)
+ print_usage(argv[0], 1);
+
+ if(devid == -1 || offset == INVALID) {
+ printf("Aux device id and/or offset missing\n");
+ print_usage(argv[0], 0);
+ }
+
+ memset(dev_name, '\0', 20);
+ snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid);
+
+ switch (cmd) {
+ case READ:
+ ret = dpcd_read(dev_name, &offset);
+ break;
+ case WRITE:
+ if (val == INVALID) {
+ igt_warn("Write value is missing\n");
+ print_usage(argv[0], 0);
+ }
+ ret = dpcd_write(dev_name, &offset, &val);
+ break;
+ case DECODE:
+ /* TO-DO: Decode dpcd reg vals
+ * drm_dpcd_decode();
+ */
+ break;
+ case INV:
+ default:
+ igt_warn("Please specify a comand: read/write/decode. See usage\n");
+ print_usage(argv[0], 0);
+ }
+
+ return ret;
+}
--
2.13.5
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 10+ messages in thread* [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas @ 2018-06-14 8:08 ` Patchwork 2018-06-14 9:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork 2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä 2 siblings, 0 replies; 10+ messages in thread From: Patchwork @ 2018-06-14 8:08 UTC (permalink / raw) To: Tarun Vyas; +Cc: igt-dev == Series Details == Series: tools: Add a simple tool to read/write/decode dpcd registers URL : https://patchwork.freedesktop.org/series/44736/ State : success == Summary == = CI Bug Log - changes from CI_DRM_4309 -> IGTPW_1459 = == Summary - WARNING == Minor unknown changes coming with IGTPW_1459 need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in IGTPW_1459, 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/44736/revisions/1/mbox/ == Possible new issues == Here are the unknown changes that may have been introduced in IGTPW_1459: === IGT changes === ==== Warnings ==== igt@gem_exec_gttfill@basic: fi-pnv-d510: SKIP -> PASS == Known issues == Here are the changes found in IGTPW_1459 that come from known issues: === IGT changes === ==== Issues hit ==== igt@gem_exec_gttfill@basic: fi-byt-n2820: PASS -> FAIL (fdo#106744) igt@kms_flip@basic-flip-vs-wf_vblank: fi-skl-6700hq: PASS -> DMESG-WARN (fdo#105998) ==== Possible fixes ==== igt@gem_exec_suspend@basic-s4-devices: fi-kbl-7500u: DMESG-WARN (fdo#105128) -> PASS igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: fi-cnl-psr: DMESG-WARN (fdo#104951) -> PASS fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951 fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128 fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998 fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744 == Participating hosts (43 -> 38) == Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-glk-j4005 == Build changes == * IGT: IGT_4517 -> IGTPW_1459 CI_DRM_4309: 2740c5b0d0f40092355b329a62ede8cced7f64b9 @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_1459: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1459/ IGT_4517: e94ce40798e35d2e3c4494f50b617908066bbf8b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1459/issues.html _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas 2018-06-14 8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork @ 2018-06-14 9:27 ` Patchwork 2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä 2 siblings, 0 replies; 10+ messages in thread From: Patchwork @ 2018-06-14 9:27 UTC (permalink / raw) To: Tarun Vyas; +Cc: igt-dev == Series Details == Series: tools: Add a simple tool to read/write/decode dpcd registers URL : https://patchwork.freedesktop.org/series/44736/ State : success == Summary == = CI Bug Log - changes from IGT_4517_full -> IGTPW_1459_full = == Summary - WARNING == Minor unknown changes coming with IGTPW_1459_full need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in IGTPW_1459_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/44736/revisions/1/mbox/ == Possible new issues == Here are the unknown changes that may have been introduced in IGTPW_1459_full: === IGT changes === ==== Warnings ==== igt@drv_suspend@shrink: shard-glk: INCOMPLETE (fdo#103359, k.org#198133) -> FAIL igt@gem_exec_schedule@deep-bsd2: shard-kbl: SKIP -> PASS igt@gem_exec_schedule@deep-vebox: shard-kbl: PASS -> SKIP +1 == Known issues == Here are the changes found in IGTPW_1459_full that come from known issues: === IGT changes === ==== Issues hit ==== igt@drv_hangman@error-state-capture-blt: shard-snb: PASS -> INCOMPLETE (fdo#105411) igt@drv_selftest@live_hangcheck: shard-kbl: PASS -> DMESG-FAIL (fdo#106560) igt@gem_exec_schedule@pi-ringfull-blt: shard-glk: NOTRUN -> FAIL (fdo#103158) +2 igt@kms_atomic_transition@1x-modeset-transitions-nonblocking: shard-glk: NOTRUN -> FAIL (fdo#105703) igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy: shard-hsw: PASS -> FAIL (fdo#104873) igt@kms_flip@2x-flip-vs-expired-vblank-interruptible: shard-glk: PASS -> FAIL (fdo#105363) igt@kms_flip@flip-vs-expired-vblank: shard-hsw: PASS -> FAIL (fdo#105189) igt@kms_flip_tiling@flip-to-y-tiled: shard-glk: NOTRUN -> FAIL (fdo#104724, fdo#103822) +1 igt@kms_flip_tiling@flip-y-tiled: shard-glk: NOTRUN -> FAIL (fdo#104724) igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite: shard-glk: PASS -> FAIL (fdo#104724, fdo#103167) igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render: shard-snb: PASS -> FAIL (fdo#104724, fdo#103167) igt@kms_setmode@basic: shard-apl: PASS -> FAIL (fdo#99912) shard-glk: NOTRUN -> FAIL (fdo#99912) igt@prime_vgem@coherency-gtt: shard-glk: NOTRUN -> FAIL (fdo#100587) igt@testdisplay: shard-glk: NOTRUN -> INCOMPLETE (fdo#103359, k.org#198133) ==== Possible fixes ==== igt@drv_selftest@live_gtt: shard-kbl: FAIL (fdo#105347) -> PASS igt@drv_suspend@shrink: shard-apl: INCOMPLETE (fdo#103927) -> PASS igt@gem_ppgtt@blt-vs-render-ctx0: shard-kbl: INCOMPLETE (fdo#106023, fdo#103665) -> PASS igt@kms_cursor_legacy@cursor-vs-flip-toggle: shard-hsw: FAIL (fdo#103355) -> PASS igt@kms_cursor_legacy@flip-vs-cursor-legacy: shard-hsw: FAIL (fdo#102670) -> PASS igt@kms_flip@flip-vs-panning-vs-hang: shard-snb: DMESG-WARN (fdo#103821) -> PASS igt@kms_flip@plain-flip-ts-check-interruptible: shard-hsw: FAIL (fdo#100368) -> PASS igt@kms_rotation_crc@primary-rotation-180: shard-snb: FAIL (fdo#104724, fdo#103925) -> PASS igt@perf_pmu@busy-accuracy-50-vcs1: shard-snb: INCOMPLETE (fdo#105411) -> SKIP ==== Warnings ==== igt@kms_sysfs_edid_timing: shard-hsw: WARN (fdo#100047) -> FAIL (fdo#100047) fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047 fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#100587 https://bugs.freedesktop.org/show_bug.cgi?id=100587 fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670 fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158 fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355 fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359 fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665 fdo#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821 fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822 fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925 fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927 fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724 fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873 fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189 fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347 fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363 fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411 fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703 fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023 fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560 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 (5 -> 5) == No changes in participating hosts == Build changes == * IGT: IGT_4517 -> IGTPW_1459 * Linux: CI_DRM_4307 -> CI_DRM_4309 CI_DRM_4307: 0cf6f8f74cad691364d738d1607bc45945f3a5f9 @ git://anongit.freedesktop.org/gfx-ci/linux CI_DRM_4309: 2740c5b0d0f40092355b329a62ede8cced7f64b9 @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_1459: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1459/ IGT_4517: e94ce40798e35d2e3c4494f50b617908066bbf8b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1459/shards.html _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas 2018-06-14 8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork 2018-06-14 9:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork @ 2018-06-14 10:43 ` Ville Syrjälä 2018-06-14 12:50 ` Jani Nikula 2018-06-15 2:00 ` Tarun Vyas 2 siblings, 2 replies; 10+ messages in thread From: Ville Syrjälä @ 2018-06-14 10:43 UTC (permalink / raw) To: Tarun Vyas; +Cc: igt-dev, dhinakaran.pandiyan On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: > This tool serves as a wrapper around the constructs provided by the > drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n] > devices created by the kernel module. > It supports reading and writing dpcd registers on the connected aux > channels. > In the follow-up patch, support for decoding these registers will be > added to facilate debugging panel related issues. > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > --- > tools/Makefile.am | 2 +- > tools/Makefile.sources | 4 +- > tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 223 insertions(+), 2 deletions(-) > create mode 100644 tools/dpcd_reg.c > > diff --git a/tools/Makefile.am b/tools/Makefile.am > index 09b6dbcc3ece..b78824dadfdb 100644 > --- a/tools/Makefile.am > +++ b/tools/Makefile.am > @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) > intel_error_decode_LDFLAGS = -lz > endif > > -bin_PROGRAMS += intel_dp_compliance > +bin_PROGRAMS += intel_dp_compliance dpcd_reg Not the best place for this I think. Missing meson.build so no one is actually going to build this. > intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) > intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > index abd23a0f4628..db606b28560f 100644 > --- a/tools/Makefile.sources > +++ b/tools/Makefile.sources > @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ > intel_dp_compliance.h \ > intel_dp_compliance_hotplug.c \ > $(NULL) > - > +dpcd_reg_SOURCES = \ > + dpcd_reg.c \ > + $(NULL) > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > new file mode 100644 > index 000000000000..2848277aa792 > --- /dev/null > +++ b/tools/dpcd_reg.c > @@ -0,0 +1,219 @@ > +/* > + * Copyright © 2018 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > + * SOFTWARE. > + * > + * DPCD register read/decode tool > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read, > + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set. > + */ > + > +#include "igt_core.h" > +#include <errno.h> > +#include <fcntl.h> > + > +#define INVALID 0xff > +#define RW_SIZE 1 > + > +const char aux_dev[] = "/dev/drm_dp_aux"; > + > +static void print_usage(char *tool, int help) > +{ > + igt_info("DPCD register rw and decode tool\n\n"); > + igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > + "to be set in the kernel config.\n"); > + igt_info("Usage %s command [options...]\n", tool); > + igt_info("Supported commands are:\n" > + "\tread: Read a dpcd reg at an offset\n" > + "\twrite: Write a dpcd reg at an offset\n" > + "\tdecode: Decode the value of a dpcd reg\n"); You didn't implement decode so no point in mentioning it. > + igt_info("Options for the above commands are\n" > + "\t--device: Aux device id, IS_REQUIRED\n" > + "\t--help: print the usage\n" > + "\t--offset: DPCD register offset in hex, IS_REQUIRED\n" > + "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n" > + "\t--val: Specify a value, for reg writes\n"); I think you should model the UI based on intel_reg. > + exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE); > +} > + > +static int dpcd_read(char *device, const uint32_t *offset) Why is offset a pointer? > +{ > + int fd, ret; > + void *buf = malloc(sizeof(uint8_t)); Pointless malloc. > + > + if(NULL != buf) > + memset(buf, 0, sizeof(uint8_t)); > + else { > + igt_warn("Can't allocate read buffer\n"); > + ret = EXIT_FAILURE; > + goto out; > + } > + > + fd = open(device, O_RDONLY); > + if (fd > 0) { > + ret = pread(fd, buf, RW_SIZE, *offset); > + close(fd); > + if (ret < 0) { > + igt_warn("Failed to read from %s aux device - error %s\n", device, strerror(errno)); > + ret = EXIT_FAILURE; > + goto free_up; > + } > + printf("Value @ offset %x is %x\n", *offset, *((int *)(buf))); > + } > + else { > + igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno)); > + ret = EXIT_FAILURE; > + } > +free_up: > + free(buf); > +out: > + return ret; > +} > + > +static int dpcd_write(char *device, const uint32_t *offset, const uint8_t *val) > +{ > + int fd, ret; > + > + fd = open(device, O_RDWR); > + if (fd > 0) { > + ret = pwrite(fd, (const void *)val, RW_SIZE, *offset); > + close(fd); > + if (ret < 0) { > + igt_warn("Failed to write to %s aux device - error %s\n", device, strerror(errno)); > + ret = EXIT_FAILURE; > + goto out; > + } > + ret = dpcd_read(device, offset); > + } > + else { > + igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno)); > + ret = EXIT_FAILURE; > + } > +out: > + return ret; > +} > + > +/*TO-DO: Decode dpcd reg value. > + * By default, the latest DP/eDP spec > + * versions will be used. To override, > + * specify the spec version through the > + * --spec option. > +static void dpcd_decode() > +{ > +} > +*/ > + > +int main(int argc, char **argv) > +{ > + char dev_name[20]; > + int ret, devid, help_flg; > + uint32_t offset; > + uint8_t val; > + > + enum command { > + INV = -1, > + READ = 2, > + WRITE, > + DECODE, > + } cmd = INV; > + > + struct option longopts [] = { > + { "device", required_argument, NULL, 'd' }, > + { "help", no_argument, &help_flg, 2 }, > + { "offset", required_argument, NULL, 'o' }, > + { "spec", optional_argument, NULL, 's' }, > + { "val", required_argument, NULL, 'v' }, > + { 0 } > + }; > + > + offset = val = INVALID; > + devid = -1; > + > + while ((ret = getopt_long(argc, argv, "-:d:o:s::v:", longopts, NULL)) != -1) { What's that "-:" about? Also the sole ":"? > + switch (ret) { > + case 'd': > + devid = strtoul(optarg, NULL, 10); > + break; > + case 'o': > + offset = strtoul(optarg, NULL, 16); > + break; > + case 's': > + /* TO-DO: Parse --spec version */ > + break; > + case 'v': > + val = strtoul(optarg, NULL, 16); > + break; > + /* Fall through for --help */ > + case 0: > + break; > + /* Command parsing */ > + case 1: > + if (strcmp(optarg, "read") == 0) > + cmd = READ; > + else if (strcmp(optarg, "write") == 0) > + cmd = WRITE; > + else if (strcmp(optarg, "decode") == 0) > + cmd = DECODE; > + break; > + case ':': > + fprintf(stderr, "The -%c option of %s requires an argument\n", optopt, argv[0]); > + print_usage(argv[0], 0); > + case '?': > + default : > + printf("%s - option %c is invalid\n", argv[0], optopt); > + print_usage(argv[0], 0); > + } > + } > + > + if (help_flg == 2) > + print_usage(argv[0], 1); > + > + if(devid == -1 || offset == INVALID) { > + printf("Aux device id and/or offset missing\n"); > + print_usage(argv[0], 0); > + } > + > + memset(dev_name, '\0', 20); > + snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid); > + > + switch (cmd) { > + case READ: > + ret = dpcd_read(dev_name, &offset); > + break; > + case WRITE: > + if (val == INVALID) { > + igt_warn("Write value is missing\n"); > + print_usage(argv[0], 0); > + } > + ret = dpcd_write(dev_name, &offset, &val); > + break; > + case DECODE: > + /* TO-DO: Decode dpcd reg vals > + * drm_dpcd_decode(); > + */ > + break; > + case INV: > + default: > + igt_warn("Please specify a comand: read/write/decode. See usage\n"); > + print_usage(argv[0], 0); > + } > + > + return ret; > +} > -- > 2.13.5 > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev -- Ville Syrjälä Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä @ 2018-06-14 12:50 ` Jani Nikula 2018-06-14 12:55 ` Ville Syrjälä 2018-06-15 2:00 ` Tarun Vyas 1 sibling, 1 reply; 10+ messages in thread From: Jani Nikula @ 2018-06-14 12:50 UTC (permalink / raw) To: Ville Syrjälä, Tarun Vyas; +Cc: igt-dev, dhinakaran.pandiyan On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: >> This tool serves as a wrapper around the constructs provided by the >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n] >> devices created by the kernel module. >> It supports reading and writing dpcd registers on the connected aux >> channels. >> In the follow-up patch, support for decoding these registers will be >> added to facilate debugging panel related issues. >> >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> >> --- >> tools/Makefile.am | 2 +- >> tools/Makefile.sources | 4 +- >> tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 223 insertions(+), 2 deletions(-) >> create mode 100644 tools/dpcd_reg.c >> >> diff --git a/tools/Makefile.am b/tools/Makefile.am >> index 09b6dbcc3ece..b78824dadfdb 100644 >> --- a/tools/Makefile.am >> +++ b/tools/Makefile.am >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) >> intel_error_decode_LDFLAGS = -lz >> endif >> >> -bin_PROGRAMS += intel_dp_compliance >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg > > Not the best place for this I think. > > Missing meson.build so no one is actually going to build this. > >> intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) >> intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la >> >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources >> index abd23a0f4628..db606b28560f 100644 >> --- a/tools/Makefile.sources >> +++ b/tools/Makefile.sources >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ >> intel_dp_compliance.h \ >> intel_dp_compliance_hotplug.c \ >> $(NULL) >> - >> +dpcd_reg_SOURCES = \ >> + dpcd_reg.c \ >> + $(NULL) >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c >> new file mode 100644 >> index 000000000000..2848277aa792 >> --- /dev/null >> +++ b/tools/dpcd_reg.c >> @@ -0,0 +1,219 @@ >> +/* >> + * Copyright © 2018 Intel Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the next >> + * paragraph) shall be included in all copies or substantial portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE >> + * SOFTWARE. >> + * >> + * DPCD register read/decode tool >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read, >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set. >> + */ >> + >> +#include "igt_core.h" >> +#include <errno.h> >> +#include <fcntl.h> >> + >> +#define INVALID 0xff >> +#define RW_SIZE 1 >> + >> +const char aux_dev[] = "/dev/drm_dp_aux"; >> + >> +static void print_usage(char *tool, int help) >> +{ >> + igt_info("DPCD register rw and decode tool\n\n"); >> + igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" >> + "to be set in the kernel config.\n"); >> + igt_info("Usage %s command [options...]\n", tool); >> + igt_info("Supported commands are:\n" >> + "\tread: Read a dpcd reg at an offset\n" >> + "\twrite: Write a dpcd reg at an offset\n" >> + "\tdecode: Decode the value of a dpcd reg\n"); > > You didn't implement decode so no point in mentioning it. > >> + igt_info("Options for the above commands are\n" >> + "\t--device: Aux device id, IS_REQUIRED\n" >> + "\t--help: print the usage\n" >> + "\t--offset: DPCD register offset in hex, IS_REQUIRED\n" >> + "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n" >> + "\t--val: Specify a value, for reg writes\n"); > > I think you should model the UI based on intel_reg. I originally had plans to plug this *into* intel_reg, with e.g. dpcd: prefix. There we have the framework ready for adding names to the registers using separate definition files, and we have all the frameworks ready for doing decoded dumps of the dpcd space. Just saying. BR, Jani. -- 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] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 12:50 ` Jani Nikula @ 2018-06-14 12:55 ` Ville Syrjälä 2018-06-14 13:20 ` Jani Nikula 0 siblings, 1 reply; 10+ messages in thread From: Ville Syrjälä @ 2018-06-14 12:55 UTC (permalink / raw) To: Jani Nikula; +Cc: igt-dev, dhinakaran.pandiyan On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote: > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: > >> This tool serves as a wrapper around the constructs provided by the > >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n] > >> devices created by the kernel module. > >> It supports reading and writing dpcd registers on the connected aux > >> channels. > >> In the follow-up patch, support for decoding these registers will be > >> added to facilate debugging panel related issues. > >> > >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > >> --- > >> tools/Makefile.am | 2 +- > >> tools/Makefile.sources | 4 +- > >> tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 223 insertions(+), 2 deletions(-) > >> create mode 100644 tools/dpcd_reg.c > >> > >> diff --git a/tools/Makefile.am b/tools/Makefile.am > >> index 09b6dbcc3ece..b78824dadfdb 100644 > >> --- a/tools/Makefile.am > >> +++ b/tools/Makefile.am > >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) > >> intel_error_decode_LDFLAGS = -lz > >> endif > >> > >> -bin_PROGRAMS += intel_dp_compliance > >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg > > > > Not the best place for this I think. > > > > Missing meson.build so no one is actually going to build this. > > > >> intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) > >> intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la > >> > >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources > >> index abd23a0f4628..db606b28560f 100644 > >> --- a/tools/Makefile.sources > >> +++ b/tools/Makefile.sources > >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ > >> intel_dp_compliance.h \ > >> intel_dp_compliance_hotplug.c \ > >> $(NULL) > >> - > >> +dpcd_reg_SOURCES = \ > >> + dpcd_reg.c \ > >> + $(NULL) > >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > >> new file mode 100644 > >> index 000000000000..2848277aa792 > >> --- /dev/null > >> +++ b/tools/dpcd_reg.c > >> @@ -0,0 +1,219 @@ > >> +/* > >> + * Copyright © 2018 Intel Corporation > >> + * > >> + * Permission is hereby granted, free of charge, to any person obtaining a > >> + * copy of this software and associated documentation files (the "Software"), > >> + * to deal in the Software without restriction, including without limitation > >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > >> + * and/or sell copies of the Software, and to permit persons to whom the > >> + * Software is furnished to do so, subject to the following conditions: > >> + * > >> + * The above copyright notice and this permission notice (including the next > >> + * paragraph) shall be included in all copies or substantial portions of the > >> + * Software. > >> + * > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > >> + * SOFTWARE. > >> + * > >> + * DPCD register read/decode tool > >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read, > >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set. > >> + */ > >> + > >> +#include "igt_core.h" > >> +#include <errno.h> > >> +#include <fcntl.h> > >> + > >> +#define INVALID 0xff > >> +#define RW_SIZE 1 > >> + > >> +const char aux_dev[] = "/dev/drm_dp_aux"; > >> + > >> +static void print_usage(char *tool, int help) > >> +{ > >> + igt_info("DPCD register rw and decode tool\n\n"); > >> + igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > >> + "to be set in the kernel config.\n"); > >> + igt_info("Usage %s command [options...]\n", tool); > >> + igt_info("Supported commands are:\n" > >> + "\tread: Read a dpcd reg at an offset\n" > >> + "\twrite: Write a dpcd reg at an offset\n" > >> + "\tdecode: Decode the value of a dpcd reg\n"); > > > > You didn't implement decode so no point in mentioning it. > > > >> + igt_info("Options for the above commands are\n" > >> + "\t--device: Aux device id, IS_REQUIRED\n" > >> + "\t--help: print the usage\n" > >> + "\t--offset: DPCD register offset in hex, IS_REQUIRED\n" > >> + "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n" > >> + "\t--val: Specify a value, for reg writes\n"); > > > > I think you should model the UI based on intel_reg. > > I originally had plans to plug this *into* intel_reg, with e.g. dpcd: > prefix. There we have the framework ready for adding names to the > registers using separate definition files, and we have all the > frameworks ready for doing decoded dumps of the dpcd space. Just saying. That's one option. But maybe people working on non-Intel hw would want to use this as well? -- Ville Syrjälä Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 12:55 ` Ville Syrjälä @ 2018-06-14 13:20 ` Jani Nikula 2018-06-14 14:00 ` Ville Syrjälä 0 siblings, 1 reply; 10+ messages in thread From: Jani Nikula @ 2018-06-14 13:20 UTC (permalink / raw) To: Ville Syrjälä; +Cc: igt-dev, dhinakaran.pandiyan On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote: >> On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: >> >> This tool serves as a wrapper around the constructs provided by the >> >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n] >> >> devices created by the kernel module. >> >> It supports reading and writing dpcd registers on the connected aux >> >> channels. >> >> In the follow-up patch, support for decoding these registers will be >> >> added to facilate debugging panel related issues. >> >> >> >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> >> >> --- >> >> tools/Makefile.am | 2 +- >> >> tools/Makefile.sources | 4 +- >> >> tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 223 insertions(+), 2 deletions(-) >> >> create mode 100644 tools/dpcd_reg.c >> >> >> >> diff --git a/tools/Makefile.am b/tools/Makefile.am >> >> index 09b6dbcc3ece..b78824dadfdb 100644 >> >> --- a/tools/Makefile.am >> >> +++ b/tools/Makefile.am >> >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) >> >> intel_error_decode_LDFLAGS = -lz >> >> endif >> >> >> >> -bin_PROGRAMS += intel_dp_compliance >> >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg >> > >> > Not the best place for this I think. >> > >> > Missing meson.build so no one is actually going to build this. >> > >> >> intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) >> >> intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la >> >> >> >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources >> >> index abd23a0f4628..db606b28560f 100644 >> >> --- a/tools/Makefile.sources >> >> +++ b/tools/Makefile.sources >> >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ >> >> intel_dp_compliance.h \ >> >> intel_dp_compliance_hotplug.c \ >> >> $(NULL) >> >> - >> >> +dpcd_reg_SOURCES = \ >> >> + dpcd_reg.c \ >> >> + $(NULL) >> >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c >> >> new file mode 100644 >> >> index 000000000000..2848277aa792 >> >> --- /dev/null >> >> +++ b/tools/dpcd_reg.c >> >> @@ -0,0 +1,219 @@ >> >> +/* >> >> + * Copyright © 2018 Intel Corporation >> >> + * >> >> + * Permission is hereby granted, free of charge, to any person obtaining a >> >> + * copy of this software and associated documentation files (the "Software"), >> >> + * to deal in the Software without restriction, including without limitation >> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> >> + * and/or sell copies of the Software, and to permit persons to whom the >> >> + * Software is furnished to do so, subject to the following conditions: >> >> + * >> >> + * The above copyright notice and this permission notice (including the next >> >> + * paragraph) shall be included in all copies or substantial portions of the >> >> + * Software. >> >> + * >> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, >> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE >> >> + * SOFTWARE. >> >> + * >> >> + * DPCD register read/decode tool >> >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read, >> >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set. >> >> + */ >> >> + >> >> +#include "igt_core.h" >> >> +#include <errno.h> >> >> +#include <fcntl.h> >> >> + >> >> +#define INVALID 0xff >> >> +#define RW_SIZE 1 >> >> + >> >> +const char aux_dev[] = "/dev/drm_dp_aux"; >> >> + >> >> +static void print_usage(char *tool, int help) >> >> +{ >> >> + igt_info("DPCD register rw and decode tool\n\n"); >> >> + igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" >> >> + "to be set in the kernel config.\n"); >> >> + igt_info("Usage %s command [options...]\n", tool); >> >> + igt_info("Supported commands are:\n" >> >> + "\tread: Read a dpcd reg at an offset\n" >> >> + "\twrite: Write a dpcd reg at an offset\n" >> >> + "\tdecode: Decode the value of a dpcd reg\n"); >> > >> > You didn't implement decode so no point in mentioning it. >> > >> >> + igt_info("Options for the above commands are\n" >> >> + "\t--device: Aux device id, IS_REQUIRED\n" >> >> + "\t--help: print the usage\n" >> >> + "\t--offset: DPCD register offset in hex, IS_REQUIRED\n" >> >> + "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n" >> >> + "\t--val: Specify a value, for reg writes\n"); >> > >> > I think you should model the UI based on intel_reg. >> >> I originally had plans to plug this *into* intel_reg, with e.g. dpcd: >> prefix. There we have the framework ready for adding names to the >> registers using separate definition files, and we have all the >> frameworks ready for doing decoded dumps of the dpcd space. Just saying. > > That's one option. But maybe people working on non-Intel hw would want > to use this as well? I know, that's a consideration. It's just that it's totally obvious any DPCD tool will grow a large bunch of code already written in intel_reg.c. Looking at the proposed patch, there's only support for reading one offset at a time now. The first thing anyone will want is ability to read N bytes. Or a bunch of offsets here and there. With intel_reg, you get all of that for free. Remember the times when we had separate and slightly different reg tools for all of the different "register ports" in intel_reg? I don't particularly miss that. Note also that for i915 DP debugging, with DPCD support in intel_reg, you could have the DPCD reads interspaced with register reads. How cool is that? So I'm not going to insist on using intel_reg. I understand the counter argument. But when the two tools have grown a bunch of overlapping and duplicated code with slightly conflicting user interfaces, I reserve the right to be all, "I totally told you so." BR, Jani. -- 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] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 13:20 ` Jani Nikula @ 2018-06-14 14:00 ` Ville Syrjälä 2018-06-14 22:32 ` Dhinakaran Pandiyan 0 siblings, 1 reply; 10+ messages in thread From: Ville Syrjälä @ 2018-06-14 14:00 UTC (permalink / raw) To: Jani Nikula; +Cc: igt-dev, dhinakaran.pandiyan On Thu, Jun 14, 2018 at 04:20:07PM +0300, Jani Nikula wrote: > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote: > >> On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > >> > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: > >> >> This tool serves as a wrapper around the constructs provided by the > >> >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n] > >> >> devices created by the kernel module. > >> >> It supports reading and writing dpcd registers on the connected aux > >> >> channels. > >> >> In the follow-up patch, support for decoding these registers will be > >> >> added to facilate debugging panel related issues. > >> >> > >> >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >> >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > >> >> --- > >> >> tools/Makefile.am | 2 +- > >> >> tools/Makefile.sources | 4 +- > >> >> tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> >> 3 files changed, 223 insertions(+), 2 deletions(-) > >> >> create mode 100644 tools/dpcd_reg.c > >> >> > >> >> diff --git a/tools/Makefile.am b/tools/Makefile.am > >> >> index 09b6dbcc3ece..b78824dadfdb 100644 > >> >> --- a/tools/Makefile.am > >> >> +++ b/tools/Makefile.am > >> >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) > >> >> intel_error_decode_LDFLAGS = -lz > >> >> endif > >> >> > >> >> -bin_PROGRAMS += intel_dp_compliance > >> >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg > >> > > >> > Not the best place for this I think. > >> > > >> > Missing meson.build so no one is actually going to build this. > >> > > >> >> intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) > >> >> intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la > >> >> > >> >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources > >> >> index abd23a0f4628..db606b28560f 100644 > >> >> --- a/tools/Makefile.sources > >> >> +++ b/tools/Makefile.sources > >> >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ > >> >> intel_dp_compliance.h \ > >> >> intel_dp_compliance_hotplug.c \ > >> >> $(NULL) > >> >> - > >> >> +dpcd_reg_SOURCES = \ > >> >> + dpcd_reg.c \ > >> >> + $(NULL) > >> >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > >> >> new file mode 100644 > >> >> index 000000000000..2848277aa792 > >> >> --- /dev/null > >> >> +++ b/tools/dpcd_reg.c > >> >> @@ -0,0 +1,219 @@ > >> >> +/* > >> >> + * Copyright © 2018 Intel Corporation > >> >> + * > >> >> + * Permission is hereby granted, free of charge, to any person obtaining a > >> >> + * copy of this software and associated documentation files (the "Software"), > >> >> + * to deal in the Software without restriction, including without limitation > >> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > >> >> + * and/or sell copies of the Software, and to permit persons to whom the > >> >> + * Software is furnished to do so, subject to the following conditions: > >> >> + * > >> >> + * The above copyright notice and this permission notice (including the next > >> >> + * paragraph) shall be included in all copies or substantial portions of the > >> >> + * Software. > >> >> + * > >> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > >> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > >> >> + * SOFTWARE. > >> >> + * > >> >> + * DPCD register read/decode tool > >> >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read, > >> >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set. > >> >> + */ > >> >> + > >> >> +#include "igt_core.h" > >> >> +#include <errno.h> > >> >> +#include <fcntl.h> > >> >> + > >> >> +#define INVALID 0xff > >> >> +#define RW_SIZE 1 > >> >> + > >> >> +const char aux_dev[] = "/dev/drm_dp_aux"; > >> >> + > >> >> +static void print_usage(char *tool, int help) > >> >> +{ > >> >> + igt_info("DPCD register rw and decode tool\n\n"); > >> >> + igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > >> >> + "to be set in the kernel config.\n"); > >> >> + igt_info("Usage %s command [options...]\n", tool); > >> >> + igt_info("Supported commands are:\n" > >> >> + "\tread: Read a dpcd reg at an offset\n" > >> >> + "\twrite: Write a dpcd reg at an offset\n" > >> >> + "\tdecode: Decode the value of a dpcd reg\n"); > >> > > >> > You didn't implement decode so no point in mentioning it. > >> > > >> >> + igt_info("Options for the above commands are\n" > >> >> + "\t--device: Aux device id, IS_REQUIRED\n" > >> >> + "\t--help: print the usage\n" > >> >> + "\t--offset: DPCD register offset in hex, IS_REQUIRED\n" > >> >> + "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n" > >> >> + "\t--val: Specify a value, for reg writes\n"); > >> > > >> > I think you should model the UI based on intel_reg. > >> > >> I originally had plans to plug this *into* intel_reg, with e.g. dpcd: > >> prefix. There we have the framework ready for adding names to the > >> registers using separate definition files, and we have all the > >> frameworks ready for doing decoded dumps of the dpcd space. Just saying. > > > > That's one option. But maybe people working on non-Intel hw would want > > to use this as well? > > I know, that's a consideration. > > It's just that it's totally obvious any DPCD tool will grow a large > bunch of code already written in intel_reg.c. Looking at the proposed > patch, there's only support for reading one offset at a time now. The > first thing anyone will want is ability to read N bytes. Or a bunch of > offsets here and there. With intel_reg, you get all of that for free. Indeed. Actually that reminds me that it would be awesome to have a way to specify offset:count pairs for intel_reg read. Currently one either has to dump a bunch of irrelevant/non-existing registers, or execute intel_reg multiple times. > Remember the times when we had separate and slightly different reg tools > for all of the different "register ports" in intel_reg? I don't > particularly miss that. > > Note also that for i915 DP debugging, with DPCD support in intel_reg, > you could have the DPCD reads interspaced with register reads. How cool > is that? Yes, that could be interesting. > > So I'm not going to insist on using intel_reg. I understand the counter > argument. But when the two tools have grown a bunch of overlapping and > duplicated code with slightly conflicting user interfaces, I reserve the > right to be all, "I totally told you so." Maybe we should rename intel_reg to igt_reg and make sure it can actually be used without an Intel GPU? Other drivers could then perhaps also plug in their own backends to the same tool? Not sure how feasible that would be or whether anyone would even be interested in doing so. -- Ville Syrjälä Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 14:00 ` Ville Syrjälä @ 2018-06-14 22:32 ` Dhinakaran Pandiyan 0 siblings, 0 replies; 10+ messages in thread From: Dhinakaran Pandiyan @ 2018-06-14 22:32 UTC (permalink / raw) To: Ville Syrjälä, Jani Nikula; +Cc: igt-dev On Thu, 2018-06-14 at 17:00 +0300, Ville Syrjälä wrote: > On Thu, Jun 14, 2018 at 04:20:07PM +0300, Jani Nikula wrote: > > > > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> > > wrote: > > > > > > On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote: > > > > > > > > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.c > > > > om> wrote: > > > > > > > > > > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: > > > > > > > > > > > > This tool serves as a wrapper around the constructs > > > > > > provided by the > > > > > > drm_dpcd_aux_dev kernel module by working on the > > > > > > /dev/drm_dp_aux[n] > > > > > > devices created by the kernel module. > > > > > > It supports reading and writing dpcd registers on the > > > > > > connected aux > > > > > > channels. > > > > > > In the follow-up patch, support for decoding these > > > > > > registers will be > > > > > > added to facilate debugging panel related issues. > > > > > > > > > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@inte > > > > > > l.com> > > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > > > --- > > > > > > tools/Makefile.am | 2 +- > > > > > > tools/Makefile.sources | 4 +- > > > > > > tools/dpcd_reg.c | 219 > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 223 insertions(+), 2 deletions(-) > > > > > > create mode 100644 tools/dpcd_reg.c > > > > > > > > > > > > diff --git a/tools/Makefile.am b/tools/Makefile.am > > > > > > index 09b6dbcc3ece..b78824dadfdb 100644 > > > > > > --- a/tools/Makefile.am > > > > > > +++ b/tools/Makefile.am > > > > > > @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) > > > > > > intel_error_decode_LDFLAGS = -lz > > > > > > endif > > > > > > > > > > > > -bin_PROGRAMS += intel_dp_compliance > > > > > > +bin_PROGRAMS += intel_dp_compliance dpcd_reg > > > > > Not the best place for this I think. > > > > > > > > > > Missing meson.build so no one is actually going to build > > > > > this. > > > > > > > > > > > > > > > > > intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) > > > > > > intel_dp_compliance_LDADD = > > > > > > $(top_builddir)/lib/libintel_tools.la > > > > > > > > > > > > diff --git a/tools/Makefile.sources > > > > > > b/tools/Makefile.sources > > > > > > index abd23a0f4628..db606b28560f 100644 > > > > > > --- a/tools/Makefile.sources > > > > > > +++ b/tools/Makefile.sources > > > > > > @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ > > > > > > intel_dp_compliance.h \ > > > > > > intel_dp_compliance_hotplug.c \ > > > > > > $(NULL) > > > > > > - > > > > > > +dpcd_reg_SOURCES = \ > > > > > > + dpcd_reg.c \ > > > > > > + $(NULL) > > > > > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > > > > > > new file mode 100644 > > > > > > index 000000000000..2848277aa792 > > > > > > --- /dev/null > > > > > > +++ b/tools/dpcd_reg.c > > > > > > @@ -0,0 +1,219 @@ > > > > > > +/* > > > > > > + * Copyright © 2018 Intel Corporation > > > > > > + * > > > > > > + * Permission is hereby granted, free of charge, to any > > > > > > person obtaining a > > > > > > + * copy of this software and associated documentation > > > > > > files (the "Software"), > > > > > > + * to deal in the Software without restriction, including > > > > > > without limitation > > > > > > + * the rights to use, copy, modify, merge, publish, > > > > > > distribute, sublicense, > > > > > > + * and/or sell copies of the Software, and to permit > > > > > > persons to whom the > > > > > > + * Software is furnished to do so, subject to the > > > > > > following conditions: > > > > > > + * > > > > > > + * The above copyright notice and this permission notice > > > > > > (including the next > > > > > > + * paragraph) shall be included in all copies or > > > > > > substantial portions of the > > > > > > + * Software. > > > > > > + * > > > > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF > > > > > > ANY KIND, EXPRESS OR > > > > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > > > > > MERCHANTABILITY, > > > > > > + * FITNESS FOR A PARTICULAR PURPOSE AND > > > > > > NONINFRINGEMENT. IN NO EVENT SHALL > > > > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY > > > > > > CLAIM, DAMAGES OR OTHER > > > > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR > > > > > > OTHERWISE, ARISING FROM, > > > > > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > > > > > OTHER DEALINGS IN THE > > > > > > + * SOFTWARE. > > > > > > + * > > > > > > + * DPCD register read/decode tool > > > > > > + * This tool wraps around DRM_DP_AUX_DEV module to provide > > > > > > DPCD register read, > > > > > > + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be > > > > > > set. > > > > > > + */ > > > > > > + > > > > > > +#include "igt_core.h" > > > > > > +#include <errno.h> > > > > > > +#include <fcntl.h> > > > > > > + > > > > > > +#define INVALID 0xff > > > > > > +#define RW_SIZE 1 > > > > > > + > > > > > > +const char aux_dev[] = "/dev/drm_dp_aux"; > > > > > > + > > > > > > +static void print_usage(char *tool, int help) > > > > > > +{ > > > > > > + igt_info("DPCD register rw and decode tool\n\n"); > > > > > > + igt_info("This tool requires > > > > > > CONFIG_DRM_DP_AUX_CHARDEV\n" > > > > > > + "to be set in the kernel config.\n"); > > > > > > + igt_info("Usage %s command [options...]\n", tool); > > > > > > + igt_info("Supported commands are:\n" > > > > > > + "\tread: Read a dpcd reg at an offset\n" > > > > > > + "\twrite: Write a dpcd reg at an > > > > > > offset\n" > > > > > > + "\tdecode: Decode the value of a dpcd > > > > > > reg\n"); > > > > > You didn't implement decode so no point in mentioning it. > > > > > > > > > > > > > > > > > + igt_info("Options for the above commands are\n" > > > > > > + "\t--device: Aux device id, > > > > > > IS_REQUIRED\n" > > > > > > + "\t--help: print the usage\n" > > > > > > + "\t--offset: DPCD register offset in hex, > > > > > > IS_REQUIRED\n" > > > > > > + "\t--spec: Specify DP/eDP spec version > > > > > > for decoding, IS_OPTIONAL\n" > > > > > > + "\t--val: Specify a value, for reg > > > > > > writes\n"); > > > > > I think you should model the UI based on intel_reg. > > > > I originally had plans to plug this *into* intel_reg, with e.g. > > > > dpcd: > > > > prefix. There we have the framework ready for adding names to > > > > the > > > > registers using separate definition files, and we have all the > > > > frameworks ready for doing decoded dumps of the dpcd space. I suppose we could go with this tool as it is now and then later reuse parts of the decode logic from intel_reg. > > > > Just saying. > > > That's one option. But maybe people working on non-Intel hw would > > > want > > > to use this as well? > > I know, that's a consideration. > > > > It's just that it's totally obvious any DPCD tool will grow a large > > bunch of code already written in intel_reg.c. Looking at the > > proposed > > patch, there's only support for reading one offset at a time now. > > The > > first thing anyone will want is ability to read N bytes. Or a bunch > > of > > offsets here and there. With intel_reg, you get all of that for > > free. > Indeed. Actually that reminds me that it would be awesome to have a > way > to specify offset:count pairs for intel_reg read. Currently one > either > has to dump a bunch of irrelevant/non-existing registers, or execute > intel_reg multiple times. > > > > > Remember the times when we had separate and slightly different reg > > tools > > for all of the different "register ports" in intel_reg? I don't > > particularly miss that. > > > > Note also that for i915 DP debugging, with DPCD support in > > intel_reg, > > you could have the DPCD reads interspaced with register reads. How > > cool > > is that? > Yes, that could be interesting. > > > > > So I'm not going to insist on using intel_reg. I understand the > > counter > > argument. But when the two tools have grown a bunch of overlapping > > and > > duplicated code with slightly conflicting user interfaces, I > > reserve the > > right to be all, "I totally told you so." > Maybe we should rename intel_reg to igt_reg and make sure it can > actually be used without an Intel GPU? Other drivers could then > perhaps also plug in their own backends to the same tool? Not sure > how feasible that would be or whether anyone would even be interested > in doing so. > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers 2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä 2018-06-14 12:50 ` Jani Nikula @ 2018-06-15 2:00 ` Tarun Vyas 1 sibling, 0 replies; 10+ messages in thread From: Tarun Vyas @ 2018-06-15 2:00 UTC (permalink / raw) To: Ville Syrjälä; +Cc: igt-dev, dhinakaran.pandiyan On Thu, Jun 14, 2018 at 01:43:59PM +0300, Ville Syrjälä wrote: > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote: > > This tool serves as a wrapper around the constructs provided by the > > drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n] > > devices created by the kernel module. > > It supports reading and writing dpcd registers on the connected aux > > channels. > > In the follow-up patch, support for decoding these registers will be > > added to facilate debugging panel related issues. > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > tools/Makefile.am | 2 +- > > tools/Makefile.sources | 4 +- > > tools/dpcd_reg.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 223 insertions(+), 2 deletions(-) > > create mode 100644 tools/dpcd_reg.c > > > > diff --git a/tools/Makefile.am b/tools/Makefile.am > > index 09b6dbcc3ece..b78824dadfdb 100644 > > --- a/tools/Makefile.am > > +++ b/tools/Makefile.am > > @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN) > > intel_error_decode_LDFLAGS = -lz > > endif > > > > -bin_PROGRAMS += intel_dp_compliance > > +bin_PROGRAMS += intel_dp_compliance dpcd_reg > > Not the best place for this I think. > > Missing meson.build so no one is actually going to build this. > Thanks for the review Ville. Will fix in v2. > > intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS) > > intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la > > > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > > index abd23a0f4628..db606b28560f 100644 > > --- a/tools/Makefile.sources > > +++ b/tools/Makefile.sources > > @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \ > > intel_dp_compliance.h \ > > intel_dp_compliance_hotplug.c \ > > $(NULL) > > - > > +dpcd_reg_SOURCES = \ > > + dpcd_reg.c \ > > + $(NULL) > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > > new file mode 100644 > > index 000000000000..2848277aa792 > > --- /dev/null > > +++ b/tools/dpcd_reg.c > > @@ -0,0 +1,219 @@ > > +/* > > + * Copyright © 2018 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > > + * SOFTWARE. > > + * > > + * DPCD register read/decode tool > > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read, > > + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set. > > + */ > > + > > +#include "igt_core.h" > > +#include <errno.h> > > +#include <fcntl.h> > > + > > +#define INVALID 0xff > > +#define RW_SIZE 1 > > + > > +const char aux_dev[] = "/dev/drm_dp_aux"; > > + > > +static void print_usage(char *tool, int help) > > +{ > > + igt_info("DPCD register rw and decode tool\n\n"); > > + igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > > + "to be set in the kernel config.\n"); > > + igt_info("Usage %s command [options...]\n", tool); > > + igt_info("Supported commands are:\n" > > + "\tread: Read a dpcd reg at an offset\n" > > + "\twrite: Write a dpcd reg at an offset\n" > > + "\tdecode: Decode the value of a dpcd reg\n"); > > You didn't implement decode so no point in mentioning it. > Will remove in v2. > > + igt_info("Options for the above commands are\n" > > + "\t--device: Aux device id, IS_REQUIRED\n" > > + "\t--help: print the usage\n" > > + "\t--offset: DPCD register offset in hex, IS_REQUIRED\n" > > + "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n" > > + "\t--val: Specify a value, for reg writes\n"); > > I think you should model the UI based on intel_reg. > WIll change in v2. > > + exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE); > > +} > > + > > +static int dpcd_read(char *device, const uint32_t *offset) > > Why is offset a pointer? > I wasn't thinking :( > > +{ > > + int fd, ret; > > + void *buf = malloc(sizeof(uint8_t)); > > Pointless malloc. > Yea, now I've added the --count option to read multiple bytes at a time, so with that we can keep the malloc in v2. > > + > > + if(NULL != buf) > > + memset(buf, 0, sizeof(uint8_t)); > > + else { > > + igt_warn("Can't allocate read buffer\n"); > > + ret = EXIT_FAILURE; > > + goto out; > > + } > > + > > + fd = open(device, O_RDONLY); > > + if (fd > 0) { > > + ret = pread(fd, buf, RW_SIZE, *offset); > > + close(fd); > > + if (ret < 0) { > > + igt_warn("Failed to read from %s aux device - error %s\n", device, strerror(errno)); > > + ret = EXIT_FAILURE; > > + goto free_up; > > + } > > + printf("Value @ offset %x is %x\n", *offset, *((int *)(buf))); > > + } > > + else { > > + igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno)); > > + ret = EXIT_FAILURE; > > + } > > +free_up: > > + free(buf); > > +out: > > + return ret; > > +} > > + > > +static int dpcd_write(char *device, const uint32_t *offset, const uint8_t *val) > > +{ > > + int fd, ret; > > + > > + fd = open(device, O_RDWR); > > + if (fd > 0) { > > + ret = pwrite(fd, (const void *)val, RW_SIZE, *offset); > > + close(fd); > > + if (ret < 0) { > > + igt_warn("Failed to write to %s aux device - error %s\n", device, strerror(errno)); > > + ret = EXIT_FAILURE; > > + goto out; > > + } > > + ret = dpcd_read(device, offset); > > + } > > + else { > > + igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno)); > > + ret = EXIT_FAILURE; > > + } > > +out: > > + return ret; > > +} > > + > > +/*TO-DO: Decode dpcd reg value. > > + * By default, the latest DP/eDP spec > > + * versions will be used. To override, > > + * specify the spec version through the > > + * --spec option. > > +static void dpcd_decode() > > +{ > > +} > > +*/ > > + > > +int main(int argc, char **argv) > > +{ > > + char dev_name[20]; > > + int ret, devid, help_flg; > > + uint32_t offset; > > + uint8_t val; > > + > > + enum command { > > + INV = -1, > > + READ = 2, > > + WRITE, > > + DECODE, > > + } cmd = INV; > > + > > + struct option longopts [] = { > > + { "device", required_argument, NULL, 'd' }, > > + { "help", no_argument, &help_flg, 2 }, > > + { "offset", required_argument, NULL, 'o' }, > > + { "spec", optional_argument, NULL, 's' }, > > + { "val", required_argument, NULL, 'v' }, > > + { 0 } > > + }; > > + > > + offset = val = INVALID; > > + devid = -1; > > + > > + while ((ret = getopt_long(argc, argv, "-:d:o:s::v:", longopts, NULL)) != -1) { > > What's that "-:" about? Also the sole ":"? > I wanted to make getopt_long parse *all* the arguments, so with the '-' in the begining, it will redirect all the non-option arguments to case 1, which in this case will be the command. The sole ':' is to distinguish between an option with missing argument vs. an invalid option which goes to '?'. Also, the first ':' after the '-' of the optstring "-:d:o:s::v:" will enable the distinction between "case :" and "case ?". > > + switch (ret) { > > + case 'd': > > + devid = strtoul(optarg, NULL, 10); > > + break; > > + case 'o': > > + offset = strtoul(optarg, NULL, 16); > > + break; > > + case 's': > > + /* TO-DO: Parse --spec version */ > > + break; > > + case 'v': > > + val = strtoul(optarg, NULL, 16); > > + break; > > + /* Fall through for --help */ > > + case 0: > > + break; > > + /* Command parsing */ > > + case 1: > > + if (strcmp(optarg, "read") == 0) > > + cmd = READ; > > + else if (strcmp(optarg, "write") == 0) > > + cmd = WRITE; > > + else if (strcmp(optarg, "decode") == 0) > > + cmd = DECODE; > > + break; > > + case ':': > > + fprintf(stderr, "The -%c option of %s requires an argument\n", optopt, argv[0]); > > + print_usage(argv[0], 0); > > + case '?': > > + default : > > + printf("%s - option %c is invalid\n", argv[0], optopt); > > + print_usage(argv[0], 0); > > + } > > + } > > + > > + if (help_flg == 2) > > + print_usage(argv[0], 1); > > + > > + if(devid == -1 || offset == INVALID) { > > + printf("Aux device id and/or offset missing\n"); > > + print_usage(argv[0], 0); > > + } > > + > > + memset(dev_name, '\0', 20); > > + snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid); > > + > > + switch (cmd) { > > + case READ: > > + ret = dpcd_read(dev_name, &offset); > > + break; > > + case WRITE: > > + if (val == INVALID) { > > + igt_warn("Write value is missing\n"); > > + print_usage(argv[0], 0); > > + } > > + ret = dpcd_write(dev_name, &offset, &val); > > + break; > > + case DECODE: > > + /* TO-DO: Decode dpcd reg vals > > + * drm_dpcd_decode(); > > + */ > > + break; > > + case INV: > > + default: > > + igt_warn("Please specify a comand: read/write/decode. See usage\n"); > > + print_usage(argv[0], 0); > > + } > > + > > + return ret; > > +} > > -- > > 2.13.5 > > > > _______________________________________________ > > igt-dev mailing list > > igt-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/igt-dev > > -- > Ville Syrjälä > Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-06-15 2:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-14 7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas 2018-06-14 8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork 2018-06-14 9:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork 2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä 2018-06-14 12:50 ` Jani Nikula 2018-06-14 12:55 ` Ville Syrjälä 2018-06-14 13:20 ` Jani Nikula 2018-06-14 14:00 ` Ville Syrjälä 2018-06-14 22:32 ` Dhinakaran Pandiyan 2018-06-15 2:00 ` Tarun Vyas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox