On Thu, Aug 04, 2016 at 01:25:08PM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > +static __always_inline unsigned
> > +__busy_read_flag(const struct drm_i915_gem_request *request)
> > +{
> > + return 0x10000 << request->engine->exec_id;
>
> Duh, this really is our ABI? No BIT(NUM_ENGINES) or something sane?
BIT(NUM_ENGINES) as uabi, you call that sane :)
> Make a comment of such situation.
Like BUILD_BUG_ON(NUM_ENGINES > 16).
> > + /* Yes, the lookups are intentionally racy.
> > + *
> > + * Even though we guard the pointer lookup by RCU, that only
> > + * guarantees that the pointer and its contents remain
> > + * dereferencable and does *not* mean that the request we
> > + * have is the same as the one being tracked by the object.
> > + *
> > + * Consider that we lookup the request just as it is being
> > + * retired and free. We take a local copy of the pointer,
>
> s/free/freed/
>
> > + * but before we add its engine into the busy set, the other
> > + * thread reallocates it and assigns it to a task on another
> > + * engine with a fresh and incomplete seqno.
> > + *
> > + * So after we lookup the engine's id, we double check that
>
> This double check is nowhere to be seen, time to update this comment
> too?
Actualy snuck it back in. I took it out thinking that there wasn't
sufficent read-dependency to guarrantee the ordering (but now reversed
my decision there) and had re-read my comment about why the check is
required.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
