On Mon, May 26, 2014 at 6:12 PM, Rob Clark <robdclark@xxxxxxxxx> wrote: > 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. I'm working on a reply to your msm patch. I think it's better we'll move that discussion over there, where we have more context with actual code. >>> > - 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? Once we've entered the check/commit hooks all locks should have been acquired (or will get acquired at the beginning of the check hook), so there's no bothersome parallelism I think. Wrt the i915 implementation I think the approach I'll try is: 1. In the set_prop stage _only_ store data in the <obj>_state structures and nowhere else, not touching any data reachable from a 2nd thread. 2. In the check hook put all the state into <obj>->new_state pointers. At that point we hold all the locks so can do that. 3. Run the ->compute_config hooks over all relevant objects. We need to change all the code from directly looking into its object's structure to go through the new_state indirection (e.g. for figuring out whether we should enable hdmi audio or similar stuff). 4. Bail out and undo al new_foo pointer changes when only checking, run the full hw commit code when doing a real atomic update. That approach should hug the current implementation closely wrt the ->new_foo handling, but still tightly integrate with the free-floating state construction atomic wants. The conversion will not be pretty though :( >> 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. I think if there's any private ww mutex locks drivers need to acquire, they should do that in their check/commit hook. Not while parsing properties. From that point on it's all under the control of the driver, so might not even need a full ww mutex, but a plain lock that nests within all the kms ww mutexes might be good enough. >>> > - 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. If you mean new functions for the atomic helper, I agree (and don't really care). If you mean new functions for the core->driver interface (i.e. what i915 gets to implement on it's own) then if the check/commit hooks really should carry all the information we need. In the end this boils down to my request to more clearly separate core->driver interface from helper code. >>> > - 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. Hm, my idea was that drivers would implement any state diffing in their ->check hooks. Only then we - hold all the locks, so know nothing relevant will change - have the entire picture of all states (and the often depend on each another in funny ways). So imo all the set_prop code should _only_ deal with creating the in-kernel representation of the desired state, not with checking it or computing a whole lot of derived date (like which parts need to be updated in hw). > 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. Oh, we'll need that definitely. At least on i915 we have a pile of connector properties which we need to track somehow, and most of those need a full modeset to put them into the hw (mostly disable display pipe, frob hw, reenable display pipe with the same state). -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
