Bug 6966

Summary: large negative refresh rates reported
Product: xorg Reporter: Kasper Peeters <kasper.peeters>
Component: Driver/rage128Assignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: high CC: alexdeucher, henry.zhao, multinymous
Version: 7.0.0   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Xorg.log
none
xorg.conf
none
Fix of negative refresh rates from merged modes
none
Hash fake dotclocks to <2^15 and avoid clipping
none
Patch based on comment #18 and comment #19
none
revision to previous patch 7435 none

Description Kasper Peeters 2006-05-19 05:46:18 UTC
I have an xorg.conf setup on a laptop with settings such that the CRT
output is always on, using default modes: the "Device" section contains 

   Option      "MonitorLayout"   "LVDS, CRT"
   Option      "MergedFB"        "on"
   Option      "CRT2Hsync"       "50-92"
   Option      "CRT2VRefresh"    "50-85"
   Option      "MetaModes"       "1400x1050 1152x864 1024x768 800x600"

Now Xrandr reports the resolutions in MetaModes, but gives bizarre negative
refresh rates:

 SZ:    Pixels          Physical       Refresh
*0   1400 x 1050   ( 474mm x 356mm )  *-8255
 1   1152 x 864    ( 474mm x 356mm )   -8269
 2   1024 x 768    ( 474mm x 356mm )   -8265
 3    800 x 600    ( 474mm x 356mm )   -8277

This is on a

  ATI Technologies, Inc. Radeon R250 Lf [Radeon Mobility 9000 M9]
Comment 1 Kasper Peeters 2006-05-19 05:47:59 UTC
Created attachment 5677 [details]
Xorg.log
Comment 2 Kasper Peeters 2006-05-19 05:48:48 UTC
Created attachment 5678 [details]
xorg.conf
Comment 3 henry.zhao@oracle.com 2006-05-26 07:50:55 UTC
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.
Comment 4 Michel Dänzer 2006-06-12 07:31:10 UTC
Alex, please remind us of the reasons behind this. IIRC it's related to
preventing metamodes from getting discarded under some circumstances.
Comment 5 Alex Deucher 2006-06-12 07:52:56 UTC
(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.
Comment 6 Kasper Peeters 2006-06-12 08:43:27 UTC
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? 
Comment 7 henry.zhao@oracle.com 2006-06-13 16:47:31 UTC
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.
Comment 8 henry.zhao@oracle.com 2006-06-19 13:39:03 UTC
Created attachment 5972 [details] [review]
Fix of negative refresh rates from merged modes
Comment 9 henry.zhao@oracle.com 2006-06-19 13:39:59 UTC
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.
Comment 10 multinymous 2006-07-21 14:15:19 UTC
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.
Comment 11 multinymous 2006-07-30 09:55:43 UTC
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?
Comment 12 Alex Deucher 2006-07-30 10:04:45 UTC
(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.
Comment 13 multinymous 2006-08-24 18:48:00 UTC
Didn't make it to 6.6.2...
Can the patch in attachment 6296 [details] [review] be comitted?
Comment 14 multinymous 2006-10-03 14:54:39 UTC
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.
Comment 15 multinymous 2006-10-05 14:08:33 UTC
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).
Comment 16 henry.zhao@oracle.com 2006-10-05 14:37:52 UTC
(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 ?
Comment 17 multinymous 2006-10-05 16:06:40 UTC
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.
Comment 18 henry.zhao@oracle.com 2006-10-05 19:07:38 UTC
(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.
Comment 19 multinymous 2006-10-06 04:33:16 UTC
(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.
Comment 20 henry.zhao@oracle.com 2006-10-10 17:15:32 UTC
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.
Comment 21 Adam Jackson 2006-10-12 18:42:11 UTC
This should be resolved in ati git now.
Comment 22 Alex Deucher 2006-10-12 18:51:37 UTC
We should probably apply Henry or Shem's latest patch before we close this.
Comment 23 henry.zhao@oracle.com 2006-10-13 15:01:20 UTC
(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.
Comment 24 multinymous 2006-10-15 07:51:31 UTC
Patch 7345 works perfectly here, and (as expected) reveals the metamodes that
were hidden by the first patch.
Comment 25 Alex Deucher 2006-10-15 09:49:42 UTC
Henry go ahead and commit it.
Comment 26 Alex Deucher 2006-10-15 14:15:08 UTC
I went ahead and committed this.  Let me know if there are any issues.
Comment 27 henry.zhao@oracle.com 2006-10-17 16:52:50 UTC
Created attachment 7454 [details] [review]
revision to previous patch 7435

This patch generates better results than previous patch 7345.
Comment 28 Alex Deucher 2006-10-17 18:18:20 UTC
(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.
Comment 29 Dave Airlie 2007-04-23 13:42:55 UTC
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 ..
Comment 30 Alex Deucher 2008-01-08 16:09:30 UTC
mergedfb has been replace by randr.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.