On Mon, May 26, 2014 at 6:12 PM, Rob Clark robdclark@gmail.com wrote:
On Mon, May 26, 2014 at 11:24 AM, Daniel Vetter daniel@ffwll.ch 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@ffwll.ch 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