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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jul 17, 2017 at 04:20:23PM -0700, John Stultz wrote:
> On Tue, Jul 11, 2017 at 9:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <john.stultz@linaro.org> wrote:
> >> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@linaro.org> 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.

It was just an idea to make debugging easier. And avoid an endless stream
of regression reports to keep you busy :-)

> 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?

mode_valid was kinda meant for simple limit stuff like max/min. If you
first need to round stuff to something the hw can do, then whitelist it,
it gets more tricky. You might just need to hand-roll stuff here, I don't
think this can be reasonably resolved with just helper infrastructure.
Hand-roll = create a dummy adjusted_mode and call the mode_fixup stuff
directly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch