On Sat, May 24, 2014 at 02:30:09PM -0400, Rob Clark wrote: > One more time, with feeling.. > > Previous revision of series: > http://lists.freedesktop.org/archives/dri-devel/2014-March/055806.html > > And if you prefer, in git form: > http://cgit.freedesktop.org/~robclark/linux/log/?h=cold-fusion > git://people.freedesktop.org/~robclark/linux cold-fusion > > This series does not include the actual atomic ioctl, but it does > include all the needed infrastructure changes. > > Compared to previous revision, I've split out drm_modeset_acquire_ctx > from drm_atomic_state, so that we can ww acquire mode_config and crtc > locks outside of atomic transactions. (Which keeps lockdep happy wrt. > drm_modeset_lock_all().) I also got to the point in juggling things > around where I wanted to let the compiler type checking do it's job, > so 's/void *state/struct drm_atomic_state *state/g'. > > At this point, I've tested this on i915 (few different generation > laptops), radeon, and msm. And of course w/ ww debug (deadlock in- > jection) enabled. So I think all the locking related paths should > be covered. > > I believe Thierry has tested a slighly older revision on tegra. I > would of course appreciate more testing on other drivers for which I > don't have the hw. But I think it is pretty much ready to go. I do > still owe some docs updates, I will send some patches for that in > the near future, but didn't want to hold up giving others a chance > to start banging on this. Ok, I've done a fairly cursory look at this only and tried to concentrate on grasping the high-level details. Patches contain a bunch of comments on the details, but here is the big stuff. First things first there's a bunch of prep work and refactoring sprinkled throughout the series. Imo we should pick those out and rebase them on top of drm-next asap to get them in before the actual interfaces. I hope I've catched them all and commented everywhere about what imo is still missing for merging. With that out of the way the real atomic review below. I like the overall design. It will be a royal pain to convert i915 over to this since thus far we've simply tucked away the staged state into obj->new_foo pointers. Which means we get to change the entire driver again ;-) But I think the interfaces to driver and overall api design need some good polish: - Imo way too much is done in driver callbacks, which then again call helpers. Parsing of the core properties for plane/crtcs should be done in the core imo. This way drivers only need to register ->set_prop callbacks if there's a driver-specific property they support. - Imo those obj->set_prop callbacks should take the object-specific drm_obj_state object, not the global atomic state structure. Will lead to _much_ less lookup code duplicated all over the place. If we then also add a state->obj pointer we could also ditch that parameter. Ofc obj would be specific to the state at hand. - One downside of that is that drivers won't be able to interfere the allocation step any more. I think we'd need an additional ->alloc_atomic_state call so that drivers can still easily subclass some objects with their own type. - There's imo a bit a confusion between what's helper and what's driver api. My big gripe here is with the set_prop stuff which imo really should be core and not helpers. The default ->commit/check implementations otoh should be more clearly delineated as helper implementations that drivers can/should overwrite. I think we should split drm_atomic.c into drm_atomic.c (with the official pieces) and drm_atomic_helper.c (with the suggested standard/transitional implemenations for commit/check). Helper functions should have the drm_atomic_helper_ prefix. - I also think we should split the vfuncs like we do with the crtc helpers. Imo the core interface should just be an ->alloc_state and ->set_prop on all kms objects, plus a global ->prep/check/commit at the driver level. The object-specific ->check/commit hooks otoh should be part of the atomic helper library and in some separate atomic_helper_funcs structure. Imo we could either extend the sturcture used by the crtc helpers (hackish imo) or change the type of that to make it clear it's for the crtc helpers and add a new pointer for atomic helpers. E.g. in i915 I expect that we'll implement our very own ->check/commit hooks using our own compute_config infrastructure. Maybe we could switch to the ->check hooks of the atomic helper eventually, but that will be a lot of work. And in any case we won't ever switch to the ->commit hook as you have it in your helpers since for truly atomic flips/modesets we need to interleave all the updates of all objects in various phases. The patches are also rather big, which makes them a bit a pain to review. Matt's approach for the primary planes was much easier: 1) Add the new core interfaces for the new world. 2) Implement helpers for drivers to transition and convert drivers. 3) Implement ioctls using the new driver interfaces. That approach would also help a lot in figuring out what's helper code and so optional, and what's core stuff. The other big problem I see still is with the locking: - Imo switching mode_config.mutex to a ww_mutex won't work. I know that the magic rules of w/w locking are _really_ tempting. But I don't think it will actually solve anything and instead only cause havoc. - I think we need ww mutexes for crtcs, but they will be fairly useless without ww mutexes on plane. Not for i915, but for all those drivers which can switch planes around between different crtcs. Imo we should do this as a first step (before getting all the atomic interfaces in), and pimp the set_plane locking a bit already like I've described in some mail. - There's a pile of state that's currently not really protected by anything in your patches. One piece is all the obj->state properties. Another one is state detected at runtime like e.g. whether a hdmi sink supports audio or what kind of link bw parameters a dp sink supports. Thus far this was all protected by mode_config.mutex, but we can't take this one in the generic atomic ioctls for pretty obvious reasons. I think we need a new lock here, e.g. dev->mode_config.state_lock. It's going to be painful, since we need to roll this out over _all_ driver callbacks which can change the meaning of some property. E.g. all the ->detect callbacks in i915. The important part is that no one is allowed to do any costly operations why holding this lock (i.e. anything like reading EDIDs or doing load-detect), it is only for updating/reading this state. - To avoid nasties with locking inversion between the plain mode_config.mutex, the state_mutex and the various ww mutexes we need to rather completely rework the locking sequence for atomic updates compared to what you have. Since in the driver's detect callbacks we first grab the mode_config.mutex and then potentiall ww mutexes (only i915) and maybe also the state_mutex (for updating autodetected properties). But for atomic updates we first need to grab the state_mutex (so that we can get at the current state) and then ww mutexes and the mode_config.mutex. And for the later we're not even good at predicting the order we want to acquire them. Stage 1: Create the new state We grab _just_ the mode_config.state_mutex and allocate all the state objects. This will allow us to grab a copy of the current state of everything without races or inconsistencies. As a consequence ->set_prop hooks may not touch anything outside of the date/state protected by the state_mutex. Stage 2: check/commit stage We drop the state_lock since it would nest the wrong way round again with mode_config.mutex. We should have a complete copy of all state already, and if something races against us (2nd ioctl or a output probe call) we don't really care. Only then will the check/commit hooks grab all the locks. For the mode_config.mutex we could add a bit to the global state the we need it (e.g. when the connector->crtc links change or for some other driver-private need). That only leaves races with a 2nd concurrent atomic modeset ioctl. Which can be avoided by grabbing yet another lock, which we drop after the driver has acquired all real locks. For that we could mandate the ->check callback and require that it grabs all ww mutexes plus the mode_config.mutex. For implementing this madness (if we agree it's the right approach) we can go step-by-step: - Convert crtc->mutex to a ww mutex. - Add plane ww mutexes, make set_plane locking more fine-grained. - Add the mode_config.state_lock, roll it out across all driver's ->detect callbacks and add it to modeset_lock_all (so that set_prop calls dtrt). - Use all this correctly in the atomic stuff. Ok, that's the two big comments on this work from my side. Now reality check: How much off the mark am I? 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
