diff mbox series

drm/edid: Fixup h/vsync_end instead of h/vtotal

Message ID 20230914112258.27156-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/edid: Fixup h/vsync_end instead of h/vtotal | expand

Commit Message

Ville Syrjala Sept. 14, 2023, 11:22 a.m. UTC
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(-)

Comments

Jani Nikula Sept. 20, 2023, 5:40 p.m. UTC | #1
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?

Anyway,

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


>  
>  	drm_mode_do_interlace_quirk(mode, pt);
Ville Syrjala Sept. 20, 2023, 7:26 p.m. UTC | #2
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
diff mbox series

Patch

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;
 
 	drm_mode_do_interlace_quirk(mode, pt);