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

[Daniel Vetter]

On Thu, May 4, 2017 at 1:55 PM, Jose Abreu <Jose.Abreu@xxxxxxxxxxxx> wrote:
> 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?

I'd call it before, because if you call it afterwards then you won't
correctly filter things in the probe path (since those don't adjust
the mode). We can't make this perfect, but since it's a helper, 90% is
good enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch