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

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

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