On Wed, Sep 20, 2023 at 08:40:00PM +0300, Jani Nikula wrote:
> On Thu, 14 Sep 2023, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
Ta.
>
>
> >
> > drm_mode_do_interlace_quirk(mode, pt);
>
> --
> Jani Nikula, Intel
--
Ville Syrjälä
Intel
