On Tue, May 31, 2016 at 04:22:57PM +0200, Maarten Lankhorst wrote:
Op 30-05-16 om 10:01 schreef Daniel Vetter:
Design ideas:
split up the actual commit into different phases, and have completions for each of them. This will be useful for the future when we want to interleave phases much more aggressively, for e.g. queue depth > 1. For not it's just a minimal optimization compared to current common nonblocking implementation patterns from drivers, which all stall for the entire commit to complete, including vblank waits and cleanups.
Extract a separate atomic_commit_hw hook since that's the part most drivers will need to overwrite, hopefully allowing even more shared code.
Enforce EBUSY seamntics by attaching one of the completions to the flip_done vblank event. Side benefit of forcing atomic drivers using these helpers to implement event handlign at least semi-correct. I'm evil that way ;-)
Ridiculously modular, as usual.
The main tracking unit for a commit stays struct drm_atomic_state, and the ownership rules for that are unchanged. Ownership still gets transferred to the driver (and subsequently to the worker) on successful commits. What is added is a small, per-crtc, refcounted structure to track pending commits called struct drm_crtc_commit. No actual state is attached to that though, it's purely for ordering and waiting.
Dependencies are implicitly handled by assuming that any CRTC part of &drm_atomic_state is a dependency, and that the current commit must wait for any commits to complete on those CRTC. This way drivers can easily add more depencies using drm_atomic_get_crtc_state(), which is very natural since in most case a dependency exists iff there's some bit of state that needs to be cross checked.
Removing depencies is not possible, drivers simply need to be careful to not include every CRTC in a commit if that's not necessary. Which is a good idea anyway, since that also avoids ww_mutex lock contention.
Queue depth > 1 sees some prep work in this patch by adding a stall paramater to drm_atomic_helper_swap_states(). To be able to push commits entirely free-standing and in a deeper queue through the back-end the driver must not access any obj->state pointers. This means we need to track the old state in drm_atomic_state (much easier with the consolidated arrays), and pass them all explicitly to driver backends (this will be serious amounts of churn).
Once that's done stall can be set to false in swap_states.
Features: Contains bugs because totally untested.
v2: Dont ask for flip_done signalling when the CRTC is off and stays off: Drivers don't handle events in that case. Instead complete right away. This way future commits don't need to have special-case logic, but can keep blocking for the flip_done completion.
v3: Tons of fixes:
- Stall for preceeding commit for real, not the current one by accident.
- Add WARN_ON in case drivers don't fire the drm event.
- Don't double-free drm events.
v4: Make legacy cursor not stall.
v5: Extend the helper hook to cover the entire commit tail. Some drivers need special code for cleanup and vblank waiting, this makes it a bit more useful. Inspired by the rockchip driver.
v6: Add WARN_ON to catch drivers who forget to send out the drm event.
v7: Fixup the stalls in swap_state for real!!
I don't think stalling belongs to swap_state, that should be a separate helper call.
Imo swap_state and stalling for hw_done is crucial that they're never split up. At least until we have queue depth > 1. Splitting it up just means there's more stuff drivers can get wrong, and I haven't found a single driver where there's a need to split things up. And I looked at all 17 of them. That's why I don't think splitting makes sense.
All the other helpers might seem like things are split up a lot, but for each separate helper function I'm adding here there's that absolutely needs that split to insert/replace a bit.
When nonblocking = false the waiting is still performed uninterruptibly. I believe that this is an error, and all blocking waiting should be completed before calling swap_state to ensure -EINTR can be propagated correctly as much as possible. Perhaps also return -EBUSY if wait times out.
The trouble with failing with -EBUSY is that drivers are broken in their drm even handling, which means they'll fail. By eventually continuing there's at least some chances that you'll see something on the screen.
Wrt interruptible waits, the stall check does wait interruptible. Everything else is imo part of the commit, which isn't allowed to fail/get interrupted. And for queue depth > 1 we'll actually move the stall for hw_done down into the worker.
You specified a timeout of 10 HZ, why is that? If we wait interruptibly then there's no need for a timeout. I also think the timeout may be too short, if we commit 3 crtc's it leaves 3.3 seconds for each. In case there's a modeset enable and disable, that leaves 1.6 seconds for each enable/disable. Might be too short..
Random value between "long enough to never be a problem in practice" and "short enought that it's still ok to wait it out on boot-up for the initial modeset". Note that wait_for_dependencies has another wait for hw_done, and that one is non-optional (and in the async worker).
Patch is also a bit hard to review with so many lines changed, could this be done in pieces instead of all at once?
The only bits I can split out is extracting commit_tail, and adding events to drm_atomic_commit->crtcs[]. If I split more that just means the code I add and the the places where it actually gets used are split up, which imo doesn't make sense since it's not longer a complete functional change on its own.
What do you want me to split out to make it easier to read? -Daniel