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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-06-14 at 17:00 +0300, Ville Syrjälä wrote:
> On Thu, Jun 14, 2018 at 04:20:07PM +0300, Jani Nikula wrote:
> > 
> > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com>
> > wrote:
> > > 
> > > On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote:
> > > > 
> > > > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.c
> > > > om> wrote:
> > > > > 
> > > > > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
> > > > > > 
> > > > > > This tool serves as a wrapper around the constructs
> > > > > > provided by the
> > > > > > drm_dpcd_aux_dev kernel module by working on the
> > > > > > /dev/drm_dp_aux[n]
> > > > > > devices created by the kernel module.
> > > > > > It supports reading and writing dpcd registers on the
> > > > > > connected aux
> > > > > > channels.
> > > > > > In the follow-up patch, support for decoding these
> > > > > > registers will be
> > > > > > added to facilate debugging panel related issues.
> > > > > > 
> > > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@inte
> > > > > > l.com>
> > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > > > > ---
> > > > > >  tools/Makefile.am      |   2 +-
> > > > > >  tools/Makefile.sources |   4 +-
> > > > > >  tools/dpcd_reg.c       | 219
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 223 insertions(+), 2 deletions(-)
> > > > > >  create mode 100644 tools/dpcd_reg.c
> > > > > > 
> > > > > > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > > > > > index 09b6dbcc3ece..b78824dadfdb 100644
> > > > > > --- a/tools/Makefile.am
> > > > > > +++ b/tools/Makefile.am
> > > > > > @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
> > > > > >  intel_error_decode_LDFLAGS = -lz
> > > > > >  endif
> > > > > >  
> > > > > > -bin_PROGRAMS += intel_dp_compliance
> > > > > > +bin_PROGRAMS += intel_dp_compliance dpcd_reg
> > > > > Not the best place for this I think.
> > > > > 
> > > > > Missing meson.build so no one is actually going to build
> > > > > this.
> > > > > 
> > > > > > 
> > > > > >  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> > > > > >  intel_dp_compliance_LDADD =
> > > > > > $(top_builddir)/lib/libintel_tools.la
> > > > > >  
> > > > > > diff --git a/tools/Makefile.sources
> > > > > > b/tools/Makefile.sources
> > > > > > index abd23a0f4628..db606b28560f 100644
> > > > > > --- a/tools/Makefile.sources
> > > > > > +++ b/tools/Makefile.sources
> > > > > > @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
> > > > > >          intel_dp_compliance.h \
> > > > > >          intel_dp_compliance_hotplug.c \
> > > > > >          $(NULL)
> > > > > > -
> > > > > > +dpcd_reg_SOURCES = \
> > > > > > +	dpcd_reg.c \
> > > > > > +	$(NULL)
> > > > > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..2848277aa792
> > > > > > --- /dev/null
> > > > > > +++ b/tools/dpcd_reg.c
> > > > > > @@ -0,0 +1,219 @@
> > > > > > +/*
> > > > > > + * Copyright © 2018 Intel Corporation
> > > > > > + *
> > > > > > + * Permission is hereby granted, free of charge, to any
> > > > > > person obtaining a
> > > > > > + * copy of this software and associated documentation
> > > > > > files (the "Software"),
> > > > > > + * to deal in the Software without restriction, including
> > > > > > without limitation
> > > > > > + * the rights to use, copy, modify, merge, publish,
> > > > > > distribute, sublicense,
> > > > > > + * and/or sell copies of the Software, and to permit
> > > > > > persons to whom the
> > > > > > + * Software is furnished to do so, subject to the
> > > > > > following conditions:
> > > > > > + *
> > > > > > + * The above copyright notice and this permission notice
> > > > > > (including the next
> > > > > > + * paragraph) shall be included in all copies or
> > > > > > substantial portions of the
> > > > > > + * Software.
> > > > > > + *
> > > > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF
> > > > > > ANY KIND, EXPRESS OR
> > > > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > > > > MERCHANTABILITY,
> > > > > > + * FITNESS FOR A PARTICULAR PURPOSE AND
> > > > > > NONINFRINGEMENT.  IN NO EVENT SHALL
> > > > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > > > > > CLAIM, DAMAGES OR OTHER
> > > > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > > > > > OTHERWISE, ARISING FROM,
> > > > > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > > > > > OTHER DEALINGS IN THE
> > > > > > + * SOFTWARE.
> > > > > > + *
> > > > > > + * 		DPCD register read/decode tool
> > > > > > + * This tool wraps around DRM_DP_AUX_DEV module to provide
> > > > > > DPCD register read,
> > > > > > + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be
> > > > > > set.
> > > > > > + */
> > > > > > +
> > > > > > +#include "igt_core.h"
> > > > > > +#include <errno.h>
> > > > > > +#include <fcntl.h>
> > > > > > +
> > > > > > +#define INVALID	0xff
> > > > > > +#define RW_SIZE	1
> > > > > > +
> > > > > > +const char aux_dev[] = "/dev/drm_dp_aux";
> > > > > > +
> > > > > > +static void print_usage(char *tool, int help)
> > > > > > +{
> > > > > > +	igt_info("DPCD register rw and decode tool\n\n");
> > > > > > +	igt_info("This tool requires
> > > > > > CONFIG_DRM_DP_AUX_CHARDEV\n"
> > > > > > +		 "to be set in the kernel config.\n");
> > > > > > +	igt_info("Usage %s command [options...]\n", tool);
> > > > > > +	igt_info("Supported commands are:\n"
> > > > > > +		 "\tread: Read a dpcd reg at an offset\n"
> > > > > > +		 "\twrite: Write a dpcd reg at an
> > > > > > offset\n"
> > > > > > +		 "\tdecode: Decode the value of a dpcd
> > > > > > reg\n");
> > > > > You didn't implement decode so no point in mentioning it.
> > > > > 
> > > > > > 
> > > > > > +	igt_info("Options for the above commands are\n"
> > > > > > +		 "\t--device: Aux device id,
> > > > > > IS_REQUIRED\n"
> > > > > > +		 "\t--help: print the usage\n"
> > > > > > +		 "\t--offset: DPCD register offset in hex,
> > > > > > IS_REQUIRED\n"
> > > > > > +		 "\t--spec: Specify DP/eDP spec version
> > > > > > for decoding, IS_OPTIONAL\n"
> > > > > > +		 "\t--val: Specify a value, for reg
> > > > > > writes\n");
> > > > > I think you should model the UI based on intel_reg.
> > > > I originally had plans to plug this *into* intel_reg, with e.g.
> > > > dpcd:
> > > > prefix. There we have the framework ready for adding names to
> > > > the
> > > > registers using separate definition files, and we have all the
> > > > frameworks ready for doing decoded dumps of the dpcd space. 

I suppose we could go with this tool as it is now and then later reuse
parts of the decode logic from intel_reg.

> > > > Just saying.
> > > That's one option. But maybe people working on non-Intel hw would
> > > want
> > > to use this as well?
> > I know, that's a consideration.
> > 
> > It's just that it's totally obvious any DPCD tool will grow a large
> > bunch of code already written in intel_reg.c. Looking at the
> > proposed
> > patch, there's only support for reading one offset at a time now.
> > The
> > first thing anyone will want is ability to read N bytes. Or a bunch
> > of
> > offsets here and there. With intel_reg, you get all of that for
> > free.
> Indeed. Actually that reminds me that it would be awesome to have a
> way
> to specify offset:count pairs for intel_reg read. Currently one
> either
> has to dump a bunch of irrelevant/non-existing registers, or execute
> intel_reg multiple times.
> 
> > 
> > Remember the times when we had separate and slightly different reg
> > tools
> > for all of the different "register ports" in intel_reg? I don't
> > particularly miss that.
> > 
> > Note also that for i915 DP debugging, with DPCD support in
> > intel_reg,
> > you could have the DPCD reads interspaced with register reads. How
> > cool
> > is that?
> Yes, that could be interesting.
> 
> > 
> > So I'm not going to insist on using intel_reg. I understand the
> > counter
> > argument. But when the two tools have grown a bunch of overlapping
> > and
> > duplicated code with slightly conflicting user interfaces, I
> > reserve the
> > right to be all, "I totally told you so."
> Maybe we should rename intel_reg to igt_reg and make sure it can
> actually be used without an Intel GPU? Other drivers could then
> perhaps also plug in their own backends to the same tool? Not sure
> how feasible that would be or whether anyone would even be interested
> in doing so.
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev