Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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