From 49d70aeaeca8f62b72b7712ecd1e29619a445866 Mon Sep 17 00:00:00 2001
From: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Sat, 15 Jul 2017 11:31:06 +0200
Subject: [PATCH] drm/atomic-helper: Fix leak in disable_all
Git-commit: 49d70aeaeca8f62b72b7712ecd1e29619a445866
Patch-mainline: v4.14-rc1
References: FATE#322643 bsc#1055900
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 ...
V3: Don't be lazy and compute the plane_mask in
commit_duplicated_state properly too, instead of just using ~0U.
Cc: martin.peres@free.fr
Cc: chris@chris-wilson.co.uk
Acked-by: Dave Airlie <airlied@gmail.com> (v1)
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20170715093106.19873-1-daniel.vetter@ffwll.ch
Acked-by: Takashi Iwai <tiwai@suse.de>
---
drivers/gpu/drm/drm_atomic_helper.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2715,6 +2715,7 @@ int drm_atomic_helper_disable_all(struct
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);
@@ -2757,10 +2758,14 @@ int drm_atomic_helper_disable_all(struct
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;
}
@@ -2891,11 +2896,16 @@ int drm_atomic_helper_commit_duplicated_
struct drm_connector_state *new_conn_state;
struct drm_crtc *crtc;
struct drm_crtc_state *new_crtc_state;
+ unsigned plane_mask = 0;
+ struct drm_device *dev = state->dev;
+ int ret;
state->acquire_ctx = ctx;
- for_each_new_plane_in_state(state, plane, new_plane_state, i)
+ for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+ plane_mask |= BIT(drm_plane_index(plane));
state->planes[i].old_state = plane->state;
+ }
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
state->crtcs[i].old_state = crtc->state;
@@ -2903,7 +2913,11 @@ int drm_atomic_helper_commit_duplicated_
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);
+ if (plane_mask)
+ drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
+ return ret;
}
EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);