| Message ID | e08c113c04191ecd66904423da4f1908f48b5ef6.1445771693.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Oct 24, 2015 at 12:27:57AM +0200, Lukas Wunner wrote: > intelfb_create() is called once on driver initialization. It either > uses the fb inherited from BIOS or allocates a new one by calling > intelfb_alloc(). Afterwards, it calls two functions which can fail: > > - drm_fb_helper_alloc_fbi() can fail with -ENOMEM. > - ioremap_wc() can fail on alignment issues etc. > > In the case where we allocated the fb with intelfb_alloc(), if either > of the two functions fail we currently unpin and unref the bo, > but leak the fb. We need to unref the fb instead of the bo here. > > In the case where the fb was inherited from BIOS, the fb struct was > allocated by dev_priv->display.get_initial_plane_config() and is in > active use by a crtc, so it seems wrong to unpin and unref its bo. > We could treat failure of the above two functions as a fatal error > and call i915_driver_unload(), or try to limp on (fbcon won't work, > but X11 might). Let's do the latter. > > However we should at least log an error message. Currently a failure > of the above two functions is not reported at all: The negative return > value is passed up the call stack to intel_fbdev_initial_config() > which ignores it. > > To be fair, this is a corner case which few users probably ever > experience, nevertheless we should try to get it right. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> I don't think there's a leak here really. We always assign ifbdev->fb in intelfb_alloc, which means the fbdev teardown code will take care of it. The correct approach is probably to not unref anything at all, or if we unref then we have to clear ifbdev->fb (since it's the reference that very pointer is holding that we're clearing up). -Daniel > --- > drivers/gpu/drm/i915/intel_fbdev.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 12597b5..b8c11a1 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > > info = drm_fb_helper_alloc_fbi(helper); > if (IS_ERR(info)) { > + DRM_ERROR("Failed to allocate fb_info\n"); > ret = PTR_ERR(info); > goto out_unpin; > } > @@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj), > size); > if (!info->screen_base) { > + DRM_ERROR("Failed to remap framebuffer into virtual memory\n"); > ret = -ENOSPC; > goto out_destroy_fbi; > } > @@ -284,9 +286,14 @@ static int intelfb_create(struct drm_fb_helper *helper, > out_destroy_fbi: > drm_fb_helper_release_fbi(helper); > out_unpin: > - i915_gem_object_ggtt_unpin(obj); > - drm_gem_object_unreference(&obj->base); > - mutex_unlock(&dev->struct_mutex); > + /* If fb was preallocated by BIOS, try to limp on. Else free it. */ > + if (prealloc) > + mutex_unlock(&dev->struct_mutex); > + else { > + i915_gem_object_ggtt_unpin(obj); > + mutex_unlock(&dev->struct_mutex); > + drm_framebuffer_unreference(&intel_fb->base); > + } > return ret; > } > > -- > 1.8.5.2 (Apple Git-48) > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 30, 2015 at 07:23:45PM +0100, Daniel Vetter wrote: > I don't think there's a leak here really. We always assign ifbdev->fb in > intelfb_alloc, which means the fbdev teardown code will take care of it. > The correct approach is probably to not unref anything at all, or if we > unref then we have to clear ifbdev->fb (since it's the reference that very > pointer is holding that we're clearing up). Oh, right, and ifbdev->helper.fb would have to be set to NULL as well. However it turns out the fbdev teardown code can't cope with an fbdev that's only partially initialized, it lacks proper null pointer checks. Patch 1 in this series fixes that. Patch 2 is another attempt at handling failure of intelfb_create(): On failure, ifbdev->helper.fbdev will be released by intelfb_create() but the intel_framebuffer and the gem_object are NOT unrefed. To make up for this, intel_fbdev_initial_config() will call intel_fbdev_fini(), which not only unrefs the intel_framebuffer and the gem_object but also frees the ifbdev. It's as if CONFIG_DRM_FBDEV_EMULATION wasn't set. Not sure if this is the right way to go but seems more sensible to me than keeping an fbdev around that's only halfway initialized. We've kicked out the VGA console at this point but failed to initialize an fbdev, the console will thus be unusable. If X11 manages to start up without errors, it will at least be able to use the memory that would otherwise be hogged by the unusable fbdev. Note that if you find patch 2 acceptable, the modifications in patch 1 to the following functions would actually become optional since they already check if ifbdev is a null pointer (let me know if you want these dropped): intel_fbdev_set_suspend(), intel_fbdev_output_poll_changed(), intel_fbdev_restore_mode(). The patches are also browsable on GitHub: https://github.com/l1k/linux/commits/intel_fbdev_fixes Best regards, Lukas Lukas Wunner (2): drm/i915: Fix oops caused by fbdev initialization failure drm/i915: Tear down fbdev if initialization fails drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++----------- drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++-- drivers/gpu/drm/i915/intel_fbdev.c | 25 +++++++++++++++---------- 4 files changed, 37 insertions(+), 23 deletions(-)
On Sun, Nov 08, 2015 at 01:56:53PM +0100, Lukas Wunner wrote: > On Fri, Oct 30, 2015 at 07:23:45PM +0100, Daniel Vetter wrote: > > I don't think there's a leak here really. We always assign ifbdev->fb in > > intelfb_alloc, which means the fbdev teardown code will take care of it. > > The correct approach is probably to not unref anything at all, or if we > > unref then we have to clear ifbdev->fb (since it's the reference that very > > pointer is holding that we're clearing up). > > Oh, right, and ifbdev->helper.fb would have to be set to NULL as well. > > However it turns out the fbdev teardown code can't cope with an fbdev > that's only partially initialized, it lacks proper null pointer checks. > Patch 1 in this series fixes that. > > Patch 2 is another attempt at handling failure of intelfb_create(): > On failure, ifbdev->helper.fbdev will be released by intelfb_create() > but the intel_framebuffer and the gem_object are NOT unrefed. To make > up for this, intel_fbdev_initial_config() will call intel_fbdev_fini(), > which not only unrefs the intel_framebuffer and the gem_object but also > frees the ifbdev. It's as if CONFIG_DRM_FBDEV_EMULATION wasn't set. > > Not sure if this is the right way to go but seems more sensible to me > than keeping an fbdev around that's only halfway initialized. We've > kicked out the VGA console at this point but failed to initialize an > fbdev, the console will thus be unusable. If X11 manages to start up > without errors, it will at least be able to use the memory that would > otherwise be hogged by the unusable fbdev. > > Note that if you find patch 2 acceptable, the modifications in patch 1 > to the following functions would actually become optional since they > already check if ifbdev is a null pointer (let me know if you want these > dropped): intel_fbdev_set_suspend(), intel_fbdev_output_poll_changed(), > intel_fbdev_restore_mode(). Yeah I like patch 2, and if we merge that before patch 1 then we can simplify the code like you describe since dev_priv->fbdev always implies the fb is there, and a valid fb means the obj is also there. Can you please do that change and resend? Patches look good otherwise. Thanks, Daniel > > The patches are also browsable on GitHub: > https://github.com/l1k/linux/commits/intel_fbdev_fixes > > Best regards, > > Lukas > > > Lukas Wunner (2): > drm/i915: Fix oops caused by fbdev initialization failure > drm/i915: Tear down fbdev if initialization fails > > drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++----------- > drivers/gpu/drm/i915/i915_dma.c | 1 + > drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++-- > drivers/gpu/drm/i915/intel_fbdev.c | 25 +++++++++++++++---------- > 4 files changed, 37 insertions(+), 23 deletions(-) > > -- > 1.8.5.2 (Apple Git-48) >
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 12597b5..b8c11a1 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper, info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { + DRM_ERROR("Failed to allocate fb_info\n"); ret = PTR_ERR(info); goto out_unpin; } @@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper, ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj), size); if (!info->screen_base) { + DRM_ERROR("Failed to remap framebuffer into virtual memory\n"); ret = -ENOSPC; goto out_destroy_fbi; } @@ -284,9 +286,14 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi: drm_fb_helper_release_fbi(helper); out_unpin: - i915_gem_object_ggtt_unpin(obj); - drm_gem_object_unreference(&obj->base); - mutex_unlock(&dev->struct_mutex); + /* If fb was preallocated by BIOS, try to limp on. Else free it. */ + if (prealloc) + mutex_unlock(&dev->struct_mutex); + else { + i915_gem_object_ggtt_unpin(obj); + mutex_unlock(&dev->struct_mutex); + drm_framebuffer_unreference(&intel_fb->base); + } return ret; }
intelfb_create() is called once on driver initialization. It either uses the fb inherited from BIOS or allocates a new one by calling intelfb_alloc(). Afterwards, it calls two functions which can fail: - drm_fb_helper_alloc_fbi() can fail with -ENOMEM. - ioremap_wc() can fail on alignment issues etc. In the case where we allocated the fb with intelfb_alloc(), if either of the two functions fail we currently unpin and unref the bo, but leak the fb. We need to unref the fb instead of the bo here. In the case where the fb was inherited from BIOS, the fb struct was allocated by dev_priv->display.get_initial_plane_config() and is in active use by a crtc, so it seems wrong to unpin and unref its bo. We could treat failure of the above two functions as a fatal error and call i915_driver_unload(), or try to limp on (fbcon won't work, but X11 might). Let's do the latter. However we should at least log an error message. Currently a failure of the above two functions is not reported at all: The negative return value is passed up the call stack to intel_fbdev_initial_config() which ignores it. To be fair, this is a corner case which few users probably ever experience, nevertheless we should try to get it right. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/gpu/drm/i915/intel_fbdev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)