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

[Daniel Vetter <daniel@ffwll.ch>]

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>

Ok, I've thought through the lifetime rules for the fbcon takeover and I
think we need some more polish for that. I also think we could simplify
the inital_config logic quite a bit. More comments below.

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   4 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>  drivers/gpu/drm/i915/intel_fbdev.c   | 235 ++++++++++++++++++++++++++++++++---
>  3 files changed, 224 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 94183af..b868331 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7817,11 +7817,11 @@ mode_fits_in_fbdev(struct drm_device *dev,
>  	if (dev_priv->fbdev == NULL)
>  		return NULL;
>  
> -	obj = dev_priv->fbdev->ifb.obj;
> +	obj = dev_priv->fbdev->fb->obj;
>  	if (obj == NULL)
>  		return NULL;
>  
> -	fb = &dev_priv->fbdev->ifb.base;
> +	fb = &dev_priv->fbdev->fb->base;
>  	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
>  							       fb->bits_per_pixel))
>  		return NULL;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4787773..5f9182e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -110,9 +110,10 @@ struct intel_framebuffer {
>  
>  struct intel_fbdev {
>  	struct drm_fb_helper helper;
> -	struct intel_framebuffer ifb;
> +	struct intel_framebuffer *fb;
>  	struct list_head fbdev_list;
>  	struct drm_display_mode *our_mode;
> +	int preferred_bpp;
>  };
>  
>  struct intel_encoder {
> @@ -671,6 +672,7 @@ int intel_framebuffer_init(struct drm_device *dev,
>  			   struct intel_framebuffer *ifb,
>  			   struct drm_mode_fb_cmd2 *mode_cmd,
>  			   struct drm_i915_gem_object *obj);
> +void intel_fbdev_init_bios(struct drm_device *dev);
>  void intel_framebuffer_fini(struct intel_framebuffer *fb);
>  void intel_prepare_page_flip(struct drm_device *dev, int plane);
>  void intel_finish_page_flip(struct drm_device *dev, int pipe);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 284c3eb..db75f22 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -62,11 +62,20 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_framebuffer *fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
>  	struct drm_i915_gem_object *obj;
>  	int size, ret;
>  
> +	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> +	if (!fb) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ifbdev->fb = fb;
> +
>  	/* we don't do packed 24bpp */
>  	if (sizes->surface_bpp == 24)
>  		sizes->surface_bpp = 32;
> @@ -97,7 +106,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  		goto out_unref;
>  	}
>  
> -	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
> +	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
>  	if (ret)
>  		goto out_unpin;
>  
> @@ -116,7 +125,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> -	struct intel_framebuffer *intel_fb = &ifbdev->ifb;
> +	struct intel_framebuffer *intel_fb = ifbdev->fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct fb_info *info;
> @@ -148,7 +157,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  
>  	info->par = helper;
>  
> -	fb = &ifbdev->ifb.base;
> +	fb = &ifbdev->fb->base;
>  
>  	ifbdev->helper.fb = fb;
>  	ifbdev->helper.fbdev = info;
> @@ -194,14 +203,14 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	 * If the object is stolen however, it will be full of whatever
>  	 * garbage was left in there.
>  	 */
> -	if (ifbdev->ifb.obj->stolen)
> +	if (ifbdev->fb->obj->stolen)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
>  
> -	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n",
> +	DRM_DEBUG_KMS("allocated %dx%d fb: map %p, bo %p, size 0x%lx\n",
>  		      fb->width, fb->height,
> -		      i915_gem_obj_ggtt_offset(obj), obj);
> +		      info->screen_base, obj, info->screen_size);
>  
>  	mutex_unlock(&dev->struct_mutex);
>  	vga_switcheroo_client_fb_set(dev->pdev, info);
> @@ -236,6 +245,96 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
>  	*blue = intel_crtc->lut_b[regno] << 8;
>  }
>  
> +static struct drm_fb_helper_crtc *
> +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> +{
> +	int i;
> +
> +	for (i = 0; i < fb_helper->crtc_count; i++)
> +		if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> +			return &fb_helper->crtc_info[i];
> +
> +	return NULL;
> +}
> +
> +/*
> + * Try to read the BIOS display configuration and use it for the initial
> + * fb configuration.
> + *
> + * The BIOS or boot loader will generally create an initial display
> + * configuration for us that includes some set of active pipes and displays.
> + * This routine tries to figure out which pipes and connectors are active
> + * and stuffs them into the crtcs and modes array given to us by the
> + * drm_fb_helper code.
> + *
> + * The overall sequence is:
> + *   intel_fbdev_init - from driver load
> + *     intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
> + *     drm_fb_helper_init - build fb helper structs
> + *     drm_fb_helper_single_add_all_connectors - more fb helper structs
> + *   intel_fbdev_initial_config - apply the config
> + *     drm_fb_helper_initial_config - call ->probe then register_framebuffer()
> + *         drm_setup_crtcs - build crtc config for fbdev
> + *           intel_fb_initial_config - find active connectors etc
> + *         drm_fb_helper_single_fb_probe - set up fbdev
> + *           intelfb_create - re-use or alloc fb, build out fbdev structs
> + *
> + * If the BIOS or boot loader leaves the display in VGA mode, there's not
> + * much we can do; switching out of that mode involves allocating a new,
> + * high res buffer, and also recalculating bandwidth requirements for the
> + * new bpp configuration.
> + */
> +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> +				    struct drm_fb_helper_crtc **crtcs,
> +				    struct drm_display_mode **modes,
> +				    bool *enabled, int width, int height)
> +{
> +	int i;
> +
> +	for (i = 0; i < fb_helper->connector_count; i++) {
> +		struct drm_connector *connector;
> +		struct drm_encoder *encoder;
> +
> +		connector = fb_helper->connector_info[i]->connector;
> +		if (!enabled[i]) {
> +			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> +				      connector->base.id);
> +			continue;
> +		}
> +
> +		encoder = connector->encoder;
> +		if (!encoder || !encoder->crtc) {
> +			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
> +				      connector->base.id);
> +			continue;
> +		}
> +
> +		if (WARN_ON(!encoder->crtc->enabled)) {
> +			DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
> +				      drm_get_connector_name(connector),
> +				      encoder->crtc->base.id);
> +			return false;
> +		}
> +
> +		if (!to_intel_crtc(encoder->crtc)->active) {
> +			DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
> +				      drm_get_connector_name(connector),
> +				      encoder->crtc->base.id);
> +			return false;
> +		}
> +
> +		modes[i] = &encoder->crtc->mode;
> +		crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> +
> +		DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
> +			      drm_get_connector_name(connector),
> +			      encoder->crtc->base.id,
> +			      modes[i]->name);
> +	}
> +
> +	return true;
> +}
> +
>  static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  	.gamma_set = intel_crtc_fb_gamma_set,
>  	.gamma_get = intel_crtc_fb_gamma_get,
> @@ -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);
> +}
> +
> +/*
> + * 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;

This here is a bit surprising - my model of operation here presumed that
if we correctly assign the crtc->fb and the ifbdev->fb pointers we could
fully rely on the fastboot setcrtc logic to eschew the modeset.

Being the ever-vary of special-purpose logic I'd much prefer this implicit
approach - otherwise we have one more special case to care about in the
fastboot=y/n and CONFIG_FB=y/n matrix.

So have you tried to ditch this special initial_config functions
(obviously only looks good with fastboot=1) or what precise corner-case
does this fix?

> +	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);

As mentioned I think this part here needs to be moved to the earlier
plane_config reconstruction loop.

But checking the fb refcounting now it looks like we leak the initial
references that the plane_config struct holds (or I didn't spot it). And
we miss the reference for ifbdev->fb when stealing the bios framebuffer.

With the current code of using

	intel_framebuffer_fini(ifbdev->fb);
	kfree(ifbdev->fb);

You get away with this since the kfree will ignore the surplus references
from the plane_config. But if you replace this with the
drm_framebuffer_unreference call like I've suggested we'll have a real
leak. So I think this needs to be fixed.


> +	}
> +
> +	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 */
> +	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);
>  }
>  
>  void intel_fbdev_restore_mode(struct drm_device *dev)
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch