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