Bug 35399 - RandR rotation broken with proprietary nvidia driver
Summary: RandR rotation broken with proprietary nvidia driver
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: 7.6 (2010.12)
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: Aaron Plattner
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks: xserver-1.10
  Show dependency treegraph
 
Reported: 2011-03-17 18:08 UTC by grai
Modified: 2011-05-30 11:23 UTC (History)
8 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description grai 2011-03-17 18:08:58 UTC
A few months ago, I could rotate the screen.  Now I can't:

    $ xrandr -o left
    X Error of failed request:  BadValue (integer parameter out of range for operation)
      Major opcode of failed request:  154 (RANDR)
      Minor opcode of failed request:  2 (RRSetScreenConfig)
      Value in failed request:  0x780
      Serial number of failed request:  14
      Current serial number in output stream:  14

nvidia-settings crashes with the same error.  Inverting works as normal (xrandr 
-o inverted).

xserver 1.9.4.901
nvidia 260.19.44
Archlinux

The xserver commit[1] "randr: check for virtual size limits before set crtc" 
was reverted[2] on the 1.7-branch because it broke rotation with nvidia's 
proprietary driver.  I assume that's what I'm seeing here.

I thought the driver must be at fault, since the change was only reverted on 
that one branch, and I haven't seen reports of other drivers being affected.

However, someone who appears to represent NVIDIA said[3] this:

    > That change doesn't take rotation into account correctly. The 
    > width/height check needs to be moved to before the rotation check, or 
    > modified to use the original mode->mode.width/height instead of the 
    > rotated width/height.

Is this an xorg bug, or an nvidia bug?

[1]: http://cgit.freedesktop.org/xorg/xserver/commit/?id=d1107918
[2]: http://cgit.freedesktop.org/xorg/xserver/commit/?h=server-1.7-branch&id=d77ffa9
[3]: http://www.nvnews.net/vbulletin/showthread.php?p=2406364
Comment 1 grai 2011-03-23 20:17:30 UTC
Of course I should have tested this before reporting (sorry), but I can now confirm that rotation works again after I delete the checks introduced by commit d1107918d4626268803b54033a07405122278e7f.
Comment 2 Sindre Fjogstad 2011-05-11 02:14:45 UTC
Also seen in Ubuntu 11.04 with the following software version:

nvidia-driver version: 270.41.04

xorg information:
 Server version number: 11.0
 sever version: 1.10.0.902 (11000902)
 nv-controll version 1.26
Comment 3 David Thomspon 2011-05-11 14:47:32 UTC
If the maintainers don't want to roll the change out, then at least add a fix for the rotation.


*** rrscreen.c	2011-02-24 20:27:32.000000000 -0700
--- rrscreen2.c	2011-05-11 15:45:27.000000000 -0600
***************
*** 914,930 ****
      {
  	width = mode->mode.height;
  	height = mode->mode.width;
!     }
! 
!     if (width < pScrPriv->minWidth || pScrPriv->maxWidth < width) {
! 	client->errorValue = width;
! 	free(pData);
! 	return BadValue;
!     }
!     if (height < pScrPriv->minHeight || pScrPriv->maxHeight < height) {
! 	client->errorValue = height;
! 	free(pData);
! 	return BadValue;
      }
  
      if (width != pScreen->width || height != pScreen->height)
--- 914,940 ----
      {
  	width = mode->mode.height;
  	height = mode->mode.width;
!         if (width < pScrPriv->minHeight || pScrPriv->maxHeight < width) {
!             client->errorValue = width;
!             free(pData);
!             return BadValue;
!         }
!         if (height < pScrPriv->minWidth || pScrPriv->maxWidth < height) {
!             client->errorValue = height;
!             free(pData);
!             return BadValue;
!         }
!     } else {
!         if (width < pScrPriv->minWidth || pScrPriv->maxWidth < width) {
! 	    client->errorValue = width;
!             free(pData);
! 	    return BadValue;
!         }
!         if (height < pScrPriv->minHeight || pScrPriv->maxHeight < height) {
! 	    client->errorValue = height;
! 	    free(pData);
! 	    return BadValue;
!         }
      }
  
      if (width != pScreen->width || height != pScreen->height)
Comment 4 Alan Coopersmith 2011-05-11 14:55:19 UTC
A proposed fix was posted to xorg-devel earlier today for review:

http://lists.x.org/archives/xorg-devel/2011-May/022104.html
http://patchwork.freedesktop.org/patch/5427/
Comment 5 Aaron Plattner 2011-05-25 10:49:39 UTC
Fix merged to master.  I'm leaving this open and marking it as a blocker for bug 31018 to nominate it for server-1.10-branch.

commit b6c7b9b2f39e970cedb6bc1e073f901e28cb0fa3
Author: Aaron Plattner <aplattner@nvidia.com>
Date:   Tue May 24 16:02:42 2011 -0700

    randr: check rotated virtual size limits correctly
    
    Commit d1107918d4626268803b54033a07405122278e7f introduced checks to
    the RandR path that cause RRSetScreenConfig requests to fail if the
    size is too large.  Unfortunately, when RandR 1.1 rotation is enabled
    it compares the rotated screen dimensions to the unrotated limits,
    which causes 90- and 270-degree rotation to fail unless your screen
    happens to be square:
    
      X Error of failed request:  BadValue (integer parameter out of range for operation)
        Major opcode of failed request:  153 (RANDR)
        Minor opcode of failed request:  2 (RRSetScreenConfig)
        Value in failed request:  0x780
        Serial number of failed request:  14
        Current serial number in output stream:  14
    
    Fix this by moving the check above the code that swaps the dimensions
    based on the rotation.
    
    Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
    Reviewed-by: Jeremy Huddleston <jeremyhu@apple.com>
    Tested-by: Robert Hooker <robert.hooker@canonical.com>
    Tested-by: Kent Baxley <kent.baxley@canonical.com>
    Signed-off-by: Keith Packard <keithp@keithp.com>

 randr/rrscreen.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Comment 6 Jeremy Huddleston Sequoia 2011-05-25 12:28:28 UTC
This will be merged in to 1.10 after the release of 1.10.2 on Saturday.  If you feel this is more urgent than I am giving it credit for, please let me know, and I will re-evaluate this decision.
Comment 7 Sindre Fjogstad 2011-05-27 13:33:56 UTC
I would really appreciate it if you could include this fix in the 1.10.2 release. 

Could you please tell me the reasons for not including it in the release?

I do not understand why you do not want to include it. As far as I can see, this fix will only fix broken behavior and not introduce any regressions
Comment 8 Jeremy Huddleston Sequoia 2011-05-28 13:29:36 UTC
(In reply to comment #7)
> I would really appreciate it if you could include this fix in the 1.10.2
> release. 

Yes. I understand.

> Could you please tell me the reasons for not including it in the release?

The only changes accepted after RC2 before the final release are:
  1) Fixes for regressions introduced from the previous stable
  2) Low-risk changes which fix crashes

This does not fall into either category, and distributions supporting the nvidia proprietary drivers are free to take the risk upon themselves by including this patch in their release.  I intend to merge this immediately after the 1.10.2 tag, so it will have visibility with distros.

> I do not understand why you do not want to include it. As far as I can see,
> this fix will only fix broken behavior and not introduce any regressions

I don't think anyone ever sees regressions before they happen.  If they did, they'd fix them in the initial patch ;)
Comment 9 Jeremy Huddleston Sequoia 2011-05-28 17:30:10 UTC
   613e0e9..50b9d31  server-1.10-branch -> server-1.10-branch

This will be in 1.10.3 RC1
Comment 10 Sindre Fjogstad 2011-05-30 11:23:41 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > I would really appreciate it if you could include this fix in the 1.10.2
> > release. 
> 
> Yes. I understand.
> 
> > Could you please tell me the reasons for not including it in the release?
> 
> The only changes accepted after RC2 before the final release are:
>   1) Fixes for regressions introduced from the previous stable
>   2) Low-risk changes which fix crashes
> 
> This does not fall into either category, and distributions supporting the
> nvidia proprietary drivers are free to take the risk upon themselves by
> including this patch in their release.  I intend to merge this immediately
> after the 1.10.2 tag, so it will have visibility with distros.
>

Can't argue against those arguments. I appreciate that you took the time to explain how it works. Thank you.
 
> > I do not understand why you do not want to include it. As far as I can see,
> > this fix will only fix broken behavior and not introduce any regressions
> 
> I don't think anyone ever sees regressions before they happen.  If they did,
> they'd fix them in the initial patch ;)

Good point...


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.