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. 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.
Created attachment 3429 [details] [review] Fixes the failure handling
The patch doesn't apply cleanly to HEAD. But looking at it, it only deals with the case of rotation using the DriverFunc call. What if we are just changing resolutions and xf86SwitchMode() failed ? We should call that again somewhere.
If xf86SwitchMode() fails, xf86RandRSetMode() sets the screen size/dimension to the old values. It then returns FALSE to the (only) caller, xf86RandRSetConfig(), which had not changed anything before calling xf86RandRSetMode(). So I don't see anything else to be done other than undoing the rotation. I am afraid I don't see what you mean. (Sorry for the "bad" patch, my editor deletes all spaces and tabs at ends of lines, and there a quite a couple in that file.)
My point being if xf86SwitchMode() failed - what state is the screen in ? It's not guaranteed, so it should be called again with the previous mode which was working with the old screen size and dimensions as you say. You are doing that for DriverFunc, so why not for xf86SwitchMode() ?
Hm, I'd say nothing happens if xf86SwitchMode() fails - the screen is in the same state as before that call. The driver's switchmode() function is the last thing it calls, and it checks its result. If this is false, nothing is done. If it's true, the frame is recalculated. If a driver is dumb enough to fail inside its switchmode() without resetting to the previous (working) mode, it's the driver's fault.
O.k. That's a reasonable solution and necessary documentation should be updated to reflect that case. Just looking at the SiS driver though I can see lots of places where the SwitchMode call would fail and it would leave the card in a pretty bad state that would need to cleaned up if this is the route taken.
Where does any documentation state that if the driver's switchmode returns FALSE, it will be called again with the old mode? And even if that is said somewhere, it does not reflect reality. (This does not relate to this bug, but don't worry about the SiS driver's error handling. It is pretty bullet-proof and it never leaves the card in any bad state.)
My point being that it doesn't explicitly say the driver needs to put back to the old mode, and we should document that it should if we do this. Drivers don't reference pScrn->currentMode at all to switch back and that would be a necessity. I don't see any drivers doing that. As for the SiS, I'm not having a go, but I can see SwitchMode calls ModeInit in the driver and there are places which can return FALSE and would leave the card in a bad way. And nowhere does it reference pScrn->currentMode to switch back. And the drivers that I've worked on don't fallback gracefully either. So I'd need to fix a few too. So again, I'm not having a go.
pScrn->currentMode is set by xf86SwitchMode AFTER the call to the driver's switchmode(), and only if SwitchMode() succeeded. The driver doesn't have to set pScrn->currentMode itself. And neither does it have to switch back. (As for SiS: Yes, there are some "return FALSE"'s, but that's before anything is written to the hardware. This is only additional validation, done by the mode switching code in init.c/init301.c. There is absolutely no way that the modeswitching, as soon as really initiated, will fail.)
You are missing the point. I didn't say that the driver should set pScrn->currentMode. I said it should reference it to switch back. xf86SwitchMode() is called with the mode we are switching to. If we fail, then the driver has to reference pScrn->currentMode to switch back. Because, as you say, pScrn->currentMode is only set when it succeeds, so by definition when xf86SwitchMode() fails pScrn->currentMode is still set to the previous mode and the new mode which was passed to xf86SwitchMode() is thrown away.
Actually, looking at the VidMode extension we can't use pScrn->currentMode anyway as it NULLifies it. So the driver needs to track modes internally. So don't worry about it.
Feel free to commit the patch Thomas.
OK, will do. Thanks for the review. In reply to comment #10: Why should the driver reference it, and when? If it returns FALSE at SwitchMode(), nothing has actually changed (assuming a sane driver that doesn't fail in the middle of a mode switch with the hardware state halfway changed). There is no need to switch back to anything, because the pre-SwitchMode() state is still valid. And so is pScrn->currentMode, as it wasn't and isn't being changed. Perhaps I *am* missing the point here, but I really don't understand what you mean.
O.k. Look at the vesa driver. It uses the BIOS to setup a mode. If that BIOS call failed, there's no guessing what it could have done to the screen. So it's really necessary for the driver to call the BIOS again with the previous mode data. It currently doesn't do that. I can see that trying to validate before you start hitting the hardware is a good approach, but there may be cases that's not possible and referencing the previous mode will be needed. That was my point. But certainly looking at parts of the common layer xf86SwitchMode() expects, already, that things don't change if it fails. So there's certainly a few drivers that don't respect that.
Committed the fix to CVS. Perhaps this should be done in the 6.8 branch as well?
*** Bug 3440 has been marked as a duplicate of this bug. ***
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.