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

[Daniel Vetter]

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.

- 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
  drm_atomic_helper_ prefix.

- 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
  various phases.

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

  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.

  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?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch