On Mon, May 26, 2014 at 08:48:52AM -0400, Rob Clark wrote: > On Mon, May 26, 2014 at 6:40 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > > 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. > > Pretty universally, the approach I took was to provide default fxns > (for ->atomic_xyz(), ->set_property(), ->create_state(), etc), and let > drivers wrap them as needed. We might need to tweak > drm_atomic_begin() a bit for the first driver that needs to wrap > drm_atomic_state, but I guess we can tackle that as the need arises. > > > - 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. > > The problem is if those callbacks need to take other driver shared > locks.. that plus initially I was trying to keep state as opaque as > possible (in case sooner or later i915 decided to re-implement :-P) > > Eventually I decided keeping state opaque was too much pita, and that > if driver wanted to do something different, it should instead > subclass. So I guess these days they could chase [pc]state->state to > get back at the global state. I don't think keeping state opaque is a feature. Imo we _want_ to have some common structures to track the common set of properties, for otherwise insanity will rule. Drivers can add whatever they whish by subclassing the existing state structs and add whatever they need to track their state. I don't have any intentions to implement something newfangled/different for i915, but instead want to port our current infrastructure over to this. Having clear prepare&check (we call it compute) and commit semantics is imo the right way to do kms. Think of i915 as the poc vehicle that gets to pay the bill of rewriting the driver twice. So I'm advocating to move even further avoid from void* everywhere than you've moved already. Wrt helpers I think at least for modeset operations we can pimp the crtc helpers and mostly reuse the hooks we already have, maybe augmented with per-object ->check hooks. But for nuclear pageflips I really don't see a chance but a single driver-gloabl ->commit (and fwiw check) hook. Maybe per-crtc at best, but that will already run into planes changing crtcs. > > - 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. > > the ->create_state() is already split out as a vfunc for planes and > crtcs. Which I think should be enough to cover most of what drivers > need. If driver needs to subclass global state too, then we need to > split up drm_atomic_begin(), but I guess that should be a relatively > small change that we can make when it becomes needed. I guess i915 will need it, to keep track of shared resources like plls. Atm we carry them around in dev_priv, without any precompute/commit semantics. Still something that needs to be fixed ... Wrt ->create_state: I simply missed that in all the patches. But if you have that I don't quite understand why the various ->set_property callbacks have checks whether the object-specific state exists already and allocate it if not through the get_plane_state and similar functions. Imo that kind of stuff should be handled in the core. Error handling code is really hard to get correct ime, and we depend upon it's correctness here (at least with the current locking scheme) for ww mutexes to work. > > - 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. > > Hmm, I think nearly *everything* could be overridden.. maybe it could > be more clear what to override vs outright replace. But otoh I'm not > quite sure yet what drivers will need to override (other than some > obvious ones, which are already broken out into vfuncs, like > {plane,crtc}->create_state() to handle driver custom properties). Like I've said above we should imo try to unify the world a bit more again. And for the state tracking that should be possible, so I don't see a need to hand drivers too much rope. The check/commit stuff otoh should be fully overrideable, since i915 will need that. > So I expect some tweaks as other drivers start adding native atomic > support. I basically added what I needed for msm, and what I thought > was pretty safe bet that other drivers would need. This at least > gives us something other drivers could start trying to use. Then see > what is missing and add it as we go. At least that was my line of > thinking. Like I've said I don't think there's much value beyong making crtc helpers atomic capable for modeset changes. Atomic pageflips pretty much need driver-specific magic (less so if there's a "GO" bit like on sane hardware). > > - 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. > > this is in fact the way it is, although 'struct drm_atomic_funcs' > maybe should have "helper" in the name. Yeah, I'm mostly just asking for sprinkling helper all over the stuff that clearly is helper code. And moving everything else into the core and making it non-overrideable. > > 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. > > not sure if it would work for you, but in msm I have a per-crtc "don't > actually commit yet" bit, which gets set for each crtc involved in > atomic update. Although in my case it is mainly a matter of not > touching the "GO" bits until everything is setup.. > > If not, I guess it shouldn't be a big deal to, for example, split up > atomic_commit() into part that did the locking dance and then call > vfunc ->atomic_apply_state() or something like that. > > Basically figure out where you need to hook in for i915 as you add > native atomic support. By the time you and maybe one or two other > drivers have added atomic support we should have it pretty well nailed > down. But until then, there might be some small adjustments > here/there. For modeset changes we need to munge everything through the full state precompute and then commit to hw machinery. Not yet fully there (since the shared dpll stuff doesn't work yet). For nuclear pageflips we're only just starting to merge all of Ville's infrastructure, but will probably be the same. So imo the only hand-off point between the core and drm is the single, global ->commit. ->check would be the identical code, except for the final commit. All the ->set_prop callbacks would do nothing else but store data into structures. > > 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. > > not so far different from what I did. Although I suppose I could have > split up adding atomic support for plane/crtc vs converting over > existing ioctls to use it. Not sure if it is worth doing at this > stage or not. Not really sure that it would help in sorting out what > is helper vs not. It's mostly for reviewing - silly me got lost countless times in your patches. > > 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. > > are you actually getting lockdep splats? If so pls send them my way > and I'll have a look. > > In non-atomic cases we are still using these as bare mutex's (other > than the one special case where we needed nested_lock before). So I > think if it worked before, it should continue to work. Didn't run it, but pretty sure. I think the other mail conversation cleared that up - mixing w/w nesting and static nesting just don't work: Static nesting means locks are filed into different (static) groups, w/w nesting means you have one single locking class, but ordered dynamically with the backoff magic. > > - 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. > > we are still taking mode_config.mutex in places where we did before > (setcrtc, setplane, etc). So it shouldn't be a problem *yet*. > > Runtime internal params about hw state, as opposed to things userspace > is asking for, should perhaps not be in obj->state? Or maybe I'm > misunderstanding you. Afaict we copy obj->state and ->set_prop (at least in i915) also looks at a bunch of other connector state. And at that point we don't hold mode_config.mutex, and I couldn't spot anything else. Thus far all the ->set_prop stuff _did_ hold mode_config.mutex. > > 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. > > so I think I need to understand the call sequence for the detect thing > in i915, since atm I'm not quite sure what the problem is, and why it > worked before.. > > > 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? > > well, I think in summary, it is (a) might need to split few things up > or re-arrange things slightly as i915 and other drivers start adding > native atomic support.. but this doesn't scare me too much, and I > think we can take it as we go. The problem I see here is massive churn over drivers. If we try to make set_prop hooks and similar stuff optional we'd avoid that, and would actually gain flexibility with fixing the interfaces down the road. If we enforce default callbacks for everyone that will only bloat diffs. > And (b) possible locking fun.. this I think actually does need to be > sorted first. Although I'm not quite sure I understand why it is > actually a problem. Plan B for this might be to go full bore on ww mutexes and also protect connectors with them. Then atomic updates would never have a need for mode_config.mutex, and the role of that lock would be restricted to some detect fun. Less complicated locking scheme, but even more code to audit. -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
