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

[Rob Clark]

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.

>> > - 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