Re: [PATCH 25/34] drm/i915: Track active timelines

[Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>]

Quoting Tvrtko Ursulin (2019-01-22 14:56:32)
> 
> On 21/01/2019 22:21, Chris Wilson wrote:
> > Now that we pin timelines around use, we have a clearly defined lifetime
> > and convenient points at which we can track only the active timelines.
> > This allows us to reduce the list iteration to only consider those
> > active timelines and not all.
> > 
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >   drivers/gpu/drm/i915/i915_gem.c      |  4 +--
> >   drivers/gpu/drm/i915/i915_reset.c    |  2 +-
> >   drivers/gpu/drm/i915/i915_timeline.c | 39 ++++++++++++++++++----------
> >   4 files changed, 29 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c00eaf2889fb..5577e0e1034f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1977,7 +1977,7 @@ struct drm_i915_private {
> >   
> >               struct i915_gt_timelines {
> >                       struct mutex mutex; /* protects list, tainted by GPU */
> > -                     struct list_head list;
> > +                     struct list_head active_list;
> >   
> >                       /* Pack multiple timelines' seqnos into the same page */
> >                       spinlock_t hwsp_lock;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 4e0de22f0166..9c499edb4c13 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3246,7 +3246,7 @@ wait_for_timelines(struct drm_i915_private *i915,
> >               return timeout;
> >   
> >       mutex_lock(&gt->mutex);
> > -     list_for_each_entry(tl, &gt->list, link) {
> > +     list_for_each_entry(tl, &gt->active_list, link) {
> >               struct i915_request *rq;
> >   
> >               rq = i915_gem_active_get_unlocked(&tl->last_request);
> > @@ -3274,7 +3274,7 @@ wait_for_timelines(struct drm_i915_private *i915,
> >   
> >               /* restart after reacquiring the lock */
> >               mutex_lock(&gt->mutex);
> > -             tl = list_entry(&gt->list, typeof(*tl), link);
> > +             tl = list_entry(&gt->active_list, typeof(*tl), link);
> >       }
> >       mutex_unlock(&gt->mutex);
> >   
> > diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> > index 09edf488f711..9b9169508139 100644
> > --- a/drivers/gpu/drm/i915/i915_reset.c
> > +++ b/drivers/gpu/drm/i915/i915_reset.c
> > @@ -852,7 +852,7 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
> >        * No more can be submitted until we reset the wedged bit.
> >        */
> >       mutex_lock(&i915->gt.timelines.mutex);
> > -     list_for_each_entry(tl, &i915->gt.timelines.list, link) {
> > +     list_for_each_entry(tl, &i915->gt.timelines.active_list, link) {
> >               struct i915_request *rq;
> >               long timeout;
> >   
> > diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
> > index 69ee33dfa340..007348b1b469 100644
> > --- a/drivers/gpu/drm/i915/i915_timeline.c
> > +++ b/drivers/gpu/drm/i915/i915_timeline.c
> > @@ -117,7 +117,6 @@ int i915_timeline_init(struct drm_i915_private *i915,
> >                      const char *name,
> >                      struct i915_vma *hwsp)
> >   {
> > -     struct i915_gt_timelines *gt = &i915->gt.timelines;
> >       void *vaddr;
> >   
> >       /*
> > @@ -161,10 +160,6 @@ int i915_timeline_init(struct drm_i915_private *i915,
> >   
> >       i915_syncmap_init(&timeline->sync);
> >   
> > -     mutex_lock(&gt->mutex);
> > -     list_add(&timeline->link, &gt->list);
> > -     mutex_unlock(&gt->mutex);
> > -
> >       return 0;
> >   }
> >   
> > @@ -173,7 +168,7 @@ void i915_timelines_init(struct drm_i915_private *i915)
> >       struct i915_gt_timelines *gt = &i915->gt.timelines;
> >   
> >       mutex_init(&gt->mutex);
> > -     INIT_LIST_HEAD(&gt->list);
> > +     INIT_LIST_HEAD(&gt->active_list);
> >   
> >       spin_lock_init(&gt->hwsp_lock);
> >       INIT_LIST_HEAD(&gt->hwsp_free_list);
> > @@ -182,6 +177,24 @@ void i915_timelines_init(struct drm_i915_private *i915)
> >       i915_gem_shrinker_taints_mutex(i915, &gt->mutex);
> >   }
> >   
> > +static void timeline_active(struct i915_timeline *tl)
> > +{
> > +     struct i915_gt_timelines *gt = &tl->i915->gt.timelines;
> > +
> > +     mutex_lock(&gt->mutex);
> > +     list_add(&tl->link, &gt->active_list);
> > +     mutex_unlock(&gt->mutex);
> > +}
> > +
> > +static void timeline_inactive(struct i915_timeline *tl)
> > +{
> > +     struct i915_gt_timelines *gt = &tl->i915->gt.timelines;
> > +
> > +     mutex_lock(&gt->mutex);
> > +     list_del(&tl->link);
> > +     mutex_unlock(&gt->mutex);
> > +}
> 
> Bike shedding comments only:
> Would it be better to use a verb suffix? Even though timeline_activate 
> also wouldn't sound perfect. Since it is file local - activate_timeline? 
> Or even just inline to pin/unpin. Unless more gets put into them later..

Haven't got any plans for more here, yet, and was thinking this is a
pinned_list myself. I picked active_list since I was using 'active'
elsewhere for active_ring, active_engines, active_contexts, etc.

I didn't like activate/deactivate enough to switch, and was trying to
avoid reusing pin/unpin along this path:
	i915_timeline_pin -> timeline_pin
begged confusion

[snip]
> Never mind the bikeshedding comments:

There's time enough for someone to open a new pot of paint.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx