[PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We had two failure modes here:

1.
Deadlock in intelfb_alloc failure path where it calls
drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
(caller of intelfb_alloc) was already holding it.

2.
Deadlock in intelfb_create failure path where it calls
drm_framebuffer_unreference, which grabs the struct mutex and
intelfb_create was already holding it.

v2:
   * Reformat commit msg to 72 chars. (Lukas Wunner)
   * Add third failure mode. (Lukas Wunner)

v3:
   * On fb alloc failure, unref gem object where it gets refed,
     fix double unref in separate commit. (Ville Syrjälä)

v4:
   * Lock struct mutex on unref. (Chris Wilson)

v5:
   * Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
     rephrase commit message. (Jani Nicula)

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina]

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 60a5ca015ffd ("drm/i915: Add locking around
    framebuffer_references--")
Reported-by: Lukas Wunner <lukas@wunner.de>
[Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 96476d7..eee3306 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
-	struct drm_framebuffer *fb;
+	struct drm_framebuffer *fb = NULL;
 	struct drm_device *dev = helper->dev;
 	struct drm_mode_fb_cmd2 mode_cmd = {};
 	struct drm_i915_gem_object *obj;
@@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
 
+	mutex_lock(&dev->struct_mutex);
+
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
 	obj = i915_gem_object_create_stolen(dev, size);
@@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
-		goto out_fb;
+		goto out_unref;
 	}
 
+	mutex_unlock(&dev->struct_mutex);
+
 	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-out_fb:
-	drm_framebuffer_remove(fb);
 out_unref:
 	drm_gem_object_unreference(&obj->base);
 out:
+	mutex_unlock(&dev->struct_mutex);
+	if (fb)
+		drm_framebuffer_remove(fb);
 	return ret;
 }
 
@@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	int size, ret;
 	bool prealloc = false;
 
-	mutex_lock(&dev->struct_mutex);
-
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
 	     sizes->fb_height > intel_fb->base.height)) {
@@ -203,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
-			goto out_unlock;
+			return ret;
 		intel_fb = ifbdev->fb;
 	} else {
 		DRM_DEBUG_KMS("re-using BIOS fb\n");
@@ -215,6 +218,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	mutex_lock(&dev->struct_mutex);
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -276,7 +281,6 @@ out_destroy_fbi:
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
-out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
-- 
2.1.0

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

[PATCH v6 3/4] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We had two failure modes here:

1.
Deadlock in intelfb_alloc failure path where it calls
drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
(caller of intelfb_alloc) was already holding it.

2.
Deadlock in intelfb_create failure path where it calls
drm_framebuffer_unreference, which grabs the struct mutex and
intelfb_create was already holding it.

[Chris Wilson on why struct_mutex needs to be locked in the second half
of intelfb_create: "there is a bug here where we don't take an explicit
pin on the VMA we setup for the fbdev which requires the lock."]

v2:
   * Reformat commit msg to 72 chars. (Lukas Wunner)
   * Add third failure mode. (Lukas Wunner)

v5:
   * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
     rephrase commit message. (Jani Nicula)

v6:
   * In intelfb_alloc, if __intel_framebuffer_create failed,
     fb will be an ERR_PTR, thus not null. So in the failure
     path we need to check for IS_ERR_OR_NULL to avoid calling
     drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner)
   * Since this is init code a drm_framebuffer_unreference should
     be all we need. drm_framebuffer_remove is for framebuffers
     that userspace has created - and is getting somewhat
     defeatured. (Daniel Vetter)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 60a5ca015ffd ("drm/i915: Add locking around
    framebuffer_references--")
Reported-by: Lukas Wunner <lukas@wunner.de>
[Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index ec82b51..12597b5 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
-	struct drm_framebuffer *fb;
+	struct drm_framebuffer *fb = NULL;
 	struct drm_device *dev = helper->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_mode_fb_cmd2 mode_cmd = {};
@@ -138,6 +138,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
 
+	mutex_lock(&dev->struct_mutex);
+
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
 
@@ -165,16 +167,19 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
-		goto out_fb;
+		goto out;
 	}
 
+	mutex_unlock(&dev->struct_mutex);
+
 	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-out_fb:
-	drm_framebuffer_remove(fb);
 out:
+	mutex_unlock(&dev->struct_mutex);
+	if (!IS_ERR_OR_NULL(fb))
+		drm_framebuffer_unreference(fb);
 	return ret;
 }
 
@@ -192,8 +197,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	int size, ret;
 	bool prealloc = false;
 
-	mutex_lock(&dev->struct_mutex);
-
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
 	     sizes->fb_height > intel_fb->base.height)) {
@@ -208,7 +211,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
-			goto out_unlock;
+			return ret;
 		intel_fb = ifbdev->fb;
 	} else {
 		DRM_DEBUG_KMS("re-using BIOS fb\n");
@@ -220,6 +223,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	mutex_lock(&dev->struct_mutex);
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -281,7 +286,6 @@ out_destroy_fbi:
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
-out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v7 3/3] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We had two failure modes here:

1.
Deadlock in intelfb_alloc failure path where it calls
drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
(caller of intelfb_alloc) was already holding it.

2.
Deadlock in intelfb_create failure path where it calls
drm_framebuffer_unreference, which grabs the struct mutex and
intelfb_create was already holding it.

[Daniel Vetter on why struct_mutex needs to be locked in the second half
of intelfb_create: "The vma [for the fbdev] is pinned, the problem is
that we re-lookup it a few times, which is racy. We should instead track
the vma directly, but oh well we don't."]

v2:
   * Reformat commit msg to 72 chars. (Lukas Wunner)
   * Add third failure mode. (Lukas Wunner)

v5:
   * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
     rephrase commit message. (Jani Nicula)

v6:
   * In intelfb_alloc, if __intel_framebuffer_create failed,
     fb will be an ERR_PTR, thus not null. So in the failure
     path we need to check for IS_ERR_OR_NULL to avoid calling
     drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner)
   * Since this is init code a drm_framebuffer_unreference should
     be all we need. drm_framebuffer_remove is for framebuffers
     that userspace has created - and is getting somewhat
     defeatured. (Daniel Vetter)

v7:
   * Clarify why struct_mutex needs to be locked in the second half
     of intelfb_create. (Daniel Vetter)

Fixes: 60a5ca015ffd ("drm/i915: Add locking around
    framebuffer_references--")
Reported-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
[Lukas: Create v3 + v4 + v5 + v6 + v7 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 2240e70..db82ad4 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
-	struct drm_framebuffer *fb;
+	struct drm_framebuffer *fb = NULL;
 	struct drm_device *dev = helper->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_mode_fb_cmd2 mode_cmd = {};
@@ -138,6 +138,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
 
+	mutex_lock(&dev->struct_mutex);
+
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
 
@@ -165,16 +167,19 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
-		goto out_fb;
+		goto out;
 	}
 
+	mutex_unlock(&dev->struct_mutex);
+
 	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-out_fb:
-	drm_framebuffer_remove(fb);
 out:
+	mutex_unlock(&dev->struct_mutex);
+	if (!IS_ERR_OR_NULL(fb))
+		drm_framebuffer_unreference(fb);
 	return ret;
 }
 
@@ -192,8 +197,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	int size, ret;
 	bool prealloc = false;
 
-	mutex_lock(&dev->struct_mutex);
-
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
 	     sizes->fb_height > intel_fb->base.height)) {
@@ -208,7 +211,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
-			goto out_unlock;
+			return ret;
 		intel_fb = ifbdev->fb;
 	} else {
 		DRM_DEBUG_KMS("re-using BIOS fb\n");
@@ -220,6 +223,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	mutex_lock(&dev->struct_mutex);
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -281,7 +286,6 @@ out_destroy_fbi:
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
-out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v5 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed

[Lukas Wunner]

Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call chain in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).

This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).

As suggested by Ville Syrjälä, improve code clarity by moving the unref
away from __intel_framebuffer_create to where the gem object gets refed.

[Changelog is in preceding patch by Tvrtko Ursulin.]

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
    allocation")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 421ac15..68d051d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10033,20 +10033,17 @@ __intel_framebuffer_create(struct drm_device *dev,
 	int ret;
 
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference(&obj->base);
+	if (!intel_fb)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
 	if (ret)
 		goto err;
 
 	return &intel_fb->base;
+
 err:
-	drm_gem_object_unreference(&obj->base);
 	kfree(intel_fb);
-
 	return ERR_PTR(ret);
 }
 
@@ -10086,6 +10083,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 				  struct drm_display_mode *mode,
 				  int depth, int bpp)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 
@@ -10100,7 +10098,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 								bpp);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
-	return intel_framebuffer_create(dev, &mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 static struct drm_framebuffer *
@@ -14289,6 +14291,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
 			      struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -14296,7 +14299,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	return intel_framebuffer_create(dev, mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 #ifndef CONFIG_DRM_FBDEV_EMULATION
-- 
2.1.0

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

[PATCH v6 1/4] drm/i915: On fb alloc failure, unref gem object where it gets refed

[Lukas Wunner]

Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call stack in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).

This invites mistakes: __intel_framebuffer_create is also called from
intelfb_alloc, and as discovered by Tvrtko Ursulin, a double unref
was introduced there with a8bb6818270c ("drm/i915: Fix error path leak
in fbdev fb allocation").

As suggested by Ville Syrjälä, fix the double unref and improve code
clarity by moving the unref away from __intel_framebuffer_create to
where the gem object gets refed.

Based on Tvrtko Ursulin's original v2.

v3: On fb alloc failure, unref gem object where it gets refed,
    fix double unref in separate commit (Ville Syrjälä)

v4: Lock struct_mutex on unref (Chris Wilson)

v5: Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
    rephrase commit message (Jani Nicula)

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina]

Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
    allocation")
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3390fcc..4e02e5f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10093,20 +10093,17 @@ __intel_framebuffer_create(struct drm_device *dev,
 	int ret;
 
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference(&obj->base);
+	if (!intel_fb)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
 	if (ret)
 		goto err;
 
 	return &intel_fb->base;
+
 err:
-	drm_gem_object_unreference(&obj->base);
 	kfree(intel_fb);
-
 	return ERR_PTR(ret);
 }
 
@@ -10146,6 +10143,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 				  struct drm_display_mode *mode,
 				  int depth, int bpp)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 
@@ -10160,7 +10158,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 								bpp);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
-	return intel_framebuffer_create(dev, &mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 static struct drm_framebuffer *
@@ -14459,6 +14461,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
 			      struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -14466,7 +14469,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	return intel_framebuffer_create(dev, mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 #ifndef CONFIG_DRM_FBDEV_EMULATION
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v7 1/3] drm/i915: On fb alloc failure, unref gem object where it gets refed

[Lukas Wunner]

Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call stack in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).

This invites mistakes: __intel_framebuffer_create is also called from
intelfb_alloc, and as discovered by Tvrtko Ursulin, a double unref
was introduced there with a8bb6818270c ("drm/i915: Fix error path leak
in fbdev fb allocation").

As suggested by Ville Syrjälä, fix the double unref and improve code
clarity by moving the unref away from __intel_framebuffer_create to
where the gem object gets refed.

Based on Tvrtko Ursulin's original v2.

v3: On fb alloc failure, unref gem object where it gets refed,
    fix double unref in separate commit (Ville Syrjälä)

v4: Lock struct_mutex on unref (Chris Wilson)

v5: Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
    rephrase commit message (Jani Nicula)

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina]

Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
    allocation")
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0811238..7fc6d9e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10078,20 +10078,17 @@ __intel_framebuffer_create(struct drm_device *dev,
 	int ret;
 
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference(&obj->base);
+	if (!intel_fb)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
 	if (ret)
 		goto err;
 
 	return &intel_fb->base;
+
 err:
-	drm_gem_object_unreference(&obj->base);
 	kfree(intel_fb);
-
 	return ERR_PTR(ret);
 }
 
@@ -10131,6 +10128,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 				  struct drm_display_mode *mode,
 				  int depth, int bpp)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 
@@ -10145,7 +10143,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 								bpp);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
-	return intel_framebuffer_create(dev, &mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 static struct drm_framebuffer *
@@ -14548,6 +14550,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
 			      struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -14555,7 +14558,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	return intel_framebuffer_create(dev, mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 #ifndef CONFIG_DRM_FBDEV_EMULATION
-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Ville Syrjälä]

On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We had two failure modes here:
> 
> 1.
> Deadlock in intelfb_alloc failure path where it calls
> drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> (caller of intelfb_alloc) was already holding it.
> 
> 2.
> Deadlock in intelfb_create failure path where it calls
> drm_framebuffer_unreference, which grabs the struct mutex and
> intelfb_create was already holding it.
> 
> v2:
>    * Reformat commit msg to 72 chars. (Lukas Wunner)
>    * Add third failure mode. (Lukas Wunner)
> 
> v3:
>    * On fb alloc failure, unref gem object where it gets refed,
>      fix double unref in separate commit. (Ville Syrjälä)
> 
> v4:
>    * Lock struct mutex on unref. (Chris Wilson)
> 
> v5:
>    * Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
>      rephrase commit message. (Jani Nicula)
> 
> Tested-by: Pierre Moreau <pierre.morrow@free.fr>
>     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 60a5ca015ffd ("drm/i915: Add locking around
>     framebuffer_references--")
> Reported-by: Lukas Wunner <lukas@wunner.de>
> [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 96476d7..eee3306 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> -	struct drm_framebuffer *fb;
> +	struct drm_framebuffer *fb = NULL;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
>  	struct drm_i915_gem_object *obj;
> @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>  							  sizes->surface_depth);
>  
> +	mutex_lock(&dev->struct_mutex);
> +
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = PAGE_ALIGN(size);
>  	obj = i915_gem_object_create_stolen(dev, size);
> @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
> -		goto out_fb;
> +		goto out_unref;
>  	}
>  
> +	mutex_unlock(&dev->struct_mutex);
> +
>  	ifbdev->fb = to_intel_framebuffer(fb);
>  
>  	return 0;
>  
> -out_fb:
> -	drm_framebuffer_remove(fb);
>  out_unref:
>  	drm_gem_object_unreference(&obj->base);

If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
will now attempt to unref one too many times.

This taking over refs stuff is confusing. Maybe it would be better if
everyone just took an extra ref when they stash the obj pointer
somewhere, and everyone would then always release whatever ref they own
and no longer need.

>  out:
> +	mutex_unlock(&dev->struct_mutex);
> +	if (fb)
> +		drm_framebuffer_remove(fb);
>  	return ret;
>  }
>  
> @@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	int size, ret;
>  	bool prealloc = false;
>  
> -	mutex_lock(&dev->struct_mutex);
> -
>  	if (intel_fb &&
>  	    (sizes->fb_width > intel_fb->base.width ||
>  	     sizes->fb_height > intel_fb->base.height)) {
> @@ -203,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>  		ret = intelfb_alloc(helper, sizes);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  		intel_fb = ifbdev->fb;
>  	} else {
>  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> @@ -215,6 +218,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	mutex_lock(&dev->struct_mutex);
> +

I'm thinking we won't even need the lock here anymore. But maybe I'm
missing something.

>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -276,7 +281,6 @@ out_destroy_fbi:
>  out_unpin:
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);

And this ref we don't own either AFAICS.

> -out_unlock:
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed

[Ville Syrjälä]

On Sat, Jul 04, 2015 at 11:50:58AM +0200, Lukas Wunner wrote:
> Currently when allocating a framebuffer fails, the gem object gets
> unrefed at the bottom of the call chain in __intel_framebuffer_create,
> not where it gets refed, which is in intel_framebuffer_create_for_mode
> (via i915_gem_alloc_object) and in intel_user_framebuffer_create
> (via drm_gem_object_lookup).
> 
> This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
> has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
> 
> As suggested by Ville Syrjälä, improve code clarity by moving the unref
> away from __intel_framebuffer_create to where the gem object gets refed.
> 
> [Changelog is in preceding patch by Tvrtko Ursulin.]
> 
> Tested-by: Pierre Moreau <pierre.morrow@free.fr>
>     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
>     allocation")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 421ac15..68d051d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10033,20 +10033,17 @@ __intel_framebuffer_create(struct drm_device *dev,
>  	int ret;
>  
>  	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> -	if (!intel_fb) {
> -		drm_gem_object_unreference(&obj->base);
> +	if (!intel_fb)
>  		return ERR_PTR(-ENOMEM);
> -	}
>  
>  	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
>  	if (ret)
>  		goto err;
>  
>  	return &intel_fb->base;
> +
>  err:
> -	drm_gem_object_unreference(&obj->base);
>  	kfree(intel_fb);
> -
>  	return ERR_PTR(ret);
>  }
>  
> @@ -10086,6 +10083,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  				  struct drm_display_mode *mode,
>  				  int depth, int bpp)
>  {
> +	struct drm_framebuffer *fb;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>  
> @@ -10100,7 +10098,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  								bpp);
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>  
> -	return intel_framebuffer_create(dev, &mode_cmd, obj);
> +	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> +	if (IS_ERR(fb))
> +		drm_gem_object_unreference_unlocked(&obj->base);
> +
> +	return fb;
>  }
>  
>  static struct drm_framebuffer *
> @@ -14289,6 +14291,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
>  			      struct drm_file *filp,
>  			      struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> +	struct drm_framebuffer *fb;
>  	struct drm_i915_gem_object *obj;
>  
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
> @@ -14296,7 +14299,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
>  	if (&obj->base == NULL)
>  		return ERR_PTR(-ENOENT);
>  
> -	return intel_framebuffer_create(dev, mode_cmd, obj);
> +	fb = intel_framebuffer_create(dev, mode_cmd, obj);
> +	if (IS_ERR(fb))
> +		drm_gem_object_unreference_unlocked(&obj->base);
> +
> +	return fb;

This one looks good to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  #ifndef CONFIG_DRM_FBDEV_EMULATION
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Chris Wilson]

On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> > +	mutex_lock(&dev->struct_mutex);
> > +
> 
> I'm thinking we won't even need the lock here anymore. But maybe I'm
> missing something.

Yeah, not so much here atm, but there is a bug here where we don't take an
explicit pin on the VMA we setup for the fbdev which requires the lock.
-chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

Hi Ville,

On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > We had two failure modes here:
> > 
> > 1.
> > Deadlock in intelfb_alloc failure path where it calls
> > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > (caller of intelfb_alloc) was already holding it.
> > 
> > 2.
> > Deadlock in intelfb_create failure path where it calls
> > drm_framebuffer_unreference, which grabs the struct mutex and
> > intelfb_create was already holding it.
> > 
> > v2:
> >    * Reformat commit msg to 72 chars. (Lukas Wunner)
> >    * Add third failure mode. (Lukas Wunner)
> > 
> > v3:
> >    * On fb alloc failure, unref gem object where it gets refed,
> >      fix double unref in separate commit. (Ville Syrjälä)
> > 
> > v4:
> >    * Lock struct mutex on unref. (Chris Wilson)
> > 
> > v5:
> >    * Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> >      rephrase commit message. (Jani Nicula)
> > 
> > Tested-by: Pierre Moreau <pierre.morrow@free.fr>
> >     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
> >     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > Tested-by: William Brown <william@blackhats.net.au>
> >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> > Tested-by: Lukas Wunner <lukas@wunner.de>
> >     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> >     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> >     framebuffer_references--")
> > Reported-by: Lukas Wunner <lukas@wunner.de>
> > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 96476d7..eee3306 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  {
> >  	struct intel_fbdev *ifbdev =
> >  		container_of(helper, struct intel_fbdev, helper);
> > -	struct drm_framebuffer *fb;
> > +	struct drm_framebuffer *fb = NULL;
> >  	struct drm_device *dev = helper->dev;
> >  	struct drm_mode_fb_cmd2 mode_cmd = {};
> >  	struct drm_i915_gem_object *obj;
> > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> >  							  sizes->surface_depth);
> >  
> > +	mutex_lock(&dev->struct_mutex);
> > +
> >  	size = mode_cmd.pitches[0] * mode_cmd.height;
> >  	size = PAGE_ALIGN(size);
> >  	obj = i915_gem_object_create_stolen(dev, size);
> > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> >  	if (ret) {
> >  		DRM_ERROR("failed to pin obj: %d\n", ret);
> > -		goto out_fb;
> > +		goto out_unref;
> >  	}
> >  
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> >  	ifbdev->fb = to_intel_framebuffer(fb);
> >  
> >  	return 0;
> >  
> > -out_fb:
> > -	drm_framebuffer_remove(fb);
> >  out_unref:
> >  	drm_gem_object_unreference(&obj->base);
> 
> If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> will now attempt to unref one too many times.
> 
> This taking over refs stuff is confusing. Maybe it would be better if
> everyone just took an extra ref when they stash the obj pointer
> somewhere, and everyone would then always release whatever ref they own
> and no longer need.
> 
> >  out:
> > +	mutex_unlock(&dev->struct_mutex);
> > +	if (fb)
> > +		drm_framebuffer_remove(fb);
> >  	return ret;
> >  }
> >  

Hm, why do you think we unref one too many times?

A bit further up in this function we call __intel_framebuffer_create()
which sets the refcount to 1. (It calls intel_framebuffer_init(), which
calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).)

So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and
tear down the fb. Thus, drm_framebuffer_remove() seems right here to me.

However, because of your objection I've noticed now that "if (fb)" seems
to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))".

Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(),
so not null, and we'd call drm_framebuffer_remove() on this. Is that
what you meant?


> > @@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	int size, ret;
> >  	bool prealloc = false;
> >  
> > -	mutex_lock(&dev->struct_mutex);
> > -
> >  	if (intel_fb &&
> >  	    (sizes->fb_width > intel_fb->base.width ||
> >  	     sizes->fb_height > intel_fb->base.height)) {
> > @@ -203,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> >  		ret = intelfb_alloc(helper, sizes);
> >  		if (ret)
> > -			goto out_unlock;
> > +			return ret;
> >  		intel_fb = ifbdev->fb;
> >  	} else {
> >  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> > @@ -215,6 +218,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	obj = intel_fb->obj;
> >  	size = obj->base.size;
> >  
> > +	mutex_lock(&dev->struct_mutex);
> > +
> 
> I'm thinking we won't even need the lock here anymore. But maybe I'm
> missing something.
> 
> >  	info = drm_fb_helper_alloc_fbi(helper);
> >  	if (IS_ERR(info)) {
> >  		ret = PTR_ERR(info);
> > @@ -276,7 +281,6 @@ out_destroy_fbi:
> >  out_unpin:
> >  	i915_gem_object_ggtt_unpin(obj);
> >  	drm_gem_object_unreference(&obj->base);
> 
> And this ref we don't own either AFAICS.

Why? We did call intelfb_alloc() above, so if something subsequently
goes wrong, we need to revert the steps that intelfb_alloc() carried
out. The drm_gem_object_unreference() therefore seems right here to me.

However I'm puzzled why we don't call drm_framebuffer_remove() under
the out_unpin: label. Aren't we leaking a framebuffer here without that?

Maybe you're referring to the fact that this function either inherits
the BIOS fb or creates a new fb with intelfb_alloc(). I'm not sure if
the cleanup on error is identical in these two cases. Maybe you meant
that we don't own the ref in the case that the fb was inherited from
BIOS?


Best regards,

Lukas

> 
> > -out_unlock:
> >  	mutex_unlock(&dev->struct_mutex);
> >  	return ret;
> >  }
> > -- 
> > 2.1.0
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Daniel Vetter]

On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote:
> Hi Ville,
> 
> On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > We had two failure modes here:
> > > 
> > > 1.
> > > Deadlock in intelfb_alloc failure path where it calls
> > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > > (caller of intelfb_alloc) was already holding it.
> > > 
> > > 2.
> > > Deadlock in intelfb_create failure path where it calls
> > > drm_framebuffer_unreference, which grabs the struct mutex and
> > > intelfb_create was already holding it.
> > > 
> > > v2:
> > >    * Reformat commit msg to 72 chars. (Lukas Wunner)
> > >    * Add third failure mode. (Lukas Wunner)
> > > 
> > > v3:
> > >    * On fb alloc failure, unref gem object where it gets refed,
> > >      fix double unref in separate commit. (Ville Syrjälä)
> > > 
> > > v4:
> > >    * Lock struct mutex on unref. (Chris Wilson)
> > > 
> > > v5:
> > >    * Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> > >      rephrase commit message. (Jani Nicula)
> > > 
> > > Tested-by: Pierre Moreau <pierre.morrow@free.fr>
> > >     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > > Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
> > >     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > > Tested-by: William Brown <william@blackhats.net.au>
> > >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> > > Tested-by: Lukas Wunner <lukas@wunner.de>
> > >     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > > Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > >     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> > >     framebuffer_references--")
> > > Reported-by: Lukas Wunner <lukas@wunner.de>
> > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 96476d7..eee3306 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  {
> > >  	struct intel_fbdev *ifbdev =
> > >  		container_of(helper, struct intel_fbdev, helper);
> > > -	struct drm_framebuffer *fb;
> > > +	struct drm_framebuffer *fb = NULL;
> > >  	struct drm_device *dev = helper->dev;
> > >  	struct drm_mode_fb_cmd2 mode_cmd = {};
> > >  	struct drm_i915_gem_object *obj;
> > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > >  							  sizes->surface_depth);
> > >  
> > > +	mutex_lock(&dev->struct_mutex);
> > > +
> > >  	size = mode_cmd.pitches[0] * mode_cmd.height;
> > >  	size = PAGE_ALIGN(size);
> > >  	obj = i915_gem_object_create_stolen(dev, size);
> > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> > >  	if (ret) {
> > >  		DRM_ERROR("failed to pin obj: %d\n", ret);
> > > -		goto out_fb;
> > > +		goto out_unref;
> > >  	}
> > >  
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +
> > >  	ifbdev->fb = to_intel_framebuffer(fb);
> > >  
> > >  	return 0;
> > >  
> > > -out_fb:
> > > -	drm_framebuffer_remove(fb);
> > >  out_unref:
> > >  	drm_gem_object_unreference(&obj->base);
> > 
> > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> > will now attempt to unref one too many times.
> > 
> > This taking over refs stuff is confusing. Maybe it would be better if
> > everyone just took an extra ref when they stash the obj pointer
> > somewhere, and everyone would then always release whatever ref they own
> > and no longer need.
> > 
> > >  out:
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +	if (fb)
> > > +		drm_framebuffer_remove(fb);
> > >  	return ret;
> > >  }
> > >  
> 
> Hm, why do you think we unref one too many times?
> 
> A bit further up in this function we call __intel_framebuffer_create()
> which sets the refcount to 1. (It calls intel_framebuffer_init(), which
> calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).)
> 
> So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and
> tear down the fb. Thus, drm_framebuffer_remove() seems right here to me.
> 
> However, because of your objection I've noticed now that "if (fb)" seems
> to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))".
> 
> Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(),
> so not null, and we'd call drm_framebuffer_remove() on this. Is that
> what you meant?

Quick aside: drm_framebuffer_remove is for framebuffers that userspace
has created. Since this is init code a drm_framebuffer_unreference should
be all we need - and drm_framebuffer_remove is getting somewhat
defeatured.
-Daniel

