On Tue, Oct 15, 2013 at 08:03:25AM -0700, Ben Widawsky wrote:
> On Tue, Oct 15, 2013 at 09:50:39AM +0100, Chris Wilson wrote:
> > On Mon, Oct 14, 2013 at 08:46:22PM -0700, Ben Widawsky wrote:
> > > -cleanup_vebox_ring:
> > > - intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> > > -cleanup_blt_ring:
> > > - intel_cleanup_ring_buffer(&dev_priv->ring[BCS]);
> > > -cleanup_bsd_ring:
> > > - intel_cleanup_ring_buffer(&dev_priv->ring[VCS]);
> > > -cleanup_render_ring:
> > > - intel_cleanup_ring_buffer(&dev_priv->ring[RCS]);
> > > +cleanup:
> > > + for_each_ring(ring, dev_priv, i) {
> > > + if (!(INTEL_INFO(dev)->ring_mask & (1<<i)) ||
> > > + !ring->name)
> > > + continue;
> >
> > This looks dubious. You don't need to check ring_mask here as that will
> > be implicit in whatever we test for completeness. ring->name is set at
> > the start of initialisation and is not cleaned upon error. A better
> > choice is ring->obj, which we already check in
> > intel_cleanup_ring_buffer.
> >
> > So this becomes:
> > cleanup:
> > for_each_ring(ring, dev_priv, i)
> > > + intel_cleanup_ring_buffer(ring);
> >
> > -Chris
> >
>
> Actually, I just plopped this code in at the last minute because I
> wanted to provide a sample usage in the patch too. I do have some issues
> in the future of using for_each_ring(), hopefully you remember those...
>
> In any event, I'll gladly drop this hunk. Can you review the rest?
No comments on the rest and lgtm, so
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
