Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 23, 2014 at 10:58:59AM -0200, Paulo Zanoni wrote:
> 2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > Hi Tood,
> >
> > Paulo already mentioned it, so I'll just rehash quickly: I think the
> > points from the internal pre-review first need to be address before we can
> > dig into details. I know that the discussion there pettered out a bit, but
> > imo it's the patch authors responsibility to pick that up again and ping
> > people. One example I've noticed is the use of signals: We really want a
> > pollable file in debugfs (see e.g. the crc stuff). You can always still
> > redirect that to give you a signal, although I highly recommend to avoid
> > signals like the plague - they're simply too much a pain.
> >
> > But now onto the real comment I wanted to make, see below.
> >
> > On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote:
> >> Adds an interface that allows for Displayport configuration information to be accessed
> >> through debugfs. The information paramters provided here are as follows:
> >>    Link rate
> >>    Lane count
> >>    Bits per pixel
> >>    Voltage swing level
> >>    Preemphasis level
> >>    Display mode
> >>
> >> These parameters are intended to be used by userspace applications that need access
> >> to the link configuration of any active Displayport connection. Additionally,
> >> applications can make adjustments to these parameters through this interface to
> >> allow userspace application to have fine-grained control over link training
> >> paramters.
> >>
> >> Signed-off-by: Todd Previte <tprevite@gmail.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_dp.c     |  13 +-
> >>  2 files changed, 397 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index da4036d..2dada18 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -33,6 +33,7 @@
> >>  #include <linux/slab.h>
> >>  #include <linux/export.h>
> >>  #include <linux/list_sort.h>
> >> +#include <linux/string.h>
> >>  #include <asm/msr-index.h>
> >>  #include <drm/drmP.h>
> >>  #include "intel_drv.h"
> >> @@ -51,6 +52,46 @@ static const char *yesno(int v)
> >>       return v ? "yes" : "no";
> >>  }
> >>
> >> +#define DP_PARAMETER_COUNT           8
> >> +#define MAX_DP_CONFIG_LINE_COUNT     64
> >> +
> >> +enum dp_config_param {
> >> +     DP_CONFIG_PARAM_INVALID = -1,
> >> +     DP_CONFIG_PARAM_CONNECTOR = 0,
> >> +     DP_CONFIG_PARAM_LINK_RATE,
> >> +     DP_CONFIG_PARAM_LANE_COUNT,
> >> +     DP_CONFIG_PARAM_VOLTAGE_SWING,
> >> +     DP_CONFIG_PARAM_PREEMPHASIS,
> >> +     DP_CONFIG_PARAM_HRES,
> >> +     DP_CONFIG_PARAM_VRES,
> >> +     DP_CONFIG_PARAM_BPP
> >> +};
> >
> > I think it's better to expose this as drm properties on the DP connector.
> > This has a pile of upsides:
> > - We don't need to invent a new parser.
> > - Users can play with them to test out different theories.
> > - We could even use this to fix broken panels/boards without vbt or
> >   anything using our plan to set up the initial config with a dt firmware
> >   blob.
> >
> > So would be a lot more useful than this private interface.
> >
> > For the properties themselves I think we should go with explicit
> > enumerations with default value "automatic" and then an enumeration of all
> > values allowed by DP. For the naming of the properties I guess something
> > like DP_link_lanes, DP_link_clock, ... would look pretty. The properties
> > should be in dev->mode_config (since they're reallly generic) and I think
> > we want one function to register them all in one go in the drm_dp_helper.c
> > library. Maybe we could even put the values into the existing dp source
> > struct so that we don't have to reinvent the decode logic every single
> > time.
> 
> I disagree. I do prefer the debugfs thing. Think about all the
> examples of people that break their systems by passing
> i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
> With debug stuff exposed as DP properties, we're going to increase
> that problem, and in a way that will make it even harder to detect. I
> really really think these things should be exposed only to the debugfs
> users, because then if the debugfs file is closed, we can just ignore
> all the arguments and keep doing "normal" unaffected modesets.
> 
> If we don't want the parser, maybe we can make it a binary protocol:
> we'lll still have to parse it, but it can get much easier.

We already expose piles of funky stuff to users in these properties (e.g.
audio on hdmi). And contrary to the module options most of these will just
result in black screens if you don't understand what you're doing, so I
think the risk is low. There's also a bit of an issue with the current
interface as it's not clear which line corresponds to which DP interface.
Using properties would solve this. Also these options have a much more
direct impact - if you set them and the screen goes dark then the user
hopefully realizes that this is something they shouldn't touch.

For fbc and rc6 and all those the problem is that nothing really happens,
the system just becomes a bit more unstable and randomly crashes. Which
users then report.

But If you're really concerned about users doing crappy stuff we could add
a module option to drm_dp_helper.c which hides these, and then taint the
kernel if any user sets them. But I really think this is overkill.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch