[Intel-gfx] [PATCH 14/20] drm/i915/gt: Track timeline activeness in enter/exit

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-22 17:14:23)
> 
> On 18/07/2019 08:00, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 884dfc1cb033..aceb990ae3b9 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -3253,6 +3253,8 @@ static void virtual_context_enter(struct intel_context *ce)
> >   
> >       for (n = 0; n < ve->num_siblings; n++)
> >               intel_engine_pm_get(ve->siblings[n]);
> > +
> > +     intel_timeline_enter(ce->ring->timeline);
> 
> Here we couldn't enter all sibling contexts instead? Would be a bit 
> wasteful I guess. And there must be a place where it is already done. 
> But can't be on picking the engine, where is it?

There's only the one context (ring, and timeline).

> > +void intel_timeline_enter(struct intel_timeline *tl)
> > +{
> > +     struct intel_gt_timelines *timelines = &tl->gt->timelines;
> > +
> > +     GEM_BUG_ON(!tl->pin_count);
> > +     if (tl->active_count++)
> > +             return;
> > +     GEM_BUG_ON(!tl->active_count); /* overflow? */
> > +
> > +     mutex_lock(&timelines->mutex);
> > +     list_add(&tl->link, &timelines->active_list);
> > +     mutex_unlock(&timelines->mutex);
> > +}
> > +
> > +void intel_timeline_exit(struct intel_timeline *tl)
> > +{
> > +     struct intel_gt_timelines *timelines = &tl->gt->timelines;
> > +
> > +     GEM_BUG_ON(!tl->active_count);
> > +     if (--tl->active_count)
> > +             return;
> > +
> > +     mutex_lock(&timelines->mutex);
> > +     list_del(&tl->link);
> > +     mutex_unlock(&timelines->mutex);
> 
> So we end up with one lock protecting tl->active_count and another for 
> the list of active timelines?

tl->active_count is local to the timeline
tl->link/ gt->timelines.active_list is on the GT.

So yes, we have separate locks so that we can submit to multiple
independent contexts and engines concurrently.

> > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > index 9a71aea7a338..b820ee76b7f5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > @@ -58,6 +58,7 @@ struct intel_timeline {
> >        */
> >       struct i915_syncmap *sync;
> >   
> > +     unsigned int active_count;
> 
> Is it becoming non-obvious what is pin_count and what is active_count? I 
> suggest some comments dropped along the members here.

pin_count is for the pinning, and active_count is for activity :)
Thanks to the need for perma-pinned contexts, we have the two layers
where we would have hoped for one.
-Chris