[PATCH 16/19] drm/atomic-helpers: document how to implement async commit

Daniel Vetter daniel.vetter at ffwll.ch
Sun Aug 3 12:25:45 PDT 2014

On Fri, Aug 1, 2014 at 1:52 PM, Rob Clark <robdclark at gmail.com> wrote:
>> + * 1. Run drm_atomic_helper_prepare_planes() first. This is the only function
>> + * which commit needs to call which can fail, so we want to run it first and
>> + * synchronously.
> Without some converted driver to look at for reference, I wasn't
> completely sure whether prepare_fb/cleanup_fb was intended to do some
> refcnt'ing, etc.   Or just driver internal pin/unpin?

Reference counting at the drm_framebuffer level is still done in the
core (and imo should stay like that with the real atomic ioctl). So
the prepare/cleanup_fb hook is just for driver-internal stuff like
pin/unpin or relocating to vram or whatever. For generic async support
I also think prepare_fb should set a plane_state->fence pointer if
some waiting is needed. Since not all drivers need this this callback
is optional.

>> + * 2. Synchronize with any outstanding async commit worker threads which might
>> + * affect the new state update. This can be done by either cancelling or
>> + * flushing the work items, depending upon whether the driver can deal with
>> + * cancelled updates. Note that it is important to ensure that the framebuffer
>> + * cleanup is still done when cancelling.
> maybe just an issue of wording..  but if an async update was writing
> the state object, then we'd need to block earlier than commit.  I
> don't think that is what you meant, because the state object data
> structure synchronization you have relies on read-only access from the
> async commit.  Which would make me expect it would be sufficient to
> move the synchronization part into the async commit worker (ie. it
> would only need to sync w/ other workers, ie if you had multiple
> work-queues)?

Yeah, wording is unfortunate here, need to improve it. There's two
reasons for the synchronization with workers here:
- We need to do it before swapping states since workers rely on the
read-only access. You're correct that we could postpone this step to
the async worker.
- We need to do it synchronously so that the next user of the atomic
interface (ioctl or fb helper) has the latest state and computes the
relative update correctly. We could make this work with an async
swap-states with some refcounting, but I wanted to avoid that
complexity. If drivers want an internal queue of state updates they
can simply do an aditional copy here on top of the plain swap and
shovel that copy into a list_head queue or something similar.

>> + *
>> + * For sufficient parallelism it is recommended to have a work item per crtc
>> + * (for updates which don't touch global state) and a global one. Then we only
>> + * need to synchronize with the crtc work items for changed crtcs and the global
>> + * work item, which allows nice concurrent updates on disjoint sets of crtcs.
> s/work item/work queue/?

work item is the important part here, since with a concurrent queue
you don't need more than one (iirc).

>> + * 3. The software state is updated synchronously with
>> + * drm_atomic_helper_swap_state. Doing this under the protection of all modeset
>> + * locks means concurrent callers never see inconsistent state. And doing this
>> + * while it's guranateed that no relevant async worker runs means that async
>> + * workers do not need grab any locks. Actually they must not grab locks, for
>> + * otherwise the work flushing will deadlock.
>> + *
>> + * 4. Schedule a work item to do all subsequent steps, using the split-out
>> + * commit helpers: a) pre-plane commit b) plane commit c) post-plane commit and
>> + * then cleaning up the framebuffers after one vblank has passed.
> the *old* framebuffers..

Yup, will clarify.

> I assume the atomic helpers don't really care how the driver does it,
> as long as it does.  Somehow or another the driver would want to
> schedule cleanup on the vblank of the *old* crtc, I guess.

Yeah. Synchronous atomic commit helper simply waits for one vblank on
all crtcs in the state update (which is probably a bit too much, but
meh for synchronous updates). The plane helper only with the new crtc
(presuming that you can't switch planes before the old crtc stopped
using it).

Cheers, Daniel
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

More information about the dri-devel mailing list