From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752125AbdEDKVQ (ORCPT ); Thu, 4 May 2017 06:21:16 -0400 Received: from smtprelay2.synopsys.com ([198.182.60.111]:38131 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751347AbdEDKVH (ORCPT ); Thu, 4 May 2017 06:21:07 -0400 Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback To: Jose Abreu , dri-devel , Linux Kernel Mailing List , Carlos Palminha , Alexey Brodkin , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Dave Airlie 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> From: Jose Abreu Message-ID: Date: Thu, 4 May 2017 11:21:02 +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: <20170503150031.rpd5xs2bb67mpsnc@phenom.ffwll.local> 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 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 > -Daniel