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

[Jesse Barnes]

On Wed, 5 Feb 2014 18:14:15 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

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

Right, I meant to just use pitches here.  And fb could be NULL, so I
need to handle that case.  What do you mean by "it preserves a
completely different fb"?

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

Ah yeah, I was trying to be tricky, doing this in one pass.  But doing
it in two is better and clearer (I had this earlier but then thought
I'd be bikeshedded into a single pass, oh well).

> 
> > +	}
> > +
> > +	/* 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.

You mean on error?  Otherwise that shouldn't be possible...  we would
have failed earlier on when we failed to find an fb suitable for all
the active pipes (or so goes the theory).  Unless I'm missing some
other implication like with the num_pipes vs fbdev ptr thing...

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

Yeah makes things clearer, thanks.

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

I wonder if we should use the BIOS output config function
unconditionally?  Or at least try it?  It's really separate from the fb
allocation stuff, and seems useful by itself...  Either way I can apply
this cleanup.

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

Right, thanks.

Jesse