Re: [Intel-gfx] [PATCH] drm/edid: Fix csync detailed mode parsing

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 27 Feb 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Remove the bogus csync check and replace it with something that:
> - triggers for all forms of csync, not just the basic analog variant
> - actually populates the mode csync flags so that drivers can
>   decide what to do with the mode
>
> Originally the code tried to outright reject csync, but that
> apparently broke some bogus LCD monitor that claimed to have
> a detailed mode that uses analog csync, despite also claiming
> the monitor only support separate sync:
> https://bugzilla.redhat.com/show_bug.cgi?id=540024
> Potentially that monitor should just be quirked or something.
>
> Anyways, what we are dealing with now is some kind of funny i915
> JSL machine with eDP where the panel claims to support a sensible
> 60Hz separate sync mode, and a 50Hz mode with bipolar analog
> csync. The 50Hz mode does not work so we want to not use it.
> Easiest way is to just correctly flag it as csync and the driver
> will reject it.
>
> TODO: or should we just reject any form of csync (or at least
> the analog variants) for digital display interfaces?
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8146
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 23 +++++++++++++++--------
>  include/drm/drm_edid.h     | 12 +++++++++---
>  2 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ebab862b8b1a..fa20b1107ce3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3424,10 +3424,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) {
> @@ -3474,10 +3470,21 @@ 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 {
> -		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;
> +		switch (pt->misc & DRM_EDID_PT_SYNC_MASK) {
> +		case DRM_EDID_PT_ANALOG_CSYNC:
> +		case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC:
> +		case DRM_EDID_PT_DIGITAL_CSYNC:
> +			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync!\n",
> +				    connector->base.id, connector->name);
> +			mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_NCSYNC;

Not sure it makes a huge difference, and I expect this case to be rare,
but what's the _N_ CSYNC based on here?

I also observe the spec appears to indicate "Horizontal Sync is Negative
(outside of V-sync)" and "Horizontal Sync is Positive (outside of
V-sync)" bit is valid also for Digital Composite Sync.

See how the bits for vertical sync have "1 1 0" or "1 1 1" but for
horizontal sync it's "1 _ _ 0" and "1 _ _ 1". Does that indicate the
polarity for digital composite sync?! The spec is not super clear.

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

I think this is the good stuff, we shouldn't be looking at these flags
and setting PHSYNC/NHSYNC/PVSYNC/NVSYNC unless we have digital separate
sync.

Overall I think

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Although it's not all completely clear. But I think being explicit is
better than assuming something and simply debug logging about csync and
not really doing anything about it.

> +			break;
> +		}
>  	}
>  
>  set_size:
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 70ae6c290bdc..49ee10272603 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)
> +#  define DRM_EDID_PT_VSYNC_POSITIVE       (1 << 2)
>  #define DRM_EDID_PT_STEREO         (1 << 5)
>  #define DRM_EDID_PT_INTERLACED     (1 << 7)

-- 
Jani Nikula, Intel Open Source Graphics Center