[igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
Tarun Vyas
tarun.vyas at intel.com
Fri Sep 21 22:10:10 UTC 2018
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 at intel.com>
> > Cc: igt-dev at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at 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 at intel.com>
> > > > Signed-off-by: Tarun Vyas <tarun.vyas at intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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
>
More information about the igt-dev
mailing list