Bug 6966

Created attachment 5677 [details]
Xorg.log

Created attachment 5678 [details]
xorg.conf

I found the same problem when using GNOME/JDS, where, if MergedFB is used, you
will find negative refresh rates in desktop resolution selection screen,
resulting in not being able to select resolutions.

The cause of the problem seems to be inappropriate setting of clock for the
merged mode, as seen in this portion of code

mode->Clock = (((i->Clock >> 3) + i->HTotal) << 16) | ((j->Clock >> 2) + 
              j->HTotal);
mode->Clock ^= ((i->VTotal << 19) | (j->VTotal << 3));

in RADEONCopyModeNLink() of radeon_mergedfb.c, where i and j represent two
modes to be merged, and the clock generated is a bogus negative number.
I don't know what this calculation means, it starts showing up in version 1.5. 
The clock is set to 0 prior to that.

For a testing (in clone mode), using

mode->Clock = i->Clock

seems to fix the problem.

Alex, please remind us of the reasons behind this. IIRC it's related to
preventing metamodes from getting discarded under some circumstances.

(In reply to comment #4)
> Alex, please remind us of the reasons behind this. IIRC it's related to
> preventing metamodes from getting discarded under some circumstances.

Henry and I were actually just discussing this.  Unfortunately, I don't recall
the exact circumstance where it was needed, however, IIRC, xrandr or xvidmode
would discard modes that looked the same but were not.  so for example, if you
had the following metamodes:

crtc1: 1024x768@60
crtc2: 1024x768@60

crtc1: 1024x768@75
crtc2: 1024x768@60

crtc1: 1024x768@60
crtc2: 1024x768@75

crtc1: 1024x768@60
crtc2: 1024x768@85

potentially they could all end up looking like the same mode (1024x768@60 or
2048x768@60, etc.) so whatever app you are using to switch modes may discard one
or more of them thinking they are the same.  there's probably a nicer way to do
it, but I had never run into any problems so I never did anything about it.

So what is the recommended way out? Is the 

  mode->Clock = i->Clock

solution going to trigger weird behaviour in other areas or is this the best
short-term patch? Or is a better solution in the works? 

mode->Clock = i->Clock is not a solution, but a workaround to not disply 
negative refresh rates so that resolution selection will function in GNOME.

In terms of unique representation of metamodes, we are more concerned about
situation where merged modes sharing same mode name, but different contents.
An example of this is 

CRT1          CRT2      merged mode name
1280x1024  1280x10241    1280x1024
1280x1024  1024x768      1280x1024

However, the curent "Clock setting" mechanism does not solve this problem, 
at least on GNOME resolution screen, only one "1280x1024" is listed.

Created attachment 5972 [details] [review]
Fix of negative refresh rates from merged modes

In randr code of xf86RandR.c, refresh rate is calculated from clock only when
mode's refresh rate is not provided in mode info, as seen in code below:

static int
xf86RandRModeRefresh (DisplayModePtr mode)
{
    if (mode->VRefresh)
        return (int) (mode->VRefresh + 0.5);
    else
        return (int) (mode->Clock * 1000.0 / mode->HTotal / mode->VTotal + 0.5);
}

That means, in radeon_mergedfb.c if mode's refresh is provided before the
"sophisticated fake DotClock" is set we can get correct refresh rate
(corresponding to the CRT1).

This fix keeps the current clock setting for merged modes, although it is not
yet proved this clock setting can ensure unique merged mode representation, at
least required by xrandr.

A patch is attached.

Created attachment 6296 [details] [review]
Hash fake dotclocks to <2^15 and avoid clipping

The patch in attachment 5972 [details] [review] means metamodes with the same size and CRTC1
refresh rate cannot be differentiated -- a major drawback.

This attachment provides an alternative solution: hash the fake dotclock to
make it less than 2^16, so it will fit into xrandr's positive shorts. Two
random metamodes will collide with probability ~1/2^16, which is the best we
can expect via this approach.

The attachment also solves another issue with the original code: 
when xf86RandR.c/xf86RandRModeRefresh() converts dotclock to refresh rate, the
division loses the least significant bits (e.g., CRTC1's dotlock). I've seen
several reasonable metamodes that collide for that reason. The formula used
here avoids that problem.

Tested with git head, works perfectly.

This is a major issue for many users, since it breaks RandR.

Isn't the above two-line patch a satisfactory workaround, until the new XrandR
matures?

(In reply to comment #11)
> This is a major issue for many users, since it breaks RandR.
> 
> Isn't the above two-line patch a satisfactory workaround, until the new XrandR
> matures?

The patch looks fine.  Henry had mentioned committing the fix (along with
serveral other of his patches) so I was going to leave it to him, but I can
commit it if he's not going to get to it soon.

Didn't make it to 6.6.2...
Can the patch in attachment 6296 [details] [review] be comitted?

I see you've applied the first patch (git commit
e742aeb28c7d9d6e75932c408bcc7c44af52e303).

In comment #10 I've explained why that patch is insufficient and proposed an
alternative one. Please reconsider.

I hate to nag, but is anyone looking a this? It seems to me like the wrong patch
has been applied and shipped (comment #10, comment #14).

(In reply to comment #14)
> I see you've applied the first patch (git commit
> e742aeb28c7d9d6e75932c408bcc7c44af52e303).
> 
> In comment #10 I've explained why that patch is insufficient and proposed an
> alternative one. Please reconsider.

I tried the patch in attachment 6296 [details] [review], but got large positive refresh rates
(in the range of above thousands), is this expected ? Can someone else try
it ?

Yes, patch 6296 is intended to create random-looking refresh rates between 0 and
32748. These are hashes of the metamode parameters. 

True, these fake refresh rates aren't very beautiful; but this is the best we
can do to minimizes the chances of metamode collisions (same size and refresh
rate) which render RANDR useless.

To stress, this isn't academic: with patch 5972 I've seen perfectly sensible
metamodes colliding on my laptop's X setup.

(In reply to comment #14)
> I see you've applied the first patch (git commit
> e742aeb28c7d9d6e75932c408bcc7c44af52e303).
> 
> In comment #10 I've explained why that patch is insufficient and proposed an
> alternative one. Please reconsider.

I tried the patch in attachment 6296 [details] [review], but got large positive refresh rates
(in the range of above thousands), is this expected ? Can someone else try
it ?(In reply to comment #17)
> Yes, patch 6296 is intended to create random-looking refresh rates between 0 and
> 32748. These are hashes of the metamode parameters. 
> 
> True, these fake refresh rates aren't very beautiful; but this is the best we
> can do to minimizes the chances of metamode collisions (same size and refresh
> rate) which render RANDR useless.
> 
> To stress, this isn't academic: with patch 5972 I've seen perfectly sensible
> metamodes colliding on my laptop's X setup.

I am concerned that large positive refresh rates up to 32748 may cause another 
bug to be generated that people don't know how to select a refresh rate because
the refresh numbers don't make sense. There could be another way to increase the 
chance of differentiating meta modes having same size and CRTC1 refresh, yet to 
generate reasonable number of refresh rates, is to use the average refresh rate 
of the two modes:

if (!(mode->VRefresh))
        mode->VRefresh = (i->Clock * 1000.0 / i->HTotal / i->VTotal / 2) +
        (j->Clock * 1000.0 / j->HTotal / j->VTotal / 2);

Plus, people are working on RandR version 1.2, I know they will address
metamode issues like this, but don't know the details.

(In reply to comment #18)

Since we're deep into kludge-land, how about a fake refresh rate of X*100+Y,
where X is the refresh rate of CRTC1 and is the refresh rate of CRTC2? On a
dual-head laptop, a typical value will be 6085 for 60Hz on LCD and 85 on the VGA
port. And even if someone has a refresh rate a bit above 100Hz, the result will
be human-decipherable (but take it modulo 2^15-1, just in case).

This seems both more informative and less likely to collide than taking the
average. It still ignores HTotal and VTotal (unlike patch 6296), but maybe
that's a reasonable price to pay for human-readable values.

Yes, keithp's RANDR 1.2 should clear away this mess, but it's some way off and
we'll still have legacy applications to deal with.

Created attachment 7345 [details] [review]
Patch based on comment  #18 and comment  #19

Per discussion with multinymous@gmail.com, and based on comment #19, this patch

will generate a merged refresh rate representing a combination of refresh rates

of CRTC1 and CRTC2, to reduce chance of collision.

This should be resolved in ati git now.

We should probably apply Henry or Shem's latest patch before we close this.

(In reply to comment #22)
> We should probably apply Henry or Shem's latest patch before we close this.

I will commit patch 7345 if no one opposes.

Patch 7345 works perfectly here, and (as expected) reveals the metamodes that
were hidden by the first patch.

Henry go ahead and commit it.

I went ahead and committed this.  Let me know if there are any issues.

Created attachment 7454 [details] [review]
revision to previous patch 7435

This patch generates better results than previous patch 7345.

(In reply to comment #27)
> Created an attachment (id=7454) [edit]
> revision to previous patch 7435
> 
> This patch generates better results than previous patch 7345.

committed.

The problem with all of this is we seem to fall over with the DIX randr code in the new server as it seems to calculate another refresh rate... which doesn't equal the refresh rate we give it ..

mergedfb has been replace by randr.