VESA Enhanced EDID Standard does not clearly describe how display
panel vendors should setup the Sync Signal Defintions (bit 4 & 3) in
the Detailed Timing Definition (relative offset 17, absolute offset
47h[+18]) for Digital Video Signal Interfaces (bit 7 at offset 14h).

In practice many eDP panels which using a Digital Video Signal
Interfaces (bit 7 at offset 14h == 1) are leaving the Sync Signal
Defintions at offset 47h[+18] blank 0x00, which would mean concerned
with the VESA Standard [1] that they are using "Analog Composite
Sync".

Fix: Just detect Analog Sync Signal if an Analog Video Signal
     Interface (bit 7 at offset 14h == 0) is in use.  Just detect
     Digital Sync Signal if an Digital Video Signal Interface is in
     use.

Reference: [1] VESA Enhanced EDID Standard, Release A, Rev.2, Page 35

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8146
Signed-off-by: Dirk Lehmann <deve...@dj-l.de>
---
 drivers/gpu/drm/drm_edid.c | 74 ++++++++++++++++++++++++++++++++------
 include/drm/drm_edid.h     | 12 +++++--
 2 files changed, 73 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1f470968ed14..6afdc34e55ce 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3437,6 +3437,7 @@ static struct drm_display_mode *drm_mode_detailed(struct 
drm_connector *connecto
        const struct drm_display_info *info = &connector->display_info;
        struct drm_device *dev = connector->dev;
        struct drm_display_mode *mode;
+       const struct edid *edid = drm_edid->edid;
        const struct detailed_pixel_timing *pt = &timing->data.pixel_data;
        unsigned hactive = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo;
        unsigned vactive = (pt->vactive_vblank_hi & 0xf0) << 4 | pt->vactive_lo;
@@ -3456,10 +3457,6 @@ static struct drm_display_mode *drm_mode_detailed(struct 
drm_connector *connecto
                            connector->base.id, connector->name);
                return NULL;
        }
-       if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) {
-               drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not 
supported\n",
-                           connector->base.id, connector->name);
-       }
 
        /* it is incorrect if hsync/vsync width is zero */
        if (!hsync_pulse_width || !vsync_pulse_width) {
@@ -3505,11 +3502,68 @@ static struct drm_display_mode 
*drm_mode_detailed(struct drm_connector *connecto
 
        if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) {
                mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC;
+       } else if (edid->input & DRM_EDID_INPUT_DIGITAL) {
+               /* !info->quirks && edid->input == DIGITAL  */
+               switch (pt->misc & DRM_EDID_PT_SYNC_MASK) {
+               /* VESA Enhanced EDID Standard, Release A, Rev.2, Page 35
+                *
+                * CASE DRM_EDID_PT_ANALOG_CSYNC:
+                *
+                * (pt->misc & DRM_EDID_PT_SYNC_MASK == 0x00) means
+                * "Analog Composite Sync" as described in VESA
+                * Standard.  But many digital display panels without
+                * composite sync are also using 0x00 here.
+                *
+                * Therefore use DEFAULT: as we are currently on an
+                * digital video signal interface.
+                */
+               case DRM_EDID_PT_DIGITAL_CSYNC:
+                       drm_dbg_kms(dev,
+                               "[CONNECTOR:%d:%s] Digital composite sync!\n",
+                               connector->base.id, connector->name);
+                       mode->flags |= DRM_MODE_FLAG_CSYNC;
+                       mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ?
+                               DRM_MODE_FLAG_PCSYNC : DRM_MODE_FLAG_NCSYNC;
+                       break;
+               case DRM_EDID_PT_DIGITAL_SEPARATE_SYNC:
+                       drm_dbg_kms(dev,
+                               "[CONNECTOR:%d:%s] Digital seperate sync!\n",
+                               connector->base.id, connector->name);
+                       goto digital_default;
+                       break; /* Missing BREAK throws a compiler warning  */
+               default:
+digital_default:
+                       mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ?
+                               DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC;
+                       mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ?
+                               DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC;
+                       break;
+               }
        } else {
-               mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ?
-                       DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC;
-               mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ?
-                       DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC;
+               /* !info->quirks && edid->input == ANALOG  */
+               switch (pt->misc & DRM_EDID_PT_SYNC_MASK) {
+               /* VESA Enhanced EDID Standard, Release A, Rev.2, Page 35
+                *
+                * CASE DRM_EDID_PT_ANALOG_CSYNC:
+                *
+                * (pt->misc & DRM_EDID_PT_SYNC_MASK == 0x00) for
+                * "Analog Composite Sync" is possible here, as we are
+                * currently on an analog video signal interface.
+                */
+               case DRM_EDID_PT_ANALOG_CSYNC:
+               case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC:
+                       drm_dbg_kms(dev,
+                               "[CONNECTOR:%d:%s] Analog composite sync!\n",
+                               connector->base.id, connector->name);
+                       mode->flags |= DRM_MODE_FLAG_CSYNC | 
DRM_MODE_FLAG_NCSYNC;
+                       break;
+               default:
+                       mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ?
+                               DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC;
+                       mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ?
+                               DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC;
+                       break;
+               }
        }
 
 set_size:
@@ -3522,8 +3576,8 @@ static struct drm_display_mode *drm_mode_detailed(struct 
drm_connector *connecto
        }
 
        if (info->quirks & EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE) {
-               mode->width_mm = drm_edid->edid->width_cm * 10;
-               mode->height_mm = drm_edid->edid->height_cm * 10;
+               mode->width_mm = edid->width_cm * 10;
+               mode->height_mm = edid->height_cm * 10;
        }
 
        mode->type = DRM_MODE_TYPE_DRIVER;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 48e93f909ef6..169755d3de19 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -61,9 +61,15 @@ struct std_timing {
        u8 vfreq_aspect;
 } __attribute__((packed));
 
-#define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1)
-#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2)
-#define DRM_EDID_PT_SEPARATE_SYNC  (3 << 3)
+#define DRM_EDID_PT_SYNC_MASK              (3 << 3)
+# define DRM_EDID_PT_ANALOG_CSYNC          (0 << 3)
+# define DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC  (1 << 3)
+# define DRM_EDID_PT_DIGITAL_CSYNC         (2 << 3)
+#  define DRM_EDID_PT_CSYNC_ON_RGB         (1 << 1) /* analog csync only */
+#  define DRM_EDID_PT_CSYNC_SERRATE        (1 << 2)
+# define DRM_EDID_PT_DIGITAL_SEPARATE_SYNC (3 << 3)
+#  define DRM_EDID_PT_HSYNC_POSITIVE       (1 << 1) /* also digital csync */
+#  define DRM_EDID_PT_VSYNC_POSITIVE       (1 << 2)
 #define DRM_EDID_PT_STEREO         (1 << 5)
 #define DRM_EDID_PT_INTERLACED     (1 << 7)
 
-- 
2.40.1

Reply via email to