Re: [PATCH] drm/i915: Replace has_bsd/blt/vebox with a mask

[Ben Widawsky <ben@bwidawsk.net>]

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