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

[Daniel Vetter]

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).

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch