Re: [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc.

[Sean Paul <seanpaul@chromium.org>]

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@lists.freedesktop.org
> Acked-by: Ben Skeggs <bskeggs@redhat.com> #irc
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx