[PATCH 00/17] prepare for atomic/nuclear modeset/pageflip

[Daniel Vetter]

On Mon, May 26, 2014 at 6:12 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Mon, May 26, 2014 at 11:24 AM, Daniel Vetter <daniel at 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 at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch