Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751363AbdGRG02 (ORCPT ); Tue, 18 Jul 2017 02:26:28 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:35061 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750931AbdGRG01 (ORCPT ); Tue, 18 Jul 2017 02:26:27 -0400 Date: Tue, 18 Jul 2017 08:26:21 +0200 From: Daniel Vetter To: John Stultz Cc: Daniel Vetter , Sean Paul , Jose Abreu , Chen Feng , lkml , Xinliang Liu , "dri-devel@lists.freedesktop.org" , Rongrong Zou , Daniel Vetter , Xinwei Kong Subject: Re: [RFC][PATCH] drm: kirin: Restrict modes to known good mode clocks Message-ID: <20170718062621.mmwdn5ddsp5gl2h6@phenom.ffwll.local> Mail-Followup-To: John Stultz , Sean Paul , Jose Abreu , Chen Feng , lkml , Xinliang Liu , "dri-devel@lists.freedesktop.org" , Rongrong Zou , Daniel Vetter , Xinwei Kong References: <1499719682-31750-1-git-send-email-john.stultz@linaro.org> <20170710211833.bprcrq5tfpzxtpeg@art_vandelay> <20170711075625.peykmamxkt7slqdz@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 4.11.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5128 Lines: 101 On Mon, Jul 17, 2017 at 04:20:23PM -0700, John Stultz wrote: > On Tue, Jul 11, 2017 at 9:27 AM, Daniel Vetter wrote: > > On Tue, Jul 11, 2017 at 5:44 PM, John Stultz wrote: > >> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter wrote: > >>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz 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