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

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Friday, April 25, 2014 10:19 AM
> To: Jani Nikula
> Cc: intel-gfx at lists.freedesktop.org; Zanoni, Paulo R
> Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: add i915.dp_link_train_policy
> option
> 
> On Fri, Apr 25, 2014 at 12:00:34PM +0300, Jani Nikula wrote:
> > 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/.
> 
> I fully agree with you. I'm working on a patch (only RFC thus far) which allows
> you to designate some module parameters as debug knobs. As soon as users
> touch them they'll get
> - a stern warning in dmesg
> - TAINT_USER'ed kernel
> 
> That should be about as good as we can make it.
> -Daniel

How hard would it be to expand the series afterwards this review to automatically try both "wide & slow" and "narrow & fast" for a given mode before giving up and logging the error?

I can foresee us replying a lot to people saying try wide or narrow when we could have the driver do that for us. The link training policy then could just be used to debug things later for those special cases. I think this would be a good thing.

For the whole series:
Reviewed-by: Rafael Barbalho <rafael.barbalho at intel.com>

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx