[PATCH 1/2] drm/edid: If no preferred mode is found assume the first mode is preferred

[PATCH 1/2] drm/edid: If no preferred mode is found assume the first mode is preferred

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Some monitors apparently forget to mark any mode as preferred in the
EDID. In this particular case we have a very generic looking ID
"PNP Model 0 Serial Number 4" / "LVDS 800x600" so a specific quirk
doesn't seem particularly wise. Also the quirk we have
(EDID_QUIRK_FIRST_DETAILED_PREFERRED) is actually defunct so we'd
have to fix it first.

As a generic fallback let's just mark the first probed mode (which
should be the first detailed mode, assuming there are any) as
preferred.

On this particular machine the VBIOS seems to pick the 800x600
60Hz EST mode, which has the opposite sync polarities to the
800x600 60Hz detailed timings. But since it works I guess we
can ignore that slight discrepancy.

Cc: Adam Jackson <ajax@redhat.com>
Cc: Roberto Viola <cagnulein@gmail.com>
Tested-by: Roberto Viola <cagnulein@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109780
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5f142530532a..6c6a93647686 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1828,7 +1828,7 @@ static void edid_fixup_preferred(struct drm_connector *connector,
 	list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) {
 		cur_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
 
-		if (cur_mode == preferred_mode)
+		if (cur_mode == preferred_mode || target_refresh == 0)
 			continue;
 
 		/* Largest mode is preferred */
@@ -1850,6 +1850,18 @@ static void edid_fixup_preferred(struct drm_connector *connector,
 	preferred_mode->type |= DRM_MODE_TYPE_PREFERRED;
 }
 
+static bool preferred_mode_exists(struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+
+	list_for_each_entry(mode, &connector->probed_modes, head) {
+		if (mode->type & DRM_MODE_TYPE_PREFERRED)
+			return true;
+	}
+
+	return false;
+}
+
 static bool
 mode_is_rb(const struct drm_display_mode *mode)
 {
@@ -4733,7 +4745,8 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
 		num_modes += add_inferred_modes(connector, edid);
 
-	if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
+	if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75) ||
+	    !preferred_mode_exists(connector))
 		edid_fixup_preferred(connector, quirks);
 
 	if (quirks & EDID_QUIRK_FORCE_6BPC)
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/edid: Remove defunct EDID_QUIRK_FIRST_DETAILED_PREFERRED

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks like EDID_QUIRK_FIRST_DETAILED_PREFERRED never did anything.
Its counterpart in f86EdidModes.c is properly hooked up but somehow
that functionality was lost when it was copied into the kernel.

Assuming that another preferred mode didn't sneak in somehow
(is that even possible?) this should now be fully covered by the
generic fallback that marks the first probed mode as preferred.
So let's just remove the quirk.

I tried to double check the known cases:
* Proview AY765C
  https://bugs.freedesktop.org/show_bug.cgi?id=15160 seems OK
* Unknown Acer
  https://bugzilla.redhat.com/show_bug.cgi?id=284231 (got the
  reference from xf86EdidModes.c), no access
* Philips 107p5 CRT
  "Reported on xorg@ with pastebin", didn't find the mail(s)
* Peacock Ergovision 19 (only in xf86EdidModes.c)
  https://bugzilla.redhat.com/show_bug.cgi?id=492359 seems OK

Cc: Adam Jackson <ajax@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6c6a93647686..59829e596910 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -68,8 +68,6 @@
  * maximum size and use that.
  */
 #define EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE	(1 << 4)
-/* Monitor forgot to set the first detailed is preferred bit. */
-#define EDID_QUIRK_FIRST_DETAILED_PREFERRED	(1 << 5)
 /* use +hsync +vsync for detailed mode */
 #define EDID_QUIRK_DETAILED_SYNC_PP		(1 << 6)
 /* Force reduced-blanking timings for detailed modes */
@@ -107,8 +105,6 @@ static const struct edid_quirk {
 	{ "ACR", 44358, EDID_QUIRK_PREFER_LARGE_60 },
 	/* Acer F51 */
 	{ "API", 0x7602, EDID_QUIRK_PREFER_LARGE_60 },
-	/* Unknown Acer */
-	{ "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
 
 	/* AEO model 0 reports 8 bpc, but is a 6 bpc panel */
 	{ "AEO", 0, EDID_QUIRK_FORCE_6BPC },
@@ -145,12 +141,6 @@ static const struct edid_quirk {
 	{ "LPL", 0, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
 	{ "LPL", 0x2a00, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
 
-	/* Philips 107p5 CRT */
-	{ "PHL", 57364, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
-
-	/* Proview AY765C */
-	{ "PTS", 765, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
-
 	/* Samsung SyncMaster 205BW.  Note: irony */
 	{ "SAM", 541, EDID_QUIRK_DETAILED_SYNC_PP },
 	/* Samsung SyncMaster 22[5-6]BW */
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/edid: If no preferred mode is found assume the first mode is preferred

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: If no preferred mode is found assume the first mode is preferred
URL   : https://patchwork.freedesktop.org/series/57306/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5666 -> Patchwork_12318
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12318 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12318, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57306/revisions/1/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12318:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_hugepages:
    - fi-apl-guc:         PASS -> DMESG-WARN

  * igt@i915_selftest@live_requests:
    - fi-bxt-j4205:       PASS -> DMESG-WARN

  * igt@i915_selftest@live_timelines:
    - fi-ivb-3770:        PASS -> DMESG-WARN

  * igt@runner@aborted:
    - fi-bxt-j4205:       NOTRUN -> FAIL
    - fi-ivb-3770:        NOTRUN -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12318 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-kbl-7560u:       PASS -> FAIL [fdo#103375]

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         PASS -> FAIL [fdo#104008]

  
#### Possible fixes ####

  * igt@i915_selftest@live_requests:
    - fi-icl-u2:          INCOMPLETE [fdo#109644] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109644]: https://bugs.freedesktop.org/show_bug.cgi?id=109644


Participating hosts (44 -> 38)
------------------------------

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y 


Build changes
-------------

    * Linux: CI_DRM_5666 -> Patchwork_12318

  CI_DRM_5666: 358ab8acaabef3cef2a7ce9e5dd7c4196a0c30fc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4860: b79007f9c575a538a63ce9301a890ed9e1a45f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12318: 5c9fa35c3ea45ef953f82bb03e1882a2f8670f68 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5c9fa35c3ea4 drm/edid: Remove defunct EDID_QUIRK_FIRST_DETAILED_PREFERRED
0aebfe5ba886 drm/edid: If no preferred mode is found assume the first mode is preferred

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12318/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/edid: If no preferred mode is found assume the first mode is preferred

[Adam Jackson]

On Wed, 2019-02-27 at 19:14 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Some monitors apparently forget to mark any mode as preferred in the
> EDID. In this particular case we have a very generic looking ID
> "PNP Model 0 Serial Number 4" / "LVDS 800x600" so a specific quirk
> doesn't seem particularly wise. Also the quirk we have
> (EDID_QUIRK_FIRST_DETAILED_PREFERRED) is actually defunct so we'd
> have to fix it first.
>
> As a generic fallback let's just mark the first probed mode (which
> should be the first detailed mode, assuming there are any) as
> preferred.

What problem is this trying to fix? Userspace (and drm for that matter)
is typically going to pick the first mode in the list anyway if there's
none marked as preferred. Not having any preferred modes was pretty
common on CRTs IIRC.

The other major case I've seen of a monitor with no preferred mode are
the early dual-link DVI displays without internal scalers (Apple
Cinema, Dell 3007WFP, etc). You end up with 1280x800 first in the list
since 2560x1600 doesn't fit in a single DVI link. It might be nice if
such monitors decided their preferred mode based on the link
capabilities; if you know it's a dual-link capable port, you'd probably
prefer 2560x1600.

Does it make more sense to run the "infer a preferred mode" logic after
we've done mode validation for the output?

- ajax

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/edid: If no preferred mode is found assume the first mode is preferred

[Ville Syrjälä]

On Wed, Feb 27, 2019 at 03:23:01PM -0500, Adam Jackson wrote:
> On Wed, 2019-02-27 at 19:14 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Some monitors apparently forget to mark any mode as preferred in the
> > EDID. In this particular case we have a very generic looking ID
> > "PNP Model 0 Serial Number 4" / "LVDS 800x600" so a specific quirk
> > doesn't seem particularly wise. Also the quirk we have
> > (EDID_QUIRK_FIRST_DETAILED_PREFERRED) is actually defunct so we'd
> > have to fix it first.
> >
> > As a generic fallback let's just mark the first probed mode (which
> > should be the first detailed mode, assuming there are any) as
> > preferred.
> 
> What problem is this trying to fix? Userspace (and drm for that matter)
> is typically going to pick the first mode in the list anyway if there's
> none marked as preferred. Not having any preferred modes was pretty
> common on CRTs IIRC.

Ah sorry, I forgot to mention the original symptoms. The problem is
that with no preferred mode i915 decided to get the fixed mode for
the panel from the VBT instead. That one turned out to be 1024x768
which made the 800x600 panel somewhat unhappy.

So instead of putting this logic into the EDID parser I guess we
could just put it into the i915 fixed mode code. But then I suppose
we should also fix EDID_QUIRK_FIRST_DETAILED_PREFERRED (assuming it
exists for a good reason).

> 
> The other major case I've seen of a monitor with no preferred mode are
> the early dual-link DVI displays without internal scalers (Apple
> Cinema, Dell 3007WFP, etc). You end up with 1280x800 first in the list
> since 2560x1600 doesn't fit in a single DVI link. It might be nice if
> such monitors decided their preferred mode based on the link
> capabilities; if you know it's a dual-link capable port, you'd probably
> prefer 2560x1600.
> 
> Does it make more sense to run the "infer a preferred mode" logic after
> we've done mode validation for the output?

For this particular case that wouldn't help since then we'd end up
picking the 800x600 75Hz EST mode. Well, I'm not sure whether the
panel would like that or not. The VBIOS at least selects the 800x600
60Hz EST mode for whatever reason.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/edid: If no preferred mode is found assume the first mode is preferred

[Adam Jackson]

On Wed, 2019-02-27 at 23:03 +0200, Ville Syrjälä wrote:

> So instead of putting this logic into the EDID parser I guess we
> could just put it into the i915 fixed mode code. But then I suppose
> we should also fix EDID_QUIRK_FIRST_DETAILED_PREFERRED (assuming it
> exists for a good reason).

I don't think it has any good reason to exist, tbh. The commit adding
it to xserver was for the Philips 107P5, which - being a CRT - would be
entirely expected to not have the preferred bit set. We should probably
have handled that instead by letting the "target a DPI near 96" logic
handle things.

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel