On Tue, May 31, 2016 at 10:43:56AM +0200, Maarten Lankhorst wrote:
Op 30-05-16 om 17:09 schreef Daniel Vetter:
On Mon, May 30, 2016 at 03:08:39PM +0200, Maarten Lankhorst wrote:
Op 29-05-16 om 20:35 schreef Daniel Vetter:
- dev is redundant, we have state->atomic
- add stall parameter, which must be set when swapping needs to stall for preceeding commits to stop looking at ->state pointers. Currently all drivers need this to be, just prep work for a glorious future.
I have to disagree, if you want to stall it should be done in a separate helper before swapping state. That function should also have the ability to fail, since we haven't called swap_state yet. :)
Maybe with nonblock as parameter, so it could fail with -EBUSY if it would stall, and -EINTR if interrupted?
This is not the stalling you're thinking of. The EBUSY check is in drm_atomic_helper_setup_commit (and as such entirely optional).
And if you don't ever use the nonblocking helpers, no struct drm_crtc_commit will ever show up in crtc->commit_list, which means this here also doesn't do anything.
This stall here is just to make sure that the previous commit doesn't get confused because we've ripped out crtc/plane/connector->state from underneath it. And as soon as we add previous_state pointers to drm_atomic_state and wire it up through all the hooks we can set stall=false here, since then pipelining with a queue depth > 1 is possible.
If a driver supports depth > 1 it could skip the extra help waiter and just call swap_state. The helper should return -EINTR when needed.
if (nonblock) { ret = drm_atomic_helper_wait_for_completion(state); if (ret) return ret; }
drm_atomic_helper_swap_state..
That's why there's this stall parameter ... Also, wait_for_completion(state) is not enough, there's 3 different completions. -Daniel