On Fri, Aug 1, 2014 at 1:52 PM, Rob Clark <robdclark@xxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
