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

[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?

Sean

> 
> Cc: nouveau@lists.freedesktop.org
> Acked-by: Ben Skeggs  #irc
> Signed-off-by: Maarten Lankhorst 
> ---
>  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, _iter);
> - drm_for_each_connector_iter(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(_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) {
> - 

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

[Maarten Lankhorst]

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.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau