The legacy plane->fb pointer is refcounted by calling drm_atomic_clean_old_fb(). In practice this isn't a real problem because: - The caller in the i915 gpu reset code restores the original state again, which means the plane->fb pointer won't change, hence can't leak. - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup first, and that usually cleans up the fb through drm_remove_framebuffer, which does this correctly. - Without fbdev the only framebuffers are from userspace, and those get cleaned up (again using drm_remove_framebuffer) befor the driver can even be unloaded. But in i915 I've switched the cleanup sequence around so that the _shutdown() calls happens after the drm_remove_framebuffer(), which is how I discovered this issue. v2: My analysis why the current code was ok for gpu reset and suspend/resume was correct, but then I totally failed to realize that we better keep this symmetric. Thanksfully CI noticed that for balance, a refcounting bug must exist at 2 places if previously there was no issue ... Cc: martin.peres@xxxxxxx Cc: chris@xxxxxxxxxxxxxxxxxx Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> --- drivers/gpu/drm/drm_atomic_helper.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b07fc30372d3..3a04619d3bec 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2726,6 +2726,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; + unsigned plane_mask = 0; int ret, i; state = drm_atomic_state_alloc(dev); @@ -2768,10 +2769,14 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, goto free; drm_atomic_set_fb_for_plane(plane_state, NULL); + plane_mask |= BIT(drm_plane_index(plane)); + plane->old_fb = plane->fb; } ret = drm_atomic_commit(state); free: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); drm_atomic_state_put(state); return ret; } @@ -2902,6 +2907,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state; + struct drm_device *dev = state->dev; + int ret; state->acquire_ctx = ctx; @@ -2914,7 +2921,10 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, for_each_new_connector_in_state(state, connector, new_conn_state, i) state->connectors[i].old_state = connector->state; - return drm_atomic_commit(state); + ret = drm_atomic_commit(state); + drm_atomic_clean_old_fb(dev, ~0U, ret); + + return ret; } EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); -- 2.13.2 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
