From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jesse Barnes <jbarnes@virtuousgeek.org>
Subject: Re: [PATCH 05/13] drm/i915: Wrap the preallocated BIOS
 framebuffer and preserve for KMS fbcon
Date: Tue, 26 Mar 2013 16:20:58 -0700
Message-ID: <20130326162058.29738e02@jbarnes-desktop>
References: <1361309508-4901-1-git-send-email-jbarnes@virtuousgeek.org>
	<1361309508-4901-6-git-send-email-jbarnes@virtuousgeek.org>
	<1363782698.13528.22.camel@intelbox>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from oproxy14-pub.unifiedlayer.com (oproxy14-pub.unifiedlayer.com
	[67.222.51.224])
	by gabe.freedesktop.org (Postfix) with SMTP id 27047E5C72
	for <intel-gfx@lists.freedesktop.org>;
	Tue, 26 Mar 2013 16:20:42 -0700 (PDT)
In-Reply-To: <1363782698.13528.22.camel@intelbox>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
To: imre.deak@intel.com
Cc: intel-gfx@lists.freedesktop.org
List-Id: intel-gfx@lists.freedesktop.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