[PATCH 00/17] prepare for atomic/nuclear modeset/pageflip
daniel at ffwll.ch
Mon May 26 03:40:34 PDT 2014
On Sat, May 24, 2014 at 02:30:09PM -0400, Rob Clark wrote:
> One more time, with feeling..
> Previous revision of series:
> And if you prefer, in git form:
> 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
But I think the interfaces to driver and overall api design need some good
- 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.
- 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.
- 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.
- 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
- 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. 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
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.
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.
- 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
- 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.
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
- 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.
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
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
For implementing this madness (if we agree it's the right approach) we can
- 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?
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel