Bug 4635

Summary: RandR failure handling broken
Product: xorg Reporter: Thomas Winischhofer <thomas>
Component: Server/DDX/XorgAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: high CC: alanh, alexdeucher
Version: git   
Hardware: All   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Fixes the failure handling none

Description Thomas Winischhofer 2005-09-29 03:32:55 UTC
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.
Comment 1 Thomas Winischhofer 2005-09-29 03:33:53 UTC
Created attachment 3429 [details] [review]
Fixes the failure handling
Comment 2 Alan Hourihane 2005-09-29 04:22:08 UTC
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.
Comment 3 Thomas Winischhofer 2005-09-29 04:36:52 UTC
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.)
Comment 4 Alan Hourihane 2005-09-29 04:44:13 UTC
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() ?
Comment 5 Thomas Winischhofer 2005-09-29 06:02:17 UTC
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.
Comment 6 Alan Hourihane 2005-09-29 06:32:48 UTC
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.
Comment 7 Thomas Winischhofer 2005-09-29 07:22:55 UTC
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.)
Comment 8 Alan Hourihane 2005-09-29 07:55:35 UTC
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.
Comment 9 Thomas Winischhofer 2005-09-29 08:12:53 UTC
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.)
Comment 10 Alan Hourihane 2005-09-29 08:30:16 UTC
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.
Comment 11 Alan Hourihane 2005-09-29 09:29:14 UTC
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.
Comment 12 Alan Hourihane 2005-09-29 09:31:01 UTC
Feel free to commit the patch Thomas.
Comment 13 Thomas Winischhofer 2005-09-29 17:02:40 UTC
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.
Comment 14 Alan Hourihane 2005-09-30 00:43:21 UTC
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.
Comment 15 Thomas Winischhofer 2005-09-30 01:56:31 UTC
Committed the fix to CVS.

Perhaps this should be done in the 6.8 branch as well?
Comment 16 Alan Coopersmith 2005-10-01 00:10:40 UTC
*** 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.