* [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
@ 2018-09-14 23:57 Tarun Vyas
2018-09-15 0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Tarun Vyas @ 2018-09-14 23:57 UTC (permalink / raw)
To: igt-dev; +Cc: dhinakaran.pandiyan, rodrigo.vivi
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.
v2: (Fixes by Rodrigo but no functional changes yet):
- Indentations, Typo, Missed spaces
- Removing mentioning to decode and spec that is not implemented yet.
- Add Makefile.sources back
- Missed s/printf/igt_warn
v3:
- Addres DK's review comments from v2 above.
- Squash Rodrigo's file handling unification patch.
- Make count, offset and device id optional.
Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
tools/Makefile.sources | 1 +
tools/dpcd_reg.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++
tools/meson.build | 1 +
3 files changed, 215 insertions(+)
create mode 100644 tools/dpcd_reg.c
diff --git a/tools/Makefile.sources b/tools/Makefile.sources
index abd23a0f..50706f41 100644
--- a/tools/Makefile.sources
+++ b/tools/Makefile.sources
@@ -7,6 +7,7 @@ noinst_PROGRAMS = \
tools_prog_lists = \
igt_stats \
+ dpcd_reg \
intel_audio_dump \
intel_reg \
intel_backlight \
diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
new file mode 100644
index 00000000..cd9fed4f
--- /dev/null
+++ b/tools/dpcd_reg.c
@@ -0,0 +1,213 @@
+/*
+ * 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/write tool
+ * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read
+ * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
+ */
+
+#include "igt_core.h"
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+
+#define MAX_OFFSET 0xf02ff
+
+const char aux_dev[] = "/dev/drm_dp_aux";
+
+static void print_usage(char *tool, int exit_code)
+{
+ printf("DPCD register read and write tool\n\n");
+ printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
+ "to be set in the kernel config.\n\n");
+ printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
+ printf("COMMAND is one of:\n");
+ printf(" read: Read [count] bytes dpcd reg at an offset\n");
+ printf(" write: Write a dpcd reg at an offset\n\n");
+ printf("Options for the above COMMANDS are\n");
+ printf(" --device=DEVID Aux device id, as listed in /dev/drm_dp_aux_dev[n]."
+ "Defaults to 0\n");
+ printf(" --offset=REG_ADDR DPCD register offset in hex. Defaults to 0x00\n");
+ printf(" --count=BYTES For reads, specify number of bytes to be read from"
+ "the offset. Defaults to 1\n");
+ printf(" --val For writes, specify a hex value to be written\n\n");
+
+ printf(" --help: print the usage\n");
+
+ exit(exit_code);
+}
+
+static int dpcd_read(int fd, const uint32_t offset, size_t count)
+{
+ int ret, i;
+ void *buf = calloc(count, sizeof(uint8_t));
+
+ if (!buf) {
+ fprintf(stderr, "Can't allocate read buffer\n");
+ return ENOMEM;
+ }
+
+ ret = pread(fd, buf, count, offset);
+ if (ret != count) {
+ fprintf(stderr, "Failed to read - %s\n", strerror(errno));
+ ret = errno;
+ goto out;
+ } else
+ ret = EXIT_SUCCESS;
+
+ printf("Read %zu byte(s) starting at offset %x\n\n", count, offset);
+ for (i = 0; i < count; i++)
+ printf(" %02x", *(((uint8_t *)(buf)) + i));
+ printf("\n");
+
+out:
+ free(buf);
+ return ret;
+}
+
+static int dpcd_write(int fd, const uint32_t offset, const uint8_t val)
+{
+ int ret;
+
+ ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
+ if (ret < 0) {
+ fprintf(stderr, "Failed to write - %s\n", strerror(errno));
+ return errno;
+ } else
+ return EXIT_SUCCESS;
+}
+
+int main(int argc, char **argv)
+{
+ char dev_name[20];
+ int ret, devid, fd, vflag = 0;
+ uint32_t offset;
+ uint8_t val;
+ size_t count;
+ int file_op = O_RDONLY;
+
+ enum command {
+ INV = -1,
+ READ = 2,
+ WRITE,
+ } cmd = INV;
+
+ struct option longopts[] = {
+ { "count", required_argument, NULL, 'c' },
+ { "device", required_argument, NULL, 'd' },
+ { "help", no_argument, NULL, 'h' },
+ { "offset", required_argument, NULL, 'o' },
+ { "value", required_argument, &vflag, 'v' },
+ { 0 }
+ };
+
+ devid = 0, count = 1, offset = 0x0;
+
+ while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, NULL)) != -1) {
+ switch (ret) {
+ case 'c':
+ count = strtoul(optarg, NULL, 10);
+ if (count == ULONG_MAX) {
+ fprintf(stderr, "Count argument too big\n");
+ exit(ERANGE);
+ }
+ break;
+ case 'd':
+ devid = strtoul(optarg, NULL, 10);
+ if (devid == ULONG_MAX) {
+ fprintf(stderr, "Devid argument too big\n");
+ exit(ERANGE);
+ }
+ break;
+ case 'h':
+ print_usage(argv[0], EXIT_SUCCESS);
+ break;
+ case 'o':
+ offset = strtoul(optarg, NULL, 16);
+ if (offset > MAX_OFFSET) {
+ fprintf(stderr, "Offset should be <= 0xf02ff\n");
+ exit(ERANGE);
+ }
+ break;
+ case 0:
+ if (vflag == 'v' && optarg)
+ val = (uint8_t) strtoul(optarg, NULL, 16);
+ break;
+ /* Command parsing */
+ case 1:
+ if (strcmp(optarg, "read") == 0) {
+ cmd = READ;
+ } else if (strcmp(optarg, "write") == 0) {
+ cmd = WRITE;
+ file_op = O_WRONLY;
+ }
+ break;
+ case ':':
+ fprintf(stderr, "The -%c option of %s requires an argument\n",
+ optopt, argv[0]);
+ print_usage(argv[0], EXIT_FAILURE);
+ case '?':
+ default:
+ fprintf(stderr, "%s - option %c is invalid\n", argv[0],
+ optopt);
+ print_usage(argv[0], EXIT_FAILURE);
+ }
+ }
+
+ if ((count + offset) > (MAX_OFFSET + 1)) {
+ fprintf(stderr, "Out of bounds. Count + Offset <= 0xf0300\n");
+ exit(ERANGE);
+ }
+
+ if ((cmd == WRITE) && (vflag != 'v')) {
+ fprintf(stderr, "Write value is missing\n");
+ print_usage(argv[0], EXIT_FAILURE);
+ }
+
+ memset(dev_name, '\0', 20);
+ snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, devid);
+
+ fd = open(dev_name, file_op);
+ if (fd < 0) {
+ fprintf(stderr, "Failed to open %s aux device - error: %s\n", dev_name,
+ strerror(errno));
+ return errno;
+ }
+
+ switch (cmd) {
+ case READ:
+ ret = dpcd_read(fd, offset, count);
+ break;
+ case WRITE:
+ ret = dpcd_write(fd, offset, val);
+ break;
+ case INV:
+ default:
+ fprintf(stderr, "Please specify a command: read/write. See usage\n");
+ close(fd);
+ print_usage(argv[0], EXIT_FAILURE);
+ }
+
+ close(fd);
+
+ return ret;
+}
diff --git a/tools/meson.build b/tools/meson.build
index e4517d66..79f36aa9 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -36,6 +36,7 @@ tools_progs = [
'intel_watermark',
'intel_gem_info',
'intel_gvtg_test',
+ 'dpcd_reg',
]
tool_deps = igt_deps
--
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: Add a simple tool to read/write/decode dpcd registers (rev3) 2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas @ 2018-09-15 0:24 ` Patchwork 2018-09-15 3:25 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork ` (2 subsequent siblings) 3 siblings, 0 replies; 8+ messages in thread From: Patchwork @ 2018-09-15 0:24 UTC (permalink / raw) To: Tarun Vyas; +Cc: igt-dev == Series Details == Series: tools: Add a simple tool to read/write/decode dpcd registers (rev3) URL : https://patchwork.freedesktop.org/series/44736/ State : success == Summary == = CI Bug Log - changes from CI_DRM_4827 -> IGTPW_1847 = == Summary - SUCCESS == No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/44736/revisions/3/mbox/ == Known issues == Here are the changes found in IGTPW_1847 that come from known issues: === IGT changes === ==== Issues hit ==== igt@drv_selftest@live_coherency: fi-gdg-551: PASS -> DMESG-FAIL (fdo#107164) igt@gem_exec_suspend@basic-s4-devices: fi-blb-e6850: NOTRUN -> INCOMPLETE (fdo#107718) igt@kms_frontbuffer_tracking@basic: fi-byt-clapper: PASS -> FAIL (fdo#103167) igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b: fi-ilk-650: PASS -> DMESG-WARN (fdo#106387) igt@kms_psr@primary_page_flip: fi-kbl-7560u: PASS -> FAIL (fdo#107336) ==== Possible fixes ==== igt@gem_exec_suspend@basic-s3: fi-blb-e6850: INCOMPLETE (fdo#107718) -> PASS igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence: fi-byt-clapper: FAIL (fdo#107362, fdo#103191) -> PASS fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191 fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387 fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164 fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336 fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362 fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718 == Participating hosts (48 -> 43) == Missing (5): fi-bsw-cyan fi-ilk-m540 fi-byt-squawks fi-hsw-4200u fi-pnv-d510 == Build changes == * IGT: IGT_4642 -> IGTPW_1847 CI_DRM_4827: 8b1968f143e8bf65acf0ea6f7ce9120259043521 @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_1847: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1847/ IGT_4642: 7b3ea4efb9713cd67e17e33202fa9d0681a284d1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1847/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: failure for tools: Add a simple tool to read/write/decode dpcd registers (rev3) 2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas 2018-09-15 0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork @ 2018-09-15 3:25 ` Patchwork 2018-09-21 6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan 2018-09-22 23:34 ` Dhinakaran Pandiyan 3 siblings, 0 replies; 8+ messages in thread From: Patchwork @ 2018-09-15 3:25 UTC (permalink / raw) To: Tarun Vyas; +Cc: igt-dev == Series Details == Series: tools: Add a simple tool to read/write/decode dpcd registers (rev3) URL : https://patchwork.freedesktop.org/series/44736/ State : failure == Summary == = CI Bug Log - changes from IGT_4642_full -> IGTPW_1847_full = == Summary - FAILURE == Serious unknown changes coming with IGTPW_1847_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in IGTPW_1847_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/3/mbox/ == Possible new issues == Here are the unknown changes that may have been introduced in IGTPW_1847_full: === IGT changes === ==== Possible regressions ==== igt@kms_busy@extended-modeset-hang-newfb-render-c: shard-glk: PASS -> DMESG-WARN +11 igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b: shard-kbl: PASS -> DMESG-WARN +8 shard-snb: NOTRUN -> DMESG-WARN igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c: shard-apl: PASS -> DMESG-WARN +10 igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b: shard-hsw: PASS -> DMESG-WARN +8 shard-snb: PASS -> DMESG-WARN +3 ==== Warnings ==== igt@pm_rc6_residency@rc6-accuracy: shard-snb: PASS -> SKIP == Known issues == Here are the changes found in IGTPW_1847_full that come from known issues: === IGT changes === ==== Issues hit ==== igt@drv_suspend@sysfs-reader: shard-kbl: PASS -> INCOMPLETE (fdo#103665) +1 igt@gem_ctx_isolation@bcs0-s3: shard-apl: PASS -> INCOMPLETE (fdo#103927) igt@gem_exec_await@wide-contexts: shard-glk: PASS -> FAIL (fdo#106680) shard-kbl: PASS -> FAIL (fdo#106680) igt@gem_ppgtt@blt-vs-render-ctxn: shard-kbl: PASS -> INCOMPLETE (fdo#103665, fdo#106023) igt@kms_cursor_crc@cursor-64x21-offscreen: shard-glk: PASS -> INCOMPLETE (k.org#198133, fdo#103359) igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic: shard-glk: PASS -> FAIL (fdo#105454, fdo#106509) igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions-varying-size: shard-hsw: PASS -> FAIL (fdo#103355) igt@kms_plane@pixel-format-pipe-a-planes: shard-snb: PASS -> FAIL (fdo#103166, fdo#107749) igt@kms_plane_scaling@pipe-a-scaler-with-rotation: shard-glk: PASS -> DMESG-WARN (fdo#105763, fdo#106538) +1 igt@kms_setmode@basic: shard-kbl: PASS -> FAIL (fdo#99912) ==== Possible fixes ==== igt@gem_exec_schedule@preempt-hang-bsd1: shard-snb: INCOMPLETE (fdo#105411) -> SKIP igt@kms_flip@2x-blocking-absolute-wf_vblank: shard-glk: INCOMPLETE (k.org#198133, fdo#103359) -> PASS igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-cpu: shard-glk: FAIL (fdo#103167) -> PASS +2 igt@kms_setmode@basic: shard-apl: FAIL (fdo#99912) -> PASS igt@kms_vblank@pipe-b-wait-forked-busy-hang: shard-glk: DMESG-WARN (fdo#105763, fdo#106538) -> PASS +2 fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166 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#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927 fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411 fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454 fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763 fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023 fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509 fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538 fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680 fdo#107749 https://bugs.freedesktop.org/show_bug.cgi?id=107749 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_4642 -> IGTPW_1847 * Linux: CI_DRM_4825 -> CI_DRM_4827 CI_DRM_4825: b42528aaa961c0d469f381b4a5c3830fe46aedfa @ git://anongit.freedesktop.org/gfx-ci/linux CI_DRM_4827: 8b1968f143e8bf65acf0ea6f7ce9120259043521 @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_1847: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1847/ IGT_4642: 7b3ea4efb9713cd67e17e33202fa9d0681a284d1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1847/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 v3] tools: Add a simple tool to read/write/decode dpcd registers 2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas 2018-09-15 0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork 2018-09-15 3:25 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork @ 2018-09-21 6:12 ` Dhinakaran Pandiyan 2018-09-21 20:36 ` Tarun Vyas 2018-09-22 23:34 ` Dhinakaran Pandiyan 3 siblings, 1 reply; 8+ messages in thread From: Dhinakaran Pandiyan @ 2018-09-21 6:12 UTC (permalink / raw) To: igt-dev; +Cc: rodrigo.vivi On Friday, September 14, 2018 4:57:02 PM PDT 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. > > v2: (Fixes by Rodrigo but no functional changes yet): > - Indentations, Typo, Missed spaces > - Removing mentioning to decode and spec that is not implemented yet. > - Add Makefile.sources back > - Missed s/printf/igt_warn > > v3: > - Addres DK's review comments from v2 above. > - Squash Rodrigo's file handling unification patch. > - Make count, offset and device id optional. > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > tools/Makefile.sources | 1 + > tools/dpcd_reg.c | 213 > +++++++++++++++++++++++++++++++++++++++++++++++++ tools/meson.build | > 1 + > 3 files changed, 215 insertions(+) > create mode 100644 tools/dpcd_reg.c > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > index abd23a0f..50706f41 100644 > --- a/tools/Makefile.sources > +++ b/tools/Makefile.sources > @@ -7,6 +7,7 @@ noinst_PROGRAMS = \ > > tools_prog_lists = \ > igt_stats \ > + dpcd_reg \ > intel_audio_dump \ > intel_reg \ > intel_backlight \ > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > new file mode 100644 > index 00000000..cd9fed4f > --- /dev/null > +++ b/tools/dpcd_reg.c > @@ -0,0 +1,213 @@ > +/* > + * 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/write tool > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set. > + */ > + > +#include "igt_core.h" What's the dependency on igt_core here? > +#include <errno.h> > +#include <fcntl.h> > +#include <limits.h> > + > +#define MAX_OFFSET 0xf02ff I think we should be able to allow allow up to the max limit of 0xfffff (DP 1.4 section 2.9.3) > + > +const char aux_dev[] = "/dev/drm_dp_aux"; > + > +static void print_usage(char *tool, int exit_code) > +{ > + printf("DPCD register read and write tool\n\n"); > + printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > + "to be set in the kernel config.\n\n"); I think it would look better to print the above lines only when --help/-h was passed. Move this under case 'h' ? > + printf("Usage: %s [OPTION ...] COMMAND\n\n", tool); > + printf("COMMAND is one of:\n"); > + printf(" read: Read [count] bytes dpcd reg at an offset\n"); > + printf(" write: Write a dpcd reg at an offset\n\n"); > + printf("Options for the above COMMANDS are\n"); > + printf(" --device=DEVID Aux device id, as listed in > /dev/drm_dp_aux_dev[n]." + "Defaults to 0\n"); > + printf(" --offset=REG_ADDR DPCD register offset in hex. Defaults to > 0x00\n"); + printf(" --count=BYTES For reads, specify number of bytes to > be read from" + "the offset. Defaults to 1\n"); > + printf(" --val For writes, specify a hex value to be written\n\n"); > + > + printf(" --help: print the usage\n"); > + > + exit(exit_code); Modify the callers so that you don't have to exit from a print function. > +} > + > +static int dpcd_read(int fd, const uint32_t offset, size_t count) const is not needed as the arguments are passed by value. > +{ > + int ret, i; > + void *buf = calloc(count, sizeof(uint8_t)); uint8_t *buf ? > + > + if (!buf) { > + fprintf(stderr, "Can't allocate read buffer\n"); > + return ENOMEM; > + } > + > + ret = pread(fd, buf, count, offset); > + if (ret != count) { > + fprintf(stderr, "Failed to read - %s\n", strerror(errno)); Print ret too? If the number of bytes read were lower, errno won't be set. > + ret = errno; > + goto out; > + } else } else { > + ret = EXIT_SUCCESS; Intialize ret and get rid of the else block? > + > + printf("Read %zu byte(s) starting at offset %x\n\n", count, offset); Print this debug message only if the expected number of bytes weren't read and something like printf("Read %zu bytes, expected %zu\n"); printf("0x"); to clarify that the printed values are in hex. > + for (i = 0; i < count; i++) > + printf(" %02x", *(((uint8_t *)(buf)) + i)); You can avoid typecasting if you define the array as type uint8_t > + printf("\n"); > + > +out: > + free(buf); > + return ret; > +} > + > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t val) const isn't needed. > +{ > + int ret; > + > + ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset); > + if (ret < 0) { > + fprintf(stderr, "Failed to write - %s\n", strerror(errno)); Same as above, print number of bytes written in case of error. > + return errno; > + } else > + return EXIT_SUCCESS; You could avoid 'else' here. > +} > + > +int main(int argc, char **argv) > +{ > + char dev_name[20]; > + int ret, devid, fd, vflag = 0; > + uint32_t offset; > + uint8_t val; > + size_t count; > + int file_op = O_RDONLY; > + > + enum command { > + INV = -1, > + READ = 2, Any reason to not use the value compiler generates? > + WRITE, > + } cmd = INV; > + > + struct option longopts[] = { > + { "count", required_argument, NULL, 'c' }, > + { "device", required_argument, NULL, 'd' }, > + { "help", no_argument, NULL, 'h' }, > + { "offset", required_argument, NULL, 'o' }, > + { "value", required_argument, &vflag, 'v' }, > + { 0 } > + }; > + > + devid = 0, count = 1, offset = 0x0; > + > + while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, NULL)) != Any reason to leave out the option -v ? And '-h' shouldn't need an argument. > -1) { + switch (ret) { > + case 'c': > + count = strtoul(optarg, NULL, 10); With strtol() you should be able reject all negative args and make use of the second argument to reject invalid numbers. > + if (count == ULONG_MAX) { > + fprintf(stderr, "Count argument too big\n"); > + exit(ERANGE); > + } > + break; > + case 'd': > + devid = strtoul(optarg, NULL, 10); Same here, strtol() to reject negatives and use **endptr to fail on invalid numbers. > + if (devid == ULONG_MAX) { > + fprintf(stderr, "Devid argument too big\n"); > + exit(ERANGE); > + } > + break; > + case 'h': > + print_usage(argv[0], EXIT_SUCCESS); > + break; > + case 'o': > + offset = strtoul(optarg, NULL, 16); Same comment as for 'd'. You might also want to check if errno is set. > + if (offset > MAX_OFFSET) { > + fprintf(stderr, "Offset should be <= 0xf02ff\n"); > + exit(ERANGE); > + } > + break; > + case 0: > + if (vflag == 'v' && optarg) I didn't get why &vflag had to be used. > + val = (uint8_t) strtoul(optarg, NULL, 16); Reject values greater than 0xff? > + break; > + /* Command parsing */ > + case 1: > + if (strcmp(optarg, "read") == 0) { > + cmd = READ; > + } else if (strcmp(optarg, "write") == 0) { > + cmd = WRITE; > + file_op = O_WRONLY; > + } > + break; } else { and exit here? or fall through to default block by re-arranging the blocks. > + case ':': > + fprintf(stderr, "The -%c option of %s requires an argument\n", > + optopt, argv[0]); Prints "The -c option of tools/dpcd_reg requires an argument", is that what you intended? Skip printing the tool path? > + print_usage(argv[0], EXIT_FAILURE); > + case '?': > + default: > + fprintf(stderr, "%s - option %c is invalid\n", argv[0], > + optopt); > + print_usage(argv[0], EXIT_FAILURE); > + } > + } > + > + if ((count + offset) > (MAX_OFFSET + 1)) { > + fprintf(stderr, "Out of bounds. Count + Offset <= 0xf0300\n"); > + exit(ERANGE); > + } > + > + if ((cmd == WRITE) && (vflag != 'v')) { > + fprintf(stderr, "Write value is missing\n"); > + print_usage(argv[0], EXIT_FAILURE); > + } > + > + memset(dev_name, '\0', 20); Not needed as snprintf includes the null byte, > + snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, devid); Allows a max of 99 for devid, which should be okay but define a macro and reject opt args greater than that? > + > + fd = open(dev_name, file_op); > + if (fd < 0) { > + fprintf(stderr, "Failed to open %s aux device - error: %s\n", dev_name, > + strerror(errno)); > + return errno; > + } > + > + switch (cmd) { > + case READ: > + ret = dpcd_read(fd, offset, count); > + break; > + case WRITE: > + ret = dpcd_write(fd, offset, val); > + break; > + case INV: INVALID is easier to understand > + default: > + fprintf(stderr, "Please specify a command: read/write. See usage\n"); Remove "See usage" > + close(fd); There's another close() below. > + print_usage(argv[0], EXIT_FAILURE); > + } > + > + close(fd); > + > + return ret; > +} > diff --git a/tools/meson.build b/tools/meson.build > index e4517d66..79f36aa9 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -36,6 +36,7 @@ tools_progs = [ > 'intel_watermark', > 'intel_gem_info', > 'intel_gvtg_test', > + 'dpcd_reg', > ] > tool_deps = igt_deps Thanks for working on this. The tool looks good overall,. just needs some minor polishing. -DK _______________________________________________ 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 v3] tools: Add a simple tool to read/write/decode dpcd registers 2018-09-21 6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan @ 2018-09-21 20:36 ` Tarun Vyas 2018-09-21 21:09 ` Pandiyan, Dhinakaran 0 siblings, 1 reply; 8+ messages in thread From: Tarun Vyas @ 2018-09-21 20:36 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: igt-dev, rodrigo.vivi On Thu, Sep 20, 2018 at 11:12:49PM -0700, Dhinakaran Pandiyan wrote: > On Friday, September 14, 2018 4:57:02 PM PDT 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. > > > > v2: (Fixes by Rodrigo but no functional changes yet): > > - Indentations, Typo, Missed spaces > > - Removing mentioning to decode and spec that is not implemented yet. > > - Add Makefile.sources back > > - Missed s/printf/igt_warn > > > > v3: > > - Addres DK's review comments from v2 above. > > - Squash Rodrigo's file handling unification patch. > > - Make count, offset and device id optional. > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > tools/Makefile.sources | 1 + > > tools/dpcd_reg.c | 213 > > +++++++++++++++++++++++++++++++++++++++++++++++++ tools/meson.build | > > 1 + > > 3 files changed, 215 insertions(+) > > create mode 100644 tools/dpcd_reg.c > > > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > > index abd23a0f..50706f41 100644 > > --- a/tools/Makefile.sources > > +++ b/tools/Makefile.sources > > @@ -7,6 +7,7 @@ noinst_PROGRAMS = \ > > > > tools_prog_lists = \ > > igt_stats \ > > + dpcd_reg \ > > intel_audio_dump \ > > intel_reg \ > > intel_backlight \ > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > > new file mode 100644 > > index 00000000..cd9fed4f > > --- /dev/null > > +++ b/tools/dpcd_reg.c > > @@ -0,0 +1,213 @@ > > +/* > > + * 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/write tool > > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register > > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set. > > + */ > > + > > +#include "igt_core.h" > What's the dependency on igt_core here? > Forgot to remove it. > > +#include <errno.h> > > +#include <fcntl.h> > > +#include <limits.h> > > + > > +#define MAX_OFFSET 0xf02ff > I think we should be able to allow allow up to the max limit of 0xfffff (DP > 1.4 section 2.9.3) > Will modify. > > + > > +const char aux_dev[] = "/dev/drm_dp_aux"; > > + > > +static void print_usage(char *tool, int exit_code) > > +{ > > + printf("DPCD register read and write tool\n\n"); > > + printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > > + "to be set in the kernel config.\n\n"); > I think it would look better to print the above lines only when --help/-h was > passed. Move this under case 'h' ? > Will do > > + printf("Usage: %s [OPTION ...] COMMAND\n\n", tool); > > + printf("COMMAND is one of:\n"); > > + printf(" read: Read [count] bytes dpcd reg at an offset\n"); > > + printf(" write: Write a dpcd reg at an offset\n\n"); > > + printf("Options for the above COMMANDS are\n"); > > + printf(" --device=DEVID Aux device id, as listed in > > /dev/drm_dp_aux_dev[n]." + "Defaults to 0\n"); > > + printf(" --offset=REG_ADDR DPCD register offset in hex. Defaults to > > 0x00\n"); + printf(" --count=BYTES For reads, specify number of bytes to > > be read from" + "the offset. Defaults to 1\n"); > > + printf(" --val For writes, specify a hex value to be written\n\n"); > > + > > + printf(" --help: print the usage\n"); > > + > > + exit(exit_code); > Modify the callers so that you don't have to exit from a print function. > Will do. > > +} > > + > > +static int dpcd_read(int fd, const uint32_t offset, size_t count) > const is not needed as the arguments are passed by value. > Will remove it. > > +{ > > + int ret, i; > > + void *buf = calloc(count, sizeof(uint8_t)); > uint8_t *buf ? > Heh, yea. > > + > > + if (!buf) { > > + fprintf(stderr, "Can't allocate read buffer\n"); > > + return ENOMEM; > > + } > > + > > + ret = pread(fd, buf, count, offset); > > + if (ret != count) { > > + fprintf(stderr, "Failed to read - %s\n", strerror(errno)); > Print ret too? If the number of bytes read were lower, errno won't be set. > Ok > > + ret = errno; > > + goto out; > > + } else > } else { > > > + ret = EXIT_SUCCESS; > Intialize ret and get rid of the else block? > Aye > > + > > + printf("Read %zu byte(s) starting at offset %x\n\n", count, offset); > Print this debug message only if the expected number of bytes weren't read and > something like printf("Read %zu bytes, expected %zu\n"); > > printf("0x"); to clarify that the printed values are in hex. Will modify > > + for (i = 0; i < count; i++) > > + printf(" %02x", *(((uint8_t *)(buf)) + i)); > You can avoid typecasting if you define the array as type uint8_t > > > + printf("\n"); > > + > > +out: > > + free(buf); > > + return ret; > > +} > > + > > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t val) > const isn't needed. > > > +{ > > + int ret; > > + > > + ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset); > > + if (ret < 0) { > > + fprintf(stderr, "Failed to write - %s\n", strerror(errno)); > Same as above, print number of bytes written in case of error. > > > + return errno; > > + } else > > + return EXIT_SUCCESS; > You could avoid 'else' here. > > > +} > > + > > +int main(int argc, char **argv) > > +{ > > + char dev_name[20]; > > + int ret, devid, fd, vflag = 0; > > + uint32_t offset; > > + uint8_t val; > > + size_t count; > > + int file_op = O_RDONLY; > > + > > + enum command { > > + INV = -1, > > + READ = 2, > Any reason to not use the value compiler generates? > I wanted to init INVALID with an -ve value. Just following the trend from intel_reg. > > + WRITE, > > + } cmd = INV; > > + > > + struct option longopts[] = { > > + { "count", required_argument, NULL, 'c' }, > > + { "device", required_argument, NULL, 'd' }, > > + { "help", no_argument, NULL, 'h' }, > > + { "offset", required_argument, NULL, 'o' }, > > + { "value", required_argument, &vflag, 'v' }, > > + { 0 } > > + }; > > + > > + devid = 0, count = 1, offset = 0x0; > > + > > + while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, NULL)) != > Any reason to leave out the option -v ? And '-h' shouldn't need an argument. > > I didnt wan't to rely on initializing the *write value* with an invalid , so based on the vflag, we'll know if the write value was ever set or not and error out accordingly. getopt_long will only complain if the user supplies --value option but no argument with it. If the user chooses to completely skip the --value option then we won't know. > > -1) { + switch (ret) { > > + case 'c': > > + count = strtoul(optarg, NULL, 10); > With strtol() you should be able reject all negative args and make use of the > second argument to reject invalid numbers. > Hmm, makes sense. > > + if (count == ULONG_MAX) { > > + fprintf(stderr, "Count argument too big\n"); > > + exit(ERANGE); > > + } > > + break; > > + case 'd': > > + devid = strtoul(optarg, NULL, 10); > Same here, strtol() to reject negatives and use **endptr to fail on invalid > numbers. > > > + if (devid == ULONG_MAX) { > > + fprintf(stderr, "Devid argument too big\n"); > > > + exit(ERANGE); > > + } > > + break; > > + case 'h': > > + print_usage(argv[0], EXIT_SUCCESS); > > + break; > > + case 'o': > > + offset = strtoul(optarg, NULL, 16); > Same comment as for 'd'. You might also want to check if errno is set. > > > + if (offset > MAX_OFFSET) { > > + fprintf(stderr, "Offset should be <= 0xf02ff\n"); > > + exit(ERANGE); > > + } > > + break; > > + case 0: > > + if (vflag == 'v' && optarg) > I didn't get why &vflag had to be used. > My idea was to use the vflag to check if --value option was used at all or not. > > + val = (uint8_t) strtoul(optarg, NULL, 16); > Reject values greater than 0xff? > uint8_t cast should truncate everything, but yea, I'll explicitly check for anything > 0xff. > > + break; > > + /* Command parsing */ > > + case 1: > > + if (strcmp(optarg, "read") == 0) { > > + cmd = READ; > > + } else if (strcmp(optarg, "write") == 0) { > > + cmd = WRITE; > > + file_op = O_WRONLY; > > + } > > + break; > } else { > and exit here? or fall through to default block by re-arranging the blocks. > Alright > > + case ':': > > + fprintf(stderr, "The -%c option of %s requires an argument\n", > > + optopt, argv[0]); > > Prints "The -c option of tools/dpcd_reg requires an argument", is that what > you intended? Skip printing the tool path? > Oh no, the %c is the format specifier. So, if count, devid, offset etc. are passed w/o an argument, then this will print something like "The -v option of tools/dpcd_reg requires an argument", for the value argument, in this case, or -d for the devid argument and so on. > > + print_usage(argv[0], EXIT_FAILURE); > > + case '?': > > + default: > > + fprintf(stderr, "%s - option %c is invalid\n", argv[0], > > + optopt); > > + print_usage(argv[0], EXIT_FAILURE); > > + } > > + } > > + > > + if ((count + offset) > (MAX_OFFSET + 1)) { > > + fprintf(stderr, "Out of bounds. Count + Offset <= 0xf0300\n"); > > + exit(ERANGE); > > + } > > + > > + if ((cmd == WRITE) && (vflag != 'v')) { > > + fprintf(stderr, "Write value is missing\n"); > > + print_usage(argv[0], EXIT_FAILURE); > > + } > > + > > + memset(dev_name, '\0', 20); > Not needed as snprintf includes the null byte, Ok. > > + snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, devid); > Allows a max of 99 for devid, which should be okay but define a macro and > reject opt args greater than that? > Will modify. > > + > > + fd = open(dev_name, file_op); > > + if (fd < 0) { > > + fprintf(stderr, "Failed to open %s aux device - error: %s\n", dev_name, > > + strerror(errno)); > > + return errno; > > + } > > + > > + switch (cmd) { > > + case READ: > > + ret = dpcd_read(fd, offset, count); > > + break; > > + case WRITE: > > + ret = dpcd_write(fd, offset, val); > > + break; > > + case INV: > INVALID is easier to understand > Right > > + default: > > + fprintf(stderr, "Please specify a command: read/write. See usage\n"); > Remove "See usage" > > > + close(fd); > There's another close() below. > print_usage was exiting so the other close() wouldnt be called. But with the removal of exit from print_usage, this will change. > > + print_usage(argv[0], EXIT_FAILURE); > > + } > > + > > + close(fd); > > + > > + return ret; > > +} > > diff --git a/tools/meson.build b/tools/meson.build > > index e4517d66..79f36aa9 100644 > > --- a/tools/meson.build > > +++ b/tools/meson.build > > @@ -36,6 +36,7 @@ tools_progs = [ > > 'intel_watermark', > > 'intel_gem_info', > > 'intel_gvtg_test', > > + 'dpcd_reg', > > ] > > tool_deps = igt_deps > > Thanks for working on this. The tool looks good overall,. just needs some > minor polishing. > Thanks for the review :) > -DK > > > _______________________________________________ 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 v3] tools: Add a simple tool to read/write/decode dpcd registers 2018-09-21 20:36 ` Tarun Vyas @ 2018-09-21 21:09 ` Pandiyan, Dhinakaran 2018-09-21 22:10 ` Tarun Vyas 0 siblings, 1 reply; 8+ messages in thread From: Pandiyan, Dhinakaran @ 2018-09-21 21:09 UTC (permalink / raw) To: Vyas, Tarun; +Cc: igt-dev@lists.freedesktop.org, Vivi, Rodrigo > -----Original Message----- > From: Vyas, Tarun > Sent: Friday, September 21, 2018 1:36 PM > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> > Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com> > Subject: Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to > read/write/decode dpcd registers > > On Thu, Sep 20, 2018 at 11:12:49PM -0700, Dhinakaran Pandiyan wrote: > > On Friday, September 14, 2018 4:57:02 PM PDT 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. > > > > > > v2: (Fixes by Rodrigo but no functional changes yet): > > > - Indentations, Typo, Missed spaces > > > - Removing mentioning to decode and spec that is not implemented > yet. > > > - Add Makefile.sources back > > > - Missed s/printf/igt_warn > > > > > > v3: > > > - Addres DK's review comments from v2 above. > > > - Squash Rodrigo's file handling unification patch. > > > - Make count, offset and device id optional. > > > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > tools/Makefile.sources | 1 + > > > tools/dpcd_reg.c | 213 > > > +++++++++++++++++++++++++++++++++++++++++++++++++ > tools/meson.build | > > > 1 + > > > 3 files changed, 215 insertions(+) > > > create mode 100644 tools/dpcd_reg.c > > > > > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources index > > > abd23a0f..50706f41 100644 > > > --- a/tools/Makefile.sources > > > +++ b/tools/Makefile.sources > > > @@ -7,6 +7,7 @@ noinst_PROGRAMS = \ > > > > > > tools_prog_lists = \ > > > igt_stats \ > > > + dpcd_reg \ > > > intel_audio_dump \ > > > intel_reg \ > > > intel_backlight \ > > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c new file mode > > > 100644 index 00000000..cd9fed4f > > > --- /dev/null > > > +++ b/tools/dpcd_reg.c > > > @@ -0,0 +1,213 @@ > > > +/* > > > + * Copyright (c) 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/write tool > > > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD > > > + register > > > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set. > > > + */ > > > + > > > +#include "igt_core.h" > > What's the dependency on igt_core here? > > > Forgot to remove it. > > > +#include <errno.h> > > > +#include <fcntl.h> > > > +#include <limits.h> > > > + > > > +#define MAX_OFFSET 0xf02ff > > I think we should be able to allow allow up to the max limit of > > 0xfffff (DP > > 1.4 section 2.9.3) > > > Will modify. > > > + > > > +const char aux_dev[] = "/dev/drm_dp_aux"; > > > + > > > +static void print_usage(char *tool, int exit_code) { > > > + printf("DPCD register read and write tool\n\n"); > > > + printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > > > + "to be set in the kernel config.\n\n"); > > I think it would look better to print the above lines only when > > --help/-h was passed. Move this under case 'h' ? > > > Will do > > > + printf("Usage: %s [OPTION ...] COMMAND\n\n", tool); > > > + printf("COMMAND is one of:\n"); > > > + printf(" read: Read [count] bytes dpcd reg at an offset\n"); > > > + printf(" write: Write a dpcd reg at an offset\n\n"); > > > + printf("Options for the above COMMANDS are\n"); > > > + printf(" --device=DEVID Aux device id, as listed in > > > /dev/drm_dp_aux_dev[n]." + > "Defaults to 0\n"); > > > + printf(" --offset=REG_ADDR DPCD register offset in hex. Defaults > to > > > 0x00\n"); + printf(" --count=BYTES For reads, specify number of > bytes to > > > be read from" + "the offset. Defaults > to 1\n"); > > > + printf(" --val For writes, specify a hex value to be > written\n\n"); > > > + > > > + printf(" --help: print the usage\n"); > > > + > > > + exit(exit_code); > > Modify the callers so that you don't have to exit from a print function. > > > Will do. > > > +} > > > + > > > +static int dpcd_read(int fd, const uint32_t offset, size_t count) > > const is not needed as the arguments are passed by value. > > > Will remove it. > > > +{ > > > + int ret, i; > > > + void *buf = calloc(count, sizeof(uint8_t)); > > uint8_t *buf ? > > > Heh, yea. > > > + > > > + if (!buf) { > > > + fprintf(stderr, "Can't allocate read buffer\n"); > > > + return ENOMEM; > > > + } > > > + > > > + ret = pread(fd, buf, count, offset); > > > + if (ret != count) { > > > + fprintf(stderr, "Failed to read - %s\n", strerror(errno)); > > Print ret too? If the number of bytes read were lower, errno won't be set. > > > Ok > > > + ret = errno; > > > + goto out; > > > + } else > > } else { > > > > > + ret = EXIT_SUCCESS; > > Intialize ret and get rid of the else block? > > > Aye > > > + > > > + printf("Read %zu byte(s) starting at offset %x\n\n", count, > > > +offset); > > Print this debug message only if the expected number of bytes weren't > > read and something like printf("Read %zu bytes, expected %zu\n"); > > > > printf("0x"); to clarify that the printed values are in hex. > Will modify > > > + for (i = 0; i < count; i++) > > > + printf(" %02x", *(((uint8_t *)(buf)) + i)); > > You can avoid typecasting if you define the array as type uint8_t > > > > > + printf("\n"); > > > + > > > +out: > > > + free(buf); > > > + return ret; > > > +} > > > + > > > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t > > > +val) > > const isn't needed. > > > > > +{ > > > + int ret; > > > + > > > + ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset); > > > + if (ret < 0) { > > > + fprintf(stderr, "Failed to write - %s\n", strerror(errno)); > > Same as above, print number of bytes written in case of error. > > > > > + return errno; > > > + } else > > > + return EXIT_SUCCESS; > > You could avoid 'else' here. > > > > > +} > > > + > > > +int main(int argc, char **argv) > > > +{ > > > + char dev_name[20]; > > > + int ret, devid, fd, vflag = 0; > > > + uint32_t offset; > > > + uint8_t val; > > > + size_t count; > > > + int file_op = O_RDONLY; > > > + > > > + enum command { > > > + INV = -1, > > > + READ = 2, > > Any reason to not use the value compiler generates? > > > I wanted to init INVALID with an -ve value. Just following the trend from > intel_reg. > > > + WRITE, > > > + } cmd = INV; > > > + > > > + struct option longopts[] = { > > > + { "count", required_argument, NULL, 'c' }, > > > + { "device", required_argument, NULL, 'd' }, > > > + { "help", no_argument, NULL, 'h' }, > > > + { "offset", required_argument, NULL, 'o' }, > > > + { "value", required_argument, &vflag, 'v' }, > > > + { 0 } > > > + }; > > > + > > > + devid = 0, count = 1, offset = 0x0; > > > + > > > + while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, > > > +NULL)) != > > Any reason to leave out the option -v ? And '-h' shouldn't need an > argument. > > > > > I didnt wan't to rely on initializing the *write value* with an invalid , so based > on the vflag, we'll know if the write value was ever set or not and error out > accordingly. > getopt_long will only complain if the user supplies --value option but no > argument with it. If the user chooses to completely skip the --value option > then we won't know. Shouldn't the tool accept -v <foo> too? > > > -1) { + switch (ret) { > > > + case 'c': > > > + count = strtoul(optarg, NULL, 10); > > With strtol() you should be able reject all negative args and make use > > of the second argument to reject invalid numbers. > > > Hmm, makes sense. > > > + if (count == ULONG_MAX) { > > > + fprintf(stderr, "Count argument too big\n"); > > > + exit(ERANGE); > > > + } > > > + break; > > > + case 'd': > > > + devid = strtoul(optarg, NULL, 10); > > Same here, strtol() to reject negatives and use **endptr to fail on > > invalid numbers. > > > > > + if (devid == ULONG_MAX) { > > > + fprintf(stderr, "Devid argument too big\n"); > > > > > + exit(ERANGE); > > > + } > > > + break; > > > + case 'h': > > > + print_usage(argv[0], EXIT_SUCCESS); > > > + break; > > > + case 'o': > > > + offset = strtoul(optarg, NULL, 16); > > Same comment as for 'd'. You might also want to check if errno is set. > > > > > + if (offset > MAX_OFFSET) { > > > + fprintf(stderr, "Offset should be <= > 0xf02ff\n"); > > > + exit(ERANGE); > > > + } > > > + break; > > > + case 0: > > > + if (vflag == 'v' && optarg) > > I didn't get why &vflag had to be used. > > > My idea was to use the vflag to check if --value option was used at all or not. Makes sense. How about failing --value or -v was used along with read? > > > + val = (uint8_t) strtoul(optarg, NULL, 16); > > Reject values greater than 0xff? > > > uint8_t cast should truncate everything, but yea, I'll explicitly check for > anything > 0xff. > > > + break; > > > + /* Command parsing */ > > > + case 1: > > > + if (strcmp(optarg, "read") == 0) { > > > + cmd = READ; > > > + } else if (strcmp(optarg, "write") == 0) { > > > + cmd = WRITE; > > > + file_op = O_WRONLY; > > > + } > > > + break; > > } else { > > and exit here? or fall through to default block by re-arranging the blocks. > > > Alright > > > + case ':': > > > + fprintf(stderr, "The -%c option of %s requires an > argument\n", > > > + optopt, argv[0]); > > > > Prints "The -c option of tools/dpcd_reg requires an argument", is that > > what you intended? Skip printing the tool path? > > > Oh no, the %c is the format specifier. So, if count, devid, offset etc. are > passed w/o an argument, then this will print something like "The -v option of > tools/dpcd_reg requires an argument", for the value argument, in this case, > or -d for the devid argument and so on. I meant printing "tools/dpcd_reg" was redundant and I used -c (count) without an arg as example. -DK _______________________________________________ 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 v3] tools: Add a simple tool to read/write/decode dpcd registers 2018-09-21 21:09 ` Pandiyan, Dhinakaran @ 2018-09-21 22:10 ` Tarun Vyas 0 siblings, 0 replies; 8+ messages in thread From: Tarun Vyas @ 2018-09-21 22:10 UTC (permalink / raw) To: Pandiyan, Dhinakaran; +Cc: igt-dev@lists.freedesktop.org, Vivi, Rodrigo On Fri, Sep 21, 2018 at 02:09:36PM -0700, Pandiyan, Dhinakaran wrote: > > > > -----Original Message----- > > From: Vyas, Tarun > > Sent: Friday, September 21, 2018 1:36 PM > > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> > > Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com> > > Subject: Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to > > read/write/decode dpcd registers > > > > On Thu, Sep 20, 2018 at 11:12:49PM -0700, Dhinakaran Pandiyan wrote: > > > On Friday, September 14, 2018 4:57:02 PM PDT 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. > > > > > > > > v2: (Fixes by Rodrigo but no functional changes yet): > > > > - Indentations, Typo, Missed spaces > > > > - Removing mentioning to decode and spec that is not implemented > > yet. > > > > - Add Makefile.sources back > > > > - Missed s/printf/igt_warn > > > > > > > > v3: > > > > - Addres DK's review comments from v2 above. > > > > - Squash Rodrigo's file handling unification patch. > > > > - Make count, offset and device id optional. > > > > > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > tools/Makefile.sources | 1 + > > > > tools/dpcd_reg.c | 213 > > > > +++++++++++++++++++++++++++++++++++++++++++++++++ > > tools/meson.build | > > > > 1 + > > > > 3 files changed, 215 insertions(+) > > > > create mode 100644 tools/dpcd_reg.c > > > > > > > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources index > > > > abd23a0f..50706f41 100644 > > > > --- a/tools/Makefile.sources > > > > +++ b/tools/Makefile.sources > > > > @@ -7,6 +7,7 @@ noinst_PROGRAMS = \ > > > > > > > > tools_prog_lists = \ > > > > igt_stats \ > > > > + dpcd_reg \ > > > > intel_audio_dump \ > > > > intel_reg \ > > > > intel_backlight \ > > > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c new file mode > > > > 100644 index 00000000..cd9fed4f > > > > --- /dev/null > > > > +++ b/tools/dpcd_reg.c > > > > @@ -0,0 +1,213 @@ > > > > +/* > > > > + * Copyright (c) 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/write tool > > > > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD > > > > + register > > > > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set. > > > > + */ > > > > + > > > > +#include "igt_core.h" > > > What's the dependency on igt_core here? > > > > > Forgot to remove it. > > > > +#include <errno.h> > > > > +#include <fcntl.h> > > > > +#include <limits.h> > > > > + > > > > +#define MAX_OFFSET 0xf02ff > > > I think we should be able to allow allow up to the max limit of > > > 0xfffff (DP > > > 1.4 section 2.9.3) > > > > > Will modify. > > > > + > > > > +const char aux_dev[] = "/dev/drm_dp_aux"; > > > > + > > > > +static void print_usage(char *tool, int exit_code) { > > > > + printf("DPCD register read and write tool\n\n"); > > > > + printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > > > > + "to be set in the kernel config.\n\n"); > > > I think it would look better to print the above lines only when > > > --help/-h was passed. Move this under case 'h' ? > > > > > Will do > > > > + printf("Usage: %s [OPTION ...] COMMAND\n\n", tool); > > > > + printf("COMMAND is one of:\n"); > > > > + printf(" read: Read [count] bytes dpcd reg at an offset\n"); > > > > + printf(" write: Write a dpcd reg at an offset\n\n"); > > > > + printf("Options for the above COMMANDS are\n"); > > > > + printf(" --device=DEVID Aux device id, as listed in > > > > /dev/drm_dp_aux_dev[n]." + > > "Defaults to 0\n"); > > > > + printf(" --offset=REG_ADDR DPCD register offset in hex. Defaults > > to > > > > 0x00\n"); + printf(" --count=BYTES For reads, specify number of > > bytes to > > > > be read from" + "the offset. Defaults > > to 1\n"); > > > > + printf(" --val For writes, specify a hex value to be > > written\n\n"); > > > > + > > > > + printf(" --help: print the usage\n"); > > > > + > > > > + exit(exit_code); > > > Modify the callers so that you don't have to exit from a print function. > > > > > Will do. > > > > +} > > > > + > > > > +static int dpcd_read(int fd, const uint32_t offset, size_t count) > > > const is not needed as the arguments are passed by value. > > > > > Will remove it. > > > > +{ > > > > + int ret, i; > > > > + void *buf = calloc(count, sizeof(uint8_t)); > > > uint8_t *buf ? > > > > > Heh, yea. > > > > + > > > > + if (!buf) { > > > > + fprintf(stderr, "Can't allocate read buffer\n"); > > > > + return ENOMEM; > > > > + } > > > > + > > > > + ret = pread(fd, buf, count, offset); > > > > + if (ret != count) { > > > > + fprintf(stderr, "Failed to read - %s\n", strerror(errno)); > > > Print ret too? If the number of bytes read were lower, errno won't be set. > > > > > Ok > > > > + ret = errno; > > > > + goto out; > > > > + } else > > > } else { > > > > > > > + ret = EXIT_SUCCESS; > > > Intialize ret and get rid of the else block? > > > > > Aye > > > > + > > > > + printf("Read %zu byte(s) starting at offset %x\n\n", count, > > > > +offset); > > > Print this debug message only if the expected number of bytes weren't > > > read and something like printf("Read %zu bytes, expected %zu\n"); > > > > > > printf("0x"); to clarify that the printed values are in hex. > > Will modify > > > > + for (i = 0; i < count; i++) > > > > + printf(" %02x", *(((uint8_t *)(buf)) + i)); > > > You can avoid typecasting if you define the array as type uint8_t > > > > > > > + printf("\n"); > > > > + > > > > +out: > > > > + free(buf); > > > > + return ret; > > > > +} > > > > + > > > > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t > > > > +val) > > > const isn't needed. > > > > > > > +{ > > > > + int ret; > > > > + > > > > + ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset); > > > > + if (ret < 0) { > > > > + fprintf(stderr, "Failed to write - %s\n", strerror(errno)); > > > Same as above, print number of bytes written in case of error. > > > > > > > + return errno; > > > > + } else > > > > + return EXIT_SUCCESS; > > > You could avoid 'else' here. > > > > > > > +} > > > > + > > > > +int main(int argc, char **argv) > > > > +{ > > > > + char dev_name[20]; > > > > + int ret, devid, fd, vflag = 0; > > > > + uint32_t offset; > > > > + uint8_t val; > > > > + size_t count; > > > > + int file_op = O_RDONLY; > > > > + > > > > + enum command { > > > > + INV = -1, > > > > + READ = 2, > > > Any reason to not use the value compiler generates? > > > > > I wanted to init INVALID with an -ve value. Just following the trend from > > intel_reg. > > > > + WRITE, > > > > + } cmd = INV; > > > > + > > > > + struct option longopts[] = { > > > > + { "count", required_argument, NULL, 'c' }, > > > > + { "device", required_argument, NULL, 'd' }, > > > > + { "help", no_argument, NULL, 'h' }, > > > > + { "offset", required_argument, NULL, 'o' }, > > > > + { "value", required_argument, &vflag, 'v' }, > > > > + { 0 } > > > > + }; > > > > + > > > > + devid = 0, count = 1, offset = 0x0; > > > > + > > > > + while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, > > > > +NULL)) != > > > Any reason to leave out the option -v ? And '-h' shouldn't need an > > argument. > > > > > > > > I didnt wan't to rely on initializing the *write value* with an invalid , so based > > on the vflag, we'll know if the write value was ever set or not and error out > > accordingly. > > getopt_long will only complain if the user supplies --value option but no > > argument with it. If the user chooses to completely skip the --value option > > then we won't know. > > Shouldn't the tool accept -v <foo> too? > Yea getopt_long will accept both short and long options. Both of them will be caught by the same case in the switch. > > > > -1) { + switch (ret) { > > > > + case 'c': > > > > + count = strtoul(optarg, NULL, 10); > > > With strtol() you should be able reject all negative args and make use > > > of the second argument to reject invalid numbers. > > > > > Hmm, makes sense. > > > > + if (count == ULONG_MAX) { > > > > + fprintf(stderr, "Count argument too big\n"); > > > > + exit(ERANGE); > > > > + } > > > > + break; > > > > + case 'd': > > > > + devid = strtoul(optarg, NULL, 10); > > > Same here, strtol() to reject negatives and use **endptr to fail on > > > invalid numbers. > > > > > > > + if (devid == ULONG_MAX) { > > > > + fprintf(stderr, "Devid argument too big\n"); > > > > > > > + exit(ERANGE); > > > > + } > > > > + break; > > > > + case 'h': > > > > + print_usage(argv[0], EXIT_SUCCESS); > > > > + break; > > > > + case 'o': > > > > + offset = strtoul(optarg, NULL, 16); > > > Same comment as for 'd'. You might also want to check if errno is set. > > > > > > > + if (offset > MAX_OFFSET) { > > > > + fprintf(stderr, "Offset should be <= > > 0xf02ff\n"); > > > > + exit(ERANGE); > > > > + } > > > > + break; > > > > + case 0: > > > > + if (vflag == 'v' && optarg) > > > I didn't get why &vflag had to be used. > > > > > My idea was to use the vflag to check if --value option was used at all or not. > > Makes sense. How about failing --value or -v was used along with read? > I thought of that, but left it out because we dont pass it the read function, so even if someone inadvertently does that, the tool will still dump the reg value. I can change that if you want. > > > > + val = (uint8_t) strtoul(optarg, NULL, 16); > > > Reject values greater than 0xff? > > > > > uint8_t cast should truncate everything, but yea, I'll explicitly check for > > anything > 0xff. > > > > + break; > > > > + /* Command parsing */ > > > > + case 1: > > > > + if (strcmp(optarg, "read") == 0) { > > > > + cmd = READ; > > > > + } else if (strcmp(optarg, "write") == 0) { > > > > + cmd = WRITE; > > > > + file_op = O_WRONLY; > > > > + } > > > > + break; > > > } else { > > > and exit here? or fall through to default block by re-arranging the blocks. > > > > > Alright > > > > + case ':': > > > > + fprintf(stderr, "The -%c option of %s requires an > > argument\n", > > > > + optopt, argv[0]); > > > > > > Prints "The -c option of tools/dpcd_reg requires an argument", is that > > > what you intended? Skip printing the tool path? > > > > > Oh no, the %c is the format specifier. So, if count, devid, offset etc. are > > passed w/o an argument, then this will print something like "The -v option of > > tools/dpcd_reg requires an argument", for the value argument, in this case, > > or -d for the devid argument and so on. > > I meant printing "tools/dpcd_reg" was redundant and I used -c (count) without an arg as example. > Ah ok, will remove it. > -DK > _______________________________________________ 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 v3] tools: Add a simple tool to read/write/decode dpcd registers 2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas ` (2 preceding siblings ...) 2018-09-21 6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan @ 2018-09-22 23:34 ` Dhinakaran Pandiyan 3 siblings, 0 replies; 8+ messages in thread From: Dhinakaran Pandiyan @ 2018-09-22 23:34 UTC (permalink / raw) To: Tarun Vyas, igt-dev; +Cc: rodrigo.vivi On Fri, 2018-09-14 at 16:57 -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. > > v2: (Fixes by Rodrigo but no functional changes yet): > - Indentations, Typo, Missed spaces > - Removing mentioning to decode and spec that is not implemented > yet. > - Add Makefile.sources back > - Missed s/printf/igt_warn > > v3: > - Addres DK's review comments from v2 above. > - Squash Rodrigo's file handling unification patch. > - Make count, offset and device id optional. > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > tools/Makefile.sources | 1 + > tools/dpcd_reg.c | 213 > +++++++++++++++++++++++++++++++++++++++++++++++++ > tools/meson.build | 1 + > 3 files changed, 215 insertions(+) > create mode 100644 tools/dpcd_reg.c > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > index abd23a0f..50706f41 100644 > --- a/tools/Makefile.sources > +++ b/tools/Makefile.sources > @@ -7,6 +7,7 @@ noinst_PROGRAMS = \ > > tools_prog_lists = \ > igt_stats \ > + dpcd_reg \ > intel_audio_dump \ > intel_reg \ > intel_backlight \ > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c > new file mode 100644 > index 00000000..cd9fed4f > --- /dev/null > +++ b/tools/dpcd_reg.c > @@ -0,0 +1,213 @@ > +/* > + * 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/write tool > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD > register read > + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set. > + */ > + > +#include "igt_core.h" > +#include <errno.h> > +#include <fcntl.h> > +#include <limits.h> > + > +#define MAX_OFFSET 0xf02ff > + > +const char aux_dev[] = "/dev/drm_dp_aux"; > + > +static void print_usage(char *tool, int exit_code) > +{ > + printf("DPCD register read and write tool\n\n"); > + printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n" > + "to be set in the kernel config.\n\n"); > + printf("Usage: %s [OPTION ...] COMMAND\n\n", tool); > + printf("COMMAND is one of:\n"); > + printf(" read: Read [count] bytes dpcd reg > at an offset\n"); > + printf(" write: Write a dpcd reg at an offset\n\n"); > + printf("Options for the above COMMANDS are\n"); > + printf(" --device=DEVID Aux device id, as > listed in /dev/drm_dp_aux_dev[n]." > + "Defaults to 0\n"); > + printf(" --offset=REG_ADDR DPCD register offset in > hex. Defaults to 0x00\n"); > + printf(" --count=BYTES For reads, specify > number of bytes to be read from" > + "the offset. Defaults to > 1\n"); > + printf(" --val For writes, specify a > hex value to be written\n\n"); > + > + printf(" --help: print the usage\n"); > + > + exit(exit_code); > +} > + > +static int dpcd_read(int fd, const uint32_t offset, size_t count) > +{ > + int ret, i; > + void *buf = calloc(count, sizeof(uint8_t)); > + > + if (!buf) { > + fprintf(stderr, "Can't allocate read buffer\n"); > + return ENOMEM; > + } > + > + ret = pread(fd, buf, count, offset); > + if (ret != count) { > + fprintf(stderr, "Failed to read - %s\n", > strerror(errno)); > + ret = errno; > + goto out; > + } else > + ret = EXIT_SUCCESS; > + > + printf("Read %zu byte(s) starting at offset %x\n\n", count, > offset); > + for (i = 0; i < count; i++) > + printf(" %02x", *(((uint8_t *)(buf)) + i)); > + printf("\n"); > + > +out: > + free(buf); > + return ret; > +} > + > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t > val) > +{ > + int ret; > + > + ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), > offset); > + if (ret < 0) { > + fprintf(stderr, "Failed to write - %s\n", > strerror(errno)); > + return errno; > + } else > + return EXIT_SUCCESS; > +} > + > +int main(int argc, char **argv) > +{ > + char dev_name[20]; > + int ret, devid, fd, vflag = 0; > + uint32_t offset; > + uint8_t val; > + size_t count; > + int file_op = O_RDONLY; > + > + enum command { > + INV = -1, > + READ = 2, > + WRITE, > + } cmd = INV; > + > + struct option longopts[] = { > + { "count", required_argument, NULL, > 'c' }, > + { "device", required_argument, NULL, > 'd' }, > + { "help", no_argument, NULL, > 'h' }, > + { "offset", required_argument, NULL, > 'o' }, > + { "value", required_argument, &vflag, > 'v' }, > + { 0 } > + }; > + > + devid = 0, count = 1, offset = 0x0; > + > + while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", > longopts, NULL)) != -1) { > + switch (ret) { > + case 'c': > + count = strtoul(optarg, NULL, 10); > + if (count == ULONG_MAX) { > + fprintf(stderr, "Count argument too > big\n"); > + exit(ERANGE); > + } > + break; > + case 'd': > + devid = strtoul(optarg, NULL, 10); > + if (devid == ULONG_MAX) { > + fprintf(stderr, "Devid argument too > big\n"); > + exit(ERANGE); > + } > + break; > + case 'h': > + print_usage(argv[0], EXIT_SUCCESS); > + break; > + case 'o': > + offset = strtoul(optarg, NULL, 16); > + if (offset > MAX_OFFSET) { > + fprintf(stderr, "Offset should be <= > 0xf02ff\n"); > + exit(ERANGE); > + } > + break; > + case 0: > + if (vflag == 'v' && optarg) > + val = (uint8_t) strtoul(optarg, > NULL, 16); > + break; > + /* Command parsing */ > + case 1: > + if (strcmp(optarg, "read") == 0) { > + cmd = READ; > + } else if (strcmp(optarg, "write") == 0) { > + cmd = WRITE; > + file_op = O_WRONLY; > + } > + break; > + case ':': > + fprintf(stderr, "The -%c option of %s > requires an argument\n", > + optopt, argv[0]); > + print_usage(argv[0], EXIT_FAILURE); > + case '?': > + default: > + fprintf(stderr, "%s - option %c is > invalid\n", argv[0], > + optopt); > + print_usage(argv[0], EXIT_FAILURE); > + } > + } > + > + if ((count + offset) > (MAX_OFFSET + 1)) { > + fprintf(stderr, "Out of bounds. Count + Offset <= > 0xf0300\n"); > + exit(ERANGE); > + } > + > + if ((cmd == WRITE) && (vflag != 'v')) { > + fprintf(stderr, "Write value is missing\n"); > + print_usage(argv[0], EXIT_FAILURE); > + } > + Another change you might want to consider is wrap all the parsing code above in a function and make it fill a struct data { int devid; enum command command; int offset; int count; uint8_t wval; }; and return error or 0 from there. You could use an union for count and val as their use is mutually exclusive. ret = parse_options(&data); if (ret) { print_usage() exit(ret) } > + memset(dev_name, '\0', 20); > + snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, > devid); > + > + fd = open(dev_name, file_op); > + if (fd < 0) { > + fprintf(stderr, "Failed to open %s aux device - > error: %s\n", dev_name, > + strerror(errno)); > + return errno; > + } > + > + switch (cmd) { > + case READ: > + ret = dpcd_read(fd, offset, count); > + break; > + case WRITE: > + ret = dpcd_write(fd, offset, val); > + break; > + case INV: > + default: > + fprintf(stderr, "Please specify a command: > read/write. See usage\n"); > + close(fd); > + print_usage(argv[0], EXIT_FAILURE); > + } > + close(fd); > + > + return ret; > +} > diff --git a/tools/meson.build b/tools/meson.build > index e4517d66..79f36aa9 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -36,6 +36,7 @@ tools_progs = [ > 'intel_watermark', > 'intel_gem_info', > 'intel_gvtg_test', > + 'dpcd_reg', > ] > tool_deps = igt_deps > _______________________________________________ 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:[~2018-09-22 23:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas 2018-09-15 0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork 2018-09-15 3:25 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 2018-09-21 6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan 2018-09-21 20:36 ` Tarun Vyas 2018-09-21 21:09 ` Pandiyan, Dhinakaran 2018-09-21 22:10 ` Tarun Vyas 2018-09-22 23:34 ` Dhinakaran Pandiyan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox