Re: [Intel-gfx] [PATCH v2 1/2] drm/edid: Fixup h/vsync_end instead of h/vtotal

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

On Thu, 21 Sep 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> There are some weird EDIDs floating around that have the sync
> pulse extending beyond the end of the blanking period.
>
> On the currently problemtic machine (HP Omni 120) EDID reports
> the following mode:
> "1600x900": 60 108000 1600 1780 1860 1800 900 910 913 1000 0x40 0x5
> which is then "corrected" to have htotal=1861 by the current drm_edid.c
> code.
>
> The fixup code was originally added in commit 7064fef56369 ("drm: work
> around EDIDs with bad htotal/vtotal values"). Googling around we end up in
> https://bugs.launchpad.net/ubuntu/hardy/+source/xserver-xorg-video-intel/+bug/297245
> where we find an EDID for a Dell Studio 15, which reports:
> (II) VESA(0): clock: 65.0 MHz   Image Size:  331 x 207 mm
> (II) VESA(0): h_active: 1280  h_sync: 1328  h_sync_end 1360 h_blank_end 1337 h_border: 0
> (II) VESA(0): v_active: 800  v_sync: 803  v_sync_end 809 v_blanking: 810 v_border: 0
>
> Note that if we use the hblank size (as opposed of the hsync_end)
> from the DTD to determine htotal we get exactly 60Hz refresh rate in
> both cases, whereas using hsync_end to determine htotal we get a
> slightly lower refresh rates. This makes me believe the using the
> hblank size is what was intended even in those cases.
>
> Also note that in case of the HP Onmi 120 the VBIOS boots with these:
>   crtc timings: 108000 1600 1780 1860 1800 900 910 913 1000, type: 0x40 flags: 0x5
> ie. it just blindly stuffs the bogus hsync_end and htotal from the DTD
> into the transcoder timing registers, and the display works. I believe
> the (at least more modern) hardware will automagically terminate the hsync
> pulse when the timing generator reaches htotal, which again points that we
> should use the hblank size to determine htotal. Unfortunatley the old bug
> reports for the Dell machines are extremely lacking in useful details so
> we have no idea what kind of timings the VBIOS programmed into the
> hardware :(
>
> Let's just flip this quirk around and reduce the length of the sync
> pulse instead of extending the blanking period. This at least seems
> to be the correct thing to do on more modern hardware. And if any
> issues crop up on older hardware we need to debug them properly.
>
> v2: Add debug message breadcrumbs (Jani)
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com> #v1

And v2 too.

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8895
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 39dd3f694544..ec1cb4890acb 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3497,11 +3497,19 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto
>  	mode->vsync_end = mode->vsync_start + vsync_pulse_width;
>  	mode->vtotal = mode->vdisplay + vblank;
>  
> -	/* Some EDIDs have bogus h/vtotal values */
> -	if (mode->hsync_end > mode->htotal)
> -		mode->htotal = mode->hsync_end + 1;
> -	if (mode->vsync_end > mode->vtotal)
> -		mode->vtotal = mode->vsync_end + 1;
> +	/* Some EDIDs have bogus h/vsync_end values */
> +	if (mode->hsync_end > mode->htotal) {
> +		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] reducing hsync_end %d->%d\n",
> +			    connector->base.id, connector->name,
> +			    mode->hsync_end, mode->htotal);
> +		mode->hsync_end = mode->htotal;
> +	}
> +	if (mode->vsync_end > mode->vtotal) {
> +		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] reducing vsync_end %d->%d\n",
> +			    connector->base.id, connector->name,
> +			    mode->vsync_end, mode->vtotal);
> +		mode->vsync_end = mode->vtotal;
> +	}
>  
>  	drm_mode_do_interlace_quirk(mode, pt);

-- 
Jani Nikula, Intel