Date:   Thu, 26 May 2022 17:42:18 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Sean Paul <seanpaul@...omium.org>
Cc:     Doug Anderson <dianders@...omium.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Stéphane Marchesin <marcheu@...omium.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "Aravind Venkateswaran (QUIC)" <quic_aravindh@...cinc.com>,
        "Kuogee Hsieh (QUIC)" <quic_khsieh@...cinc.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Rob Clark <robdclark@...il.com>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        "Abhinav Kumar (QUIC)" <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sean Paul <seanpaul@...gle.com>
Subject: Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

On Thu, 26 May 2022 at 03:28, Sean Paul <seanpaul@...omium.org> wrote:
>
> On Wed, May 25, 2022 at 9:26 AM Daniel Vetter <daniel@...ll.ch> wrote:
> >
> > On Mon, May 23, 2022 at 05:59:02PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, May 20, 2022 at 5:01 PM Doug Anderson <dianders@...omium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@...e.de> wrote:
> > > > >
> > > > > Hi Douglas,
> > > > >
> > > > > I understand that you're trying to tell userspace that the modelist has
> > > > > been made up, but it's not something that should be done via fragile
> > > > > heuristics IMHO.
> > > > >
> > > > > I looked at the Chromium source code that you linked, but I cannot say
> > > > > whether it's doing the correct thing. It all depends on what your
> > > > > program needs.
> > > > >
> > > > > In that function, you could also search for 'DRM_MODE_TYPE_USERDEF'.
> > > > > It's the mode that the user specified on the kernel command line. If
> > > > > Chromium's automatic mode selection fails, you'd give your users direct
> > > > > control over it.
> > > >
> > > > That doesn't really work for Chrome OS. Certainly a kernel hacker
> > > > could do this, but it's not something I could imagine us exposing to
> > > > an average user of a Chromebook.
> > > >
> > > >
> > > > > When there's no flagged mode or if
> > > > > /sys/class/drm/card<...>/status contains "unconnected", you can assume
> > > > > that the modelist is artificial and try the modes in an appropriate order.
> > > >
> > > > So "no flagged" means that nothing is marked as preferred, correct?
> > > >
> > > > ...so I guess what you're suggesting is that the order that the kernel
> > > > is presenting the modes to userspace is not ABI. If there are no
> > > > preferred modes then userspace shouldn't necessarily assume that the
> > > > first mode returned is the best mode. Instead it should assume that if
> > > > there is no preferred mode then the mode list is made up and it should
> > > > make its own decisions about the best mode to start with. If this is
> > > > the ABI from the kernel then plausibly I could convince people to
> > > > change userspace to pick 640x480 first in this case.
> > > >
> > > > > If we really want the kernel to give additional guarantees, we should
> > > > > have a broader discussion about this topic IMHO.
> > > >
> > > > Sure. I've added Stéphane Marchesin to this thread in case he wants to
> > > > chime in about anything.
> > > >
> > > > Overall, my take on the matter:
> > > >
> > > > * Mostly I got involved because, apparently, a DP compliance test was
> > > > failing. The compliance test was upset that when it presented us with
> > > > no EDID that we didn't default to 640x480. There was a push to make a
> > > > fix for this in the Qualcomm specific driver but that didn't sit right
> > > > with me.
> > > >
> > > > * On all devices I'm currently working with (laptops), the DP is a
> > > > secondary display. If a user was trying to plug in a display with a
> > > > bad EDID and the max mode (1024x768) didn't work, they could just use
> > > > the primary display to choose a different resolution. It seems
> > > > unlikely a user would truly be upset and would probably be happy they
> > > > could get their broken display to work at all. Even if this is a
> > > > primary display, I believe there are documented key combos to change
> > > > the resolution of the primary display even if you can't see anything.
> > > >
> > > > * That all being said, defaulting to 640x480 when there's no EDID made
> > > > sense to me, especially since it's actually defined in the DP spec. So
> > > > I'm trying to do the right thing and solve this corner case. That
> > > > being said, if it's truly controversial I can just drop it.
> > > >
> > > >
> > > > So I guess my plan will be to give Stéphane a little while in case he
> > > > wants to chime in. If not then I guess I'll try a Chrome patch...
> > > > ...and if that doesn't work, I'll just drop it.
> > >
> > > OK, this userspace code seems to work:
> > >
> > > https://crrev.com/c/3662501 - ozone/drm: Try 640x480 before picking
> > > the first mode if no EDID
> > >
> > > ...so we'll see how review of that goes. :-)
>
> Mirroring some of my comments on that review here :-)
>
> IMO, this should be addressed in the kernel, or not at all. The kernel
> ensures other aspects of DisplayPort implementation are compliant, so
> I don't think this would be any exception. Further, the kernel is the
> one creating the "safe" mode list, so it seems odd that userspace
> would override that. Finally, relying on every userspace to do the
> right thing is asking for trouble (we have 3 places which would need
> this logic in CrOS).

Oh I missed the part that this is defined in the DP spec as _the_ fallback mode.

I think the probe helpers could check whether it's a DP connector and
then dtrt per DP spec? I think that should have a solid chance of
avoiding the regression mess, since the really shoddy stuff tends to
be VGA/HDMI.

Also if DP says only 640x480 should be the fallback if there's no
other mode list source, then I think we should trim it down to only
that. But also only for DP.

Also ofc that patch should reference the right DP spec sections :-)
-Daniel

>
> >
> > Yeah it sucks a bit but I'm mildly afraid that if we muck around with the
> > absolute fallback mode list in upstream we get whacked by a regression
> > report :-/
>
> Yeah, this seems likely (unfortunately).
>
> >
> > There's the additional fun that on modern displays probably 720p (or maybe
> > 720i) is a lot more likely to work than anything else really, so best we
> > can do here maybe is to make it an uapi guarantee that if there's no
> > preferred mode, then most likely the kernel invent random noise out of
> > thin air, and userspace has to be careful and do its own magic heuristics.
> > Or maybe we should add a flag for "this stuff is invented, buyer beware".
> >
>
> This seems like a reasonable compromise. Perhaps marking 640x480 as
> preferred would be a middle road?
>
> > I think clarifying that would be good. Changing defaults feels a bit too
> > risky, we had some really hilarious regression reports in the past along
> > the lines of the infamous xkcd.
>
> FWIW, I don't really have a strong opinion as to whether this should
> be fixed or not. I have a hard time believing that either 1024x768 or
> 640x480 would result in a happy result for the user, so we're really
> just choosing a mode which is bad enough for the user to
> unplug/replug. If 640x480 makes the compliance machine happy, I
> suppose that's a compelling reason, but I don't really feel like this
> is worth special casing each userspace.
>
> Sean
>
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists