On Sun, Nov 08, 2015 at 01:56:53PM +0100, Lukas Wunner wrote: > On Fri, Oct 30, 2015 at 07:23:45PM +0100, Daniel Vetter wrote: > > I don't think there's a leak here really. We always assign ifbdev->fb in > > intelfb_alloc, which means the fbdev teardown code will take care of it. > > The correct approach is probably to not unref anything at all, or if we > > unref then we have to clear ifbdev->fb (since it's the reference that very > > pointer is holding that we're clearing up). > > Oh, right, and ifbdev->helper.fb would have to be set to NULL as well. > > However it turns out the fbdev teardown code can't cope with an fbdev > that's only partially initialized, it lacks proper null pointer checks. > Patch 1 in this series fixes that. > > Patch 2 is another attempt at handling failure of intelfb_create(): > On failure, ifbdev->helper.fbdev will be released by intelfb_create() > but the intel_framebuffer and the gem_object are NOT unrefed. To make > up for this, intel_fbdev_initial_config() will call intel_fbdev_fini(), > which not only unrefs the intel_framebuffer and the gem_object but also > frees the ifbdev. It's as if CONFIG_DRM_FBDEV_EMULATION wasn't set. > > Not sure if this is the right way to go but seems more sensible to me > than keeping an fbdev around that's only halfway initialized. We've > kicked out the VGA console at this point but failed to initialize an > fbdev, the console will thus be unusable. If X11 manages to start up > without errors, it will at least be able to use the memory that would > otherwise be hogged by the unusable fbdev. > > Note that if you find patch 2 acceptable, the modifications in patch 1 > to the following functions would actually become optional since they > already check if ifbdev is a null pointer (let me know if you want these > dropped): intel_fbdev_set_suspend(), intel_fbdev_output_poll_changed(), > intel_fbdev_restore_mode(). Yeah I like patch 2, and if we merge that before patch 1 then we can simplify the code like you describe since dev_priv->fbdev always implies the fb is there, and a valid fb means the obj is also there. Can you please do that change and resend? Patches look good otherwise. Thanks, Daniel > > The patches are also browsable on GitHub: > https://github.com/l1k/linux/commits/intel_fbdev_fixes > > Best regards, > > Lukas > > > Lukas Wunner (2): > drm/i915: Fix oops caused by fbdev initialization failure > drm/i915: Tear down fbdev if initialization fails > > drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++----------- > drivers/gpu/drm/i915/i915_dma.c | 1 + > drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++-- > drivers/gpu/drm/i915/intel_fbdev.c | 25 +++++++++++++++---------- > 4 files changed, 37 insertions(+), 23 deletions(-) > > -- > 1.8.5.2 (Apple Git-48) > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
