[PATCH] Revert "drm/edid: Fix csync detailed mode parsing"

[PATCH] Revert "drm/edid: Fix csync detailed mode parsing"

[Jani Nikula]

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.

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@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.4+
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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

Re: [PATCH] Revert "drm/edid: Fix csync detailed mode parsing"

[Ville Syrjälä]

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@linux.intel.com>

> 
> 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@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.4+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  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

Re: [PATCH] Revert "drm/edid: Fix csync detailed mode parsing"

[Jani Nikula]

On Thu, 17 Aug 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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@linux.intel.com>

Thanks, pushed to drm-misc-fixes.

BR,
Jani.


>
>> 
>> 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@linux.intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v6.4+
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  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

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 2/2] drm/edid: Fix "Analog composite sync!" for current eDP display panels

[Jani Nikula]

On Thu, 17 Aug 2023, Dirk Lehmann <develop@dj-l.de> wrote:
> 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

Please don't reply with patches in-reply-to other people's patches.

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789

This is now fixed by the revert that I just pushed.

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8146

I don't think this patch fixes that one; see below.

> Signed-off-by: Dirk Lehmann <develop@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  */

fallthrough; will do the trick.

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

The failing mode in bug [1] has 0x0a for flags, so it ends up here, and
for that display this patch is a no-op.

As I explained in [2], all the problematic cases have invalid data, but
the ones fixed by the revert need to ignore the analog sync *flags* and
accept the mode, and the display in [1] needs to have the whole *mode*
rejected.

[1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146
[2] https://gitlab.freedesktop.org/drm/intel/-/issues/8789#note_2047902

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

Another angle is that for analog displays, EDID offset 0x14 has info
about the supported sync modes. If we're separating digital/analog sync
handling, we should probably filter the analog sync with the info from
0x14.

BR,
Jani.


>  	}
>  
>  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)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 2/2] drm/edid: Fix "Analog composite sync!" for current eDP display panels

[Dirk Lehmann]

Hey Jani,

On 8/17/23 15:05, Jani Nikula wrote:
> On Thu, 17 Aug 2023, Dirk Lehmann <develop@dj-l.de> wrote:
>> 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
> 
> Please don't reply with patches in-reply-to other people's patches.

Sorry, I am new with such patch lists :/ ...

> 
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789
> 
> This is now fixed by the revert that I just pushed.
> 
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8146
> 
> I don't think this patch fixes that one; see below.
> 
>> Signed-off-by: Dirk Lehmann <develop@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  */
> 
> fallthrough; will do the trick.

Cool, that's new for me.  FALLTHROUGH really works, great :)

> 
>> +		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;
> 
> The failing mode in bug [1] has 0x0a for flags, so it ends up here, and
> for that display this patch is a no-op.

Aaaeem yes, something is wrong here.  If the EDID data are correct, then 
since 0001-*_5.15.patch in [1] should broke the implementation.

   (0x1a & (3 << 3)) == (3 << 3) == DRM_EDID_PT_DIGITAL_SEPARATE_SYNC

But this implementation here I made and we are discussing should be have 
the same behavior as before in that case of [1].  The no-op should also 
be before.  But then the fix ca62297b2085b before is useless.  Yes, you 
are right, then reverting is the better idea xD

> 
> As I explained in [2], all the problematic cases have invalid data, but
> the ones fixed by the revert need to ignore the analog sync *flags* and
> accept the mode, and the display in [1] needs to have the whole *mode*
> rejected.
> 

I don't know about the other problematic cases.  I don't have the data 
of the use cases for that.

> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146
> [2] https://gitlab.freedesktop.org/drm/intel/-/issues/8789#note_2047902
> 
>> +			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;
>> +		}
> 
> Another angle is that for analog displays, EDID offset 0x14 has info
> about the supported sync modes. If we're separating digital/analog sync
> handling, we should probably filter the analog sync with the info from
> 0x14.

Yeah I also saw that in the VESA Standard, but I reflected that the 
analog display implementation should be more stable, as they are longer 
exist.  For current digital eDP panels which have 0x00 at 47h are these 
composite bits at 14h not reachable --- because bit 7 of 14h is 1.  It 
may just have effects on older analog displays.

> 
> BR,
> Jani.
> 

Okay, summary: You may be right, as the fix commit ca62297b2085b makes 
less sense.

Yeah, greets and thanks for the fast fix.  Really great =D
Dirk

> 
>>   	}
>>   
>>   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)
> 

Re: [PATCH v2 2/2] drm/edid: Fix "Analog composite sync!" for current eDP display panels

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
Subject: [PATCH v2 2/2] drm/edid: Fix "Analog composite sync!" for current eDP display panels
Link: https://lore.kernel.org/stable/E1qWbpR-0007ey-0B%40djlnb.local

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki