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 <Jose.Abreu@xxxxxxxxxxxx> 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 <joabreu@xxxxxxxxxxxx> >>>>>>> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx> >>>>>>> Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx> >>>>>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >>>>>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> >>>>>>> Cc: Dave Airlie <airlied@xxxxxxxx> >>>>>> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
