Nouveau - Feb 2017 - [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc.

It seems that nouveau requires this, so best to do this in the helper.
This allows nouveau to use the atomic suspend helper.

Cc: nouveau at lists.freedesktop.org
Acked-by: Ben Skeggs <bskeggs at redhat.com> #irc
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       |  51 ++++++++++----
 drivers/gpu/drm/nouveau/nouveau_display.c | 113 +-----------------------------
 2 files changed, 38 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 9203f3e933f7..ff361066381e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2427,9 +2427,13 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
 	struct drm_connector *conn;
-	struct drm_connector_list_iter conn_iter;
-	int err;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int ret, i;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -2437,29 +2441,48 @@ int drm_atomic_helper_disable_all(struct drm_device
*dev,
 
 	state->acquire_ctx = ctx;
 
-	drm_connector_list_iter_get(dev, &conn_iter);
-	drm_for_each_connector_iter(conn, &conn_iter) {
-		struct drm_crtc *crtc = conn->state->crtc;
-		struct drm_crtc_state *crtc_state;
-
-		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
-			continue;
-
+	drm_for_each_crtc(crtc, dev) {
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state)) {
-			err = PTR_ERR(crtc_state);
+			ret = PTR_ERR(crtc_state);
 			goto free;
 		}
 
 		crtc_state->active = false;
+
+		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
+		if (ret < 0)
+			goto free;
+
+		ret = drm_atomic_add_affected_planes(state, crtc);
+		if (ret < 0)
+			goto free;
+
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret < 0)
+			goto free;
 	}
 
-	err = drm_atomic_commit(state);
+	for_each_connector_in_state(state, conn, conn_state, i) {
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+		if (ret < 0)
+			goto free;
+	}
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret < 0)
+			goto free;
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+	}
+
+	ret = drm_atomic_commit(state);
 free:
-	drm_connector_list_iter_put(&conn_iter);
 	drm_atomic_state_put(state);
-	return err;
+	return ret;
 }
