On Mon, May 26, 2014 at 11:24 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > 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. The per-object hooks will work with at least three display controller blocks that I know of (two in msm, and omapdrm), so I guess it is not *that* uncommon. We'll need to split up an existing fxn or two so that other drivers can do an all-at-once commit more easily. That seems like a fairly small tweak, and I think it would be ok to merge along w/ i915 atomic support. >> > - 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 ... it would seem tempting to put in i915_atomic_state, but then how does it work with multiple parallel but independent atomic updates, or should that not be allowed? > 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. Well, the current approach gave the driver more flexibility about what locks to grab, etc.. although I suppose some of this might be less needed now with primary planes. Formerly I had to fwd some properties from crtc to my own internal primary plane, which required the flexibility that the current approach gives. >> > - 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). right, there is a very tiny bit of driver magic in msm_atomic_commit().. anyways, I don't disagree that we probably need to add something for some drivers to be able to hook in to ->atomic_commit() after locking magic but before loop over planes/crtcs. I think that would simply be addition to the api, ie. new fxn ptr a driver could populate. I wouldn't need it for msm. >> > - 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. I suspect I'll want to see which core properties change eventually.. when I have a chance to implement YUV and scaling, I'd want to flag that scaling/csc coefficients need updating, etc. It doesn't seem unreasonable to pass plane/crtc state rather than global state. (Although we would need connector_state.. which is something I've not found a good use for yet.) But other than that, I'd kinda like to leave the ->set_property() as it is. BR, -R > >> > 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
