[Intel-gfx] [PATCH 3/3] drm/i915: add i915.dp_link_train_policy option

[Daniel Vetter]

On Fri, Apr 25, 2014 at 02:48:07PM -0300, Paulo Zanoni wrote:
> 2014-04-25 6:00 GMT-03:00 Jani Nikula <jani.nikula at linux.intel.com>:
> > On Fri, 25 Apr 2014, Daniel Vetter <daniel at ffwll.ch> wrote:
> >> On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote:
> >>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>>
> >>> We still have way too many bugs with DP link training. We keep
> >>> switching between "narrow and fast" and "wide and slow", we recently
> >>> added 5GHz support, and whenever there's a bug report, we have to ask
> >>> people to apply patches and test them.
> >>>
> >>> Wouldn't it be so much better if we could just ask them to boot with
> >>> some specific Kernel boot option instead of applying a patch? This
> >>> will move the situation from "i915.ko is completely broken!" to
> >>> "i915.ko's default values are broken, but there's an option I can set
> >>> to fix it, so I won't need to learn how to compile a Kernel!".
> >>>
> >>> Some useful values:
> >>>  - i915.dp_link_train_policy=1 for "wide and slow"
> >>>  - i915.dp_link_train_policy=0x120 for DP_LINK_BW_2_7 and 2 lanes,
> >>>    which should be able to fit 1920x1080 at 60Hz and 24bpp
> >>>  - i915.dp_link_train_policy=0x210 to force DP 5GHz testing on
> >>>    not-so-huge modes
> >>>
> >>> The default behavior is still the same.
> >>>
> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>
> >> Yeah, I like this. I'll sign up Todd to review this all.
> >
> > I guess we'll go with this then, but I'll step back from this particular
> > patch for a bit, and share my concerns over module parameters and
> > quirks.
> >
> > I am generally opposed to adding module parameters or quirks to
> > workaround issues in features that should just work. They are an easy
> > way out for things we should root cause and fix properly.
> >
> > Do not mistake me for an idealist for thinking this way, as I'm being
> > pragmatic.
> >
> > The people who report bugs to us are roughly the same people who are
> > capable of setting the module parameter. Once they figure that out,
> > they'll stop responding to our requests for testing and info. We've seen
> > this happen before. We'd hurt our chances of making things work out of
> > the box for the average user.
> >
> > The more we add module parameters, the combinations of them
> > explode. Debugging *other* problems becomes harder. In the bugs I work
> > on, the #1 request I have is full dmesg, partially because I want to see
> > all the wild kernel parameters the user might have set. And all too
> > often they have. When there are module parameters that fix some bugs,
> > the blogs and forums get filled with tips about them, and people use
> > them, whether they strictly have the same bug or not. Search for i915
> > invert brightness for example.
> >
> > It's also not easy to drop module parameters after we've added them. You
> > know the drill. Even after we've fixed everything the module parameter
> > was supposed to fix, dropping it leads to https://xkcd.com/1172/.
> 
> Hi
> 
> I totally understand your point and I agree that your concerns are
> valid. But OTOH the patch could substantially improve the lives of our
> users, and it allow allows much easier debugging for the developers.
> So I really think there's a tradeoff between "making it easier for
> users" vs "hiding bugs" vs "making it easier to debug". In the ideal
> world, we will make the default parameter work for everybody, and that
> will be the only option. But while that's not the case, we could at
> least try to make something better. I also remember the RC6 case,
> where the addition of the Kernel parameter allowed more people to
> actually test RC6 and report the problems, and in the end we finally
> fixed the remaining issue that was preventing RC6 from being enabled
> by default. But yes, we still have people that use RC6 parameters
> values they are not supposed to use, but OTOH we also have users who
> can enable RC6 on ILK without any problems.
> 
> I wrote this patch for my own debugging purposes, and it was very
> useful to my development efforts. I have quite a few panels here, and
> I was trying to find a panel that doesn't work with either "wide and
> slow" or "narrow and fast", so the command-line option allowed me to
> quickly replace panels and retry. I also found a panel that doesn't
> work at all, and the finer-grained option allowed me to test it.
> 
> I also have written other similar debug options in the past, that I
> never sent to the mailing list. For example, I have local patches that
> allow me to choose a different policy for the panel power sequencing
> registers: I wrote these when I noticed that, in a Mac machine, the
> power sequencing values set on boot were completely different from the
> ones set on the VBT, and the values chosen by our driver were the
> slowest of both worlds.
> 
> So I guess this discussion is a nice opportunity for us to reach a
> general conclusion on what is the policy we should adopt for adding
> parameters that allow better debugging. Maybe if we had a way to
> specify that some parameters are strictly for debugging, or that some
> parameters are expected to break user's machines...
> 
> Anyway, I can discard the patch if we want. I can also remove the
> option to arbitrarily force a given bw+lane configuration.
> 
> I also found a bug on my own patch (missing "break" statement), so I
> need to send V2 anyway.

Imo for anything related to modesetting or which needs to be set at driver
load we want module options - for users it's really hard to set something
in debugfs with a black screen.

But we also want to ensure that users to play around with this for fun, so
I'll work on the debug module option thing a bit more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch