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?
P.S. if you want my acked-by on this above mentioned cleanup, have it.
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
