Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback

[Ville Syrjälä]

On Wed, May 03, 2017 at 05:00:31PM +0200, 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.

We don't actually want the codepaths to match exactly. In i915
we allow the user to exceed some of the display/dongle limits
because those things often tell us that something shouldn't work
when in fact it does. And some users are quick to complain if
something stops working for them.

-- 
Ville Syrjälä
Intel OTC