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.
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.
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..
Patch is also a bit hard to review with so many lines changed, could this be done in pieces instead of all at once?