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

[Chris Wilson]

On Wed, Feb 05, 2014 at 05:30:48PM +0000, Jesse Barnes wrote:
> +static bool intel_fbdev_init_bios(struct drm_device *dev,
> +				 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, max_size = 0, tmp;
> +
> +	/* 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) {
> +			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> +				      pipe_name(intel_crtc->pipe));
> +			continue;
> +		}
> +
> +		tmp = intel_crtc->config.adjusted_mode.crtc_hdisplay *
> +			intel_crtc->config.adjusted_mode.crtc_vdisplay *
> +			intel_crtc->plane_config.fb->base.pitches[0];

This reads as width * height * stride. I presume you meant bpp/8 here,
but since we keep the fb and its pitch intact, you need height * stride.
Actually, it preserves a completely different fb, so this estimate of
size is incomplete.

> +		max_size = max(max_size, tmp);
> +
> +		/*
> +		 * Make sure the fb will fit the biggest active pipe.  If
> +		 * not, clear out any previous usage.
> +		 */
> +		if (intel_crtc->plane_config.size > last_size &&
> +		    intel_crtc->plane_config.size >= max_size) {
> +			plane_config = &intel_crtc->plane_config;
> +			last_size = plane_config->size;
> +			fb = plane_config->fb;
> +		} else {
> +			plane_config = NULL;
> +			fb = NULL;
> +		}

Two CRTCs sharing a plane_config will now end up with plane_config/fb =
NULL, and so bail out. This is confusing me. If we here just found the
largest fb and then did a second pass confirming that all CRTC and planes
fitted into it, I would find it easier to understand.

> +	}
> +
> +	/* 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;
> +	}
> +
> +	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);
> +	}
> +
> +	DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> +	return true;

Somewhere around here we must disable all CRTCs that are active and have
no fb.

> +
> +out_unref_obj:
> +	intel_framebuffer_fini(fb);
> +out:
> +
> +	return false;
> +}
> +
>  int intel_fbdev_init(struct drm_device *dev)
>  {
>  	struct intel_fbdev *ifbdev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;

A useful assertion here would be:

if (WARN_ON(INTEL_INFO(dev)->num_pipes == 0)) return -ENODEV;

> -	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> -	if (!ifbdev)
> +	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> +	if (ifbdev == NULL)
>  		return -ENOMEM;
>  
> -	dev_priv->fbdev = ifbdev;
>  	ifbdev->helper.funcs = &intel_fb_helper_funcs;
> +	dev_priv->fbdev = ifbdev;
> +
> +	if (!i915.fastboot || !intel_fbdev_init_bios(dev, ifbdev))
> +		ifbdev->preferred_bpp = 32;

Bikeshed: move i915.fastboot to intel_fbdev_init_bios and rearrange like

  if (!intel_fbdev_init(intel_fbdev_init_bios(dev, ifbdev)) {
    ifbdev->helper.funcs = &intel_fb_helper_funcs;
    ifbdev->preferred_bpp = 32;
  }

  (then dev_priv->fbdev = ifbdev can be done last on success as before)

>  
>  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
> -				 INTEL_INFO(dev)->num_pipes,
> -				 4);
> +				 INTEL_INFO(dev)->num_pipes, 4);
>  	if (ret) {
> +		dev_priv->fbdev = NULL;
>  		kfree(ifbdev);
>  		return ret;
>  	}

>  void intel_fbdev_restore_mode(struct drm_device *dev)
> @@ -441,7 +546,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>  	int ret;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (INTEL_INFO(dev)->num_pipes == 0)
> +	if (INTEL_INFO(dev)->num_pipes == 0 || !dev_priv->fbdev)

num_pipes == 0 => dev_priv->fbdev == NULL
ergo this can be reduced to just
if (dev_priv->fbdev == NULL)
>  		return;
>  
>  	drm_modeset_lock_all(dev);

-- 
Chris Wilson, Intel Open Source Technology Centre