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

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