Bug 35399

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.

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

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)

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/

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(-)

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.

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

(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 ;)

   613e0e9..50b9d31  server-1.10-branch -> server-1.10-branch

This will be in 1.10.3 RC1

(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...