On Tue, Aug 15, 2023 at 01:19:07PM +0300, Jani Nikula wrote:
> This reverts commit ca62297b2085b5b3168bd891ca24862242c635a1.
>
> Commit ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") fixed
> EDID detailed mode sync parsing. Unfortunately, there are quite a few
> displays out there that have bogus (zero) sync field that are broken by
> the change. Zero means analog composite sync, which is not right for
> digital displays, and the modes get rejected. Regardless, it used to
> work, and it needs to continue to work. Revert the change.
Bah. I guess one option would be to quirk the bogus EDIDs, or maybe just
ignore bogus sync flags for the eDP preferred mode. But maybe needs a
bit more thinking, so
Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Rejecting modes with analog composite sync was the part that fixed the
> gitlab issue 8146 [1]. We'll need to get back to the drawing board with
> that.
>
> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8930
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9044
> Fixes: ca62297b2085 ("drm/edid: Fix csync detailed mode parsing")
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.4+
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_edid.c | 29 ++++++++---------------------
> include/drm/drm_edid.h | 12 +++---------
> 2 files changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f95152fac427..340da8257b51 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3457,6 +3457,10 @@ 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) {
> @@ -3503,27 +3507,10 @@ 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 {
> - switch (pt->misc & DRM_EDID_PT_SYNC_MASK) {
> - 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;
> - 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:
> - 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;
> - }
> + 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;
> }
>
> set_size:
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 169755d3de19..48e93f909ef6 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -61,15 +61,9 @@ struct std_timing {
> u8 vfreq_aspect;
> } __attribute__((packed));
>
> -#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_HSYNC_POSITIVE (1 << 1)
> +#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2)
> +#define DRM_EDID_PT_SEPARATE_SYNC (3 << 3)
> #define DRM_EDID_PT_STEREO (1 << 5)
> #define DRM_EDID_PT_INTERLACED (1 << 7)
>
> --
> 2.39.2
--
Ville Syrjälä
Intel
