Re: [git pull] drm fixes

[Linus Torvalds]

On Sun, Mar 1, 2015 at 1:00 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Back to the drawing board.

Ok, many hours later, but I found it.

The bisection was a disaster, having to work around other bugs in this
area, but it ended up getting "close enough" that I figured out what
went wrong.

The "intel_plane_duplicate_state()" is horribly horribly buggy. It
looks at the state->fb pointer, but it may have been free'd already.

This workaround "works for me", but it's really still very
questionable, because while the "kref_get_unless_zero()" works
correctly when the last reference has been dropped, I'm not sure that
there is any guarantee that the whole allocation even exists any more,
so I think the *correct* thing to do would be to clear state->fb when
dropping the kref. But this was the smallest working patch I could
come up with. Somebody who actually knows the code should start
looking at the places that do drm_framebuffer_unreference(), and
actually clear that pointer instead.

Added Matt Roper and Ander Conselvan de Oliveira to the discussion,
since they are the ones git says are involved with the original broken
intel_plane_duplicate_state().

Anyway, attached is

 (a) the patch with a big comment

 (b) the warnings I get on that machine that show where this problem
triggers (and another warning earlier).

Comments? I'm sure this probably only triggers with *old* X servers
that don't do all the modern dri stuff.

                               Linus
From c182b15c3abee75cdc9d9564b6ab826403690f4e Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxxx>
Date: Sat, 28 Feb 2015 21:44:48 -0800
Subject: [PATCH] Workaround for drm bug

Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 9e6f727..72714d3 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -85,8 +85,23 @@ intel_plane_duplicate_state(struct drm_plane *plane)
 		return NULL;
 
 	state = &intel_state->base;
-	if (state->fb)
-		drm_framebuffer_reference(state->fb);
+
+	/*
+	 * We cannot do drm_framebuffer_reference(), because the reference
+	 * may already have been dropped.
+	 *
+	 * So we do what drm_framebuffer_lookup() does, namely do a
+	 * kref_get_unless_zero(). Even that is somewhat questionable,
+	 * in that maybe the 'fb' already got free'd. So warn loudly
+	 * about this.
+	 *
+	 * Maybe the base.fb should be cleared by whatever drops the
+	 * reference?
+	 */
+	if (state->fb && !kref_get_unless_zero(&state->fb->refcount)) {
+		state->fb = NULL;
+		WARN_ONCE(1, "intel_plane_duplicate_state got plane with dead frame buffer");
+	}
 
 	return state;
 }
-- 
2.3.1.167.g7f4ba4b

Attachment: drm-bug-dmesg
Description: Binary data