RandR bugs

[Alan Hourihane]

On Thu, 2005-09-29 at 11:56 +0200, Thomas Winischhofer wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> During my current work to implement RandR rotation support, I discovered
> several bugs in RandR, in xf86RandR.c:
> 
> 1) xf86RandRSetMode: This one calls EnableDisableFBAccess(FALSE) on
> entry. Later it calls xf86SwitchMode(). If xf86SwitchMode() fails, it
> immediately returns, without EnableDisableFBAccess(TRUE). Bad things
> happen afterwards.

Indeed. This should be fixed.

> 2) xf86RandRSetConfig: This one calls the driver function for rotation
> before xf86RandRSetMode(). However, it "forgets" to call it again to
> undo the rotation setting, in case xf86RandRSetMode() fails. So while
> the client is informed about the failure to set the RandR config, the
> driver still thinks it succeeded. Not especially smart either.

Right, we should probably call xf86SwitchMode() to put back the previous
mode and undo the rotation changes and still return FALSE.

> 3) This is a more conceptual issue:
> 
> The sis driver allows switching output devices during server-runtime.
> However, not all modes are supported for all available output devices.
> Under normal circumstances, the driver's mode validation takes care of this.
> 
> But:
> 
> RandR does not allow setting a configuration, be it display size, be it
> rotation/reflection, without (re-)setting the display mode. This is
> suboptimal.
> 
> For example, if I have a 1280x800 virtual screen and a current display
> mode of 1024x768 (eg because I switched from LCD to TV, and TV does not
> support 1280x800), setting the RandR config just in order to change the
> rotation will cause RandR to try to switch the display mode to 1280x800
> (because that is the desired screen size). This will fail (due to the
> driver validating the display mode).
> 
> At this point everything goes havoc due to the bugs mentioned in 1) and
> 2) above.
> 
> However, even if the failures of xf86SwitchMode() are handled properly,
> the whole RandR request will fail.
> 
> So, I propose the following to solve this: RandR should only touch the
> display mode if it is too large for the desired screen size. Otherwise
> the display mode should be left untouched.
> 
> Comments?

Sounds reasonable. Have you got a patch ? That's much easier to comment
on. It might be reasonable to open a bug on bugzilla for this so
reasonable patches can be made and reviewed.

Also, I'm not sure RRFunc is needed at all. It seems to me that
xf86SwitchMode() is good enough to let the driver handle the necessary
switch.

The only thing that is missing is a call that the driver can make to
obtain the current rotation mode (i.e. randrp->rotation). Removing the
need completely for RRFunc.

Alan.