[PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Douglas Anderson]

If we're unable to read the EDID for a display because it's corrupt /
bogus / invalid then we'll add a set of standard modes for the
display. Since we have no true information about the connected
display, these modes are essentially guesses but better than nothing.
None of the modes returned is marked as preferred, but the modes are
currently sorted such that the higher resolution modes are listed
first.

When userspace sees these modes presented by the kernel it needs to
figure out which one to pick. At least one userspace, ChromeOS [1]
seems to use the rules:
1. Try to pick the first mode marked as preferred.
2. If no modes were marked as preferred then pick the first mode.

The rules above seem pretty reasonable, but they have unfortunate side
effect that when we have no EDID present we'll default to the highest
resolution (least likely to work) mode.

Let's change things slightly. In the case of a failed EDID read we
still won't mark anything preferred but we _won't_ sort the modes at
the end of drm_helper_probe_single_connector_modes(). The
drm_add_modes_noedid() adds 640x480 first and so by skipping the call
to drm_mode_sort() it will stay first. That will be a hint to
userspace to default to 640x480.

This change makes userspace that behaves like ChromeOS does compliant
with section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a Link
CTS. That section indicates that, at least on DP, if we have a corrupt
EDID userspace may allow other modes to be tried but should default to
640x480 in the absence of more information. Note that if
drm_add_modes_noedid() ever changes to _not_ list 640x480 first we
might need to do more here, but that seems unlikely and, in any case,
it would be caught by a future run of DP compliance testing.

Note: this change could pave the way to further improvement to
drm_helper_probe_single_connector_modes(). Specifically, in the case
of no EDID we could add additional "standard" modes that are riskier
than 1024x768 (the current max we add). Now that we're giving
userspace the hint that it should default to 640x480 perhaps it would
be OK to offer the options of the higher resolution modes just in case
they work. This further improvement is left as an exercise to the
reader.

[1] https://source.chromium.org/chromium/chromium/src/+/a051f741d0a15caff2251301efe081c30e0f4a96:ui/ozone/platform/drm/common/drm_util.cc;l=488

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Note that this is the second of two related and similar-sounding but
different patches. See also ("drm/probe-helper: For DP, add 640x480 if
all other modes are bad") [2]. I'm hoping to land _both_ of the
patches since they address different issues. This patch addresses the
case of a corrupt EDID and having 640x480 be the default in the
"guessed" modes. The other patch handles the case where the EDID
_isn't_ corrupt but all the modes listed can't be made with the
existing situations. The two patches can land in either order.

Also note that I didn't carry any Tested-by / Reviewed-by tags since
the patch is now quite different (yet again for v2 to v3).

[2] https://lore.kernel.org/r/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid

Changes in v3:
- Don't set preferred, just disable the sort.

Changes in v2:
- Don't modify drm_add_modes_noedid() 'cause that'll break others
- Set 640x480 as preferred in drm_helper_probe_single_connector_modes()

 drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 682359512996..21dd60f30cc7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -425,6 +425,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	bool verbose_prune = true;
 	enum drm_connector_status old_status;
 	struct drm_modeset_acquire_ctx ctx;
+	bool sort_list = true;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 
@@ -516,8 +517,16 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		count = drm_add_override_edid_modes(connector);
 
 	if (count == 0 && (connector->status == connector_status_connected ||
-			   connector->status == connector_status_unknown))
+			   connector->status == connector_status_unknown)) {
 		count = drm_add_modes_noedid(connector, 1024, 768);
+		/*
+		 * Want lower res modes, like 640x480, first. That indicates
+		 * to userspace that these are "better" modes. Since we have
+		 * no EDID the modes are a guess anyway, so guess the safer
+		 * mode first.
+		 */
+		sort_list = false;
+	}
 	count += drm_helper_probe_add_cmdline_mode(connector);
 	if (count == 0)
 		goto prune;
@@ -576,7 +585,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	if (list_empty(&connector->modes))
 		return 0;
 
-	drm_mode_sort(&connector->modes);
+	if (sort_list)
+		drm_mode_sort(&connector->modes);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed modes :\n", connector->base.id,
 			connector->name);
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6683 bytes --]

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

If we really want the kernel to give additional guarantees, we should 
have a broader discussion about this topic IMHO.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.17.8/source/drivers/gpu/drm/drm_sysfs.c#L196

Am 13.05.22 um 22:06 schrieb Douglas Anderson:
> If we're unable to read the EDID for a display because it's corrupt /
> bogus / invalid then we'll add a set of standard modes for the
> display. Since we have no true information about the connected
> display, these modes are essentially guesses but better than nothing.
> None of the modes returned is marked as preferred, but the modes are
> currently sorted such that the higher resolution modes are listed
> first.
> 
> When userspace sees these modes presented by the kernel it needs to
> figure out which one to pick. At least one userspace, ChromeOS [1]
> seems to use the rules:
> 1. Try to pick the first mode marked as preferred.
> 2. If no modes were marked as preferred then pick the first mode.
> 
> The rules above seem pretty reasonable, but they have unfortunate side
> effect that when we have no EDID present we'll default to the highest
> resolution (least likely to work) mode.
> 
> Let's change things slightly. In the case of a failed EDID read we
> still won't mark anything preferred but we _won't_ sort the modes at
> the end of drm_helper_probe_single_connector_modes(). The
> drm_add_modes_noedid() adds 640x480 first and so by skipping the call
> to drm_mode_sort() it will stay first. That will be a hint to
> userspace to default to 640x480.
> 
> This change makes userspace that behaves like ChromeOS does compliant
> with section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a Link
> CTS. That section indicates that, at least on DP, if we have a corrupt
> EDID userspace may allow other modes to be tried but should default to
> 640x480 in the absence of more information. Note that if
> drm_add_modes_noedid() ever changes to _not_ list 640x480 first we
> might need to do more here, but that seems unlikely and, in any case,
> it would be caught by a future run of DP compliance testing.
> 
> Note: this change could pave the way to further improvement to
> drm_helper_probe_single_connector_modes(). Specifically, in the case
> of no EDID we could add additional "standard" modes that are riskier
> than 1024x768 (the current max we add). Now that we're giving
> userspace the hint that it should default to 640x480 perhaps it would
> be OK to offer the options of the higher resolution modes just in case
> they work. This further improvement is left as an exercise to the
> reader.
> 
> [1] https://source.chromium.org/chromium/chromium/src/+/a051f741d0a15caff2251301efe081c30e0f4a96:ui/ozone/platform/drm/common/drm_util.cc;l=488
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Note that this is the second of two related and similar-sounding but
> different patches. See also ("drm/probe-helper: For DP, add 640x480 if
> all other modes are bad") [2]. I'm hoping to land _both_ of the
> patches since they address different issues. This patch addresses the
> case of a corrupt EDID and having 640x480 be the default in the
> "guessed" modes. The other patch handles the case where the EDID
> _isn't_ corrupt but all the modes listed can't be made with the
> existing situations. The two patches can land in either order.
> 
> Also note that I didn't carry any Tested-by / Reviewed-by tags since
> the patch is now quite different (yet again for v2 to v3).
> 
> [2] https://lore.kernel.org/r/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
> 
> Changes in v3:
> - Don't set preferred, just disable the sort.
> 
> Changes in v2:
> - Don't modify drm_add_modes_noedid() 'cause that'll break others
> - Set 640x480 as preferred in drm_helper_probe_single_connector_modes()
> 
>   drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 682359512996..21dd60f30cc7 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -425,6 +425,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   	bool verbose_prune = true;
>   	enum drm_connector_status old_status;
>   	struct drm_modeset_acquire_ctx ctx;
> +	bool sort_list = true;
>   
>   	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>   
> @@ -516,8 +517,16 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   		count = drm_add_override_edid_modes(connector);
>   
>   	if (count == 0 && (connector->status == connector_status_connected ||
> -			   connector->status == connector_status_unknown))
> +			   connector->status == connector_status_unknown)) {
>   		count = drm_add_modes_noedid(connector, 1024, 768);
> +		/*
> +		 * Want lower res modes, like 640x480, first. That indicates
> +		 * to userspace that these are "better" modes. Since we have
> +		 * no EDID the modes are a guess anyway, so guess the safer
> +		 * mode first.
> +		 */
> +		sort_list = false;
> +	}
>   	count += drm_helper_probe_add_cmdline_mode(connector);
>   	if (count == 0)
>   		goto prune;
> @@ -576,7 +585,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>   	if (list_empty(&connector->modes))
>   		return 0;
>   
> -	drm_mode_sort(&connector->modes);
> +	if (sort_list)
> +		drm_mode_sort(&connector->modes);
>   
>   	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed modes :\n", connector->base.id,
>   			connector->name);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Doug Anderson]

Hi,

On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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.


> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/v5.17.8/source/drivers/gpu/drm/drm_sysfs.c#L196
>
> Am 13.05.22 um 22:06 schrieb Douglas Anderson:
> > If we're unable to read the EDID for a display because it's corrupt /
> > bogus / invalid then we'll add a set of standard modes for the
> > display. Since we have no true information about the connected
> > display, these modes are essentially guesses but better than nothing.
> > None of the modes returned is marked as preferred, but the modes are
> > currently sorted such that the higher resolution modes are listed
> > first.
> >
> > When userspace sees these modes presented by the kernel it needs to
> > figure out which one to pick. At least one userspace, ChromeOS [1]
> > seems to use the rules:
> > 1. Try to pick the first mode marked as preferred.
> > 2. If no modes were marked as preferred then pick the first mode.
> >
> > The rules above seem pretty reasonable, but they have unfortunate side
> > effect that when we have no EDID present we'll default to the highest
> > resolution (least likely to work) mode.
> >
> > Let's change things slightly. In the case of a failed EDID read we
> > still won't mark anything preferred but we _won't_ sort the modes at
> > the end of drm_helper_probe_single_connector_modes(). The
> > drm_add_modes_noedid() adds 640x480 first and so by skipping the call
> > to drm_mode_sort() it will stay first. That will be a hint to
> > userspace to default to 640x480.
> >
> > This change makes userspace that behaves like ChromeOS does compliant
> > with section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a Link
> > CTS. That section indicates that, at least on DP, if we have a corrupt
> > EDID userspace may allow other modes to be tried but should default to
> > 640x480 in the absence of more information. Note that if
> > drm_add_modes_noedid() ever changes to _not_ list 640x480 first we
> > might need to do more here, but that seems unlikely and, in any case,
> > it would be caught by a future run of DP compliance testing.
> >
> > Note: this change could pave the way to further improvement to
> > drm_helper_probe_single_connector_modes(). Specifically, in the case
> > of no EDID we could add additional "standard" modes that are riskier
> > than 1024x768 (the current max we add). Now that we're giving
> > userspace the hint that it should default to 640x480 perhaps it would
> > be OK to offer the options of the higher resolution modes just in case
> > they work. This further improvement is left as an exercise to the
> > reader.
> >
> > [1] https://source.chromium.org/chromium/chromium/src/+/a051f741d0a15caff2251301efe081c30e0f4a96:ui/ozone/platform/drm/common/drm_util.cc;l=488
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > Note that this is the second of two related and similar-sounding but
> > different patches. See also ("drm/probe-helper: For DP, add 640x480 if
> > all other modes are bad") [2]. I'm hoping to land _both_ of the
> > patches since they address different issues. This patch addresses the
> > case of a corrupt EDID and having 640x480 be the default in the
> > "guessed" modes. The other patch handles the case where the EDID
> > _isn't_ corrupt but all the modes listed can't be made with the
> > existing situations. The two patches can land in either order.
> >
> > Also note that I didn't carry any Tested-by / Reviewed-by tags since
> > the patch is now quite different (yet again for v2 to v3).
> >
> > [2] https://lore.kernel.org/r/20220510131309.v2.2.I4ac7f55aa446699f8c200a23c10463256f6f439f@changeid
> >
> > Changes in v3:
> > - Don't set preferred, just disable the sort.
> >
> > Changes in v2:
> > - Don't modify drm_add_modes_noedid() 'cause that'll break others
> > - Set 640x480 as preferred in drm_helper_probe_single_connector_modes()
> >
> >   drivers/gpu/drm/drm_probe_helper.c | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 682359512996..21dd60f30cc7 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -425,6 +425,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >       bool verbose_prune = true;
> >       enum drm_connector_status old_status;
> >       struct drm_modeset_acquire_ctx ctx;
> > +     bool sort_list = true;
> >
> >       WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> >
> > @@ -516,8 +517,16 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >               count = drm_add_override_edid_modes(connector);
> >
> >       if (count == 0 && (connector->status == connector_status_connected ||
> > -                        connector->status == connector_status_unknown))
> > +                        connector->status == connector_status_unknown)) {
> >               count = drm_add_modes_noedid(connector, 1024, 768);
> > +             /*
> > +              * Want lower res modes, like 640x480, first. That indicates
> > +              * to userspace that these are "better" modes. Since we have
> > +              * no EDID the modes are a guess anyway, so guess the safer
> > +              * mode first.
> > +              */
> > +             sort_list = false;
> > +     }
> >       count += drm_helper_probe_add_cmdline_mode(connector);
> >       if (count == 0)
> >               goto prune;
> > @@ -576,7 +585,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >       if (list_empty(&connector->modes))
> >               return 0;
> >
> > -     drm_mode_sort(&connector->modes);
> > +     if (sort_list)
> > +             drm_mode_sort(&connector->modes);
> >
> >       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed modes :\n", connector->base.id,
> >                       connector->name);
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Doug Anderson]

Hi,

On Fri, May 20, 2022 at 5:01 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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. :-)

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Daniel Vetter]

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@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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. :-)

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 :-/

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

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Sean Paul]

On Wed, May 25, 2022 at 9:26 AM Daniel Vetter <daniel@ffwll.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@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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).

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

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Daniel Vetter]

On Thu, 26 May 2022 at 03:28, Sean Paul <seanpaul@chromium.org> wrote:
>
> On Wed, May 25, 2022 at 9:26 AM Daniel Vetter <daniel@ffwll.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@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Doug Anderson]

Hi,

On Thu, May 26, 2022 at 8:42 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, 26 May 2022 at 03:28, Sean Paul <seanpaul@chromium.org> wrote:
> >
> > On Wed, May 25, 2022 at 9:26 AM Daniel Vetter <daniel@ffwll.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@chromium.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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.

I'm fine with making this DP-specific if that's what people think is best.


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

So the DP spec says that 640x480 is _the_ default fallback, but it
also says that we're also allowed to have some implementation-specific
fall-back modes as well, so I'd rather not fully trim the list and
just make it clear (somehow) that 640x480 ought to be the default.
Would you be OK going back to v2 of this patch [1] but adding a check
that the connector type is DP and also making sure that the spec is
referenced?


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

My original patch description for this patch (v3) did reference
section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a Link CTS.
...or did you want this in inline comments in the patch itself?


[1] https://lore.kernel.org/r/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid

Re: [PATCH v3] drm/probe-helper: Make 640x480 first if no EDID

[Daniel Vetter]

On Thu, May 26, 2022 at 09:01:03AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 26, 2022 at 8:42 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, 26 May 2022 at 03:28, Sean Paul <seanpaul@chromium.org> wrote:
> > >
> > > On Wed, May 25, 2022 at 9:26 AM Daniel Vetter <daniel@ffwll.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@chromium.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, May 16, 2022 at 3:28 AM Thomas Zimmermann <tzimmermann@suse.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.
> 
> I'm fine with making this DP-specific if that's what people think is best.
> 
> 
> > 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.
> 
> So the DP spec says that 640x480 is _the_ default fallback, but it
> also says that we're also allowed to have some implementation-specific
> fall-back modes as well, so I'd rather not fully trim the list and
> just make it clear (somehow) that 640x480 ought to be the default.
> Would you be OK going back to v2 of this patch [1] but adding a check
> that the connector type is DP and also making sure that the spec is
> referenced?

Sounds reasonable.
-Daniel

> 
> 
> > Also ofc that patch should reference the right DP spec sections :-)
> 
> My original patch description for this patch (v3) did reference
> section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a Link CTS.
> ...or did you want this in inline comments in the patch itself?
> 
> 
> [1] https://lore.kernel.org/r/20220510135101.v2.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid

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