Created attachment 10211 [details] [review]
Patch to fix the problem

Lower hsync[0].lo and vrefresh[0].lo to 31.5 and 50.0 respectively, so that
they are used as default low numbers. These numbers are consistent with
default low numbers used in xf86ValidateMode().

(In reply to comment #0)
> DDCGuessRangesFromModes() guesses monitor's low and high hsync/vsync from
> EDID modes when EDID does not provide these numbers. It sets initial low
> and high numbers to be 1024 and 0, that means basically it uses the lowerst
> number of all EDID modes as the low number, and the highest number of all
> EDID modes as the higher number, for both hsync and vsync, respectively.

This is the correct behaviour.

> For monitors where EDID only provides ONE mode (I saw two instances), then
> the guess result is low number equals high number, for both hsync and vsync,
> respectively. The effect is that during validation all modes, except the
> EDID mode (from which guess numbers are derived), are rejected.

This is, once again, the correct behaviour.

> The worst
> case occurs in vesa mode when VBE modes happen to not include this EDID mode,
> then no no mode can be validated.

This is a problem endemic to VBE, not with EDID parsing.

> The fix is to set fixed lower numbers for both hsync and vsync, respectively.

What if a display doesn't support these timings? How do you know whether the display really supports it? The fact that the timing came from old code doesn't make it right, as maybe it was right last millenium, it is no longer always right today.

If you just look at VRefresh, how certain are you that all panels support 50Hz dead? If a panel supports only a single 60Hz mode, you're going to allow 50Hz modes as well?

VBE is the problem. Any proper driver would've accepted the single mode np.

If VBE can't support the single mode, then the user should use NoDCC on his own risk.

If you chose to use VBE, then you get to sit on the blisters it always gives you.

Fix the real problem, get rid of VBE or work around it in the driver. Don't saddle us all up with this.

And here i was thinking that people were finally properly blaming VBE for the havoc it causes.

(In reply to comment #2)

> > The fix is to set fixed lower numbers for both hsync and vsync, respectively.
> 
> What if a display doesn't support these timings? How do you know whether the
> display really supports it? The fact that the timing came from old code doesn't
> make it right, as maybe it was right last millenium, it is no longer always
> right today.
> 
> If you just look at VRefresh, how certain are you that all panels support 50Hz
> dead? If a panel supports only a single 60Hz mode, you're going to allow 50Hz
> modes as well?

If no DDC, or skip DDCGuessRangesFromModes(), we will get default setting in
xf86ValidateMode() any way: 31.5 and 60.0 for hsync's low and high; 50 and 70
for vrefresh's low and high. Where do these numbers come from ?

> 
> VBE is the problem. Any proper driver would've accepted the single mode np.
> 
> Fix the real problem, get rid of VBE or work around it in the driver. Don't
> saddle us all up with this.
> 
> And here i was thinking that people were finally properly blaming VBE for the
> havoc it causes.

The problem was reported with vesa driver. See this scenario:

On Acer TravelMate3295 with ATI x1600 laptop, since x1600 is not currently
supported in ati/radeon driver, X server automatically picks up vesa driver
as fallback. Monitor EDID reports only one mode 1280x800 which is not included
in VBE modes:

(II) VESA(0): Manufacturer: CMO  Model: 1408  Serial#: 0
(II) VESA(0): Year: 2005  Week: 21
(II) VESA(0): EDID Version: 1.3
(II) VESA(0): Digital Display Input
(II) VESA(0): Max H-Image Size [cm]: horiz.: 30  vert.: 19
(II) VESA(0): Gamma: 2.20
(II) VESA(0): No DPMS capabilities specified; RGB/Color Display
(II) VESA(0): First detailed timing is preferred mode
(II) VESA(0): redX: 0.590 redY: 0.340   greenX: 0.317 greenY: 0.535
(II) VESA(0): blueX: 0.150 blueY: 0.121   whiteX: 0.313 whiteY: 0.329
(II) VESA(0): Manufacturer's mask: 0
(II) VESA(0): Supported additional Video Mode:
(II) VESA(0): clock: 71.0 MHz   Image Size:  303 x 190 mm
(II) VESA(0): h_active: 1280  h_sync: 1328  h_sync_end 1360 h_blank_end 1440
              h_border: 0
(II) VESA(0): v_active: 800  v_sync: 803  v_sync_end 809 v_blanking: 823 
              v_border: 0
(II) VESA(0):  N141I1-L02

(1) With the current 7.2 code, we have blank screen with
    
    (EE) VESA(0): No matching modes

    [Equal low and high monitor rates. All VBE modes are rejected.]

(2) With 6.9 code without new validation code, we have a screen of 680x480

    [Use default rates set in xf86ValidateMode().]

(3) With the patch, we have a screen of 1024x768.

    [Use the same low default rates set in xf86ValidateMode(), use high
     rates derived from EDID mode.]

Which is the preferred behavior ?

(In reply to comment #3)
> 
> If no DDC, or skip DDCGuessRangesFromModes(), we will get default setting in
> xf86ValidateMode() any way: 31.5 and 60.0 for hsync's low and high; 50 and 70
> for vrefresh's low and high. Where do these numbers come from ?

Yes, but we do have DCC and we do have a mode, and this changes the whole game here. We have information, we know what's right and what's not, even if this is too limited for the most clueless driver replacement ever.

Plus, your change affects even the case where there are multiple modes, where VBE should already be slightly happier.

And, it influences _all_ drivers, even all those are able to set modes properly.

> The problem was reported with vesa driver. See this scenario:
> 
> On Acer TravelMate3295 with ATI x1600 laptop, since x1600 is not currently
> supported in ati/radeon driver, X server automatically picks up vesa driver
> as fallback. Monitor EDID reports only one mode 1280x800 which is not included
> in VBE modes:

If the monitor only reports one mode, then only one mode should ever be set on it.

Why is sun wasting so much of its time trying to fix this useless piece of junk?
Why don't you support proper driver development instead?

> (II) VESA(0): clock: 71.0 MHz   Image Size:  303 x 190 mm
> (II) VESA(0): h_active: 1280  h_sync: 1328  h_sync_end 1360 h_blank_end 1440
>               h_border: 0
> (II) VESA(0): v_active: 800  v_sync: 803  v_sync_end 809 v_blanking: 823 
>               v_border: 0

Right, no other mode apparently should be set on this monitor.

Hrm, isn't this the native mode of the directly connected panel?

> (1) With the current 7.2 code, we have blank screen with
> 
>     (EE) VESA(0): No matching modes
> 
>     [Equal low and high monitor rates. All VBE modes are rejected.]

Of course. VIAs windows users are in the same mess. Lack of proper driver, and they're forced to beg $board_maker for a new bios. So you really shouldn't feel as if you're personally responsible for this, it happens for more people on VIA in windows than it happens on ATI on free software.

> (2) With 6.9 code without new validation code, we have a screen of 680x480
> 
>     [Use default rates set in xf86ValidateMode().]

Right, old and incorrect.

> (3) With the patch, we have a screen of 1024x768.
> 
>     [Use the same low default rates set in xf86ValidateMode(), use high
>      rates derived from EDID mode.]

But you mess up so many things. And, you end up with a crappy scaled or centered mode. You don't want that as default behaviour at all.

> Which is the preferred behavior ?

Option "NoDCC" "True" in the short term. Tell acer to provide a replacement BIOS , i'm sure that a company like SUN is able to weigh in, and see this "fixed". Once this is provided, users can be told to upgrade their bioses.

The long term and correct solution is to provide a proper driver.

The absolute last thing you should do is mess things up for the rest of us who are working on proper drivers.

Also, if it only was a slight difference in timing, one should be able to just space the range out in the driver.

But allowing 1024x768 directly on a monitor that only claims to accept 1280x1024 is completely wrong.

Just because it works in this case here doesn't mean that this should be the default behaviour. So i urge you to start fixing things some place else than in the general code.

(In reply to comment #3)
> On Acer TravelMate3295 with ATI x1600 laptop, since x1600 is not currently
> supported in ati/radeon driver, X server automatically picks up vesa driver
> as fallback. Monitor EDID reports only one mode 1280x800 which is not included
> in VBE modes:

That's a bug in the vesa driver.  That's not policy you want to enforce in all drivers.

You want something like the following patches:

http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commitdiff;h=5b41d4e60be35cfb96bedec0931fd5922823b4b9
http://cvs.fedoraproject.org/viewcvs/rpms/xorg-x11-drv-vesa/devel/vesa-1.9-mode-heuristics.patch?rev=1.1&view=auto

The first prevents the VBE module from stripping modes away before giving them to the driver.  The second changes the mode validation heuristics in the vesa driver to try several things in sequence: first strict intersection of VBE and EDID modes, then (if that fails) a range-based validation with a wild guess at the low end of the range.

The second patch contains at least one bug, for the case where the EDID list contains multiple timings for the same resolution and only one of them actually validates (for example, 1600x1200 in both normal and reduced-blanking, even though only the RB variant would actually fit).  I should have a fix for that soon, at which point I'll push it to master.

Hardware is X1600 on a laptop.

Henry: please use the radeonhd driver and live happily ever after, as this will handle the panel correctly.