[PATCH 1/3] drm/atomic-helper: Fix leak in disable_all

* [PATCH 1/3] drm/atomic-helper: Fix leak in disable_all
@ 2017-07-14 22:46 Daniel Vetter
  2017-07-14 22:46 ` [PATCH 2/3] drm/i915: Fix fbdev unload sequence Daniel Vetter
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Daniel Vetter @ 2017-07-14 22:46 UTC (permalink / raw)
  To: DRI Development
  Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development, martin.peres

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@free.fr
Cc: chris@chris-wilson.co.uk
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 11+ messages in thread