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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
---
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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux