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

[Daniel Vetter]

On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
>> On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes <jbarnes at 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)

Imo if a core piece of the driver fails to initialize we should just
fail driver loading. We've had tons of crazy lore around contexts
failing, ppgtt failing and other similar stuff, and I think it just
makes the normal code more fragile with no real gain.

If you think something slips through the cracks maybe just throw a
BUG_ON in there instead.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch