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

[Jose Abreu]

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