> 
> 
> > > @@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	int size, ret;
> > >  	bool prealloc = false;
> > >  
> > > -	mutex_lock(&dev->struct_mutex);
> > > -
> > >  	if (intel_fb &&
> > >  	    (sizes->fb_width > intel_fb->base.width ||
> > >  	     sizes->fb_height > intel_fb->base.height)) {
> > > @@ -203,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> > >  		ret = intelfb_alloc(helper, sizes);
> > >  		if (ret)
> > > -			goto out_unlock;
> > > +			return ret;
> > >  		intel_fb = ifbdev->fb;
> > >  	} else {
> > >  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> > > @@ -215,6 +218,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	obj = intel_fb->obj;
> > >  	size = obj->base.size;
> > >  
> > > +	mutex_lock(&dev->struct_mutex);
> > > +
> > 
> > I'm thinking we won't even need the lock here anymore. But maybe I'm
> > missing something.
> > 
> > >  	info = drm_fb_helper_alloc_fbi(helper);
> > >  	if (IS_ERR(info)) {
> > >  		ret = PTR_ERR(info);
> > > @@ -276,7 +281,6 @@ out_destroy_fbi:
> > >  out_unpin:
> > >  	i915_gem_object_ggtt_unpin(obj);
> > >  	drm_gem_object_unreference(&obj->base);
> > 
> > And this ref we don't own either AFAICS.
> 
> Why? We did call intelfb_alloc() above, so if something subsequently
> goes wrong, we need to revert the steps that intelfb_alloc() carried
> out. The drm_gem_object_unreference() therefore seems right here to me.
> 
> However I'm puzzled why we don't call drm_framebuffer_remove() under
> the out_unpin: label. Aren't we leaking a framebuffer here without that?
> 
> Maybe you're referring to the fact that this function either inherits
> the BIOS fb or creates a new fb with intelfb_alloc(). I'm not sure if
> the cleanup on error is identical in these two cases. Maybe you meant
> that we don't own the ref in the case that the fb was inherited from
> BIOS?
> 
> 
> Best regards,
> 
> Lukas
> 
> > 
> > > -out_unlock:
> > >  	mutex_unlock(&dev->struct_mutex);
> > >  	return ret;
> > >  }
> > > -- 
> > > 2.1.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote:
> Hi Ville,
> 
> On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > We had two failure modes here:
> > > 
> > > 1.
> > > Deadlock in intelfb_alloc failure path where it calls
> > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > > (caller of intelfb_alloc) was already holding it.
> > > 
> > > 2.
> > > Deadlock in intelfb_create failure path where it calls
> > > drm_framebuffer_unreference, which grabs the struct mutex and
> > > intelfb_create was already holding it.
> > > 
> > > v2:
> > >    * Reformat commit msg to 72 chars. (Lukas Wunner)
> > >    * Add third failure mode. (Lukas Wunner)
> > > 
> > > v3:
> > >    * On fb alloc failure, unref gem object where it gets refed,
> > >      fix double unref in separate commit. (Ville Syrjälä)
> > > 
> > > v4:
> > >    * Lock struct mutex on unref. (Chris Wilson)
> > > 
> > > v5:
> > >    * Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> > >      rephrase commit message. (Jani Nicula)
> > > 
> > > Tested-by: Pierre Moreau <pierre.morrow@free.fr>
> > >     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > > Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
> > >     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > > Tested-by: William Brown <william@blackhats.net.au>
> > >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> > > Tested-by: Lukas Wunner <lukas@wunner.de>
> > >     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > > Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > >     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> > >     framebuffer_references--")
> > > Reported-by: Lukas Wunner <lukas@wunner.de>
> > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 96476d7..eee3306 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  {
> > >  	struct intel_fbdev *ifbdev =
> > >  		container_of(helper, struct intel_fbdev, helper);
> > > -	struct drm_framebuffer *fb;
> > > +	struct drm_framebuffer *fb = NULL;
> > >  	struct drm_device *dev = helper->dev;
> > >  	struct drm_mode_fb_cmd2 mode_cmd = {};
> > >  	struct drm_i915_gem_object *obj;
> > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > >  							  sizes->surface_depth);
> > >  
> > > +	mutex_lock(&dev->struct_mutex);
> > > +
> > >  	size = mode_cmd.pitches[0] * mode_cmd.height;
> > >  	size = PAGE_ALIGN(size);
> > >  	obj = i915_gem_object_create_stolen(dev, size);
> > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> > >  	if (ret) {
> > >  		DRM_ERROR("failed to pin obj: %d\n", ret);
> > > -		goto out_fb;
> > > +		goto out_unref;
> > >  	}
> > >  
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +
> > >  	ifbdev->fb = to_intel_framebuffer(fb);
> > >  
> > >  	return 0;
> > >  
> > > -out_fb:
> > > -	drm_framebuffer_remove(fb);
> > >  out_unref:
> > >  	drm_gem_object_unreference(&obj->base);
> > 
> > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> > will now attempt to unref one too many times.
> > 
> > This taking over refs stuff is confusing. Maybe it would be better if
> > everyone just took an extra ref when they stash the obj pointer
> > somewhere, and everyone would then always release whatever ref they own
> > and no longer need.
> > 
> > >  out:
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +	if (fb)
> > > +		drm_framebuffer_remove(fb);
> > >  	return ret;
> > >  }
> > >  
> 
> Hm, why do you think we unref one too many times?

Because the fb now owns the reference, so it gets unreffed by the fb
.destroy() hook... I think.

> 
> A bit further up in this function we call __intel_framebuffer_create()
> which sets the refcount to 1. (It calls intel_framebuffer_init(), which
> calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).)
> 
> So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and
> tear down the fb. Thus, drm_framebuffer_remove() seems right here to me.

I wasn't complaining about the fb unref, but the bo unref.

> 
> However, because of your objection I've noticed now that "if (fb)" seems
> to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))".
> 
> Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(),
> so not null, and we'd call drm_framebuffer_remove() on this. Is that
> what you meant?

No, but that's a good observation too.

> 
> 
> > > @@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	int size, ret;
> > >  	bool prealloc = false;
> > >  
> > > -	mutex_lock(&dev->struct_mutex);
> > > -
> > >  	if (intel_fb &&
> > >  	    (sizes->fb_width > intel_fb->base.width ||
> > >  	     sizes->fb_height > intel_fb->base.height)) {
> > > @@ -203,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> > >  		ret = intelfb_alloc(helper, sizes);
> > >  		if (ret)
> > > -			goto out_unlock;
> > > +			return ret;
> > >  		intel_fb = ifbdev->fb;
> > >  	} else {
> > >  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> > > @@ -215,6 +218,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	obj = intel_fb->obj;
> > >  	size = obj->base.size;
> > >  
> > > +	mutex_lock(&dev->struct_mutex);
> > > +
> > 
> > I'm thinking we won't even need the lock here anymore. But maybe I'm
> > missing something.
> > 
> > >  	info = drm_fb_helper_alloc_fbi(helper);
> > >  	if (IS_ERR(info)) {
> > >  		ret = PTR_ERR(info);
> > > @@ -276,7 +281,6 @@ out_destroy_fbi:
> > >  out_unpin:
> > >  	i915_gem_object_ggtt_unpin(obj);
> > >  	drm_gem_object_unreference(&obj->base);
> > 
> > And this ref we don't own either AFAICS.
> 
> Why? We did call intelfb_alloc() above, so if something subsequently
> goes wrong, we need to revert the steps that intelfb_alloc() carried
> out. The drm_gem_object_unreference() therefore seems right here to me.

Here too the fb (if succesfully created) now owns that reference.

> 
> However I'm puzzled why we don't call drm_framebuffer_remove() under
> the out_unpin: label. Aren't we leaking a framebuffer here without that?
> 
> Maybe you're referring to the fact that this function either inherits
> the BIOS fb or creates a new fb with intelfb_alloc(). I'm not sure if
> the cleanup on error is identical in these two cases. Maybe you meant
> that we don't own the ref in the case that the fb was inherited from
> BIOS?
> 
> 
> Best regards,
> 
> Lukas
> 
> > 
> > > -out_unlock:
> > >  	mutex_unlock(&dev->struct_mutex);
> > >  	return ret;
> > >  }
> > > -- 
> > > 2.1.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner]

Hi Ville,

On Thu, Oct 15, 2015 at 08:34:23PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote:
> > Hi Ville,
> > 
> > On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote:
> > > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > 
> > > > We had two failure modes here:
> > > > 
> > > > 1.
> > > > Deadlock in intelfb_alloc failure path where it calls
> > > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> > > > (caller of intelfb_alloc) was already holding it.
> > > > 
> > > > 2.
> > > > Deadlock in intelfb_create failure path where it calls
> > > > drm_framebuffer_unreference, which grabs the struct mutex and
> > > > intelfb_create was already holding it.
> > > > 
> > > > v2:
> > > >    * Reformat commit msg to 72 chars. (Lukas Wunner)
> > > >    * Add third failure mode. (Lukas Wunner)
> > > > 
> > > > v3:
> > > >    * On fb alloc failure, unref gem object where it gets refed,
> > > >      fix double unref in separate commit. (Ville Syrjälä)
> > > > 
> > > > v4:
> > > >    * Lock struct mutex on unref. (Chris Wilson)
> > > > 
> > > > v5:
> > > >    * Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC,
> > > >      rephrase commit message. (Jani Nicula)
> > > > 
> > > > Tested-by: Pierre Moreau <pierre.morrow@free.fr>
> > > >     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> > > > Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
> > > >     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> > > > Tested-by: William Brown <william@blackhats.net.au>
> > > >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> > > > Tested-by: Lukas Wunner <lukas@wunner.de>
> > > >     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> > > > Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > > >     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> > > > 
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> > > >     framebuffer_references--")
> > > > Reported-by: Lukas Wunner <lukas@wunner.de>
> > > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]
> > > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
> > > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > index 96476d7..eee3306 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > > >  {
> > > >  	struct intel_fbdev *ifbdev =
> > > >  		container_of(helper, struct intel_fbdev, helper);
> > > > -	struct drm_framebuffer *fb;
> > > > +	struct drm_framebuffer *fb = NULL;
> > > >  	struct drm_device *dev = helper->dev;
> > > >  	struct drm_mode_fb_cmd2 mode_cmd = {};
> > > >  	struct drm_i915_gem_object *obj;
> > > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > > >  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > > >  							  sizes->surface_depth);
> > > >  
> > > > +	mutex_lock(&dev->struct_mutex);
> > > > +
> > > >  	size = mode_cmd.pitches[0] * mode_cmd.height;
> > > >  	size = PAGE_ALIGN(size);
> > > >  	obj = i915_gem_object_create_stolen(dev, size);
> > > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > > >  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> > > >  	if (ret) {
> > > >  		DRM_ERROR("failed to pin obj: %d\n", ret);
> > > > -		goto out_fb;
> > > > +		goto out_unref;
> > > >  	}
> > > >  
> > > > +	mutex_unlock(&dev->struct_mutex);
> > > > +
> > > >  	ifbdev->fb = to_intel_framebuffer(fb);
> > > >  
> > > >  	return 0;
> > > >  
> > > > -out_fb:
> > > > -	drm_framebuffer_remove(fb);
> > > >  out_unref:
> > > >  	drm_gem_object_unreference(&obj->base);
> > > 
> > > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove()
> > > will now attempt to unref one too many times.
> > > 
> > > This taking over refs stuff is confusing. Maybe it would be better if
> > > everyone just took an extra ref when they stash the obj pointer
> > > somewhere, and everyone would then always release whatever ref they own
> > > and no longer need.
> > > 
> > > >  out:
> > > > +	mutex_unlock(&dev->struct_mutex);
> > > > +	if (fb)
> > > > +		drm_framebuffer_remove(fb);
> > > >  	return ret;
> > > >  }
> > > >  
> > 
> > Hm, why do you think we unref one too many times?
> 
> Because the fb now owns the reference, so it gets unreffed by the fb
> .destroy() hook... I think.

You're right. drm_framebuffer_remove() calls drm_framebuffer_unreference(),
if this was the last ref then drm_framebuffer_free() gets called,
which invokes the ->destroy callback intel_user_framebuffer_destroy(),
which in turn calls drm_gem_object_unreference().

So indeed it gets unrefed twice here.


> > 
> > A bit further up in this function we call __intel_framebuffer_create()
> > which sets the refcount to 1. (It calls intel_framebuffer_init(), which
> > calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).)
> > 
> > So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and
> > tear down the fb. Thus, drm_framebuffer_remove() seems right here to me.
> 
> I wasn't complaining about the fb unref, but the bo unref.
> 
> > 
> > However, because of your objection I've noticed now that "if (fb)" seems
> > to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))".
> > 
> > Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(),
> > so not null, and we'd call drm_framebuffer_remove() on this. Is that
> > what you meant?
> 
> No, but that's a good observation too.
> 
> > 
> > 
> > > > @@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > >  	int size, ret;
> > > >  	bool prealloc = false;
> > > >  
> > > > -	mutex_lock(&dev->struct_mutex);
> > > > -
> > > >  	if (intel_fb &&
> > > >  	    (sizes->fb_width > intel_fb->base.width ||
> > > >  	     sizes->fb_height > intel_fb->base.height)) {
> > > > @@ -203,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > >  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> > > >  		ret = intelfb_alloc(helper, sizes);
> > > >  		if (ret)
> > > > -			goto out_unlock;
> > > > +			return ret;
> > > >  		intel_fb = ifbdev->fb;
> > > >  	} else {
> > > >  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> > > > @@ -215,6 +218,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > >  	obj = intel_fb->obj;
> > > >  	size = obj->base.size;
> > > >  
> > > > +	mutex_lock(&dev->struct_mutex);
> > > > +
> > > 
> > > I'm thinking we won't even need the lock here anymore. But maybe I'm
> > > missing something.
> > > 
> > > >  	info = drm_fb_helper_alloc_fbi(helper);
> > > >  	if (IS_ERR(info)) {
> > > >  		ret = PTR_ERR(info);
> > > > @@ -276,7 +281,6 @@ out_destroy_fbi:
> > > >  out_unpin:
> > > >  	i915_gem_object_ggtt_unpin(obj);
> > > >  	drm_gem_object_unreference(&obj->base);
> > > 
> > > And this ref we don't own either AFAICS.
> > 
> > Why? We did call intelfb_alloc() above, so if something subsequently
> > goes wrong, we need to revert the steps that intelfb_alloc() carried
> > out. The drm_gem_object_unreference() therefore seems right here to me.
> 
> Here too the fb (if succesfully created) now owns that reference.

But here the ->destroy callback isn't invoked because the fb isn't unrefed.
There's no call to drm_framebuffer_unreference() / drm_framebuffer_remove(),
so the gem object isn't unrefed twice but we seem to leak the fb.

That's exactly what I find confusing here, if intelfb_alloc() above was
successful and something else subsequently goes awry, we need to unref
the fb, right? Or am I missing something?

In the case when we've inherited the fb from BIOS (instead of creating it
with intelfb_alloc()), is it okay to unref the fb as well?


Best regards,

Lukas


> > However I'm puzzled why we don't call drm_framebuffer_remove() under
> > the out_unpin: label. Aren't we leaking a framebuffer here without that?
> > 
> > Maybe you're referring to the fact that this function either inherits
> > the BIOS fb or creates a new fb with intelfb_alloc(). I'm not sure if
> > the cleanup on error is identical in these two cases. Maybe you meant
> > that we don't own the ref in the case that the fb was inherited from
> > BIOS?
> > 
> > 
> > Best regards,
> > 
> > Lukas
> > 
> > > 
> > > > -out_unlock:
> > > >  	mutex_unlock(&dev->struct_mutex);
> > > >  	return ret;
> > > >  }
> > > > -- 
> > > > 2.1.0
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 2/4] drm/i915: Fix double unref in intelfb_alloc failure path

[Lukas Wunner]

In intelfb_alloc(), if the call to intel_pin_and_fence_fb_obj() fails,
the bo is unrefed twice: By drm_framebuffer_remove() and once more by
drm_gem_object_unreference(). Fix it.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4fd5fdf..ec82b51 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -156,8 +156,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
 	if (IS_ERR(fb)) {
+		drm_gem_object_unreference(&obj->base);
 		ret = PTR_ERR(fb);
-		goto out_unref;
+		goto out;
 	}
 
 	/* Flush everything out, we'll be doing GTT only from now on */
@@ -173,8 +174,6 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 out_fb:
 	drm_framebuffer_remove(fb);
-out_unref:
-	drm_gem_object_unreference(&obj->base);
 out:
 	return ret;
 }
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v7 2/3] drm/i915: Fix double unref in intelfb_alloc failure path

[Lukas Wunner]

In intelfb_alloc(), if the call to intel_pin_and_fence_fb_obj() fails,
the bo is unrefed twice: By drm_framebuffer_remove() and once more by
drm_gem_object_unreference(). Fix it.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 840d6bf..2240e70 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -156,8 +156,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
 	if (IS_ERR(fb)) {
+		drm_gem_object_unreference(&obj->base);
 		ret = PTR_ERR(fb);
-		goto out_unref;
+		goto out;
 	}
 
 	/* Flush everything out, we'll be doing GTT only from now on */
@@ -173,8 +174,6 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 out_fb:
 	drm_framebuffer_remove(fb);
-out_unref:
-	drm_gem_object_unreference(&obj->base);
 out:
 	return ret;
 }
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v6 4/4] drm/i915: Fix error handling in intelfb_create

[Lukas Wunner]

intelfb_create() is called once on driver initialization. It either
uses the fb inherited from BIOS or allocates a new one by calling
intelfb_alloc(). Afterwards, it calls two functions which can fail:

- drm_fb_helper_alloc_fbi() can fail with -ENOMEM.
- ioremap_wc() can fail on alignment issues etc.

In the case where we allocated the fb with intelfb_alloc(), if either
of the two functions fail we currently unpin and unref the bo,
but leak the fb. We need to unref the fb instead of the bo here.

In the case where the fb was inherited from BIOS, the fb struct was
allocated by dev_priv->display.get_initial_plane_config() and is in
active use by a crtc, so it seems wrong to unpin and unref its bo.
We could treat failure of the above two functions as a fatal error
and call i915_driver_unload(), or try to limp on (fbcon won't work,
but X11 might). Let's do the latter.

However we should at least log an error message. Currently a failure
of the above two functions is not reported at all: The negative return
value is passed up the call stack to intel_fbdev_initial_config()
which ignores it.

To be fair, this is a corner case which few users probably ever
experience, nevertheless we should try to get it right.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 12597b5..b8c11a1 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fb_info\n");
 		ret = PTR_ERR(info);
 		goto out_unpin;
 	}
@@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
 			   size);
 	if (!info->screen_base) {
+		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
 		ret = -ENOSPC;
 		goto out_destroy_fbi;
 	}
@@ -284,9 +286,14 @@ static int intelfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
-	i915_gem_object_ggtt_unpin(obj);
-	drm_gem_object_unreference(&obj->base);
-	mutex_unlock(&dev->struct_mutex);
+	/* If fb was preallocated by BIOS, try to limp on. Else free it. */
+	if (prealloc)
+		mutex_unlock(&dev->struct_mutex);
+	else {
+		i915_gem_object_ggtt_unpin(obj);
+		mutex_unlock(&dev->struct_mutex);
+		drm_framebuffer_unreference(&intel_fb->base);
+	}
 	return ret;
 }
 
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v6 0/4] fbdev deadlock & failure path fixes

[Lukas Wunner]

Next iteration of these fixes, taking into account the additional
issues discovered in the recent exchange with Ville (this series
is in-reply-to that thread):

* Patch 1 now with Reviewed-by tag by Ville (thanks!), moved to the
  front of the series.

* Patch 2 is new, fixes a double unref of the bo in a failure path
  of intelfb_alloc, spotted by Ville.

* Patch 3 is another attempt to eliminate the deadlocks in
  intelfb_alloc and intelfb_create, patch originally by Tvrtko.
  New in v6: Avoid unrefing the fb if it's an ERR_PTR,
  replace drm_framebuffer_remove with drm_framebuffer_unreference
  (Daniel).

* Patch 4 is new, plugs leak of a struct intel_fbdev in
  intelfb_create. Not sure if I made the right call in the case
  where the fb was inherited from BIOS, comments welcome.
  (I decided to leave the fb as is and limp on. Alternative would
  be to call drm_framebuffer_remove to free the memory occupied
  by the fb and disable the crtc; the fbcon will be unusable anyway
  at this point and if X11 is able to start up without errors,
  it should be able to reinitialize the crtc.)

Browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixes


Lukas Wunner (3):
  drm/i915: On fb alloc failure, unref gem object where it gets refed
  drm/i915: Fix double unref in intelfb_alloc failure path
  drm/i915: Fix error handling in intelfb_create

Tvrtko Ursulin (1):
  drm/i915: Fix failure paths around initial fbdev allocation

 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++-------
 drivers/gpu/drm/i915/intel_fbdev.c   | 38 +++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 21 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH v6 2/4] drm/i915: Fix double unref in intelfb_alloc failure path

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:37:18PM +0200, Lukas Wunner wrote:
> In intelfb_alloc(), if the call to intel_pin_and_fence_fb_obj() fails,
> the bo is unrefed twice: By drm_framebuffer_remove() and once more by
> drm_gem_object_unreference(). Fix it.
> 
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 4fd5fdf..ec82b51 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -156,8 +156,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  
>  	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
>  	if (IS_ERR(fb)) {
> +		drm_gem_object_unreference(&obj->base);
>  		ret = PTR_ERR(fb);
> -		goto out_unref;
> +		goto out;
>  	}
>  
>  	/* Flush everything out, we'll be doing GTT only from now on */
> @@ -173,8 +174,6 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  
>  out_fb:
>  	drm_framebuffer_remove(fb);
> -out_unref:
> -	drm_gem_object_unreference(&obj->base);
>  out:
>  	return ret;
>  }
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 4/4] drm/i915: Fix error handling in intelfb_create

[Daniel Vetter]

On Sat, Oct 24, 2015 at 12:27:57AM +0200, Lukas Wunner wrote:
> intelfb_create() is called once on driver initialization. It either
> uses the fb inherited from BIOS or allocates a new one by calling
> intelfb_alloc(). Afterwards, it calls two functions which can fail:
> 
> - drm_fb_helper_alloc_fbi() can fail with -ENOMEM.
> - ioremap_wc() can fail on alignment issues etc.
> 
> In the case where we allocated the fb with intelfb_alloc(), if either
> of the two functions fail we currently unpin and unref the bo,
> but leak the fb. We need to unref the fb instead of the bo here.
> 
> In the case where the fb was inherited from BIOS, the fb struct was
> allocated by dev_priv->display.get_initial_plane_config() and is in
> active use by a crtc, so it seems wrong to unpin and unref its bo.
> We could treat failure of the above two functions as a fatal error
> and call i915_driver_unload(), or try to limp on (fbcon won't work,
> but X11 might). Let's do the latter.
> 
> However we should at least log an error message. Currently a failure
> of the above two functions is not reported at all: The negative return
> value is passed up the call stack to intel_fbdev_initial_config()
> which ignores it.
> 
> To be fair, this is a corner case which few users probably ever
> experience, nevertheless we should try to get it right.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

I don't think there's a leak here really. We always assign ifbdev->fb in
intelfb_alloc, which means the fbdev teardown code will take care of it.
The correct approach is probably to not unref anything at all, or if we
unref then we have to clear ifbdev->fb (since it's the reference that very
pointer is holding that we're clearing up).
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 12597b5..b8c11a1 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
> +		DRM_ERROR("Failed to allocate fb_info\n");
>  		ret = PTR_ERR(info);
>  		goto out_unpin;
>  	}
> @@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
>  			   size);
>  	if (!info->screen_base) {
> +		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
>  		ret = -ENOSPC;
>  		goto out_destroy_fbi;
>  	}
> @@ -284,9 +286,14 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_destroy_fbi:
>  	drm_fb_helper_release_fbi(helper);
>  out_unpin:
> -	i915_gem_object_ggtt_unpin(obj);
> -	drm_gem_object_unreference(&obj->base);
> -	mutex_unlock(&dev->struct_mutex);
> +	/* If fb was preallocated by BIOS, try to limp on. Else free it. */
> +	if (prealloc)
> +		mutex_unlock(&dev->struct_mutex);
> +	else {
> +		i915_gem_object_ggtt_unpin(obj);
> +		mutex_unlock(&dev->struct_mutex);
> +		drm_framebuffer_unreference(&intel_fb->base);
> +	}
>  	return ret;
>  }
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 3/4] drm/i915: Fix failure paths around initial fbdev allocation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We had two failure modes here:
> 
> 1.
> Deadlock in intelfb_alloc failure path where it calls
> drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> (caller of intelfb_alloc) was already holding it.
> 
> 2.
> Deadlock in intelfb_create failure path where it calls
> drm_framebuffer_unreference, which grabs the struct mutex and
> intelfb_create was already holding it.
> 
> [Chris Wilson on why struct_mutex needs to be locked in the second half
> of intelfb_create: "there is a bug here where we don't take an explicit
> pin on the VMA we setup for the fbdev which requires the lock."]

The vma is pinned, the problem is that we re-lookup it a few times, which
is racy. We should instead track the vma directly, but oh well we don't.

With that clarified in the commit message this is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> v2:
>    * Reformat commit msg to 72 chars. (Lukas Wunner)
>    * Add third failure mode. (Lukas Wunner)
> 
> v5:
>    * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
>      rephrase commit message. (Jani Nicula)
> 
> v6:
>    * In intelfb_alloc, if __intel_framebuffer_create failed,
>      fb will be an ERR_PTR, thus not null. So in the failure
>      path we need to check for IS_ERR_OR_NULL to avoid calling
>      drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner)
>    * Since this is init code a drm_framebuffer_unreference should
>      be all we need. drm_framebuffer_remove is for framebuffers
>      that userspace has created - and is getting somewhat
>      defeatured. (Daniel Vetter)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 60a5ca015ffd ("drm/i915: Add locking around
>     framebuffer_references--")
> Reported-by: Lukas Wunner <lukas@wunner.de>
> [Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index ec82b51..12597b5 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> -	struct drm_framebuffer *fb;
> +	struct drm_framebuffer *fb = NULL;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
> @@ -138,6 +138,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>  							  sizes->surface_depth);
>  
> +	mutex_lock(&dev->struct_mutex);
> +
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = PAGE_ALIGN(size);
>  
> @@ -165,16 +167,19 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
> -		goto out_fb;
> +		goto out;
>  	}
>  
> +	mutex_unlock(&dev->struct_mutex);
> +
>  	ifbdev->fb = to_intel_framebuffer(fb);
>  
>  	return 0;
>  
> -out_fb:
> -	drm_framebuffer_remove(fb);
>  out:
> +	mutex_unlock(&dev->struct_mutex);
> +	if (!IS_ERR_OR_NULL(fb))
> +		drm_framebuffer_unreference(fb);
>  	return ret;
>  }
>  
> @@ -192,8 +197,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	int size, ret;
>  	bool prealloc = false;
>  
> -	mutex_lock(&dev->struct_mutex);
> -
>  	if (intel_fb &&
>  	    (sizes->fb_width > intel_fb->base.width ||
>  	     sizes->fb_height > intel_fb->base.height)) {
> @@ -208,7 +211,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>  		ret = intelfb_alloc(helper, sizes);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  		intel_fb = ifbdev->fb;
>  	} else {
>  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> @@ -220,6 +223,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	mutex_lock(&dev->struct_mutex);
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -281,7 +286,6 @@ out_destroy_fbi:
>  out_unpin:
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
> -out_unlock:
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/2] drm/i915: Tear down fbdev if initialization fails

[Lukas Wunner]

Currently if intelfb_create() errors out, it unrefs the bo even though
the fb now owns that reference. (Spotted by Ville Syrjälä.) We should
unref the fb instead of the bo.

However the fb was not necessarily allocated by intelfb_create(),
it could be inherited from BIOS (the fb struct was then allocated by
dev_priv->display.get_initial_plane_config()) and be in active use by
a crtc. In this case we should call drm_framebuffer_remove() instead
of _unreference() to also disable the crtc.

Daniel Vetter suggested that "fbdev teardown code will take care of it.
The correct approach is probably to not unref anything at all".

But if fbdev initialization fails, the fbdev isn't torn down and
occupies memory even though it's unusable. Therefore clobber it in
intel_fbdev_initial_config(). (Currently we ignore a negative return
value there.) The idea is that if fbdev initialization fails, the driver
behaves as if CONFIG_DRM_FBDEV_EMULATION wasn't set.

Also, log errors in intelfb_create().

Move async_synchronize_full() from intel_fbdev_fini() to its sole caller
i915_driver_unload() to avoid deadlock if fbdev initialization fails.

v2: Instead of calling drm_framebuffer_unreference() (if fb was not
    inherited from BIOS), call intel_fbdev_fini().

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/i915_dma.c    | 1 +
 drivers/gpu/drm/i915/intel_fbdev.c | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ffcb9c6..68ab51d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1154,6 +1154,7 @@ int i915_driver_unload(struct drm_device *dev)
 
 	acpi_video_unregister();
 
+	async_synchronize_full();
 	intel_fbdev_fini(dev);
 
 	drm_vblank_cleanup(dev);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 05484ba..2969447 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fb_info\n");
 		ret = PTR_ERR(info);
 		goto out_unpin;
 	}
@@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
 			   size);
 	if (!info->screen_base) {
+		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
 		ret = -ENOSPC;
 		goto out_destroy_fbi;
 	}
@@ -285,7 +287,6 @@ out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
-	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
@@ -713,7 +714,9 @@ void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
+	if (drm_fb_helper_initial_config(&ifbdev->helper,
+					 ifbdev->preferred_bpp))
+		intel_fbdev_fini(dev_priv->dev);
 }
 
 void intel_fbdev_fini(struct drm_device *dev)
@@ -723,8 +726,6 @@ void intel_fbdev_fini(struct drm_device *dev)
 		return;
 
 	flush_work(&dev_priv->fbdev_suspend_work);
-
-	async_synchronize_full();
 	intel_fbdev_destroy(dev, dev_priv->fbdev);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v2 1/2] drm/i915: Fix oops caused by fbdev initialization failure

[Lukas Wunner]

intelfb_create() is called once on driver initialization. If it fails,
ifbdev->helper.fbdev, ifbdev->fb or ifbdev->fb->obj may be NULL.

intel_fbdev_destroy() is called on driver unload and dereferences
ifbdev->fb.

intel_fbdev_set_suspend() is called on suspend/resume and dereferences
ifbdev->helper.fbdev and ifbdev->fb->obj.

intel_fbdev_output_poll_changed() is called on hotplug events and calls
drm_fb_helper_hotplug_event(). If ifbdev->helper.fb is NULL it will
signal a delayed_hotplug ad infinitum; if it is not NULL it will call
drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev.

intel_fbdev_restore_mode() is called on lastclose and dereferences
ifbdev->fb->obj. It also calls __drm_atomic_helper_set_config() which
WARNs if ifbdev->fb is NULL, and drm_fb_helper_set_par() which
dereferences ifbdev->helper.fbdev.

i915_gem_framebuffer_info() is called on access to debugfs file
"i915_gem_framebuffer" and dereferences ifbdev, ifbdev->helper.fb
and ifbdev->helper.fb->obj.

intel_connector_add_to_fbdev() / intel_connector_remove_from_fbdev()
are called when registering / unregistering an mst connector and
dereference ifbdev.

Basically if fbdev creation fails, we end up with an ifbdev that's
only partially initialized and the driver will oops on unload,
the next suspend/resume cycle, hotplug events, when X11 is stopped
or when accessing debugfs file "i915_gem_framebuffer".

After the fbdev was destroyed with intel_fbdev_fini(), the driver
will oops on mst hotplug events.

Fix it.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_fbdev.c  | 16 ++++++++++------
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5659d4c..17821cf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1877,17 +1877,19 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	ifbdev = dev_priv->fbdev;
-	fb = to_intel_framebuffer(ifbdev->helper.fb);
-
-	seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
-		   fb->base.width,
-		   fb->base.height,
-		   fb->base.depth,
-		   fb->base.bits_per_pixel,
-		   fb->base.modifier[0],
-		   atomic_read(&fb->base.refcount.refcount));
-	describe_obj(m, fb->obj);
-	seq_putc(m, '\n');
+	if (ifbdev && ifbdev->helper.fb && ifbdev->fb->obj) {
+		fb = to_intel_framebuffer(ifbdev->helper.fb);
+
+		seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
+			   fb->base.width,
+			   fb->base.height,
+			   fb->base.depth,
+			   fb->base.bits_per_pixel,
+			   fb->base.modifier[0],
+			   atomic_read(&fb->base.refcount.refcount));
+		describe_obj(m, fb->obj);
+		seq_putc(m, '\n');
+	}
 #endif
 
 	mutex_lock(&dev->mode_config.fb_lock);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 0639275..735368e 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -414,7 +414,10 @@ static void intel_connector_add_to_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
+						&connector->base);
 #endif
 }
 
@@ -422,7 +425,10 @@ static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
+						   &connector->base);
 #endif
 }
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index db82ad4..05484ba 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -529,8 +529,10 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	drm_framebuffer_remove(&ifbdev->fb->base);
+	if (ifbdev->fb) {
+		drm_framebuffer_unregister_private(&ifbdev->fb->base);
+		drm_framebuffer_remove(&ifbdev->fb->base);
+	}
 }
 
 /*
@@ -734,7 +736,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 	struct fb_info *info;
 
-	if (!ifbdev)
+	if (!ifbdev || !ifbdev->fb || !ifbdev->fb->obj || !ifbdev->helper.fbdev)
 		return;
 
 	info = ifbdev->helper.fbdev;
@@ -780,8 +782,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (dev_priv->fbdev)
-		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
+
+	if (ifbdev && ifbdev->fb && ifbdev->fb->obj && ifbdev->helper.fbdev)
+		drm_fb_helper_hotplug_event(&ifbdev->helper);
 }
 
 void intel_fbdev_restore_mode(struct drm_device *dev)
@@ -791,7 +795,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 	struct drm_fb_helper *fb_helper;
 
-	if (!ifbdev)
+	if (!ifbdev || !ifbdev->fb || !ifbdev->fb->obj || !ifbdev->helper.fbdev)
 		return;
 
 	fb_helper = &ifbdev->helper;
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v7 0/3] fbdev fixes (reviewed)

[Lukas Wunner]

Hi Jani,

three patches with fbdev deadlock & failure path fixes,
each with Reviewed-by tag by Ville or Daniel, the third one
with amended commit message as requested by Daniel in
<20151030182818.GR16848@phenom.ffwll.local>.

Patch 1 fixes double unref of bo in failure path of intelfb_alloc().
Patch 2 fixes another double unref of bo in failure path of intelfb_alloc().
Patch 3 fixes deadlocks in intelfb_create(), one during driver
initialization and one in failure path.

Browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixes

The previous iteration (v6, <cover.1445771693.git.lukas@wunner.de>)
contained a 4th patch, a new version of which I'll submit separately.

Thanks,

Lukas


Lukas Wunner (2):
  drm/i915: On fb alloc failure, unref gem object where it gets refed
  drm/i915: Fix double unref in intelfb_alloc failure path

Tvrtko Ursulin (1):
  drm/i915: Fix failure paths around initial fbdev allocation

 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_fbdev.c   | 25 ++++++++++++++-----------
 2 files changed, 28 insertions(+), 18 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v2 0/2] fbdev fixes (need review)

[Lukas Wunner]

On Fri, Oct 30, 2015 at 07:23:45PM +0100, Daniel Vetter wrote:
> I don't think there's a leak here really. We always assign ifbdev->fb in
> intelfb_alloc, which means the fbdev teardown code will take care of it.
> The correct approach is probably to not unref anything at all, or if we
> unref then we have to clear ifbdev->fb (since it's the reference that very
> pointer is holding that we're clearing up).

Oh, right, and ifbdev->helper.fb would have to be set to NULL as well.

However it turns out the fbdev teardown code can't cope with an fbdev
that's only partially initialized, it lacks proper null pointer checks.
Patch 1 in this series fixes that.

Patch 2 is another attempt at handling failure of intelfb_create():
On failure, ifbdev->helper.fbdev will be released by intelfb_create()
but the intel_framebuffer and the gem_object are NOT unrefed. To make
up for this, intel_fbdev_initial_config() will call intel_fbdev_fini(),
which not only unrefs the intel_framebuffer and the gem_object but also
frees the ifbdev. It's as if CONFIG_DRM_FBDEV_EMULATION wasn't set.

Not sure if this is the right way to go but seems more sensible to me
than keeping an fbdev around that's only halfway initialized. We've
kicked out the VGA console at this point but failed to initialize an
fbdev, the console will thus be unusable. If X11 manages to start up
without errors, it will at least be able to use the memory that would
otherwise be hogged by the unusable fbdev.

Note that if you find patch 2 acceptable, the modifications in patch 1
to the following functions would actually become optional since they
already check if ifbdev is a null pointer (let me know if you want these
dropped): intel_fbdev_set_suspend(), intel_fbdev_output_poll_changed(),
intel_fbdev_restore_mode().

The patches are also browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixes

Best regards,

Lukas


Lukas Wunner (2):
  drm/i915: Fix oops caused by fbdev initialization failure
  drm/i915: Tear down fbdev if initialization fails

 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/i915_dma.c     |  1 +
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_fbdev.c  | 25 +++++++++++++++----------
 4 files changed, 37 insertions(+), 23 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH v7 0/3] fbdev fixes (reviewed)

[Jani Nikula]

On Sat, 07 Nov 2015, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Jani,
>
> three patches with fbdev deadlock & failure path fixes,
> each with Reviewed-by tag by Ville or Daniel, the third one
> with amended commit message as requested by Daniel in
> <20151030182818.GR16848@phenom.ffwll.local>.
>
> Patch 1 fixes double unref of bo in failure path of intelfb_alloc().
> Patch 2 fixes another double unref of bo in failure path of intelfb_alloc().
> Patch 3 fixes deadlocks in intelfb_create(), one during driver
> initialization and one in failure path.
>
> Browsable on GitHub:
> https://github.com/l1k/linux/commits/intel_fbdev_fixes
>
> The previous iteration (v6, <cover.1445771693.git.lukas@wunner.de>)
> contained a 4th patch, a new version of which I'll submit separately.

Pushed all three to drm-intel-next-queued. Thanks for the patches and
review.

For future reference, please consider posting new versions of series as
new threads. This one got pretty messy in the end, with so many
different versions.

Also, I don't know how you generate and send your patches, but even the
updated patches had dates of the original or very old versions of
them. Like [1] is June 30 although you sent it just a couple of days
ago. Please look into that.

BR,
Jani.


[1] http://mid.gmane.org/47d4e88c91b3bf0f7a280cabec54c8c8cf0cf6f2.1446892879.git.lukas@wunner.de

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 0/3] fbdev fixes (reviewed)

[Lukas Wunner]

Hi Jani,

On Mon, Nov 09, 2015 at 04:23:34PM +0200, Jani Nikula wrote:
> On Sat, 07 Nov 2015, Lukas Wunner <lukas@wunner.de> wrote:
> > three patches with fbdev deadlock & failure path fixes,
> > each with Reviewed-by tag by Ville or Daniel, the third one
> > with amended commit message as requested by Daniel in
> > <20151030182818.GR16848@phenom.ffwll.local>.
> Pushed all three to drm-intel-next-queued. Thanks for the patches and
> review.

Thank you!


> Also, I don't know how you generate and send your patches, but even the
> updated patches had dates of the original or very old versions of
> them. Like [1] is June 30 although you sent it just a couple of days
> ago. Please look into that.

git format-patch tries to tunnel the author date through RFC 5322
by setting the Date-header to it.

git send-email, which most people use, strips that and replaces it
with the date of its invocation.

I use msmtp instead of git send-email, which preserves the Date-header.

When I edit commits, git commit --amend updates the commit date but not
the author date.

That's why you see these ancient timestamps.

If you find that annoying I'll see to it that I modify the author date
manually before sending out a new series. (At least in commits of my own.
I try to avoid tampering with other people's commits.)


> For future reference, please consider posting new versions of series as
> new threads. This one got pretty messy in the end, with so many
> different versions.

Daniel asked me to submit a patch "in-reply the previous version" in
<20150922091757.GZ3383@phenom.ffwll.local> and I adhered to that request
also when sending a new version of an entire series. In that case I'll
*not* submit in-reply-to in the future, got that.

Best regards,

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

Re: [PATCH v7 0/3] fbdev fixes (reviewed)

[Jani Nikula]

On Thu, 12 Nov 2015, Lukas Wunner <lukas@wunner.de> wrote:
> I use msmtp instead of git send-email, which preserves the Date-header.

Any particular reason not to use git send-email?

You can configure it to use a sendmail-like program instead of a server,
and actually that's the default. If you have msmtp installed as sendmail
(for example msmtp-mta package does this on Debian) git send-email will
use that out of the box. FWIW this is exactly what I do.

> When I edit commits, git commit --amend updates the commit date but not
> the author date.
>
> That's why you see these ancient timestamps.
>
> If you find that annoying I'll see to it that I modify the author date
> manually before sending out a new series. (At least in commits of my own.
> I try to avoid tampering with other people's commits.)

It's just that my mail client uses the Date: header for sorting, and the
old ones tend to get buried. It's disadvantageous to you to use old
dates. ;)

>> For future reference, please consider posting new versions of series as
>> new threads. This one got pretty messy in the end, with so many
>> different versions.
>
> Daniel asked me to submit a patch "in-reply the previous version" in
> <20150922091757.GZ3383@phenom.ffwll.local> and I adhered to that request
> also when sending a new version of an entire series. In that case I'll
> *not* submit in-reply-to in the future, got that.

You'll probably get a different reply from everyone you ask. ;)

My rule of thumb is that if you update individual patches (whether in a
series or not), send them in-reply to the previous version. Each patch
in-reply to its preceding version in the series. If you update more than
about half the patches in a series, it's perhaps less confusing to send
a new series with no in-reply-to. (Oh, and this contradicts with the
example in git send-email man page.)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 0/3] fbdev fixes (reviewed)

[Daniel Vetter]

On Fri, Nov 13, 2015 at 09:06:29AM +0200, Jani Nikula wrote:
> On Thu, 12 Nov 2015, Lukas Wunner <lukas@wunner.de> wrote:
> >> For future reference, please consider posting new versions of series as
> >> new threads. This one got pretty messy in the end, with so many
> >> different versions.
> >
> > Daniel asked me to submit a patch "in-reply the previous version" in
> > <20150922091757.GZ3383@phenom.ffwll.local> and I adhered to that request
> > also when sending a new version of an entire series. In that case I'll
> > *not* submit in-reply-to in the future, got that.
> 
> You'll probably get a different reply from everyone you ask. ;)
> 
> My rule of thumb is that if you update individual patches (whether in a
> series or not), send them in-reply to the previous version. Each patch
> in-reply to its preceding version in the series. If you update more than
> about half the patches in a series, it's perhaps less confusing to send
> a new series with no in-reply-to. (Oh, and this contradicts with the
> example in git send-email man page.)

I have the same rule-of-thumb, but probably failed to convey it clearly.
Sorry for the confusion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 0/2] fbdev fixes (need review)

[Daniel Vetter]

On Sun, Nov 08, 2015 at 01:56:53PM +0100, Lukas Wunner wrote:
> On Fri, Oct 30, 2015 at 07:23:45PM +0100, Daniel Vetter wrote:
> > I don't think there's a leak here really. We always assign ifbdev->fb in
> > intelfb_alloc, which means the fbdev teardown code will take care of it.
> > The correct approach is probably to not unref anything at all, or if we
> > unref then we have to clear ifbdev->fb (since it's the reference that very
> > pointer is holding that we're clearing up).
> 
> Oh, right, and ifbdev->helper.fb would have to be set to NULL as well.
> 
> However it turns out the fbdev teardown code can't cope with an fbdev
> that's only partially initialized, it lacks proper null pointer checks.
> Patch 1 in this series fixes that.
> 
> Patch 2 is another attempt at handling failure of intelfb_create():
> On failure, ifbdev->helper.fbdev will be released by intelfb_create()
> but the intel_framebuffer and the gem_object are NOT unrefed. To make
> up for this, intel_fbdev_initial_config() will call intel_fbdev_fini(),
> which not only unrefs the intel_framebuffer and the gem_object but also
> frees the ifbdev. It's as if CONFIG_DRM_FBDEV_EMULATION wasn't set.
> 
> Not sure if this is the right way to go but seems more sensible to me
> than keeping an fbdev around that's only halfway initialized. We've
> kicked out the VGA console at this point but failed to initialize an
> fbdev, the console will thus be unusable. If X11 manages to start up
> without errors, it will at least be able to use the memory that would
> otherwise be hogged by the unusable fbdev.
> 
> Note that if you find patch 2 acceptable, the modifications in patch 1
> to the following functions would actually become optional since they
> already check if ifbdev is a null pointer (let me know if you want these
> dropped): intel_fbdev_set_suspend(), intel_fbdev_output_poll_changed(),
> intel_fbdev_restore_mode().

Yeah I like patch 2, and if we merge that before patch 1 then we can
simplify the code like you describe since dev_priv->fbdev always implies
the fb is there, and a valid fb means the obj is also there.

Can you please do that change and resend? Patches look good otherwise.

Thanks, Daniel

> 
> The patches are also browsable on GitHub:
> https://github.com/l1k/linux/commits/intel_fbdev_fixes
> 
> Best regards,
> 
> Lukas
> 
> 
> Lukas Wunner (2):
>   drm/i915: Fix oops caused by fbdev initialization failure
>   drm/i915: Tear down fbdev if initialization fails
> 
>  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
>  drivers/gpu/drm/i915/i915_dma.c     |  1 +
>  drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
>  drivers/gpu/drm/i915/intel_fbdev.c  | 25 +++++++++++++++----------
>  4 files changed, 37 insertions(+), 23 deletions(-)
> 
> -- 
> 1.8.5.2 (Apple Git-48)
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx