Re: [PATCH 00/81] modeset rework

Daniel Vetter
2012-07-15T14:57:29Z

On Sun, Jul 15, 2012 at 02:13:41PM +0100, Chris Wilson wrote:
> On Wed, 11 Jul 2012 16:27:43 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Or: How to fix cpu edp in 81 simple steps. Admittedly this includes some minor
> > detours.
>
> Let me quickly jump in with my demands:
>
> That modeset should detect failure and propagate that back to the user.
>
> The modesetting code has outgrown its origins of setting a couple of
> registers and be done, as we are now in the domain of fiddly link
> training and end-point negotations. So it is fragile, can conceivably
> fail and so the errors need to be reported rather than blindly assume
> the output is now working.
>
> So all of these nice new WARNs I'm seeing need to return error codes as
> well.

Tbh failure propagation hasn't been front-and-center when I've done this
rework, but I've certainly noticed that we currenlty suck at this. Hence
nothing too coherent, but just a few thoughts:

- I think the WARNs should stay as-is. With the new code I only call them
once the modeset has completely, or at least once our code thinks it has
completed. Imo it's a really good tool to catch state-tracking goof-ups
or modeset sequence ordering issues, not more. I.e. all the WARNs I've
hit cleared indicated an issue in the code itself, and usually where the
harbingers of this pesky permanent-black-screen-till-reboot issue.

- The current error code handling inherited from the crtc helpers is
rather lacking, i.e. I wondered whether I should smash a quick patch on
top of this series to make set_base interruptible. And then noticed that
we only return a bool in set_base (and many other such places). So even
improving this for some simple&know cases will be quite some work.

- I think we should also differentiate between the different reasons why a
modeset can fail: From modes that the output doesn't support, failure in
pinning the fb because the gpu died, global resource allocation issues
(e.g. lack of pll, fdi links, ...) to the hw actually doing something
wrong. Current set_mode ioctl is restricted to -ERRNO, but I wonder
whether we should add something more flexible for the global modeset
code (similar to how we already have tons of reasons to reject a mode in
the ->mode_valid functions).

- For global resource allocation and reservation (get_pch, pin_and_fence
...) I think we should move this kind of stuff out of the crtc/encoder
callbacks into a prepare step (maybe merged together with the
->mode_fixup step). That way the actual modeset code should only fail if
the hw is in a non-cooperative mood. This is also a necessary step to
implement Jesse's idea of a modeset configuration checker. If we'd add
some drm modeset specific error codes the mode configuration selection
tool could even show nice tooltips on greyed-out-modes, explaning why
the mode doesn't work (e.g. "not enough memory bandwidth", "not enough
plls", or some generic "not enough resources" error ...). Yeah, I can
have dreams ...

- After all that, we'd be left with with the true hw issues like link
training failures or stuck pipes. Judging by bug reports and own
experience, once we're in such a state, we're stuck in such a state and
re-doing the modeset doesn't help too much. We already try to restore
the old mode, but if that also fails I'm not too sure what userspace
should do with the resulting -EIO.

At least for the short-term I think that our modeset code will profits the
most if we exploit the simplified state machine of the new code and cut
down on the statespace we're handling in the code. Like this patch series
already does for DP. Also, all these WARNs should help in improving our
modeset experience indirectly, through more bug reports and their fixes
;-)

Long term I agree that we need robuster error handling, but I think
handling of hw error should be about the last item: Ime we tend to keep on
flailing once we start, and I expect the pay-off in code quality and
robustness of the previous items to be better. In any case, this is all
stuff that should sit on top of this modeset rework.

Cc'ing Ville, maybe he has some further ideas from his global modeset
work.

Cheers, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48