On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxx>
>
> 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@xxxxxxxxx>
> Fixes: 60a5ca015ffd ("drm/i915: Add locking around
> framebuffer_references--")
> Reported-by: Lukas Wunner <lukas@xxxxxxxxx>
> [Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
