On Fri, Jul 29, 2016 at 10:40:09AM +0300, Joonas Lahtinen wrote:
> On ke, 2016-07-27 at 12:14 +0100, Chris Wilson wrote:
> > +static inline void
> > +i915_gem_object_set_active(struct drm_i915_gem_object *obj, int engine)
> > +{
> > + obj->flags |= 1 << (engine + I915_BO_ACTIVE_SHIFT);
>
> BIT(engine) << I915_BO_ACTIVE_SHIFT would be more readable to my taste,
> but I guess it's debatable.
I didn't change this to be BIT(engine + I915_BO_ACTIVE_SHIFT) because of
i915_gem_object_is_active() not following the pattern.
> > if (!readonly) {
> > active = obj->last_read;
> > - active_mask = obj->active;
> > + active_mask = i915_gem_object_is_active(obj);
>
> _is_active() does not really fit to be assigned to _mask. maybe have
> object_active_mask() and then
>
> _is_idle/inactive/whatever() { return !object_active_mask() }
>
> Because the negation is used lot more.
10 i915_gem_object_is_active(), 1 !i915_gem_object_is_active(). Of which
4 use the mask and the rest as a boolean.
I'm still liking i915_gem_object_is_active() over
i915_gem_object_active
i915_gem_object_active_mask
i915_gem_object_has_active
> > @@ -993,7 +993,7 @@ static int
> > i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> > struct list_head *vmas)
> > {
> > - const unsigned other_rings = ~intel_engine_flag(req->engine);
> > + const unsigned int other_rings = (~intel_engine_flag(req->engine) & I915_BO_ACTIVE_MASK) << I915_BO_ACTIVE_SHIFT;
>
> Horribly long line, is this intermediary?
No. Sadly not, requires
eb_other_engines() /* the rings were an allusion to something that will break later */
{
unsigned int mask;
mask = ~intel_engine_flag(req->engine) & I915_BO_ACTIVE_MASK;
mask <<= I915_BO_ACTIVE_SHIFT;
return mask;
}
to get a reasonable split.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