+
 EXPORT_SYMBOL(drm_atomic_helper_disable_all);
 
 /**
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
index d479aad97cd4..820f44bef0bd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -625,117 +625,6 @@ nouveau_display_destroy(struct drm_device *dev)
 	kfree(disp);
 }
 
-static int
-nouveau_atomic_disable_connector(struct drm_atomic_state *state,
-				 struct drm_connector *connector)
-{
-	struct drm_connector_state *connector_state;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_plane_state *plane_state;
-	struct drm_plane *plane;
-	int ret;
-
-	if (!(crtc = connector->state->crtc))
-		return 0;
-
-	connector_state = drm_atomic_get_connector_state(state, connector);
-	if (IS_ERR(connector_state))
-		return PTR_ERR(connector_state);
-
-	ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
-	if (ret)
-		return ret;
-
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
-	if (ret)
-		return ret;
-
-	crtc_state->active = false;
-
-	drm_for_each_plane_mask(plane, connector->dev, crtc_state->plane_mask) {
-		plane_state = drm_atomic_get_plane_state(state, plane);
-		if (IS_ERR(plane_state))
-			return PTR_ERR(plane_state);
-
-		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
-		if (ret)
-			return ret;
-
-		drm_atomic_set_fb_for_plane(plane_state, NULL);
-	}
-
-	return 0;
-}
-
-static int
-nouveau_atomic_disable(struct drm_device *dev,
-		       struct drm_modeset_acquire_ctx *ctx)
-{
-	struct drm_atomic_state *state;
-	struct drm_connector *connector;
-	int ret;
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = ctx;
-
-	drm_for_each_connector(connector, dev) {
-		ret = nouveau_atomic_disable_connector(state, connector);
-		if (ret)
-			break;
-	}
-
-	if (ret == 0)
-		ret = drm_atomic_commit(state);
-	drm_atomic_state_put(state);
-	return ret;
-}
-
-static struct drm_atomic_state *
-nouveau_atomic_suspend(struct drm_device *dev)
-{
-	struct drm_modeset_acquire_ctx ctx;
-	struct drm_atomic_state *state;
-	int ret;
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-retry:
-	ret = drm_modeset_lock_all_ctx(dev, &ctx);
-	if (ret < 0) {
-		state = ERR_PTR(ret);
-		goto unlock;
-	}
-
-	state = drm_atomic_helper_duplicate_state(dev, &ctx);
-	if (IS_ERR(state))
-		goto unlock;
-
-	ret = nouveau_atomic_disable(dev, &ctx);
-	if (ret < 0) {
-		drm_atomic_state_put(state);
-		state = ERR_PTR(ret);
-		goto unlock;
-	}
-
-unlock:
-	if (PTR_ERR(state) == -EDEADLK) {
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-	return state;
-}
-
 int
 nouveau_display_suspend(struct drm_device *dev, bool runtime)
 {
@@ -744,7 +633,7 @@ nouveau_display_suspend(struct drm_device *dev, bool
runtime)
 
 	if (drm_drv_uses_atomic_modeset(dev)) {
 		if (!runtime) {
-			disp->suspend = nouveau_atomic_suspend(dev);
+			disp->suspend = drm_atomic_helper_suspend(dev);
 			if (IS_ERR(disp->suspend)) {
 				int ret = PTR_ERR(disp->suspend);
 				disp->suspend = NULL;
-- 
2.7.4

On Tue, Feb 21, 2017 at 02:51:40PM +0100, Maarten Lankhorst
wrote:
> It seems that nouveau requires this, so best to do this in the helper.
> This allows nouveau to use the atomic suspend helper.
Pardon the stupid question, but why can't nouveau just do the right thing
when
crtc_state->active becomes false?

Sean
> 
> Cc: nouveau at lists.freedesktop.org
> Acked-by: Ben Skeggs <bskeggs at redhat.com> #irc
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at
linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c       |  51 ++++++++++----
>  drivers/gpu/drm/nouveau/nouveau_display.c | 113
+-----------------------------
>  2 files changed, 38 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
> index 9203f3e933f7..ff361066381e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2427,9 +2427,13 @@ int drm_atomic_helper_disable_all(struct drm_device
*dev,
>  				  struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_atomic_state *state;
> +	struct drm_connector_state *conn_state;
>  	struct drm_connector *conn;
> -	struct drm_connector_list_iter conn_iter;
> -	int err;
> +	struct drm_plane_state *plane_state;
> +	struct drm_plane *plane;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int ret, i;
>  
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
> @@ -2437,29 +2441,48 @@ int drm_atomic_helper_disable_all(struct drm_device
*dev,
>  
>  	state->acquire_ctx = ctx;
>  
> -	drm_connector_list_iter_get(dev, &conn_iter);
> -	drm_for_each_connector_iter(conn, &conn_iter) {
> -		struct drm_crtc *crtc = conn->state->crtc;
> -		struct drm_crtc_state *crtc_state;
> -
> -		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
> -			continue;
> -
> +	drm_for_each_crtc(crtc, dev) {
>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>  		if (IS_ERR(crtc_state)) {
> -			err = PTR_ERR(crtc_state);
> +			ret = PTR_ERR(crtc_state);
>  			goto free;
>  		}
>  
>  		crtc_state->active = false;
> +
> +		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
> +		if (ret < 0)
> +			goto free;
> +
> +		ret = drm_atomic_add_affected_planes(state, crtc);
> +		if (ret < 0)
> +			goto free;
> +
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		if (ret < 0)
> +			goto free;
>  	}
>  
> -	err = drm_atomic_commit(state);
> +	for_each_connector_in_state(state, conn, conn_state, i) {
> +		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +		if (ret < 0)
> +			goto free;
> +	}
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret < 0)
> +			goto free;
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +	}
> +
> +	ret = drm_atomic_commit(state);
>  free:
> -	drm_connector_list_iter_put(&conn_iter);
>  	drm_atomic_state_put(state);
> -	return err;
> +	return ret;
>  }
> +
>  EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>  
>  /**
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
> index d479aad97cd4..820f44bef0bd 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -625,117 +625,6 @@ nouveau_display_destroy(struct drm_device *dev)
>  	kfree(disp);
>  }
>  
> -static int
> -nouveau_atomic_disable_connector(struct drm_atomic_state *state,
> -				 struct drm_connector *connector)
> -{
> -	struct drm_connector_state *connector_state;
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_plane_state *plane_state;
> -	struct drm_plane *plane;
> -	int ret;
> -
> -	if (!(crtc = connector->state->crtc))
> -		return 0;
> -
> -	connector_state = drm_atomic_get_connector_state(state, connector);
> -	if (IS_ERR(connector_state))
> -		return PTR_ERR(connector_state);
> -
> -	ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
> -	if (ret)
> -		return ret;
> -
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state))
> -		return PTR_ERR(crtc_state);
> -
> -	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> -	if (ret)
> -		return ret;
> -
> -	crtc_state->active = false;
> -
> -	drm_for_each_plane_mask(plane, connector->dev,
crtc_state->plane_mask) {
> -		plane_state = drm_atomic_get_plane_state(state, plane);
> -		if (IS_ERR(plane_state))
> -			return PTR_ERR(plane_state);
> -
> -		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> -		if (ret)
> -			return ret;
> -
> -		drm_atomic_set_fb_for_plane(plane_state, NULL);
> -	}
> -
> -	return 0;
> -}
> -
> -static int
> -nouveau_atomic_disable(struct drm_device *dev,
> -		       struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_atomic_state *state;
> -	struct drm_connector *connector;
> -	int ret;
> -
> -	state = drm_atomic_state_alloc(dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> -	state->acquire_ctx = ctx;
> -
> -	drm_for_each_connector(connector, dev) {
> -		ret = nouveau_atomic_disable_connector(state, connector);
> -		if (ret)
> -			break;
> -	}
> -
> -	if (ret == 0)
> -		ret = drm_atomic_commit(state);
> -	drm_atomic_state_put(state);
> -	return ret;
> -}
> -
> -static struct drm_atomic_state *
> -nouveau_atomic_suspend(struct drm_device *dev)
> -{
> -	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_atomic_state *state;
> -	int ret;
> -
> -	drm_modeset_acquire_init(&ctx, 0);
> -
> -retry:
> -	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> -	if (ret < 0) {
> -		state = ERR_PTR(ret);
> -		goto unlock;
> -	}
> -
> -	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> -	if (IS_ERR(state))
> -		goto unlock;
> -
> -	ret = nouveau_atomic_disable(dev, &ctx);
> -	if (ret < 0) {
> -		drm_atomic_state_put(state);
> -		state = ERR_PTR(ret);
> -		goto unlock;
> -	}
> -
> -unlock:
> -	if (PTR_ERR(state) == -EDEADLK) {
> -		drm_modeset_backoff(&ctx);
> -		goto retry;
> -	}
> -
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -	return state;
> -}
> -
>  int
>  nouveau_display_suspend(struct drm_device *dev, bool runtime)
>  {
> @@ -744,7 +633,7 @@ nouveau_display_suspend(struct drm_device *dev, bool
runtime)
>  
>  	if (drm_drv_uses_atomic_modeset(dev)) {
>  		if (!runtime) {
> -			disp->suspend = nouveau_atomic_suspend(dev);
> +			disp->suspend = drm_atomic_helper_suspend(dev);
>  			if (IS_ERR(disp->suspend)) {
>  				int ret = PTR_ERR(disp->suspend);
>  				disp->suspend = NULL;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Sean Paul, Software Engineer, Google / Chromium OS

Op 23-02-17 om 16:03 schreef Sean Paul:
> On Tue, Feb 21, 2017 at 02:51:40PM +0100, Maarten Lankhorst wrote:
>> It seems that nouveau requires this, so best to do this in the helper.
>> This allows nouveau to use the atomic suspend helper.
> Pardon the stupid question, but why can't nouveau just do the right
thing when
> crtc_state->active becomes false?
This patch is also required to clean up a connector leak in i915 driver unload
as well, and also to clean up plane state destruction there. Nouveau needs it to
unpin some framebuffers during suspend, but even if none of this was true, this
is the right way to handle disable_all.

Looking at DPMS is fragile.