Re: [Intel-gfx] [PATCH 7/9] drm/i915: Update i915_scheduler to operate on i915_sched_engine

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Jun 04, 2021 at 02:17:58PM -0500, Jason Ekstrand wrote:
> On Thu, Jun 3, 2021 at 4:09 PM Matthew Brost <matthew.brost@intel.com> wrote:
> >
> > Rather passing around an intel_engine_cs in the scheduling code, pass
> > around a i915_sched_engine.
> 
> 👍
> 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  .../drm/i915/gt/intel_execlists_submission.c  | 11 +++--
> >  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  2 +-
> >  drivers/gpu/drm/i915/i915_scheduler.c         | 46 +++++++++----------
> >  drivers/gpu/drm/i915/i915_scheduler.h         |  2 +-
> >  4 files changed, 32 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index 3fac17103837..7240c153be35 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -382,7 +382,8 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> >                 GEM_BUG_ON(rq_prio(rq) == I915_PRIORITY_INVALID);
> >                 if (rq_prio(rq) != prio) {
> >                         prio = rq_prio(rq);
> > -                       pl = i915_sched_lookup_priolist(engine, prio);
> > +                       pl = i915_sched_lookup_priolist(engine->sched_engine,
> > +                                                       prio);
> >                 }
> >                 GEM_BUG_ON(i915_sched_engine_is_empty(engine->sched_engine));
> >
> > @@ -1096,7 +1097,8 @@ static void defer_active(struct intel_engine_cs *engine)
> >         if (!rq)
> >                 return;
> >
> > -       defer_request(rq, i915_sched_lookup_priolist(engine, rq_prio(rq)));
> > +       defer_request(rq, i915_sched_lookup_priolist(engine->sched_engine,
> > +                                                    rq_prio(rq)));
> >  }
> >
> >  static bool
> > @@ -2083,7 +2085,7 @@ static void __execlists_unhold(struct i915_request *rq)
> >
> >                 i915_request_clear_hold(rq);
> >                 list_move_tail(&rq->sched.link,
> > -                              i915_sched_lookup_priolist(rq->engine,
> > +                              i915_sched_lookup_priolist(rq->engine->sched_engine,
> >                                                           rq_prio(rq)));
> >                 set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
> >
> > @@ -2452,7 +2454,8 @@ static void queue_request(struct intel_engine_cs *engine,
> >  {
> >         GEM_BUG_ON(!list_empty(&rq->sched.link));
> >         list_add_tail(&rq->sched.link,
> > -                     i915_sched_lookup_priolist(engine, rq_prio(rq)));
> > +                     i915_sched_lookup_priolist(engine->sched_engine,
> > +                                                rq_prio(rq)));
> >         set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 4c5bbec0775d..7ed21670ef14 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -529,7 +529,7 @@ static inline void queue_request(struct intel_engine_cs *engine,
> >  {
> >         GEM_BUG_ON(!list_empty(&rq->sched.link));
> >         list_add_tail(&rq->sched.link,
> > -                     i915_sched_lookup_priolist(engine, prio));
> > +                     i915_sched_lookup_priolist(engine->sched_engine, prio));
> >         set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> > index 035b88f2e4aa..3d36e4447b5d 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler.c
> > +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> > @@ -61,14 +61,13 @@ static void assert_priolists(struct i915_sched_engine * const sched_engine)
> >  }
> >
> >  struct list_head *
> > -i915_sched_lookup_priolist(struct intel_engine_cs *engine, int prio)
> > +i915_sched_lookup_priolist(struct i915_sched_engine *sched_engine, int prio)
> >  {
> > -       struct i915_sched_engine * const sched_engine = engine->sched_engine;
> >         struct i915_priolist *p;
> >         struct rb_node **parent, *rb;
> >         bool first = true;
> >
> > -       lockdep_assert_held(&engine->sched_engine->lock);
> > +       lockdep_assert_held(&sched_engine->lock);
> >         assert_priolists(sched_engine);
> >
> >         if (unlikely(sched_engine->no_priolist))
> > @@ -130,13 +129,13 @@ struct sched_cache {
> >         struct list_head *priolist;
> >  };
> >
> > -static struct intel_engine_cs *
> > -sched_lock_engine(const struct i915_sched_node *node,
> > -                 struct intel_engine_cs *locked,
> > +static struct i915_sched_engine *
> > +lock_sched_engine(struct i915_sched_node *node,
> > +                 struct i915_sched_engine *locked,
> >                   struct sched_cache *cache)
> >  {
> >         const struct i915_request *rq = node_to_request(node);
> > -       struct intel_engine_cs *engine;
> > +       struct i915_sched_engine *sched_engine;
> >
> >         GEM_BUG_ON(!locked);
> >
> > @@ -146,14 +145,14 @@ sched_lock_engine(const struct i915_sched_node *node,
> >          * engine lock. The simple ploy we use is to take the lock then
> >          * check that the rq still belongs to the newly locked engine.
> >          */
> > -       while (locked != (engine = READ_ONCE(rq->engine))) {
> > -               spin_unlock(&locked->sched_engine->lock);
> > +       while (locked != (sched_engine = rq->engine->sched_engine)) {
> 
> You lost the READ_ONCE here.  Based on the comment above, that may
> matter.  I guess it depends on what all barriers spin_lock() implies.
> 

Ah, good catch. I think it should be:

READ_ONCE(rq->engine)->sched_engine

As the rq->engine is the unstable pointer.

Matt

> Assuming that's all good,
> 
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> 
> > +               spin_unlock(&locked->lock);
> >                 memset(cache, 0, sizeof(*cache));
> > -               spin_lock(&engine->sched_engine->lock);
> > -               locked = engine;
> > +               spin_lock(&sched_engine->lock);
> > +               locked = sched_engine;
> >         }
> >
> > -       GEM_BUG_ON(locked != engine);
> > +       GEM_BUG_ON(locked != sched_engine);
> >         return locked;
> >  }
> >
> > @@ -161,7 +160,7 @@ static void __i915_schedule(struct i915_sched_node *node,
> >                             const struct i915_sched_attr *attr)
> >  {
> >         const int prio = max(attr->priority, node->attr.priority);
> > -       struct intel_engine_cs *engine;
> > +       struct i915_sched_engine *sched_engine;
> >         struct i915_dependency *dep, *p;
> >         struct i915_dependency stack;
> >         struct sched_cache cache;
> > @@ -236,23 +235,24 @@ static void __i915_schedule(struct i915_sched_node *node,
> >         }
> >
> >         memset(&cache, 0, sizeof(cache));
> > -       engine = node_to_request(node)->engine;
> > -       spin_lock(&engine->sched_engine->lock);
> > +       sched_engine = node_to_request(node)->engine->sched_engine;
> > +       spin_lock(&sched_engine->lock);
> >
> >         /* Fifo and depth-first replacement ensure our deps execute before us */
> > -       engine = sched_lock_engine(node, engine, &cache);
> > +       sched_engine = lock_sched_engine(node, sched_engine, &cache);
> >         list_for_each_entry_safe_reverse(dep, p, &dfs, dfs_link) {
> >                 INIT_LIST_HEAD(&dep->dfs_link);
> >
> >                 node = dep->signaler;
> > -               engine = sched_lock_engine(node, engine, &cache);
> > -               lockdep_assert_held(&engine->sched_engine->lock);
> > +               sched_engine = lock_sched_engine(node, sched_engine, &cache);
> > +               lockdep_assert_held(&sched_engine->lock);
> >
> >                 /* Recheck after acquiring the engine->timeline.lock */
> >                 if (prio <= node->attr.priority || node_signaled(node))
> >                         continue;
> >
> > -               GEM_BUG_ON(node_to_request(node)->engine != engine);
> > +               GEM_BUG_ON(node_to_request(node)->engine->sched_engine !=
> > +                          sched_engine);
> >
> >                 WRITE_ONCE(node->attr.priority, prio);
> >
> > @@ -270,17 +270,17 @@ static void __i915_schedule(struct i915_sched_node *node,
> >                 if (i915_request_in_priority_queue(node_to_request(node))) {
> >                         if (!cache.priolist)
> >                                 cache.priolist =
> > -                                       i915_sched_lookup_priolist(engine,
> > +                                       i915_sched_lookup_priolist(sched_engine,
> >                                                                    prio);
> >                         list_move_tail(&node->link, cache.priolist);
> >                 }
> >
> >                 /* Defer (tasklet) submission until after all of our updates. */
> > -               if (engine->sched_engine->kick_backend)
> > -                       engine->sched_engine->kick_backend(node_to_request(node), prio);
> > +               if (sched_engine->kick_backend)
> > +                       sched_engine->kick_backend(node_to_request(node), prio);
> >         }
> >
> > -       spin_unlock(&engine->sched_engine->lock);
> > +       spin_unlock(&sched_engine->lock);
> >  }
> >
> >  void i915_schedule(struct i915_request *rq, const struct i915_sched_attr *attr)
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> > index 713c38c99de9..0014745bda30 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler.h
> > +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> > @@ -39,7 +39,7 @@ void i915_schedule(struct i915_request *request,
> >                    const struct i915_sched_attr *attr);
> >
> >  struct list_head *
> > -i915_sched_lookup_priolist(struct intel_engine_cs *engine, int prio);
> > +i915_sched_lookup_priolist(struct i915_sched_engine *sched_engine, int prio);
> >
> >  void __i915_priolist_free(struct i915_priolist *p);
> >  static inline void i915_priolist_free(struct i915_priolist *p)
> > --
> > 2.28.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx