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

Message ID 20170714191439.31169-1-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 14, 2017, 7:14 p.m. UTC
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.

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 | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Chris Wilson July 14, 2017, 7:27 p.m. UTC | #1
Quoting Daniel Vetter (2017-07-14 20:14:37)
> 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.

If only we have refcounted fb now and didn't need every caller manually
tracking the old pointers. </s>

Given the requirement for the caller to cleanup old_fb around
drm_atomic_commit(), this looks correct to me.

> Cc: martin.peres@free.fr
> Cc: chris@chris-wilson.co.uk
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b07fc30372d3..71b5f71e61e0 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;
 }