[Intel-gfx] [PATCH 13/22] drm/i915: Combine all i915_vma bitfields into a single set of flags

[Chris Wilson]

On Fri, Jul 29, 2016 at 10:30:26AM +0300, Joonas Lahtinen wrote:
> On ke, 2016-07-27 at 12:14 +0100, Chris Wilson wrote:
> > @@ -2979,7 +2980,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> >  	u64 min_alignment;
> >  	int ret;
> >  
> > -	GEM_BUG_ON(vma->bound);
> > +	GEM_BUG_ON(vma->flags & (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND));
> 
> How bout i915_vma_is_bound?
> 
> >  	/* Pin early to prevent the shrinker/eviction logic from destroying
> > @@ -3712,7 +3714,7 @@ i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> >  	 */
> >  	__i915_vma_pin(vma);
> >  
> > -	if (!bound) {
> > +	if ((bound & (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND)) == 0) {
> 
> In this case especially !(bound & ...) looks far more readable. Again,
> I'm against flip-flopping between styles, but I understand these are
> old patches, so we can unify stuff at the end of churn.

Here I intentionally used GLOBAL | LOCAL for two reasons: it looks more
like the existing use inside i915_vma_bind() and the contrast is very
important for the next patch where we add a fake BIND bit.

> > @@ -3682,8 +3682,8 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
> >  	if (WARN_ON(!vma->obj->map_and_fenceable))
> >  		return IO_ERR_PTR(-ENODEV);
> >  
> > -	GEM_BUG_ON(!vma->is_ggtt);
> > -	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> > +	GEM_BUG_ON(!i915_vma_is_ggtt(vma));
> > +	GEM_BUG_ON((vma->flags & I915_VMA_GLOBAL_BIND) == 0);
> 
> Again !(vma->flags & ) is more readable.

I disagree. I find the ! lost before the brackets and == matches the
pattern for checking bits. So I generally prefer (value & mask) == result.
 
> But GEM_BUG_ON(!i915_vma_is_bound(vma)) would again be possible.

GEM_BUG_ON(!i915_vma_is_bound_to_global(vma)) here though.
GEM_BUG_ON(!i915_vma_any_bound(vma, I915_VMA_GLOBAL_BIND))
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre