On Mon, Jul 28, 2014 at 1:17 PM, Rob Clark <robdclark@xxxxxxxxx> wrote: > On Sun, Jul 27, 2014 at 5:41 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: >> Hi all, >> >> So I've figured instead of just talking I should draw up my ideas for atomic >> helpers and related stuff in some draft patches. Major differences compared to >> Rob's series: >> >> - The software side state update is done synchronously, at least when using the >> helper code. Drivers are free to do whatever they want, as usual. This has a >> bunch of nice upsides: >> >> + All the tricky locking dances can be abolished, and if the ordering and >> synchronization is done correctly async worker threads don't need any >> modeset state locking at all. I'm fairly sure that Rob's scheme won't blow >> up, but I much prefer we don't need it at all. >> >> + In Rob's patches any atomic operation must wait for outstanding updates to >> complete. Otherwise the software state isn't committed yet. With the >> synchronous updates we can immediately proceed with the state construction >> and verification and will only block right before applying the state update. >> And even for that drivers can implement cancellation of the previous update. > > fwiw, the rough plan here for my version was to (since state was > refcnt'd) eventually allow things to chain up (ie. copying N+1'th > state from N'th state, rather than mode objects themselves) which > could be done without locks for check-only.. > > That said, I'm not super sold on the idea that userspace needs to be > able to queue up multiple frames ahead.. but it was something that > people ask for from time to time.. We should still be able to queue up more than one update I think - the drm-visible software state will always reflect the latest update, even when it's not yet put into the hw. Internally the driver can keep a linked-list (maybe we could move that to the official drm_atomic_state even) to know where it is exactly with pushing out updates. One issue though with either approach is canceling updates in-between. I guess a lot of this will be highly driver-specific. In a way canceling an entire queue is just a more generic version of the "how can we cancel the current pending update" issue. So I think we should postpone the queue problem until we have the normal cancel solved (for lower-latency gaming). Also I think queued updates will only be worth it for some very special workloads like video display, when shutting down both the cpu and gpu is actually possible and might be worth some power saving. And hw engineers seem to flip-flop on whether that's worth it for video or whether it's better to have a slow, but power-sipping special decoder which feeds out frames constantly instead of the rush-stop-rush-stop queued frames would need. >> This has the nice benefit that check-only operations (which compositors need >> to do right after having completed the current frame to know how to render >> the next one) can be done without blocking for completion of the previous >> update. >> >> - Internal interfaces for atomic updates don't need to go through properties, >> but can directly set the state. Drivers can always compare updates in their >> ->check hook later on, so we don't lose expressiveness in the interface. But >> the resulting code in the callers looks much better. > > hmm, in cases where drivers have to fwd one property to another object > or other sort of side effect, I think my way was nicer, imho ;-) Well for driver-private properties you can do that still. E.g. with panel upscaling which you internally implement you could push that to the crtc. > Sure, we can figure it out in check_hook later, but it is a bit > awkward. And means we need to be careful about sequence of checks > (potentially having to re-check) of different objects. Looks worse to > me ;-) Otoh you can't just chase the connector_state->crtc link, since we don't promise to deliver the state updates in any order. So that might still change. I think in the end you really have to do all that cross-object verification/propagation only in the ->check function, and can't do it any earlier. Also I hope that most hw really sets e.g. plane coordinates in their plane functions, otherwise kms would be a serious misfit ;-) So hopefully not too much forwarding of state required. Wrt sequencing checks correctly: We have that problem any way, but the upside is that the order in which the atomic_check helper callbacks are run is well-defined. But the order in which ->atomic_set_prop is run is totally abitrary. So you can't really rely on any ordering in there (which rather seriously restricts what you can check). >> In general I've ditched a lot of interfaces where drivers could overwrite >> default behaviour and boiled down the interface two state handling functions >> (duplicate+destroy), setting driver-private properties and check/commit. >> >> - There is a clear helper separation between core code which should be used to >> use the atomic interface to drivers, and helper functions. Core never uses >> helper functions so that we can keep the clear separation we have in all other >> places in the kms api. >> >> There's a set of helpers sitting in-between to implement legacy interfaces on >> top of atomic. Those _only_ use the core atomic interfaces. >> >> - I've added a new set of plane helpers. Mostly this was motivated to make the >> atomic helpers work on less stellar hardware where you can't just stream the >> state and then atomically commit with some GO bits. But those helpers should >> also be useful for more gradual transitions of drivers to the atomic >> interface. Since a requirement for atomic is to have universal plane support >> they can be used bare-bones just for that, without all the other atomic >> baggage (i.e. all the state tracking for crtcs/connectors). >> >> - State tracking for connectors. i915 has piles of relevant connector >> properties, which we want to save/restore in atomic ops (e.g. hdmi metadata >> and stuff). >> >> - fbdev panic locking is implemented with trylock instead of nolock. >> >> - I've thought a bit about resume. See the proposed state handling for the >> default reset functions and the addition of a plane reset callback. >> >> There's also a metric pile of stuff missing: >> >> - It's completely untested (except for the parts touching currently used code) >> since I lack suitable hw. Converting i915 is just a bit too much of a pain for >> a quick w/e code draft ;-) >> >> - I didn't add all the properties for things currently exposed through ioctl >> structures. Since the interface here doesn't force internal code to use >> properties for everything (which really makes things look much better) we >> don't have a need for that until we merge the actual atomic ioctl. So I left >> that out. >> >> - I didn't write code for the fbdev helper to use atomic. The w/e ran out of >> time for that ... >> >> - Kerneldoc is occasionally not updated or still missing. > > Unless I'm missing something you also completely lost all the core > crtc and plane check code (except for what you left in legacy > codepaths.. tbh, this was one thing that I liked about my approach is > it unified all the core error checking for legacy and atomic paths.. Yeah, one downside of keeping the legacy interfaces as-is is that we duplicate the checking a bit. But we already have that problem a bit with primary plane helpers and solved it there by extracting the checking done by the core so that drivers and helpers could reuse. A few more of those and I think writing an appropriate plane->atomic_check function should be fairly trivial. Current ->update_plane functions also duplicate a bunch of that. Also I think we can reconsider this once we pull in the atomic ioctl. Then it should be much clearer where the responsibilities for checking which state lie. We'll probably move a lot down into helper functions for drivers to use in the various ->atomic_check callbacks. 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
