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@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. 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
