Bug 4635

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