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@xxxxxxxxxxxxxxx>: > > On Fri, 25 Apr 2014, Daniel Vetter <daniel@xxxxxxxx> wrote: > >> On Thu, Apr 24, 2014 at 06:22:59PM -0300, Paulo Zanoni wrote: > >>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >>> > >>> 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@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@xxxxxxxxx> > >> > >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
