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

[John Stultz]

On Tue, Jul 11, 2017 at 9:27 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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.

So got some time to tinker here, and I've got two issues I'm not sure
how to move on.

1) The kirin driver doesn't seem to have a connector (just
encoder/crtc on the kirin side), the connector seems to be on the
adv7511 bridge, which isn't the component that has the mode
restrictions. So I'm not sure how to push the mode_valid check into
the connector.


2) In trying to move away from the whitelist, the kirin encoder is
where we calculate the phy byte clock which we want to match
(depending on the lanes used, by a fraction of) the mode clock.
However, the kirin crtc logic tweaks the adj_mode at fixup/set time.
This means the mode->clock we check against in the encoder mode_valid
ends up not being the mode we actually try to use at encoder mode_set
time.

Am I just missing something? Do we need to run the modes through the
pipeline's mode_fixups before checking its mode_valids?  Or should the
encoder mode_valid be asking the crtc to fix up the modes before
testing?

thanks
-john