| Message ID | 1487685102-31991-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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;