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