RandR bugs

[Alan Hourihane]

On Thu, 2005-09-29 at 12:40 +0200, Thomas Winischhofer wrote:
> -----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.

Mmm, I've not heard about that.

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

RandR doesn't mess with the display mode at all. It just validates the
calling data against the current modepool.

We already use xf86SwitchMode(), as RandR allows us to switch to
different resolutions. Semantically, it's no different to handle
rotation in that call as well. Given that the driver doesn't have to
implement DriverFunc at all, and it could conceivably just deal with
things in it's own SwitchMode, I've got no problems with DriverFunc
staying around.

I'll take a look at the patch, and comment on the bug report itself.

Alan.