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

[Daniel Vetter]

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

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

Cheers, Daniel

>  }
>  
>  void intel_fbdev_restore_mode(struct drm_device *dev)
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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