Re: [PATCH v2 0/2] fbdev fixes (need review)

[Daniel Vetter <daniel@ffwll.ch>]

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx