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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 12 Dec 2013 23:54:37 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> > Retrieve current framebuffer config info from the regs and create an fb
> > object for the buffer the BIOS or boot loader left us.  This should
> > allow for smooth transitions to userspace apps once we finish the
> > initial configuration construction.
> > 
> > v2: check for non-native modes and adjust (Jesse)
> >     fixup aperture and cmap frees (Imre)
> >     use unlocked unref if init_bios fails (Jesse)
> >     fix curly brace around DSPADDR check (Imre)
> >     comment failure path for pin_and_fence (Imre)
> > v3: fixup fixup of aperture frees (Chris)
> > v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> > v5: move fb config fetch to display code (Jesse)
> >     re-order hw state readout on initial load to suit fb inherit (Jesse)
> >     re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> > v6: rename to plane_config (Daniel)
> >     check for valid object when initializing BIOS fb (Jesse)
> >     split from plane_config readout and other display changes (Jesse)
> >     drop use_bios_fb option (Chris)
> >     update comments (Jesse)
> >     rework fbdev_init_bios for clarity (Jesse)
> >     drop fb obj ref under lock (Chris)
> > v7: use fb object from plane_config instead (Ville)
> >     take ref on fb object (Jesse)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> [snip]
> 
> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >  
> >  	drm_fb_helper_fini(&ifbdev->helper);
> >  
> > -	drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> > -	intel_framebuffer_fini(&ifbdev->ifb);
> > +	drm_framebuffer_unregister_private(&ifbdev->fb->base);
> > +	intel_framebuffer_fini(ifbdev->fb);
> > +	kfree(ifbdev->fb);
> 
> No need to go the private fb route here anymore since now the fb is
> free-standing. Normal refcounting should work. But a separate prep/cleanup
> patch (prep since switching ifbdev->fb from struct to point would look
> neat as a separate patch).
> 
> > +}
> > +
> > +/*
> > + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
> > + * The core display code will have read out the current plane configuration,
> > + * so we use that to figure out if there's an object for us to use as the
> > + * fb, and if so, we re-use it for the fbdev configuration.
> > + *
> > + * Note we only support a single fb shared across pipes for boot (mostly for
> > + * fbcon), so we just find the biggest and use that.
> > + */
> > +void intel_fbdev_init_bios(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_fbdev *ifbdev;
> > +	struct intel_framebuffer *fb = NULL;
> > +	struct drm_crtc *crtc;
> > +	struct intel_crtc *intel_crtc;
> > +	struct intel_plane_config *plane_config = NULL;
> > +	unsigned int last_size = 0;
> > +
> > +	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> > +	if (ifbdev == NULL) {
> > +		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> > +		return;
> > +	}
> > +
> > +	/* Find the largest framebuffer to use, then free the others */
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		intel_crtc = to_intel_crtc(crtc);
> > +
> > +		if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
> > +			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> > +				      pipe_name(intel_crtc->pipe));
> > +			continue;
> > +		}
> > +
> > +		if (intel_crtc->plane_config.size > last_size) {
> > +			plane_config = &intel_crtc->plane_config;
> > +			last_size = plane_config->size;
> > +			fb = plane_config->fb;
> > +		}
> > +	}
> > +
> > +	/* Free unused fbs */
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		struct intel_framebuffer *cur_fb;
> > +
> > +		intel_crtc = to_intel_crtc(crtc);
> > +		cur_fb = intel_crtc->plane_config.fb;
> > +
> > +		if (cur_fb && cur_fb != fb)
> > +			intel_framebuffer_fini(cur_fb);
> > +	}
> > +
> > +	if (!fb) {
> > +		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> > +		goto out_free;
> > +	}
> > +
> > +	ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> > +	ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > +	ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> > +	ifbdev->fb = fb;
> > +
> > +	/* Assuming a single fb across all pipes here */
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		intel_crtc = to_intel_crtc(crtc);
> > +
> > +		if (!intel_crtc->active)
> > +			continue;
> > +
> > +		/*
> > +		 * This should only fail on the first one so we don't need
> > +		 * to cleanup any secondary crtc->fbs
> > +		 */
> > +		if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
> > +			goto out_unref_obj;
> > +
> > +		crtc->fb = &fb->base;
> > +		drm_gem_object_reference(&fb->obj->base);
> > +		drm_framebuffer_reference(&fb->base);
> > +	}
> > +
> > +	dev_priv->fbdev = ifbdev;
> > +
> > +	DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> > +	return;
> > +
> > +out_unref_obj:
> > +	intel_framebuffer_fini(fb);
> > +out_free:
> > +	kfree(ifbdev);
> >  }
> >  
> >  int intel_fbdev_init(struct drm_device *dev)
> > @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > -	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> > -	if (!ifbdev)
> > -		return -ENOMEM;
> > +	/* This may fail, if so, dev_priv->fbdev won't be set below */
> 
> If you need a comment to explain your control flow, it's probably too
> clever ;-)

I could add a return value I suppose, but I didn't think it was too
clever...

> 
> > +	intel_fbdev_init_bios(dev);
> >  
> > -	dev_priv->fbdev = ifbdev;
> > -	ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > +	if ((ifbdev = dev_priv->fbdev) == NULL) {
> > +		ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> > +		if (ifbdev == NULL)
> > +			return -ENOMEM;
> > +
> > +		ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > +		ifbdev->preferred_bpp = 32;
> > +
> > +		dev_priv->fbdev = ifbdev;
> > +	}
> >  
> >  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
> >  				 INTEL_INFO(dev)->num_pipes,
> >  				 4);
> >  	if (ret) {
> > +		dev_priv->fbdev = NULL;
> >  		kfree(ifbdev);
> >  		return ret;
> >  	}
> > @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
> >  void intel_fbdev_initial_config(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> >  
> >  	/* Due to peculiar init order wrt to hpd handling this is separate. */
> > -	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
> > +	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
> >  }
> >  
> >  void intel_fbdev_fini(struct drm_device *dev)
> > @@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
> >  	 * been restored from swap. If the object is stolen however, it will be
> >  	 * full of whatever garbage was left in there.
> >  	 */
> > -	if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
> > +	if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> >  		memset_io(info->screen_base, 0, info->screen_size);
> >  
> >  	fb_set_suspend(info, state);
> > @@ -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);
> 
> Ok, here's the real reason I've actually replied to this patch. This looks
> like a separate bugfix. And there's not mention of it in the commit
> message. Please split it out and give it the nice commit message
> explanation it deserves (whatever it's doing here).

Oh this hunk may be spurious... I think it hit this case when we were
failing to init dev_priv->fbdev and got an early hotplug event.  But
that should no longer be the case.

-- 
Jesse Barnes, Intel Open Source Technology Center