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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 20, 2023 at 08:40:00PM +0300, Jani Nikula wrote:
> On Thu, 14 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.
> >
> > 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 | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 39dd3f694544..0c5563b4d21e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3497,11 +3497,11 @@ 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 */
> > +	/* Some EDIDs have bogus h/vsync_end values */
> >  	if (mode->hsync_end > mode->htotal)
> > -		mode->htotal = mode->hsync_end + 1;
> > +		mode->hsync_end = mode->htotal;
> >  	if (mode->vsync_end > mode->vtotal)
> > -		mode->vtotal = mode->vsync_end + 1;
> > +		mode->vsync_end = mode->vtotal;
> 
> I wonder if we shouldn't debug log these to help our future selves?

Yeah, might be a good idea. I can respin with that. I noticed
that our VBT parser has the exact same code in it as well so
I'll be wanting to cook up a patch that as well.

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

Ta.

> 
> 
> >  
> >  	drm_mode_do_interlace_quirk(mode, pt);
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel