Re: [PATCH] drm/atomic: Paper over locking WARN in default_state_clear

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm/atomic: Paper over locking WARN in default_state_clear
Date: Fri, 31 Jul 2015 10:34:43 +0200	[thread overview]
Message-ID: <55BB3323.1050604@linux.intel.com> (raw)
In-Reply-To: <1438167101-20704-1-git-send-email-daniel.vetter@ffwll.ch>

Hey,

Op 29-07-15 om 12:51 schreef Daniel Vetter:
> In
>
> commit 6f75cea66c8dd043ced282016b21a639af176642
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  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. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx