From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Jani Nikula <jani.nikula@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation
Date: Tue, 13 Oct 2015 18:04:40 +0300 [thread overview]
Message-ID: <20151013150440.GR26517@intel.com> (raw)
In-Reply-To: <f0cd66427039ce1bdc61460a9d833e6d858cff3e.1441363361.git.lukas@wunner.de>
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
next prev parent reply other threads:[~2015-10-13 15:05 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-30 9:06 [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
2015-07-04 9:50 ` [PATCH v5 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
2015-10-13 15:39 ` Ville Syrjälä
2015-10-13 15:04 ` Ville Syrjälä [this message]
2015-10-14 9:35 ` [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation Chris Wilson
2015-10-15 17:14 ` Lukas Wunner
2015-10-15 17:22 ` Daniel Vetter
2015-10-15 17:34 ` Ville Syrjälä
2015-10-18 18:03 ` Lukas Wunner
2015-10-25 11:14 ` [PATCH v6 0/4] fbdev deadlock & failure path fixes Lukas Wunner
2015-06-30 9:06 ` [PATCH v6 3/4] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
2015-10-30 18:28 ` Daniel Vetter
2015-11-07 10:41 ` [PATCH v7 0/3] fbdev fixes (reviewed) Lukas Wunner
2015-06-30 9:06 ` [PATCH v7 3/3] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
2015-07-04 9:50 ` [PATCH v7 1/3] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
2015-10-22 11:37 ` [PATCH v7 2/3] drm/i915: Fix double unref in intelfb_alloc failure path Lukas Wunner
2015-11-09 14:23 ` [PATCH v7 0/3] fbdev fixes (reviewed) Jani Nikula
2015-11-12 19:20 ` Lukas Wunner
2015-11-13 7:06 ` Jani Nikula
2015-11-17 13:44 ` Daniel Vetter
2015-07-04 9:50 ` [PATCH v6 1/4] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
2015-10-22 11:37 ` [PATCH v6 2/4] drm/i915: Fix double unref in intelfb_alloc failure path Lukas Wunner
2015-10-30 18:17 ` Daniel Vetter
2015-10-23 22:27 ` [PATCH v6 4/4] drm/i915: Fix error handling in intelfb_create Lukas Wunner
2015-10-30 18:23 ` Daniel Vetter
2015-11-08 12:56 ` [PATCH v2 0/2] fbdev fixes (need review) Lukas Wunner
2015-11-03 7:00 ` [PATCH v2 2/2] drm/i915: Tear down fbdev if initialization fails Lukas Wunner
2015-11-05 9:42 ` [PATCH v2 1/2] drm/i915: Fix oops caused by fbdev initialization failure Lukas Wunner
2015-11-17 13:51 ` [PATCH v2 0/2] fbdev fixes (need review) Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151013150440.GR26517@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=lukas@wunner.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.