Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 17 Dec 2013 22:17:22 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
> >> > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
> >> >  void intel_fbdev_output_poll_changed(struct drm_device *dev)
> >> >  {
> >> >     struct drm_i915_private *dev_priv = dev->dev_private;
> >> > -   drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >> > +   if (dev_priv->fbdev)
> >> > +           drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >> >  }
> >>
> >> Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
> >
> > Fixed.
> 
> I still don't get why we need this check - for CONFIG_FB=n we have a
> special dummy function and we are really careful in the setup code to
> only enable the interrupt handling code once fbdev is fully set up. Or
> do I miss some change here which makes this required? If so the right
> fix imo would be to shuffle the init sequence again (and update all
> the tons of comments about it, ofc).

In the init code I'm more careful now to avoid leaving a bogus
pointer around:

 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
 				 INTEL_INFO(dev)->num_pipes,
 				 4);
 	if (ret) {
+		dev_priv->fbdev = NULL;
 		kfree(ifbdev);
 		return ret;
 	}

So in the unlikely event that the fb helper code fails I don't want to
fall over.

But that shouldn't happen in practice.  I only have the checks in place
to catch when I failed to set the fbdev field in one path (which is now
fixed).

-- 
Jesse Barnes, Intel Open Source Technology Center