* [PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again. @ 2017-02-21 13:51 Maarten Lankhorst [not found] ` <1487685102-31991-1-git-send-email-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-21 13:51 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx This fixes a connector leak first when disabling the device, and then converts rmfb to atomic. The behavior change is done as a separate patch, so it can be reverted if it breaks, again.. Maarten Lankhorst (3): drm/atomic: Make disable_all helper fully disable the crtc. drm: Convert drm_framebuffer_remove to atomic, v4. drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb. drivers/gpu/drm/drm_atomic.c | 106 ++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 51 ++++++++++---- drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_framebuffer.c | 7 ++ drivers/gpu/drm/nouveau/nouveau_display.c | 113 +----------------------------- 5 files changed, 152 insertions(+), 126 deletions(-) -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <1487685102-31991-1-git-send-email-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]
* [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc. [not found] ` <1487685102-31991-1-git-send-email-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> @ 2017-02-21 13:51 ` Maarten Lankhorst 2017-02-23 15:03 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-21 13:51 UTC (permalink / raw) To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Maarten Lankhorst 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@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 _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc. 2017-02-21 13:51 ` [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc Maarten Lankhorst @ 2017-02-23 15:03 ` Sean Paul 2017-02-23 20:10 ` Maarten Lankhorst 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2017-02-23 15:03 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: nouveau, intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc. 2017-02-23 15:03 ` Sean Paul @ 2017-02-23 20:10 ` Maarten Lankhorst 0 siblings, 0 replies; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-23 20:10 UTC (permalink / raw) To: Sean Paul; +Cc: nouveau, intel-gfx, dri-devel 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. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4. 2017-02-21 13:51 [PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again Maarten Lankhorst [not found] ` <1487685102-31991-1-git-send-email-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> @ 2017-02-21 13:51 ` Maarten Lankhorst 2017-02-23 15:09 ` Sean Paul 2017-02-21 13:51 ` [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb Maarten Lankhorst 2017-02-21 15:23 ` ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again Patchwork 3 siblings, 1 reply; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-21 13:51 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, intel-gfx, Daniel Vetter Instead of trying to do everything in 1 go, just do a basic safe conversion first. We've been bitten by too many regressions in the past. This patch only converts drm_framebuffer_remove to atomic. The regression sensitive part is split out to a separate patch. v2: - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. - Add WARN_ON when atomic_remove_fb fails. - Always call drm_atomic_state_put. v3: - Use drm_drv_uses_atomic_modeset - Handle the case where the first plane-disable-only commit fails with -EINVAL. Some drivers do not support this, fall back to disabling all crtc's in this case. v4: - Solve vmwgfx compatibility issue in their driver, was fixed in this patch by v3. - Move only disabling primary to a separate patch. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic.c | 88 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_framebuffer.c | 7 +++ 3 files changed, 96 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index afec53832145..285e1c23e8c9 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2061,6 +2061,94 @@ static void complete_crtc_signaling(struct drm_device *dev, kfree(fence_state); } +int drm_atomic_remove_fb(struct drm_framebuffer *fb) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_device *dev = fb->dev; + struct drm_atomic_state *state; + struct drm_plane *plane; + struct drm_connector *conn; + struct drm_connector_state *conn_state; + int i, ret = 0; + unsigned plane_mask; + + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + + drm_modeset_acquire_init(&ctx, 0); + state->acquire_ctx = &ctx; + +retry: + plane_mask = 0; + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto unlock; + + drm_for_each_plane(plane, dev) { + struct drm_plane_state *plane_state; + + if (plane->state->fb != fb) + continue; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto unlock; + } + + if (plane_state->crtc->primary == plane) { + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); + + ret = drm_atomic_add_affected_connectors(state, plane_state->crtc); + if (ret) + goto unlock; + + crtc_state->active = false; + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL); + if (ret) + goto unlock; + } + + drm_atomic_set_fb_for_plane(plane_state, NULL); + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (ret) + goto unlock; + + plane_mask |= BIT(drm_plane_index(plane)); + + plane->old_fb = plane->fb; + } + + for_each_connector_in_state(state, conn, conn_state, i) { + ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); + + if (ret) + goto unlock; + } + + if (plane_mask) + ret = drm_atomic_commit(state); + +unlock: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); + + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + drm_atomic_state_put(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return ret; +} + int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 955c5690bf64..e0678f8a51cf 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -183,6 +183,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val); int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_atomic_remove_fb(struct drm_framebuffer *fb); /* drm_plane.c */ diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 28a0108a1ab8..c0e593a7f9b4 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -773,6 +773,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) * in this manner. */ if (drm_framebuffer_read_refcount(fb) > 1) { + if (drm_drv_uses_atomic_modeset(dev)) { + int ret = drm_atomic_remove_fb(fb); + WARN(ret, "atomic remove_fb failed with %i\n", ret); + goto out; + } + drm_modeset_lock_all(dev); /* remove from any CRTC */ drm_for_each_crtc(crtc, dev) { @@ -790,6 +796,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) drm_modeset_unlock_all(dev); } +out: drm_framebuffer_unreference(fb); } EXPORT_SYMBOL(drm_framebuffer_remove); -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4. 2017-02-21 13:51 ` [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4 Maarten Lankhorst @ 2017-02-23 15:09 ` Sean Paul 2017-02-28 12:03 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2017-02-23 15:09 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel, Daniel Vetter On Tue, Feb 21, 2017 at 02:51:41PM +0100, Maarten Lankhorst wrote: > Instead of trying to do everything in 1 go, just do a basic safe > conversion first. We've been bitten by too many regressions in the > past. > > This patch only converts drm_framebuffer_remove to atomic. The > regression sensitive part is split out to a separate patch. > > v2: > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > - Add WARN_ON when atomic_remove_fb fails. > - Always call drm_atomic_state_put. > v3: > - Use drm_drv_uses_atomic_modeset > - Handle the case where the first plane-disable-only commit fails > with -EINVAL. Some drivers do not support this, fall back to > disabling all crtc's in this case. > v4: > - Solve vmwgfx compatibility issue in their driver, was fixed in this > patch by v3. > - Move only disabling primary to a separate patch. > Reviewed-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 88 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc_internal.h | 1 + > drivers/gpu/drm/drm_framebuffer.c | 7 +++ > 3 files changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index afec53832145..285e1c23e8c9 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2061,6 +2061,94 @@ static void complete_crtc_signaling(struct drm_device *dev, > kfree(fence_state); > } > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb) > +{ > + struct drm_modeset_acquire_ctx ctx; > + struct drm_device *dev = fb->dev; > + struct drm_atomic_state *state; > + struct drm_plane *plane; > + struct drm_connector *conn; > + struct drm_connector_state *conn_state; > + int i, ret = 0; > + unsigned plane_mask; > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return -ENOMEM; > + > + drm_modeset_acquire_init(&ctx, 0); > + state->acquire_ctx = &ctx; > + > +retry: > + plane_mask = 0; > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > + if (ret) > + goto unlock; > + > + drm_for_each_plane(plane, dev) { > + struct drm_plane_state *plane_state; > + > + if (plane->state->fb != fb) > + continue; > + > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) { > + ret = PTR_ERR(plane_state); > + goto unlock; > + } > + > + if (plane_state->crtc->primary == plane) { > + struct drm_crtc_state *crtc_state; > + > + crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > + > + ret = drm_atomic_add_affected_connectors(state, plane_state->crtc); > + if (ret) > + goto unlock; > + > + crtc_state->active = false; > + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL); > + if (ret) > + goto unlock; > + } > + > + drm_atomic_set_fb_for_plane(plane_state, NULL); > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > + if (ret) > + goto unlock; > + > + plane_mask |= BIT(drm_plane_index(plane)); > + > + plane->old_fb = plane->fb; > + } > + > + for_each_connector_in_state(state, conn, conn_state, i) { > + ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > + > + if (ret) > + goto unlock; > + } > + > + if (plane_mask) > + ret = drm_atomic_commit(state); > + > +unlock: > + if (plane_mask) > + drm_atomic_clean_old_fb(dev, plane_mask, ret); > + > + if (ret == -EDEADLK) { > + drm_modeset_backoff(&ctx); > + goto retry; > + } > + > + drm_atomic_state_put(state); > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + return ret; > +} > + > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > index 955c5690bf64..e0678f8a51cf 100644 > --- a/drivers/gpu/drm/drm_crtc_internal.h > +++ b/drivers/gpu/drm/drm_crtc_internal.h > @@ -183,6 +183,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, > struct drm_property *property, uint64_t *val); > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv); > +int drm_atomic_remove_fb(struct drm_framebuffer *fb); > > > /* drm_plane.c */ > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 28a0108a1ab8..c0e593a7f9b4 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -773,6 +773,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > * in this manner. > */ > if (drm_framebuffer_read_refcount(fb) > 1) { > + if (drm_drv_uses_atomic_modeset(dev)) { > + int ret = drm_atomic_remove_fb(fb); > + WARN(ret, "atomic remove_fb failed with %i\n", ret); > + goto out; > + } > + > drm_modeset_lock_all(dev); > /* remove from any CRTC */ > drm_for_each_crtc(crtc, dev) { > @@ -790,6 +796,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > drm_modeset_unlock_all(dev); > } > > +out: > drm_framebuffer_unreference(fb); > } > EXPORT_SYMBOL(drm_framebuffer_remove); > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4. 2017-02-23 15:09 ` Sean Paul @ 2017-02-28 12:03 ` Daniel Vetter 0 siblings, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2017-02-28 12:03 UTC (permalink / raw) To: Sean Paul; +Cc: Daniel Vetter, intel-gfx, dri-devel, Daniel Vetter On Thu, Feb 23, 2017 at 10:09:27AM -0500, Sean Paul wrote: > On Tue, Feb 21, 2017 at 02:51:41PM +0100, Maarten Lankhorst wrote: > > Instead of trying to do everything in 1 go, just do a basic safe > > conversion first. We've been bitten by too many regressions in the > > past. > > > > This patch only converts drm_framebuffer_remove to atomic. The > > regression sensitive part is split out to a separate patch. > > > > v2: > > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > > - Add WARN_ON when atomic_remove_fb fails. > > - Always call drm_atomic_state_put. > > v3: > > - Use drm_drv_uses_atomic_modeset > > - Handle the case where the first plane-disable-only commit fails > > with -EINVAL. Some drivers do not support this, fall back to > > disabling all crtc's in this case. > > v4: > > - Solve vmwgfx compatibility issue in their driver, was fixed in this > > patch by v3. > > - Move only disabling primary to a separate patch. > > > > Reviewed-by: Sean Paul <seanpaul@chromium.org> Ok, merged the first 2 from this series to drm-misc-next. Thanks, Daniel > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 88 +++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/drm_crtc_internal.h | 1 + > > drivers/gpu/drm/drm_framebuffer.c | 7 +++ > > 3 files changed, 96 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index afec53832145..285e1c23e8c9 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -2061,6 +2061,94 @@ static void complete_crtc_signaling(struct drm_device *dev, > > kfree(fence_state); > > } > > > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb) > > +{ > > + struct drm_modeset_acquire_ctx ctx; > > + struct drm_device *dev = fb->dev; > > + struct drm_atomic_state *state; > > + struct drm_plane *plane; > > + struct drm_connector *conn; > > + struct drm_connector_state *conn_state; > > + int i, ret = 0; > > + unsigned plane_mask; > > + > > + state = drm_atomic_state_alloc(dev); > > + if (!state) > > + return -ENOMEM; > > + > > + drm_modeset_acquire_init(&ctx, 0); > > + state->acquire_ctx = &ctx; > > + > > +retry: > > + plane_mask = 0; > > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > > + if (ret) > > + goto unlock; > > + > > + drm_for_each_plane(plane, dev) { > > + struct drm_plane_state *plane_state; > > + > > + if (plane->state->fb != fb) > > + continue; > > + > > + plane_state = drm_atomic_get_plane_state(state, plane); > > + if (IS_ERR(plane_state)) { > > + ret = PTR_ERR(plane_state); > > + goto unlock; > > + } > > + > > + if (plane_state->crtc->primary == plane) { > > + struct drm_crtc_state *crtc_state; > > + > > + crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > > + > > + ret = drm_atomic_add_affected_connectors(state, plane_state->crtc); > > + if (ret) > > + goto unlock; > > + > > + crtc_state->active = false; > > + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL); > > + if (ret) > > + goto unlock; > > + } > > + > > + drm_atomic_set_fb_for_plane(plane_state, NULL); > > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > > + if (ret) > > + goto unlock; > > + > > + plane_mask |= BIT(drm_plane_index(plane)); > > + > > + plane->old_fb = plane->fb; > > + } > > + > > + for_each_connector_in_state(state, conn, conn_state, i) { > > + ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > > + > > + if (ret) > > + goto unlock; > > + } > > + > > + if (plane_mask) > > + ret = drm_atomic_commit(state); > > + > > +unlock: > > + if (plane_mask) > > + drm_atomic_clean_old_fb(dev, plane_mask, ret); > > + > > + if (ret == -EDEADLK) { > > + drm_modeset_backoff(&ctx); > > + goto retry; > > + } > > + > > + drm_atomic_state_put(state); > > + > > + drm_modeset_drop_locks(&ctx); > > + drm_modeset_acquire_fini(&ctx); > > + > > + return ret; > > +} > > + > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > void *data, struct drm_file *file_priv) > > { > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > > index 955c5690bf64..e0678f8a51cf 100644 > > --- a/drivers/gpu/drm/drm_crtc_internal.h > > +++ b/drivers/gpu/drm/drm_crtc_internal.h > > @@ -183,6 +183,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, > > struct drm_property *property, uint64_t *val); > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > void *data, struct drm_file *file_priv); > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb); > > > > > > /* drm_plane.c */ > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index 28a0108a1ab8..c0e593a7f9b4 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -773,6 +773,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > > * in this manner. > > */ > > if (drm_framebuffer_read_refcount(fb) > 1) { > > + if (drm_drv_uses_atomic_modeset(dev)) { > > + int ret = drm_atomic_remove_fb(fb); > > + WARN(ret, "atomic remove_fb failed with %i\n", ret); > > + goto out; > > + } > > + > > drm_modeset_lock_all(dev); > > /* remove from any CRTC */ > > drm_for_each_crtc(crtc, dev) { > > @@ -790,6 +796,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > > drm_modeset_unlock_all(dev); > > } > > > > +out: > > drm_framebuffer_unreference(fb); > > } > > EXPORT_SYMBOL(drm_framebuffer_remove); > > -- > > 2.7.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Sean Paul, Software Engineer, Google / Chromium OS -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb. 2017-02-21 13:51 [PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again Maarten Lankhorst [not found] ` <1487685102-31991-1-git-send-email-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 2017-02-21 13:51 ` [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4 Maarten Lankhorst @ 2017-02-21 13:51 ` Maarten Lankhorst 2017-02-23 15:24 ` Sean Paul 2017-02-21 15:23 ` ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again Patchwork 3 siblings, 1 reply; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-21 13:51 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx This introduces a slight behavioral change to rmfb. Instead of disabling a crtc when the primary plane is disabled, we try to preserve it. Apart from old versions of the vmwgfx xorg driver, there is nothing depending on rmfb disabling a crtc. Since vmwgfx is a legacy driver we can safely only disable the plane with atomic. In the conversion to atomic the driver will reject the config without primary plane. If this commit is rejected by the driver then we will still fall back to the old behavior and turn off the crtc. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 285e1c23e8c9..0d5b0065208e 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) struct drm_connector *conn; struct drm_connector_state *conn_state; int i, ret = 0; - unsigned plane_mask; + unsigned plane_mask, disable_crtcs = false; state = drm_atomic_state_alloc(dev); if (!state) @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) goto unlock; } - if (plane_state->crtc->primary == plane) { + /* + * Some drivers do not support keeping crtc active with the + * primary plane disabled. If we fail to commit with -EINVAL + * then we will try to perform the same commit but with all + * crtc's disabled for primary planes as well. + */ + if (disable_crtcs && plane_state->crtc->primary == plane) { struct drm_crtc_state *crtc_state; crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) plane->old_fb = plane->fb; } + /* This list is only not empty when disable_crtcs is set. */ for_each_connector_in_state(state, conn, conn_state, i) { ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) drm_atomic_state_put(state); + if (ret == -EINVAL && !disable_crtcs) { + disable_crtcs = true; + + state = drm_atomic_state_alloc(dev); + if (state) { + state->acquire_ctx = &ctx; + goto retry; + } + ret = -ENOMEM; + } + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb. 2017-02-21 13:51 ` [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb Maarten Lankhorst @ 2017-02-23 15:24 ` Sean Paul 2017-02-27 12:01 ` Maarten Lankhorst 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2017-02-23 15:24 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel On Tue, Feb 21, 2017 at 02:51:42PM +0100, Maarten Lankhorst wrote: > This introduces a slight behavioral change to rmfb. Instead of > disabling a crtc when the primary plane is disabled, we try to > preserve it. > > Apart from old versions of the vmwgfx xorg driver, there is > nothing depending on rmfb disabling a crtc. Since vmwgfx is > a legacy driver we can safely only disable the plane with atomic. > In the conversion to atomic the driver will reject the config > without primary plane. > > If this commit is rejected by the driver then we will still fall > back to the old behavior and turn off the crtc. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 285e1c23e8c9..0d5b0065208e 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > struct drm_connector *conn; > struct drm_connector_state *conn_state; > int i, ret = 0; > - unsigned plane_mask; > + unsigned plane_mask, disable_crtcs = false; > > state = drm_atomic_state_alloc(dev); > if (!state) > @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > goto unlock; > } > > - if (plane_state->crtc->primary == plane) { > + /* > + * Some drivers do not support keeping crtc active with the > + * primary plane disabled. If we fail to commit with -EINVAL > + * then we will try to perform the same commit but with all > + * crtc's disabled for primary planes as well. > + */ > + if (disable_crtcs && plane_state->crtc->primary == plane) { > struct drm_crtc_state *crtc_state; > > crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > plane->old_fb = plane->fb; > } > > + /* This list is only not empty when disable_crtcs is set. */ > for_each_connector_in_state(state, conn, conn_state, i) { > ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > > @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > > drm_atomic_state_put(state); > > + if (ret == -EINVAL && !disable_crtcs) { > + disable_crtcs = true; > + > + state = drm_atomic_state_alloc(dev); > + if (state) { > + state->acquire_ctx = &ctx; > + goto retry; Re-using the retry label is a little fishy here. You need to re-allocate state, but we don't drop the locks, which makes things a little unbalanced (I realize modeset_lock will s/EALREADY/0/). Perhaps something like this above would be more clear: + drm_modeset_acquire_init(&ctx, 0); +retry: + plane_mask = 0; + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto unlock; + +retry_disable: + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + state->acquire_ctx = &ctx; + Then you can do: + if (ret == -EINVAL && !disable_crtcs) { + disable_crtcs = true; + goto retry_disable; + } (you would also need to ensure you moved the state_put above the EDEADLK handling). Thoughts? Sean > + } > + ret = -ENOMEM; > + } > + > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb. 2017-02-23 15:24 ` Sean Paul @ 2017-02-27 12:01 ` Maarten Lankhorst 0 siblings, 0 replies; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-27 12:01 UTC (permalink / raw) To: Sean Paul; +Cc: intel-gfx, dri-devel Op 23-02-17 om 16:24 schreef Sean Paul: > On Tue, Feb 21, 2017 at 02:51:42PM +0100, Maarten Lankhorst wrote: >> This introduces a slight behavioral change to rmfb. Instead of >> disabling a crtc when the primary plane is disabled, we try to >> preserve it. >> >> Apart from old versions of the vmwgfx xorg driver, there is >> nothing depending on rmfb disabling a crtc. Since vmwgfx is >> a legacy driver we can safely only disable the plane with atomic. >> In the conversion to atomic the driver will reject the config >> without primary plane. >> >> If this commit is rejected by the driver then we will still fall >> back to the old behavior and turn off the crtc. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 285e1c23e8c9..0d5b0065208e 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> struct drm_connector *conn; >> struct drm_connector_state *conn_state; >> int i, ret = 0; >> - unsigned plane_mask; >> + unsigned plane_mask, disable_crtcs = false; >> >> state = drm_atomic_state_alloc(dev); >> if (!state) >> @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> goto unlock; >> } >> >> - if (plane_state->crtc->primary == plane) { >> + /* >> + * Some drivers do not support keeping crtc active with the >> + * primary plane disabled. If we fail to commit with -EINVAL >> + * then we will try to perform the same commit but with all >> + * crtc's disabled for primary planes as well. >> + */ >> + if (disable_crtcs && plane_state->crtc->primary == plane) { >> struct drm_crtc_state *crtc_state; >> >> crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); >> @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> plane->old_fb = plane->fb; >> } >> >> + /* This list is only not empty when disable_crtcs is set. */ >> for_each_connector_in_state(state, conn, conn_state, i) { >> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); >> >> @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> >> drm_atomic_state_put(state); >> >> + if (ret == -EINVAL && !disable_crtcs) { >> + disable_crtcs = true; >> + >> + state = drm_atomic_state_alloc(dev); >> + if (state) { >> + state->acquire_ctx = &ctx; >> + goto retry; > Re-using the retry label is a little fishy here. You need to re-allocate state, > but we don't drop the locks, which makes things a little unbalanced (I realize > modeset_lock will s/EALREADY/0/). drm_modeset_lock can be called any arbitrary numer of times. I'm not worried about it. :) > Perhaps something like this above would be more clear: > > + drm_modeset_acquire_init(&ctx, 0); > +retry: > + plane_mask = 0; > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > + if (ret) > + goto unlock; > + > +retry_disable: > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return -ENOMEM; > + state->acquire_ctx = &ctx; > + > > Then you can do: > > > + if (ret == -EINVAL && !disable_crtcs) { > + disable_crtcs = true; > + goto retry_disable; > + } > > (you would also need to ensure you moved the state_put above the EDEADLK > handling). > > Thoughts? In general we try to call drm_atomic_state_clear instead of drm_atomic_state_put, this would be a source of confusion for being different from other users of atomic. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again. 2017-02-21 13:51 [PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again Maarten Lankhorst ` (2 preceding siblings ...) 2017-02-21 13:51 ` [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb Maarten Lankhorst @ 2017-02-21 15:23 ` Patchwork 2017-02-22 11:48 ` Maarten Lankhorst 3 siblings, 1 reply; 12+ messages in thread From: Patchwork @ 2017-02-21 15:23 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/atomic: Make drm_framebuffer_remove atomic, again. URL : https://patchwork.freedesktop.org/series/20008/ State : failure == Summary == Series 20008v1 drm/atomic: Make drm_framebuffer_remove atomic, again. https://patchwork.freedesktop.org/api/1.0/series/20008/revisions/1/mbox/ Test gem_exec_basic: Subgroup gtt-render: incomplete -> PASS (fi-byt-j1900) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-a-frame-sequence: pass -> DMESG-FAIL (fi-snb-2520m) Subgroup nonblocking-crc-pipe-b: pass -> DMESG-FAIL (fi-snb-2520m) Subgroup nonblocking-crc-pipe-b-frame-sequence: pass -> DMESG-FAIL (fi-snb-2520m) Subgroup read-crc-pipe-a: pass -> DMESG-FAIL (fi-snb-2520m) Subgroup read-crc-pipe-a-frame-sequence: pass -> INCOMPLETE (fi-snb-2520m) fi-bdw-5557u total:253 pass:242 dwarn:0 dfail:0 fail:0 skip:11 fi-bsw-n3050 total:253 pass:214 dwarn:0 dfail:0 fail:0 skip:39 fi-bxt-j4205 total:253 pass:234 dwarn:0 dfail:0 fail:0 skip:19 fi-bxt-t5700 total:83 pass:70 dwarn:0 dfail:0 fail:0 skip:12 fi-byt-j1900 total:253 pass:226 dwarn:0 dfail:0 fail:0 skip:27 fi-byt-n2820 total:253 pass:222 dwarn:0 dfail:0 fail:0 skip:31 fi-hsw-4770 total:253 pass:237 dwarn:0 dfail:0 fail:0 skip:16 fi-hsw-4770r total:253 pass:237 dwarn:0 dfail:0 fail:0 skip:16 fi-ilk-650 total:253 pass:203 dwarn:0 dfail:0 fail:0 skip:50 fi-ivb-3520m total:253 pass:235 dwarn:0 dfail:0 fail:0 skip:18 fi-ivb-3770 total:253 pass:235 dwarn:0 dfail:0 fail:0 skip:18 fi-kbl-7500u total:253 pass:235 dwarn:0 dfail:0 fail:0 skip:18 fi-skl-6260u total:253 pass:243 dwarn:0 dfail:0 fail:0 skip:10 fi-skl-6700hq total:253 pass:236 dwarn:0 dfail:0 fail:0 skip:17 fi-skl-6700k total:253 pass:231 dwarn:4 dfail:0 fail:0 skip:18 fi-skl-6770hq total:253 pass:243 dwarn:0 dfail:0 fail:0 skip:10 fi-snb-2520m total:205 pass:179 dwarn:0 dfail:4 fail:0 skip:21 fi-snb-2600 total:253 pass:224 dwarn:0 dfail:0 fail:0 skip:29 3d47f9476bdc68afdb73ddbc96375366ea689a79 drm-tip: 2017y-02m-21d-14h-13m-06s UTC integration manifest b55a0ee drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb. fad2fa6 drm: Convert drm_framebuffer_remove to atomic, v4. ee2d848 drm/atomic: Make disable_all helper fully disable the crtc. == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3913/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again. 2017-02-21 15:23 ` ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again Patchwork @ 2017-02-22 11:48 ` Maarten Lankhorst 0 siblings, 0 replies; 12+ messages in thread From: Maarten Lankhorst @ 2017-02-22 11:48 UTC (permalink / raw) To: intel-gfx, dri-devel Op 21-02-17 om 16:23 schreef Patchwork: > == Series Details == > > Series: drm/atomic: Make drm_framebuffer_remove atomic, again. > URL : https://patchwork.freedesktop.org/series/20008/ > State : failure > > == Summary == > > Series 20008v1 drm/atomic: Make drm_framebuffer_remove atomic, again. > https://patchwork.freedesktop.org/api/1.0/series/20008/revisions/1/mbox/ > > Test gem_exec_basic: > Subgroup gtt-render: > incomplete -> PASS (fi-byt-j1900) > Test kms_pipe_crc_basic: > Subgroup nonblocking-crc-pipe-a-frame-sequence: > pass -> DMESG-FAIL (fi-snb-2520m) > Subgroup nonblocking-crc-pipe-b: > pass -> DMESG-FAIL (fi-snb-2520m) > Subgroup nonblocking-crc-pipe-b-frame-sequence: > pass -> DMESG-FAIL (fi-snb-2520m) > Subgroup read-crc-pipe-a: > pass -> DMESG-FAIL (fi-snb-2520m) > Subgroup read-crc-pipe-a-frame-sequence: > pass -> INCOMPLETE (fi-snb-2520m) Sigh, last patch regresses on the crc tests again. snb-2600 is fine so something specific to the configuration there.. https://patchwork.freedesktop.org/series/20012/ are the ci results for this series without the last patch, so please apply patch 1 and 2 only? ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-02-28 12:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-21 13:51 [PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again Maarten Lankhorst [not found] ` <1487685102-31991-1-git-send-email-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 2017-02-21 13:51 ` [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc Maarten Lankhorst 2017-02-23 15:03 ` Sean Paul 2017-02-23 20:10 ` Maarten Lankhorst 2017-02-21 13:51 ` [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4 Maarten Lankhorst 2017-02-23 15:09 ` Sean Paul 2017-02-28 12:03 ` Daniel Vetter 2017-02-21 13:51 ` [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb Maarten Lankhorst 2017-02-23 15:24 ` Sean Paul 2017-02-27 12:01 ` Maarten Lankhorst 2017-02-21 15:23 ` ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again Patchwork 2017-02-22 11:48 ` Maarten Lankhorst
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.