From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jesse Barnes <jbarnes@virtuousgeek.org>
Subject: Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS
 framebuffer and preserve for KMS fbcon v7
Date: Tue, 17 Dec 2013 13:30:12 -0800
Message-ID: <20131217133012.10c197d2@jbarnes-desktop>
References: <1387240469-932-1-git-send-email-jbarnes@virtuousgeek.org>
	<1387240469-932-6-git-send-email-jbarnes@virtuousgeek.org>
	<20131217103439.GA22448@nuc-i3427.alporthouse.com>
	<20131217130529.68cb2b68@jbarnes-desktop>
	<CAKMK7uF=NwkcXRY0xz-Hto2d_8nReszPyrhsNJ8eZ0DqPqUwDw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces@lists.freedesktop.org>
Received: from oproxy9-pub.mail.unifiedlayer.com
	(oproxy9-pub.mail.unifiedlayer.com [69.89.24.6])
	by gabe.freedesktop.org (Postfix) with SMTP id 4923DFD334
	for <intel-gfx@lists.freedesktop.org>;
	Tue, 17 Dec 2013 13:28:22 -0800 (PST)
In-Reply-To: <CAKMK7uF=NwkcXRY0xz-Hto2d_8nReszPyrhsNJ8eZ0DqPqUwDw@mail.gmail.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces@lists.freedesktop.org
Errors-To: intel-gfx-bounces@lists.freedesktop.org
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
List-Id: intel-gfx@lists.freedesktop.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