On Fri, Jul 31, 2015 at 10:34:43AM +0200, Maarten Lankhorst wrote: > Hey, > > Op 29-07-15 om 12:51 schreef Daniel Vetter: > > In > > > > commit 6f75cea66c8dd043ced282016b21a639af176642 > > Author: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Date: Wed Nov 19 18:38:07 2014 +0100 > > > > drm/atomic: Only destroy connector states with connection mutex held > > > > I tried to fix races of atomic commits against connector > > hot-unplugging. The idea is to ensure lifetimes by holding the > > connection_mutex long enough. That works for synchronous commits, but > > not for async ones. > > > > For async atomic commit we really need to fix up connector lifetimes > > for real. But that's a much bigger task, so just add more duct-tape: > > For cleaning up connector states we currently don't need the connector > > itself. So NULL it out and remove the locking check. Of course that > > check was to protect the entire sequence, but the modeset itself > > should be save since currently DP MST hot-removal does a dpms-off. And > > that should synchronize with any outstanding async atomic commit. > > > > Or at least that's my hope, this is all a giant mess. > > > > Reported-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > --- > > drivers/gpu/drm/drm_atomic.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 3efd91c0c6cb..434915448ea0 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -153,9 +153,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > if (!connector) > > continue; > > > > - WARN_ON(!drm_modeset_is_locked(&config->connection_mutex)); > > - > > - connector->funcs->atomic_destroy_state(connector, > > + /* > > + * FIXME: Async commits can race with connector unplugging and > > + * there's currently nothing that prevents cleanup up state for > > + * deleted connectors. As long as the callback doesn't look at > > + * the connector we'll be fine though, so make sure that's the > > + * case by setting all connector pointers to NULL. > > + */ > > + state->connector_states[i]->connector = NULL; > > + connector->funcs->atomic_destroy_state(NULL, > > state->connector_states[i]); > > > This wouldn't provide any additional guarantee during the async commit > itself, so please don't do this. :-) Nope, it's really just a big reminder that we have a bug here. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
