RandR bugs

[Thomas Winischhofer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Alan Hourihane wrote:
> On Thu, 2005-09-29 at 11:56 +0200, Thomas Winischhofer wrote:
>>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.


I created bug #4635 and attached a patch. Verfied working.


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


I somewhat like the idea of the DriverFunc (as it is called in HEAD).
And it's already used for Egbert's method to eventually run the server
without being root.

And using the driver's switchmode function does not work around the
issue neither. Basically, I don't see what stuff like screen size or
rotation or reflection have to do with the display mode. It is not
RandR's business to mess with the display mode, IMHO.

I will come up with a patch the issue 3) soon.

Thomas

- --
Thomas Winischhofer
Vienna/Austria
thomas AT winischhofer DOT net	       *** http://www.winischhofer.net
twini AT xfree86 DOT org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)

iD8DBQFDO8SdzydIRAktyUcRApjsAJ92iNEVqFBrEvsgCT+fUHJVCKs2mwCgoKhF
bSL5IvY0tt1hMh4AwaWQ8qQ=
=QfTG
-----END PGP SIGNATURE-----