Re: [PATCH 10/27] drm/atomic-helper: nonblocking commit support

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 08, 2016 at 04:44:24PM +0200, Maarten Lankhorst wrote:
> Op 08-06-16 om 14:19 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).
> ^Typo, and was done 9 commits before?

Hm, what typo? And the patches are just prep, what we still need is
explicitly passing crtc/plane/connector state into driver callbacks, so
that they don't look at crtc/plane/connector->state any more.

> >   Once that's done stall can be set to false in swap_states.
> >
> > Features: Contains bugs because totally untested.
> ^I Hope not..

Indeed, tested on iirc 5 drivers now. Will remove when merging.
-Daniel

> > 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!!
> >
> > v8:
> > - Fixup trailing whitespace, spotted by Maarten.
> > - Actually wait for flip_done in cleanup_done, like the comment says
> >   we should do. Thanks a lot for Tomeu for helping with debugging this
> >   on.
> >
> > v9: Now with awesome kerneldoc!
> >
> > v10: Split out drm_crtc_commit tracking infrastructure.
> >
> > v:
> > - Add missing static (Gustavo).
> > - Split out the sync functions, only do the actual nonblocking
> >   logic in this patch (Maarten).
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel