On Wed, Jul 4, 2018 at 5:44 PM, Daniel Stone daniel@fooishbar.org wrote:
Hi, The atomic API being super-explicit about how userspace sequences its calls is great and all, but having shared global state implicitly dragged in is kind of ruining my day.
Currently on Intel, Weston sometimes fails on hotplug, because a commit which only enables CRTC B (not touching CRTC A or any other CRTC!), causes all commits to CRTC A to fail until CRTC B's modeset commit has fully retired: https://gitlab.freedesktop.org/wayland/weston/issues/24
The reason is that committing CRTC B resizes the DDB allocation for CRTC A as well, pulling CRTC A's CRTC state into the commit. This makes sense, but on the other hand it's totally opaque to userspace, and impossible for us to reason about when making commits.
I suggested some options in that GitLab commit, none of which I like:
- if any other CRTCs are pulled into a commit state, always execute
a blocking commit in the kernel
- if we're passing ALLOW_MODESET in userspace, only ever do blocking commits
- whenever we get -EBUSY in userspace, assume we've been screwed by
the kernel and defer until other outputs have completed
- whenever we want to reconfigure any output in userspace, wait
until all outputs are completely quiescent and do a single atomic commit covering all outputs
The first one seems completely non-obvious from the kernel, but on the other hand the current -EBUSY failing behaviour is also non-obvious.
The second is maybe the most reasonable, but on the other hand just working around a painful leaky abstraction: we also can't know upfront from userspace if this is actually going to be required, or if we're just killing responsiveness blocking for no reason.
The third is the thing I least want to do, because it might well paper over legitimate bugs in userspace, and complicates our state tracking for no reason.
The fourth is probably the most legitimate, but, well ... someone has to type up all the code to make our output-configuration API completely asynchronous.
I suspect we're the first ones to be hitting this, because Weston has a truly independent per-CRTC repaint loop, we're one of the few atomic users, and also because Pekka did some seriously brutal hotplug testing whilst reworking Weston's output configuration API. Also because our approach to failed output repaints is to just freeze the output until it next cycles off and on, which is much more apparent than just silently dropping a frame here and there. ;)
Any bright ideas on what could practically be done here?
Cheers, Daniel
We had an entirely inconclusive chat on irc about this topic. Random notes:
- This can fail both on the commit doing a modeset (when it adds some random other CRTC which still has a pending flip), and for normal flips (if they run into a CRTC which has been affected by a modeset).
- Just waiting for all nonblocking modesets to complete is not enough, because you don't get an event for these affected CRTC. And they might take longer. So you can still get an EBUSY even when it looks like everything is done.
- If you don't combine nonblocking with ALLOW_MODESET there's no issue, because everyone just blocks (and maybe misses a few frames) until the modeset is done.
- We want to tell apart an EBUSY due to the redraw loop having lost sync from an EBUSY due to these modesets that affect more than just the passed-in CRTCs. It's a really nice debug feature for compositor developers.
-> One way or another we need new/clarified uapi, atomic as implemented doesn't really cut it.
A few of the options we've discussed:
- Eating all the EBUSY for ALLOW_MODESET. A bit a bigger hammer. Slightly smaller hammer is just eating all the EBUSY for crtc not in the original request. It would still be nice to tell userspace about the additional CRTCs its atomic comit affects.
- Some new flags to send out events (what about fence_ptr?) for these affected crtcs, plus reporting them out in a new affected_crtcs_mask in the atomic ioctl struct.
- One tricky issue is that drivers might pull in CRTCs which are disabled and stay disabled. Events aren't well-defined on these at all, but they might still provoke an EBUSY!
- Compositors would like to know how many frames they've lost. One idea was to split up the retry error codes: EINTR would mean retrying immedetialy due to a signal. EAGAIN would mean the kernel had to insert something to make a modeset possible, retry only after you waited for one vblank. With current userspace this would only result in cpu wasted, but updated userspace could be more intelligent about things. We could also do a special flag or whatever.
We didn't zero in on anything specific since it's not really clear what compositors want here at all.
Cheers, Daniel