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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 1/3] drm/atomic-helper: Fix leak in disable_all
Date: Fri, 14 Jul 2017 20:27:32 +0100	[thread overview]
Message-ID: <150006045243.13756.3660978119602117566@mail.alporthouse.com> (raw)
In-Reply-To: <20170714191439.31169-1-daniel.vetter@ffwll.ch>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel