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

[Paulo Zanoni]

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.

Thanks,
Paulo

>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Paulo Zanoni