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

[Chris Wilson]

On Thu, Feb 06, 2014 at 09:47:47PM +0000, Jesse Barnes wrote:
> 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"?

tmp here is meant to be the size required for this pipe in the target
framebuffer, but we are calculating the size required in its current
framebuffer. They may be different.
 
> > 
> > > +		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).

I thought you had tried to reduce the two-passes into one, but I think
it has come unstuck.
 
> > > +	}
> > > +
> > > +	/* 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...

No, both paths (success, failure) should disable any active CRTC that is
scanning out from (even writing to) memory that is no longer valid (i.e.
no longer wrapped by a GEM object).
 
> > > +
> > > +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.

Indeed. Dangerous bikeshed territory, just aim for readable code. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre