Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks

[Daniel Vetter]

On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>>>>> > be even better if you could calculate whether the mode is valid, but I didn't
>>>>> > spend enough time to figure out if this is possible.
>>>>>
>>>>> Theoretically that might be possible, checking if the requested freq
>>>>> matches the calculated freq, and I've tried that but so far I've not
>>>>> been able to get it to work, as in some cases the freq on the current
>>>>> whitelist don't exactly match but do work on the large majority of
>>>>> monitors tested (while other freq have a similar error but don't work
>>>>> on most monitors).
>>>>>
>>>>> I'd like to spend some more time to try to refine and tune this, but
>>>>> having the current whitelist works fairly well, so I'm not sure its
>>>>> worth sitting on (this is basically the last functional patch
>>>>> outstanding for HiKey to fully work upstream - except the mali gpu of
>>>>> course), while I try to tinker and tune it.
>>>>>
>>>>> Thanks so much for the feedback!
>>>>
>>>> Yeah the proper approach is to compute your pll/clock settings and bail
>>>> out if those don't work. That's generally a magic spreadsheet supplied by
>>>> the hw validation engineers, and I honestly don't want to know how they
>>>> create it. Explicit modelist in the kernel sounds like a very bad hack.
>>>
>>> So without such a magic spreadsheet, how would you suggest I move this forward?
>>> Not having the whitelist hack and picking modes the device can't
>>> generate is a fairly major usability issue.
>>
>> I guess if the whitelist is the only thing I'd do 2 things differently:
>> - Whitelist the clocks, not modes, since that's what seems to matter here.
>> - Put it as close as possible to the code that comuptes the clock
>> settings (yet if you use the clock subsystem that's a bit hard, but
>> for an atomic driver this should be where this is done ...).
>>
>> Whitelist of modes looks super-hacky.
>
> Sure. The whitelist modes were easiest to use initially dealing with
> problem reports since the EDID numbers were what users could report
> working or not.  But this feedback sounds reasonable, as I can map
> those to the underlying pixel clocks and generate a whitelist of
> those.
>
> I really appreciate the feedback here!

Another one: If you put this into the encoders ->mode_valid it will be
enforced both when listing modes, and when trying to set a mode. Which
means your users won't be able to see unsupported modes nor try them
out.

But it's not really a hard hw limit, just our current best guess, and
so makes testing new modes unecessarily complicated.

If you instead move this into the connectors ->mode_valid, then it's
only used to validate the connectors mode list, but not at modeset
time. Which means users could still test modes manually added to
xrandr and then tell you what modes to add.

We do that e.g. for sink mode limits, because some sinks have buggy
EDIDs and report wrong limits. Users can then still set modes at their
own risk, and be happy when they work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch