On Wed, Dec 14, 2016 at 12:08:06AM +0100, Daniel Vetter wrote:
> The requirements for connector_list locking are a bit tricky:
> - We need to be able to jump over zombie conectors (i.e. with refcount
> == 0, but not yet removed from the list). If instead we require that
> there's no zombies on the list then the final kref_put must happen
> under the list protection lock, which means that locking context
> leaks all over the place. Not pretty - better to deal with zombies
> and wrap the locking just around the list_del in the destructor.
>
> - When we walk the list we must _not_ hold the connector list lock. We
> walk the connector list at an absolutely massive amounts of places,
> if all those places can't ever call drm_connector_unreference the
> code would get unecessarily complicated.
>
> - connector_list needs it own lock, again too many places that walk it
> that we could reuse e.g. mode_config.mutex without resulting in
> inversions.
>
> - Lots of code uses these loops to look-up a connector, i.e. they want
> to be able to call drm_connector_reference. But on the other hand we
> want connectors to stay on that list until they're dead (i.e.
> connector_list can't hold a full reference), which means despite the
> "can't hold lock for the loop body" rule we need to make sure a
> connector doesn't suddenly become a zombie.
>
> At first Dave&I discussed various horror-show approaches using srcu,
> but turns out it's fairly easy:
>
> - For the loop body we always hold an additional reference to the
> current connector. That means it can't zombify, and it also means
> it'll stay on the list, which means we can use it as our iterator to
> find the next connector.
>
> - When we try to find the next connector we only have to jump over
> zombies. To make sure we don't chase bad pointers that entire loop
> is protected with the new connect_list_lock spinlock. And because we
> know that we're starting out with a non-zombie (need to drop our
> reference for the old connector only after we have our new one),
> we're guranteed to still be on the connector_list and either find
> the next non-zombie or complete the iteration.
>
> - Only downside is that we need to make sure that the temporary
> reference for the loop body doesn't leak. iter_get/put() functions +
> lockdep make sure that's the case.
>
> - To avoid a flag day the new iterator macro has an _iter postfix. We
> can rename it back once all the users of the unsafe version are gone
> (there's about 100 list walkers for the connector_list).
>
> For now this patch only converts all the list walking in the core,
> leaving helpers and drivers for later patches. The nice thing is that
> we can now finally remove 2 FIXME comments from the
> register/unregister functions.
>
> v2:
> - use irqsafe spinlocks, so that we can use this in drm_state_dump
> too.
> - nuke drm_modeset_lock_all from drm_connector_init, now entirely
> cargo-culted nonsense.
>
> v3:
> - do {} while (!kref_get_unless_zero), makes for a tidier loop (Dave).
> - pretty kerneldoc
> - add EXPORT_SYMBOL, helpers&drivers are supposed to use this.
>
> v4: Change lockdep annotations to only check whether we release the
> iter fake lock again (i.e. make sure that iter_put is called), but
> not check any locking dependecies itself. That seams to require a
> recursive read lock in trylock mode.
>
> Cc: Dave Airlie <airlied@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic.c | 14 ++++-
> drivers/gpu/drm/drm_connector.c | 116 ++++++++++++++++++++++++++++++++------
> drivers/gpu/drm/drm_encoder.c | 6 +-
> drivers/gpu/drm/drm_mode_config.c | 34 +++++------
> include/drm/drm_connector.h | 38 +++++++++++++
> include/drm/drm_mode_config.h | 12 +++-
> 6 files changed, 177 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 60697482b94c..b23b4abd67be 100644
> int drm_connector_register_all(struct drm_device *dev)
> {
> struct drm_connector *connector;
> - int ret;
> + struct drm_connector_list_iter conn_iter;
> + int ret = 0;
>
> - /* FIXME: taking the mode config mutex ends up in a clash with
> - * fbcon/backlight registration */
> - list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> + drm_connector_list_iter_get(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> ret = drm_connector_register(connector);
> if (ret)
> - goto err;
> + break;
> }
> + drm_connector_list_iter_put(&conn_iter);
>
> - return 0;
> -
> -err:
> - mutex_unlock(&dev->mode_config.mutex);
> - drm_connector_unregister_all(dev);
> + if (ret)
> + drm_connector_unregister_all(dev);
Just poked my head up here, because the thought of a hotplug connector
register vs an error here made my head hurt. I think the only way to
solve that (and general initialisation vs hpd) is to ensure/document no
hpd until after the registration phase.
The locking looks solid, the list is guarded by the spinlock, iterators
hold a reference, and the kref_unless_zero vs the list is guarded by the
spinlock inside connector release.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1417,6 +1417,7 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
> struct drm_mode_config *config = &state->dev->mode_config;
> struct drm_connector *connector;
> struct drm_connector_state *conn_state;
> + struct drm_connector_list_iter conn_iter;
> int ret;
>
> ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> @@ -1430,14 +1431,18 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
> * Changed connectors are already in @state, so only need to look at the
> * current configuration.
> */
> - drm_for_each_connector(connector, state->dev) {
> + drm_connector_list_iter_get(state->dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> if (connector->state->crtc != crtc)
> continue;
>
> conn_state = drm_atomic_get_connector_state(state, connector);
> - if (IS_ERR(conn_state))
> + if (IS_ERR(conn_state)) {
> + drm_connector_list_iter_put(&conn_iter);
> return PTR_ERR(conn_state);
I wuold have gone with ret = PTR_ERR(conn_state); break; just because I
don't like have more paths through the critical section than necessary
(i.e. more than one unlock).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
