drm/atomic: use connector references (v3)
diff mbox

Message ID 1462305785-28567-1-git-send-email-airlied@gmail.com
State New
Headers show

Commit Message

Dave Airlie May 3, 2016, 8:03 p.m. UTC
From: Dave Airlie <airlied@redhat.com>

Take a reference when setting a crtc on a connecter,
also take one when duplicating if a crtc is set,
and drop one on destroy if a crtc is set.

v2: take Daniel Stone's advice and simplify the
ref/unref dances, also take care of NULL as connector
to state reset.

v3: remove need for connector NULL check.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_atomic.c        | 14 +++++---------
 drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Daniel Vetter May 3, 2016, 8:09 p.m. UTC | #1
On Wed, May 04, 2016 at 06:03:05AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Take a reference when setting a crtc on a connecter,
> also take one when duplicating if a crtc is set,
> and drop one on destroy if a crtc is set.
> 
> v2: take Daniel Stone's advice and simplify the
> ref/unref dances, also take care of NULL as connector
> to state reset.
> 
> v3: remove need for connector NULL check.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

This patch series seems cursed with lots of embarrassement for reviewers.
I totally missed/forgot the FIXME in drm_atomic_state_default_clear(),
which this patch fixes. Anyway, I'm happy to burn my hands again ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c        | 14 +++++---------
>  drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9d5e3c8..0df87a5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -142,15 +142,7 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		if (!connector)
>  			continue;
>  
> -		/*
> -		 * 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,
> +		connector->funcs->atomic_destroy_state(connector,
>  						       state->connector_states[i]);
>  		state->connectors[i] = NULL;
>  		state->connector_states[i] = NULL;
> @@ -1160,6 +1152,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  {
>  	struct drm_crtc_state *crtc_state;
>  
> +	if (crtc)
> +		drm_connector_reference(conn_state->connector);
>  	if (conn_state->crtc && conn_state->crtc != crtc) {
>  		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
>  								conn_state->crtc);
> @@ -1177,6 +1171,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  			1 << drm_connector_index(conn_state->connector);
>  	}
>  
> +	if (conn_state->crtc)
> +		drm_connector_unreference(conn_state->connector);
>  	conn_state->crtc = crtc;
>  
>  	if (crtc)
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d25abce..c48446d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2762,6 +2762,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>  					    struct drm_connector_state *state)
>  {
>  	memcpy(state, connector->state, sizeof(*state));
> +	if (state->crtc)
> +		drm_connector_reference(connector);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>  
> @@ -2889,6 +2891,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  	 * state will automatically do the right thing if code is ever added
>  	 * to this function.
>  	 */
> +	if (state->crtc)
> +		drm_connector_unreference(state->connector);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Stone May 3, 2016, 8:13 p.m. UTC | #2
Hi,

On 3 May 2016 at 21:09, Daniel Vetter <daniel@ffwll.ch> wrote:
> This patch series seems cursed with lots of embarrassement for reviewers.
> I totally missed/forgot the FIXME in drm_atomic_state_default_clear(),
> which this patch fixes. Anyway, I'm happy to burn my hands again ;-)

Let's share the embarrassment. What could possibly go wrong?

Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9d5e3c8..0df87a5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -142,15 +142,7 @@  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		if (!connector)
 			continue;
 
-		/*
-		 * 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,
+		connector->funcs->atomic_destroy_state(connector,
 						       state->connector_states[i]);
 		state->connectors[i] = NULL;
 		state->connector_states[i] = NULL;
@@ -1160,6 +1152,8 @@  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 {
 	struct drm_crtc_state *crtc_state;
 
+	if (crtc)
+		drm_connector_reference(conn_state->connector);
 	if (conn_state->crtc && conn_state->crtc != crtc) {
 		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
 								conn_state->crtc);
@@ -1177,6 +1171,8 @@  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 			1 << drm_connector_index(conn_state->connector);
 	}
 
+	if (conn_state->crtc)
+		drm_connector_unreference(conn_state->connector);
 	conn_state->crtc = crtc;
 
 	if (crtc)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d25abce..c48446d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2762,6 +2762,8 @@  __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 					    struct drm_connector_state *state)
 {
 	memcpy(state, connector->state, sizeof(*state));
+	if (state->crtc)
+		drm_connector_reference(connector);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -2889,6 +2891,8 @@  __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 	 * state will automatically do the right thing if code is ever added
 	 * to this function.
 	 */
+	if (state->crtc)
+		drm_connector_unreference(state->connector);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);