If we're going to do that, we need to be careful and make sure we detect things like Apple flat panels who have no scaler and only support a fixed mode. In those cases, we must enable use of RMX.

Created attachment 9571 [details] [review]
disable on-chip RMX for DFP monitor

There are several issues with the patch:

 - You completely comment yout the ValidateFPMode call, instead of just calling it when more than one mode was found... that will make RMX not work on panels that don't support scaling like most Apple panels.

 - Your patch seems to change all sorts of unrelated bits, some of them possibly reverting fixes, or at least without any explanation of why you did those changes.  If some of these are fixes on their own, please post them as separate patches.

Created attachment 9572 [details] [review]
validate ddcmodes for DFP monitor by default

do not use on-chip RMX modes for digital monitors by default.
only if one or no modes found by ddc modes, then on-chip RMX modes will be used instead.

Created attachment 9617 [details] [review]
Updated patch from LisaWu

This is an updated patch. It influences this particular IBM server and doesn't change behavior otherwise.

I would prefer to the variable ignoreRMX instead of IsIBMServer, and to make it (additionally) settable by and option. It's always better to track feature sets than to track specific hardware (IsDellServer is actually a very bad example). Also the patch has some whitespace problems.

Would a fixed version WRT the stated issues be ok for upstream?

The other two changes in the first patch are an obvious bug fix (linked list termination), and a signal polarity semantic change of which I'm a bit unsure whether it might have some regressions with monitors with broken DDC:

LisaWu:
on IBM Lewis server, if (d_timings->sync==3)is set,the signal polarity
restrictions are not considered by radeon driver,which cause video corruption
on remote video controller. when if(d_timing->sync == 3)is removed, the
required signal polarity is passed to the output video mode, thus the remote
video show correctly.

Benjamin, do you know whether this if-clause was introduced later, or whether it came with the original implementation? I've tracked it down until the initial git version from 2003-11-14, and even back then the whole clause was present as it is today.

I believe bugs #8541 is related to RMX, thus it exist monitor
out there that doesn't like the card RMX scaler, would be nice
if we can have some kind of policy to choose btw enabling or
disabling rmx. Is it possible to detect monitor without RMX
(like apple one) and enable RMX only for them ?

can you post an Xorg.0.log from one of these machines?

Created attachment 9620 [details]
xorg.conf file which will cause the video corruption on IBM server

Created attachment 9621 [details]
log file with corrupted video output

refer to the log file, please notice that the on-chip RMX is enabled and the video output mode is 
(**) RADEON(0): *Mode "1024x768": 65.0 MHz (scaled from 0.0 MHz), 48.4 kHz, 60.0 Hz
(II) RADEON(0): Modeline "1024x768"   65.00  1024 1048 1184 1344  768 771 777 806
with no signal polarity defined.
this timing is not acceptable to the remote video for the IBM server.

Created attachment 9622 [details]
log file with RMX disabled

when disable on-chip RMX in our driver, and remove "if(d_timings->sync == 3)"(mentioned in comment #6)
both remote video and local monitor show clear video out.
please refer to the log file to have a detailed information of the valid timings for the server. and please pay attention to the signal polarity of every valid mode.

Dave asked about a logfile of an Xserver with the card connected to a monitor w/o RMX scaler like the Apple one. So one of those (apparently rare) cases, where we still have to use the internal RMX of the gfx card.

oops,misunderstood :(

(In reply to comment #6)
> LisaWu:
> on IBM Lewis server, if (d_timings->sync==3)is set,the signal polarity
> restrictions are not considered by radeon driver,which cause video corruption
> 
> Benjamin, do you know whether this if-clause was introduced later, or whether
> it came with the original implementation? I've tracked it down until the

I've further tracked down the if-clause. It has been introduced by Hui Yu, on 2002/07/11:

      - Fix Dell OEM VE card support
      - Add better clone mode support
      - Fix large panel (>= 1600x1200) detection and initialization problems
      - Remove "PanelSize" and "CrtScreen" options since they are no longer
        needed with new CloneMode and improved flat panel support
      - Add "DDCMode" option to detect and use DDC modes
      - Add "PanelOff" option to disable panel on laptops
      - Fix corrupted console problem
      - Other misc fixes
      (#A.1043, Hui Yu@ATI)

which is a huge 2279 lines patch, which basically introduces RMX and turns mode selection upside down. Also, the infamous IsDell logic was introduced.

All in all I think this change is safe.

Fixed in git.

We probably still need a better logic for when RMX should enabled and when not.

(In reply to comment #15)
> Fixed in git.
> 
> We probably still need a better logic for when RMX should enabled and when not.
Isn't it possible to determine this just by the edid data? I think if there's a frequency range field, the monitor has a scaler and it's safe to disable RMX, and if there's no such field it doesn't (unless it's edid 1.0 but I guess only crts used that). Or are we talking about monitors with buggy (or no) edid data?

The mode list handling patches seem to miss the fact that list if circular or not

they definitely regress things for benh...

Created attachment 9690 [details]
log file from benh on irc...

from the log, it seems radeon failed to read out the EDID information from the primary monitor which lead to a mode validation failure.
the final patch enables DDCModes to disable RMX mode.if you want to use on-chip RMX mode for LCD monitors, you may disable the DDCModes option and have another try.

The patches don't activate ddc_mode by default, so this shouldn't be the source for the regression.

Unfortunately benh isn't online on xorg-devel right now.

Commit 0abce69f0d826a7ca1a41d963cd4730b6e01c145 or 16ef77df4ebaf5ea13baa82972aaf98e71ac32ee is probably the culprit so if benh could test these two commits, it help be of great help.

(In reply to comment #17)
> The mode list handling patches seem to miss the fact that list if circular or
> not

I most certainly do not understand what you want to tell me.
Can the modelist be a circular list? In that case, how did it ever work, the mode list scanning end conditions certainly didn't test for that so far. Or only in a way that is so non-obvious that I still can see how.

actually I relooked at the modelist handling and they looked correct to me, mode list handling is very arcane in X and whether something is a circular list or not isn't always obvious ...

(In reply to comment #21)
> actually I relooked at the modelist handling and they looked correct to me,

I assume you mean the old version, pre-commit 0abce69f?

> mode list handling is very arcane in X and whether something is a circular list
> or not isn't always obvious ...

Sure, but how does "p && p->next" check for circular lists? To me this stops on the previous to last entry of a regular linked list.
The other change (not going in steps of 2 over the list) has been proposed by Lisa, and it is changing code originally submitted by ATI.

so the patch that got checked in for this bug seems to be in the tree anymore.. anyone got any way to test master on one of these machines to see if we regressed it?

(In reply to comment #23)
> so the patch that got checked in for this bug seems to be in the tree anymore..
> anyone got any way to test master on one of these machines to see if we
> regressed it?
> 

In ati master we already turn rmx off by default for TMDS. Works fine on my DVI panels with or without RMX.  I suppose 6.6.x may need a fix though.

closing due to lack of feedback.