Re: [PATCH 05/13] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 20 Mar 2013 14:31:38 +0200
Imre Deak <imre.deak@intel.com> wrote:

> > +			offset = I915_READ(DSPSURF(plane));
> > +		} else
> > +			offset = I915_READ(DSPADDR(plane));
> 
> Nitpick: the second branch should be inside { } too.

Fixed.

> 
> > +		if (!obj_offset)
> > +			obj_offset = offset;
> > +
> > +		pitch = I915_READ(DSPSTRIDE(plane));
> > +		if (mode_cmd.pitches[0] == 0)
> > +			mode_cmd.pitches[0] = pitch;
> > +
> > +		if (offset != obj_offset || pitch != mode_cmd.pitches[0]) {
> > +			DRM_DEBUG_KMS("multiple pipe setup not in clone mode, sjipping\n");
> 
> s/sjipping/skipping/

Fixed.

> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		int ret;
> > +
> > +		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
> > +			continue;
> > +
> > +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> > +		if (ret)
> > +			goto out_unref_obj;
> 
> Since fb will be destroyed, is it ok to leave references to it in
> crtc->fb set in previous iterations?

It should only fail the first time (if ever).  I've commented it.  I
think we need to keep it in the loop so that each crtc has a ref on the
fb right?

> >  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
> > -				 dev_priv->num_pipe,
> > -				 INTELFB_CONN_LIMIT);
> > +			dev_priv->num_pipe,
> > +			INTELFB_CONN_LIMIT);
> 
> Unnecessary w/s change.

Things look correct here, maybe I've fixed it or made the tabs sensible.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center