On Wed, 20 Mar 2013 14:31:38 +0200
Imre Deak <imre.deak at 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
