drm/i915: Fix failure paths around initial fbdev allocation
Subsystems (2)
PATCH v5 1/2 drm/i915: Fix failure paths around initial fbdev allocation
From: Tvrtko Ursulin <[email protected]>
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)
[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
[MBP 11,3 2013 intel HSW + nvidia GK107 retina]
framebuffer_references--")
--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +12 -8
@@ -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; }
Signature · 6 lines
2.1.0 _______________________________________________ Intel-gfx mailing list [email protected] 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
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.]
[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
[MBP 11,3 2013 intel HSW + nvidia GK107 retina]
allocation")
Cc: Chris Wilson <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Ville Syrjälä <[email protected]>
--- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/intel_display.c +14 -7
@@ -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
Signature · 6 lines
2.1.0 _______________________________________________ Intel-gfx mailing list [email protected] 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
Quoted 92 lines · 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 inintel_framebuffer_create_for_mode(viai915_gem_alloc_object) and inintel_user_framebuffer_create(viadrm_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_createto where the gem object gets refed.[Changelog is in preceding patch by Tvrtko Ursulin.]
Tested-by Pierre Moreau <[email protected]>[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina]Signed-off-by Lukas Wunner <[email protected]>Fixes a8bb6818270c ("drm/i915: Fix error path leak in fbdev fballocation")
Cc: Chris Wilson <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Ville Syrjälä <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)drivers/gpu/drm/i915/intel_display.c +9
-6@@ -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,
drivers/gpu/drm/i915/intel_display.c intel_user_framebuffer_create(struct drm_device *dev, · line 14306
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c +5
-1@@ -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
Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +0
-0}#ifndef CONFIG_DRM_FBDEV_EMULATION Signature · 1 line
2.1.0
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 83 lines · Lukas Wunner
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock inintelfb_allocfailure path where it callsdrm_framebuffer_remove, which grabs the struct mutex andintelfb_create(caller ofintelfb_alloc) was already holding it.2.
Deadlock inintelfb_createfailure path where it callsdrm_framebuffer_unreference, which grabs the struct mutex andintelfb_createwas 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 <[email protected]>[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina]Signed-off-by Tvrtko Ursulin <[email protected]>Fixes 60a5ca015ffd ("drm/i915: Add locking aroundframebuffer_references--")
Reported-by Lukas Wunner <[email protected]>[Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]Signed-off-by Lukas Wunner <[email protected]>Cc Chris Wilson <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +4
-2@@ -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; }
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 173
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-2+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.
Quoted 26 lines
drivers/gpu/drm/i915/intel_fbdev.c +4
-3out: +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" );
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 222
Quoted 5 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -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.
drivers/gpu/drm/i915/intel_fbdev.c out_destroy_fbi: · line 283
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-0info =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.
Quoted 6 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-1-out_unlock:mutex_unlock (&dev->struct_mutex);return ret; }Signature · 1 line
2.1.0
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 5 lines · Ville Syrjälä
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Ville,
Quoted 103 lines · Ville Syrjälä
Quoted 83 lines · Lukas Wunner
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock inintelfb_allocfailure path where it callsdrm_framebuffer_remove, which grabs the struct mutex andintelfb_create(caller ofintelfb_alloc) was already holding it.2.
Deadlock inintelfb_createfailure path where it callsdrm_framebuffer_unreference, which grabs the struct mutex andintelfb_createwas 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 <[email protected]>[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina]Signed-off-by Tvrtko Ursulin <[email protected]>Fixes 60a5ca015ffd ("drm/i915: Add locking aroundframebuffer_references--")
Reported-by Lukas Wunner <[email protected]>[Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]Signed-off-by Lukas Wunner <[email protected]>Cc Chris Wilson <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +4
-2@@ -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; }drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 173
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-2+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.
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 180
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +3
-0out: +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?
Quoted 28 lines
Quoted 19 lines
drivers/gpu/drm/i915/intel_fbdev.c +1
-3@@ -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" );drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 222
Quoted 5 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -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.
Quoted 9 lines
drivers/gpu/drm/i915/intel_fbdev.c out_destroy_fbi: · line 283
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-0info =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
Quoted 11 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-1-out_unlock:mutex_unlock (&dev->struct_mutex);return ret; }Signature · 1 line
2.1.0Signature · 2 lines
Ville Syrjälä Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 123 lines · Lukas Wunner
Hi Ville,
Quoted 103 lines · Ville Syrjälä
Quoted 83 lines · Lukas Wunner
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock inintelfb_allocfailure path where it callsdrm_framebuffer_remove, which grabs the struct mutex andintelfb_create(caller ofintelfb_alloc) was already holding it.2.
Deadlock inintelfb_createfailure path where it callsdrm_framebuffer_unreference, which grabs the struct mutex andintelfb_createwas 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 <[email protected]>[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina]Signed-off-by Tvrtko Ursulin <[email protected]>Fixes 60a5ca015ffd ("drm/i915: Add locking aroundframebuffer_references--")
Reported-by Lukas Wunner <[email protected]>[Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]Signed-off-by Lukas Wunner <[email protected]>Cc Chris Wilson <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +4
-2@@ -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; }drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 173
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-2+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.
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 180
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +3
-0out: +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 callsintel_framebuffer_init(), which callsdrm_framebuffer_init(), which callskref_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.
Quoted 6 lines
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 aPTR_ERR(), so not null, and we'd calldrm_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
Quoted 73 lines
Quoted 30 lines
Quoted 21 lines
drivers/gpu/drm/i915/intel_fbdev.c +1
-3@@ -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" );drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 222
Quoted 5 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -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.
Quoted 9 lines
drivers/gpu/drm/i915/intel_fbdev.c out_destroy_fbi: · line 283
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-0info =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 thatintelfb_alloc()carried out. Thedrm_gem_object_unreference()therefore seems right here to me.However I'm puzzled why we don't call
drm_framebuffer_remove()under theout_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
Quoted 11 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-1-out_unlock:mutex_unlock (&dev->struct_mutex);return ret; }Signature · 1 line
2.1.0Signature · 2 lines
Ville Syrjälä Intel OTC_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 106 lines · Lukas Wunner
Hi Ville,
Quoted 103 lines · Ville Syrjälä
Quoted 83 lines · Lukas Wunner
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock inintelfb_allocfailure path where it callsdrm_framebuffer_remove, which grabs the struct mutex andintelfb_create(caller ofintelfb_alloc) was already holding it.2.
Deadlock inintelfb_createfailure path where it callsdrm_framebuffer_unreference, which grabs the struct mutex andintelfb_createwas 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 <[email protected]>[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina]Signed-off-by Tvrtko Ursulin <[email protected]>Fixes 60a5ca015ffd ("drm/i915: Add locking aroundframebuffer_references--")
Reported-by Lukas Wunner <[email protected]>[Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]Signed-off-by Lukas Wunner <[email protected]>Cc Chris Wilson <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +4
-2@@ -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; }drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 173
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-2+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.
Quoted 9 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 180
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +3
-0out: +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.
Quoted 7 lines
A bit further up in this function we call
__intel_framebuffer_create()which sets the refcount to 1. (It callsintel_framebuffer_init(), which callsdrm_framebuffer_init(), which callskref_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.
Quoted 7 lines
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 aPTR_ERR(), so not null, and we'd calldrm_framebuffer_remove()on this. Is that what you meant?
No, but that's a good observation too.
Quoted 40 lines
Quoted 30 lines
Quoted 21 lines
drivers/gpu/drm/i915/intel_fbdev.c +1
-3@@ -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" );drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 222
Quoted 5 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -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.
Quoted 9 lines
drivers/gpu/drm/i915/intel_fbdev.c out_destroy_fbi: · line 283
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-0info =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.
Quoted 3 lines
Why? We did call
intelfb_alloc()above, so if something subsequently goes wrong, we need to revert the steps thatintelfb_alloc()carried out. Thedrm_gem_object_unreference()therefore seems right here to me.
Here too the fb (if succesfully created) now owns that reference.
Quoted 26 lines
However I'm puzzled why we don't call
drm_framebuffer_remove()under theout_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
Quoted 11 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-1-out_unlock:mutex_unlock (&dev->struct_mutex);return ret; }Signature · 1 line
2.1.0Signature · 2 lines
Ville Syrjälä Intel OTC
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Ville,
Quoted 117 lines · Ville Syrjälä
Quoted 106 lines · Lukas Wunner
Hi Ville,
Quoted 103 lines · Ville Syrjälä
Quoted 83 lines · Lukas Wunner
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock inintelfb_allocfailure path where it callsdrm_framebuffer_remove, which grabs the struct mutex andintelfb_create(caller ofintelfb_alloc) was already holding it.2.
Deadlock inintelfb_createfailure path where it callsdrm_framebuffer_unreference, which grabs the struct mutex andintelfb_createwas 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 <[email protected]>[MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina]Signed-off-by Tvrtko Ursulin <[email protected]>Fixes 60a5ca015ffd ("drm/i915: Add locking aroundframebuffer_references--")
Reported-by Lukas Wunner <[email protected]>[Lukas: Create v3 + v4 + v5 based on Tvrtko's v2]Signed-off-by Lukas Wunner <[email protected]>Cc Chris Wilson <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +4
-2@@ -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; }drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 173
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-2+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.
Quoted 9 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_alloc(struct drm_fb_helper *helper, · line 180
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +3
-0out: +mutex_unlock (&dev->struct_mutex); +if (fb) +drm_framebuffer_remove (fb);return ret; }Hm, why do you think we unref one too many times?
Quoted 2 lines
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.
Quoted 60 lines
Quoted 7 lines
A bit further up in this function we call
__intel_framebuffer_create()which sets the refcount to 1. (It callsintel_framebuffer_init(), which callsdrm_framebuffer_init(), which callskref_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.
Quoted 7 lines
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 aPTR_ERR(), so not null, and we'd calldrm_framebuffer_remove()on this. Is that what you meant?No, but that's a good observation too.
Quoted 40 lines
Quoted 39 lines
Quoted 21 lines
drivers/gpu/drm/i915/intel_fbdev.c +1
-3@@ -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" );drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 222
Quoted 5 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -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.
drivers/gpu/drm/i915/intel_fbdev.c out_destroy_fbi: · line 283
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-0info =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.
Quoted 5 lines
Quoted 3 lines
Why? We did call
intelfb_alloc()above, so if something subsequently goes wrong, we need to revert the steps thatintelfb_alloc()carried out. Thedrm_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
Quoted 22 lines
Quoted 22 lines
However I'm puzzled why we don't call
drm_framebuffer_remove()under theout_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
Quoted 8 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-1-out_unlock:mutex_unlock (&dev->struct_mutex);return ret; }Signature · 1 line
2.1.0
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Signature · 2 lines
Ville Syrjälä Intel OTCSignature · 2 lines
Ville Syrjälä Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
v6 cover fbdev deadlock & failure path fixes
In reply to Ville Syrjälä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(-)
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] 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
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)
[MBP 5,3 2009 nvidia MCP79 + G96 pre-retina]
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
[MBP 11,3 2013 intel HSW + nvidia GK107 retina]
allocation")
--- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/intel_display.c +14 -7
@@ -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
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v6 2/4 drm/i915: Fix double unref in intelfb_alloc failure path
In reply to Lukas WunnerIn 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.
--- drivers/gpu/drm/i915/intel_fbdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +2 -3
@@ -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; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v6 2/4] drm/i915: Fix double unref in intelfb_alloc failure path
Quoted 6 lines · Lukas Wunner
In
intelfb_alloc(), if the call tointel_pin_and_fence_fb_obj()fails, the bo is unrefed twice: Bydrm_framebuffer_remove()and once more bydrm_gem_object_unreference(). Fix it.Reported-by Ville Syrjälä <[email protected]>Signed-off-by Lukas Wunner <[email protected]>
Quoted 35 lines
--- drivers/gpu/drm/i915/intel_fbdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +2
-3@@ -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; }Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v6 3/4 drm/i915: Fix failure paths around initial fbdev allocation
In reply to Lukas WunnerFrom: Tvrtko Ursulin <[email protected]>
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)
framebuffer_references--")
--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +12 -8
@@ -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; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 9 lines · Lukas Wunner
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock inintelfb_allocfailure path where it callsdrm_framebuffer_remove, which grabs the struct mutex andintelfb_create(caller ofintelfb_alloc) was already holding it.
Quoted 8 lines
2.
Deadlock inintelfb_createfailure path where it callsdrm_framebuffer_unreference, which grabs the struct mutex andintelfb_createwas already holding it.[Chris Wilson on why
struct_mutexneeds to be locked in the second half ofintelfb_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
Quoted 118 lines
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:
* Inintelfb_alloc, if__intel_framebuffer_createfailed, fb will be anERR_PTR, thus not null. So in the failure path we need to check forIS_ERR_OR_NULLto avoid callingdrm_framebuffer_removeon theERR_PTR. (Lukas Wunner)
* Since this is init code adrm_framebuffer_unreferenceshould be all we need.drm_framebuffer_removeis for framebuffers that userspace has created - and is getting somewhat defeatured. (Daniel Vetter)Signed-off-by Tvrtko Ursulin <[email protected]>Fixes 60a5ca015ffd ("drm/i915: Add locking aroundframebuffer_references--")
Reported-by Lukas Wunner <[email protected]>[Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]Signed-off-by Lukas Wunner <[email protected]>Cc Chris Wilson <[email protected]>Cc Ville Syrjälä <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +12
-8@@ -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; }Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
v7 cover fbdev fixes (reviewed)
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 <[email protected]>.
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, <[email protected]>) 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(-)
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] 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
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)
[MBP 5,3 2009 nvidia MCP79 + G96 pre-retina]
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
[MBP 11,3 2013 intel HSW + nvidia GK107 retina]
allocation")
--- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/intel_display.c +14 -7
@@ -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
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v7 2/3 drm/i915: Fix double unref in intelfb_alloc failure path
In reply to Lukas WunnerIn 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.
--- drivers/gpu/drm/i915/intel_fbdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +2 -3
@@ -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; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v7 3/3 drm/i915: Fix failure paths around initial fbdev allocation
In reply to Lukas WunnerFrom: Tvrtko Ursulin <[email protected]>
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)
framebuffer_references--")
--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +12 -8
@@ -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; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v7 0/3] fbdev fixes (reviewed)
In reply to Lukas WunnerQuoted 12 lines · 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 <[email protected]>.
Patch 1 fixes double unref of bo in failure path of
intelfb_alloc(). Patch 2 fixes another double unref of bo in failure path ofintelfb_alloc(). Patch 3 fixes deadlocks inintelfb_create(), one during driver initialization and one in failure path.
Quoted 5 lines
Browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixesThe previous iteration (v6, <[email protected]>) 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
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v7 0/3] fbdev fixes (reviewed)
Hi Jani,
Quoted 7 lines · Jani Nikula
Quoted 4 lines · Lukas Wunner
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 <[email protected]>.
Pushed all three to drm-intel-next-queued. Thanks for the patches and review.
Thank you!
Quoted 4 lines
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.)
Quoted 3 lines
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 <[email protected]> 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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v7 0/3] fbdev fixes (reviewed)
Quoted 1 line · Lukas Wunner
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.
Quoted 8 lines
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. ;)
Quoted 8 lines
Quoted 3 lines
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 <[email protected]> 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.
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v7 0/3] fbdev fixes (reviewed)
Quoted 10 lines · Jani Nikula
Quoted 8 lines · Lukas Wunner
Quoted 3 lines
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 <[email protected]> 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.
Quoted 8 lines
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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v6 4/4 drm/i915: Fix error handling in intelfb_create
In reply to Lukas Wunnerintelfb_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.
--- drivers/gpu/drm/i915/intel_fbdev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +10 -3
@@ -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; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v6 4/4] drm/i915: Fix error handling in intelfb_create
Quoted 23 lines · Lukas Wunner
intelfb_create()is called once on driver initialization. It either uses the fb inherited from BIOS or allocates a new one by callingintelfb_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 calli915_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.
Quoted 4 lines
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 <[email protected]>
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
Quoted 49 lines
--- drivers/gpu/drm/i915/intel_fbdev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_fbdev.c +10
-3@@ -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; }Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
v2 cover fbdev fixes (need review)
Quoted 5 lines · Daniel Vetter
I don't think there's a leak here really. We always assign
ifbdev->fbinintelfb_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 clearifbdev->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(-)
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v2 1/2 drm/i915: Fix oops caused by fbdev initialization failure
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.
--- 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(-)
drivers/gpu/drm/i915/i915_debugfs.c +13 -11
@@ -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);
drivers/gpu/drm/i915/intel_dp_mst.c +8 -2
@@ -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 }
drivers/gpu/drm/i915/intel_fbdev.c +10 -6
@@ -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 synchronousstruct 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 synchronousvoid 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;
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v2 2/2 drm/i915: Tear down fbdev if initialization fails
In reply to Lukas WunnerCurrently 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().
--- drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/intel_fbdev.c | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_dma.c +1 -0
@@ -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);
drivers/gpu/drm/i915/intel_fbdev.c +5 -4
@@ -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 ;
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v2 0/2] fbdev fixes (need review)
In reply to Lukas WunnerQuoted 27 lines · Lukas Wunner
Quoted 5 lines · Daniel Vetter
I don't think there's a leak here really. We always assign
ifbdev->fbinintelfb_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 clearifbdev->fb(since it's the reference that very pointer is holding that we're clearing up).Oh, right, and
ifbdev->helper.fbwould 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.fbdevwill be released byintelfb_create()but theintel_framebufferand thegem_objectare NOT unrefed. To make up for this,intel_fbdev_initial_config()will callintel_fbdev_fini(), which not only unrefs theintel_framebufferand thegem_objectbut also frees the ifbdev. It's as ifCONFIG_DRM_FBDEV_EMULATIONwasn'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.
Quoted 5 lines
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
Quoted 22 lines
The patches are also browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixesBest regards,
Lukas
Lukas Wunner (2):
drm/i915: Fix oops caused by fbdev initialization failure drm/i915: Tear down fbdev if initialization failsdrivers/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(-)Signature · 1 line
1.8.5.2 (Apple Git-48)
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx