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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
