[Intel-gfx] [PATCH 17/50] drm/i915: Switch back to an array of logical per-engine HW contexts

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-12 14:31:31)
> 
> On 12/04/2019 09:53, Chris Wilson wrote:
> > @@ -875,22 +927,27 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> >       i915_active_init(i915, &cb->base, cb_retire);
> >       i915_active_acquire(&cb->base);
> >   
> > -     rbtree_postorder_for_each_entry_safe(ce, next, &ctx->hw_contexts, node) {
> > -             struct intel_engine_cs *engine = ce->engine;
> > +     for_each_gem_engine(it, ctx) {
> > +             struct intel_context *ce = it.context;
> >               struct i915_request *rq;
> >   
> > -             if (!(engine->mask & engines))
> > +             if (!(ce->engine->mask & engines))
> > +                     continue;
> > +
> > +             if (!intel_context_is_pinned(ce))
> >                       continue;
> >   
> >               if (I915_SELFTEST_ONLY(context_barrier_inject_fault &
> > -                                    engine->mask)) {
> > +                                    ce->engine->mask)) {
> >                       err = -ENXIO;
> > +                     i915_gem_engines_iter_fini(&it);
> 
> This would be our only iterator which needs manual cleanup. But only on 
> abort, not normal termination.
> 
> Would it be cleaner if for_each_gem_engine took engines which the caller 
> would previously have to obtain via i915_gem_context_engine_list_lock? 
> And the caller would also be obviously responsible for unlocking.

But then it's just a for (i = 0; i < engines->num_engines; i++) again.
I'm not really convinced that if the caller has to do everything anyway,
replacing the for() is much of a gain :)

> >                       break;
> >               }
> >   
> > -             rq = i915_request_alloc(engine, ctx);
> > +             rq = intel_context_create_request(ce);
> >               if (IS_ERR(rq)) {
> >                       err = PTR_ERR(rq);
> > +                     i915_gem_engines_iter_fini(&it);
> >                       break;
> >               }
> >   
> > @@ -901,8 +958,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> >                       err = i915_active_ref(&cb->base, rq->fence.context, rq);
> >   
> >               i915_request_add(rq);
> > -             if (err)
> > +             if (err) {
> > +                     i915_gem_engines_iter_fini(&it);
> >                       break;
> > +             }
> >       }
> >   
> >       cb->task = err ? NULL : task; /* caller needs to unwind instead */
> > @@ -1739,6 +1798,43 @@ int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> >       return err;
> >   }
> >   
> > +/* GEM context-engines iterator: for_each_gem_engine() */
> > +bool i915_gem_engines_iter_next(struct i915_gem_engines_iter *it)
> > +{
> > +     if (!it->engines)
> > +             goto end;
> 
> When can engines be null? Should it be GEM_BUG_ON if onyl when used 
> outside the for_each_gem_engine?

Hmm, it can't. I seem to have been unusually defensive!

> > +     do {
> > +             if (it->idx >= it->engines->num_engines)
> > +                     goto end;
> > +
> > +             it->context = it->engines->engines[it->idx++];
> > +     } while (!it->context);
> > +
> > +     return true;
> > +
> > +end:
> > +     i915_gem_engines_iter_fini(it);
> > +     return false;
> > +}
> > +
> > +void i915_gem_engines_iter_init(struct i915_gem_engines_iter *it,
> > +                             struct i915_gem_context *ctx)
> > +{
> > +     it->gem_context = ctx;
> > +     it->engines = i915_gem_context_engine_list_lock(ctx);
> > +     it->idx = 0;
> 
> Clear it->context just in case?
> 
> > +}
> > +
> > +void i915_gem_engines_iter_fini(struct i915_gem_engines_iter *it)
> > +{
> > +     struct i915_gem_context *ctx;
> > +
> > +     ctx = fetch_and_zero(&it->gem_context);
> 
> This is iter_fini can be called multiple times? When does that come to 
> play? Oh... when i915_gem_engines_iter_next went past the array and the 
> loop decides to bail out.

Yeah, I was just worrying about the mix of the user calling
	iter_fini();
	continue;

> If iterator took engines as parameter this could also go.

Equally if there was no iterator :)

So is the iterator worth it? If I have to do the locking and list management
in the caller, I would say no. Yay or nay?
-Chris