drm/fbdev: Update legacy plane->fb refcounting for atomic restore
diff mbox

Message ID 1442919402-4451-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Sept. 22, 2015, 10:56 a.m. UTC
From: Matt Roper <matthew.d.roper@intel.com>

Starting with commit

        commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
        Author: Rob Clark <robdclark@gmail.com>
        Date:   Tue Aug 25 15:36:00 2015 -0400

            drm/i915: enable atomic fb-helper

I've been seeing some panics on i915 when the DRM master shuts down that appear
to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
dropping our initial FB's reference count to 0 and freeing it, which causes a
crash when we try to restore it later).  Digging deeper, the state FB
refcounting is working as expected, but we seem to be missing proper
refcounting on the legacy plane->fb pointers in the new atomic fbdev code.

Tracking plane->old_fb and then doing a ref/unref at the end of the
fbdev restore like we do in the legacy ioctl's ensures we don't miscount
references on plane->fb and avoids the panics.

v2 from Daniel:

Really do what the atomic ioctl does:
- Also update plane->fb and plane->crtc.
- Clear out plane->old_fb on failures too.

v3: git add everything. Oops.

Cc: Rob Clark <robdclark@gmail.com>
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

David Herrmann Sept. 22, 2015, 11:02 a.m. UTC | #1
Hi

On Tue, Sep 22, 2015 at 12:56 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> From: Matt Roper <matthew.d.roper@intel.com>
>
> Starting with commit
>
>         commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
>         Author: Rob Clark <robdclark@gmail.com>
>         Date:   Tue Aug 25 15:36:00 2015 -0400
>
>             drm/i915: enable atomic fb-helper
>
> I've been seeing some panics on i915 when the DRM master shuts down that appear
> to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
> dropping our initial FB's reference count to 0 and freeing it, which causes a
> crash when we try to restore it later).  Digging deeper, the state FB
> refcounting is working as expected, but we seem to be missing proper
> refcounting on the legacy plane->fb pointers in the new atomic fbdev code.
>
> Tracking plane->old_fb and then doing a ref/unref at the end of the
> fbdev restore like we do in the legacy ioctl's ensures we don't miscount
> references on plane->fb and avoids the panics.
>
> v2 from Daniel:
>
> Really do what the atomic ioctl does:
> - Also update plane->fb and plane->crtc.
> - Clear out plane->old_fb on failures too.
>
> v3: git add everything. Oops.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64fc5ca8fda2..8af522afdfc1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -352,6 +352,8 @@ retry:
>         drm_for_each_plane(plane, dev) {
>                 struct drm_plane_state *plane_state;
>
> +               plane->old_fb = plane->fb;
> +
>                 plane_state = drm_atomic_get_plane_state(state, plane);
>                 if (IS_ERR(plane_state)) {
>                         ret = PTR_ERR(plane_state);
> @@ -382,6 +384,21 @@ retry:
>         }
>
>         ret = drm_atomic_commit(state);
> +
> +       drm_for_each_plane(plane, dev) {
> +               if (ret == 0) {
> +                       struct drm_framebuffer *new_fb = plane->state->fb;
> +                       if (new_fb)
> +                               drm_framebuffer_reference(new_fb);
> +                       plane->fb = new_fb;
> +                       plane->crtc = plane->state->crtc;
> +
> +                       if (plane->old_fb)
> +                               drm_framebuffer_unreference(plane->old_fb);
> +               }
> +               plane->old_fb = NULL;

You still leak "old_fb" if something jumps to "fail:" before
drm_atomic_commit() is called. But I don't mind:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> +       }
> +
>         if (ret != 0)
>                 goto fail;
>
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 64fc5ca8fda2..8af522afdfc1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -352,6 +352,8 @@  retry:
 	drm_for_each_plane(plane, dev) {
 		struct drm_plane_state *plane_state;
 
+		plane->old_fb = plane->fb;
+
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
 			ret = PTR_ERR(plane_state);
@@ -382,6 +384,21 @@  retry:
 	}
 
 	ret = drm_atomic_commit(state);
+
+	drm_for_each_plane(plane, dev) {
+		if (ret == 0) {
+			struct drm_framebuffer *new_fb = plane->state->fb;
+			if (new_fb)
+				drm_framebuffer_reference(new_fb);
+			plane->fb = new_fb;
+			plane->crtc = plane->state->crtc;
+
+			if (plane->old_fb)
+				drm_framebuffer_unreference(plane->old_fb);
+		}
+		plane->old_fb = NULL;
+	}
+
 	if (ret != 0)
 		goto fail;