From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754056AbdEDL4K (ORCPT ); Thu, 4 May 2017 07:56:10 -0400 Received: from smtprelay.synopsys.com ([198.182.47.9]:56309 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752323AbdEDLzw (ORCPT ); Thu, 4 May 2017 07:55:52 -0400 Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback To: Daniel Vetter References: <3e1dba0cd5fc053e56f1c9c94d0cb8789ecca4ae.1493203049.git.joabreu@synopsys.com> <20170502084807.k7ioo7a5j33xtnpj@phenom.ffwll.local> <80752cfe-4bc7-b7c6-c29b-e634c356a509@synopsys.com> <9edb415e-4d0b-5a2a-a735-0dbb98d77984@synopsys.com> <20170503150031.rpd5xs2bb67mpsnc@phenom.ffwll.local> CC: Jose Abreu , dri-devel , Linux Kernel Mailing List , Carlos Palminha , Alexey Brodkin , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Dave Airlie From: Jose Abreu Message-ID: <8e78710b-6986-d491-ba00-84e91ba3c93d@synopsys.com> Date: Thu, 4 May 2017 12:55:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.107.19.89] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Daniel, On 04-05-2017 11:21, Jose Abreu wrote: > Hi Daniel, > > > On 03-05-2017 16:00, Daniel Vetter wrote: >> On Wed, May 03, 2017 at 03:16:13PM +0100, Jose Abreu wrote: >>> Hi Daniel, >>> >>> >>> On 03-05-2017 07:19, Daniel Vetter wrote: >>>> On Tue, May 2, 2017 at 11:29 AM, Jose Abreu wrote: >>>>> On 02-05-2017 09:48, Daniel Vetter wrote: >>>>>> On Wed, Apr 26, 2017 at 11:48:34AM +0100, Jose Abreu wrote: >>>>>>> Some crtc's may have restrictions in the mode they can display. In >>>>>>> this patch a new callback (crtc->mode_valid()) is introduced that >>>>>>> is called at the same stage of connector->mode_valid() callback. >>>>>>> >>>>>>> This shall be implemented if the crtc has some sort of restriction >>>>>>> so that we don't probe modes that will fail in the commit() stage. >>>>>>> For example: A given crtc may be responsible to set a clock value. >>>>>>> If the clock can not produce all the values for the available >>>>>>> modes then this callback can be used to restrict the number of >>>>>>> probbed modes to only the ones that can be displayed. >>>>>>> >>>>>>> If the crtc does not implement the callback then the behaviour will >>>>>>> remain the same. Also, for a given set of crtcs that can be bound to >>>>>>> the connector, if at least one can display the mode then the mode >>>>>>> will be probbed. >>>>>>> >>>>>>> Signed-off-by: Jose Abreu >>>>>>> Cc: Carlos Palminha >>>>>>> Cc: Alexey Brodkin >>>>>>> Cc: Ville Syrjälä >>>>>>> Cc: Daniel Vetter >>>>>>> Cc: Dave Airlie >>>>>> Not sure this is useful, since you still have to duplicate the exact same >>>>>> check into your ->mode_fixup hook. That seems to make things even more >>>>>> confusing. >>>>> Yeah, in arcpgu I had to duplicate the code in ->atomic_check. >>>>> >>>>>> Also this doesn't update the various kerneldoc comments. For the existing >>>>>> hooks. Since this topic causes so much confusion, I don't think a >>>>>> half-solution will help, but has some good potential to make things worse. >>>>> I only documented the callback in drm_modeset_helper_vtables.h. >>>>> >>>>> Despite all of this, I think it doesn't makes sense delivering >>>>> modes to userspace which can never be used. >>>>> >>>>> This is really annoying in arcpgu. Imagine: I try to use mpv to >>>>> play a video, the full set of modes from EDID were probed so if I >>>>> just start mpv it will pick the native mode of the TV instead of >>>>> the one that is supported, so mpv will fail to play. I know the >>>>> value of clock which will work (so I know what mode shall be >>>>> used), but a normal user which is not aware of the HW will have >>>>> to cycle through the list of modes and try them all until it hits >>>>> one that works. Its really boring. >>>>> >>>>> For the modes that user specifies manually there is nothing we >>>>> can do, but we should not trick users into thinking that a given >>>>> mode is supported when it will always fail at commit. >>>> Yes, you are supposed to filter these out in ->mode_valid. But my >>>> stance is that only adding a half-baked support for a new callback to >>>> the core isn't going to make life easier for drivers, it will just add >>>> to the confusion. There's already piles of docs for both @mode_valid >>>> and @mode_fixup hooks explaining this, I don't want to make the >>>> documentation even more complex. And half-baked crtc checking is >>>> _much_ easier to implement in the driver directly (e.g. i915 checks >>>> for crtc constraints since forever, as do the other big x86 drivers). >>> But i915 crtc checks are done after handing the mode to >>> userspace, arcpgu also does that. We must let users specify >>> manually a mode but there is no point in returning modes in >>> get_connector which will always fail to commit. I get your point >>> and this can lead to code duplication, but I don't think it will >>> lead to confusion as long as it is well documented. And besides, >>> the callback is completely optional. >> Look closer, e.g. intel_dp_mode_valid calls >> intel_dp_downstream_max_dotclock which also looks at >> dev_priv->max_dotclkc_freq (which is the source dotclk limit, yeah it's a >> misnamed function). >> >> And the max dotclk is very much a crtc limit, not a port limit. Note that >> a bunch of other ports have port limits which are guaranteed to be lower >> than the crtc limit, hence the absence of the checks. >> >>>> So all taken together, if we add a ->mode_valid to crtcs, then imo we >>>> should do it right and actually make life easier for drivers. A good >>>> proof would be if your patch would allow us to drop a lot of the >>>> lenghty language from the @mode_valid hooks. >>> I completely agree that it should make life easier for drivers >>> but unfortunately I don't really see how :/ >>> >>> So, in summary: >>> Disadvantage 1: Code duplication >>> Disadvantage 2: Confusing documentation can lead to callback >>> misuse >>> >>> Advantage 1: User will get life simpler >> Ok, let me try to explain a bit in more detail what I think would be a >> real improvement: >> - Add ->mode_valid checks to all the places where we currently have >> ->mode_fixup. That'd be crtc, encoder and bridges. >> >> - Pimp the probe helper code to go through all of the combinations, >> filtering out those that aren't allowed by possible_* masks (essentially >> do the same thing that userspace is supposed to do). >> >> - Call all these ->mode_valid checks from the atomic check functions (I >> think we can forget about the legacy crtc helpers for old drivers). Do >> this also for connector->mode_valid. >> >> Taken all together this gives us the guarantee that that any mode which >> fails the check in the probe path is guaranteed to never pass in an atomic >> commit. And since the probed mode list is what developers generally see, >> that's hopefully enough to make sure the filtering is correct. >> >> It is a bit more code than what you've typed here, but not a lot: >> - probe path needs to loop over all CRTCxEncoder combos (the >> encoder->bridge routing is fixed) instead over just CRTCs. >> - Call ->mode_valid in all the places we already call ->mode_fixup. You >> don't need a new loop over all connectors to be able to call >> ->mode_valid since we already have that connector loop in >> check_modesets(). >> >> With that we should also be able to simplify the documentation and rip out >> all the warnings about how this is tricky. > This seems very nice! So we essentially can remove the validation > of modes in atomic_check as mode_valid will be called before, right? > > Best regards, > Jose Miguel Abreu One more thing: When should the mode_valid callback be called? Before or after mode_fixup/atomic_check? I think it makes sense to call it after, so that if a fixup to the mode is needed then we call mode_valid() after with the adjusted mode. What do you think? Best regards, Jose Miguel Abreu > >> -Daniel