On 7/16/2021 13:16, Matthew Brost wrote:

Implement GuC submission tasklet for new interface. The new GuC interface uses H2G to submit contexts to the GuC. Since H2G use a single channel, a single tasklet submits is used for the submission path.

This still needs fixing - 'a single tasklet submits is used' is not valid English.

It also seems that the idea of splitting all the deletes of old code into a separate patch didn't happen. It really does obfuscate things significantly having completely unrelated deletes and adds interspersed :(.

John.

Also the per engine interrupt handler has been updated to disable the rescheduling of the physical engine tasklet, when using GuC scheduling, as the physical engine tasklet is no longer used.

In this patch the field, guc_id, has been added to intel_context and is not assigned. Patches later in the series will assign this value.

v2: (John Harrison)

• Clean up some comments

Cc: John Harrison john.c.harrison@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_context_types.h | 9 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 231 +++++++++--------- 3 files changed, 127 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 90026c177105..6d99631d19b9 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -137,6 +137,15 @@ struct intel_context { struct intel_sseu sseu;

u8 wa_bb_page; /* if set, page num reserved for context workarounds */

• /* GuC scheduling state flags that do not require a lock. */

• atomic_t guc_sched_state_no_lock;

• /*

• * GuC LRC descriptor ID - Not assigned in this patch but future patches
  

• * in the series will.
  

• */
  

• u16 guc_id; };

  #endif /* __INTEL_CONTEXT_TYPES__ */

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 35783558d261..8c7b92f699f1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -30,6 +30,10 @@ struct intel_guc { struct intel_guc_log log; struct intel_guc_ct ct;

• /* Global engine used to submit requests to GuC */
• struct i915_sched_engine *sched_engine;
• struct i915_request *stalled_request;
• /* intel_guc_recv interrupt related state */ spinlock_t irq_lock; unsigned int msg_enabled_mask;

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 23a94a896a0b..ca0717166a27 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -60,6 +60,31 @@

#define GUC_REQUEST_SIZE 64 /* bytes */

+/*

• • Below is a set of functions which control the GuC scheduling state which do
• • not require a lock as all state transitions are mutually exclusive. i.e. It
• • is not possible for the context pinning code and submission, for the same
• • context, to be executing simultaneously. We still need an atomic as it is
• • possible for some of the bits to changing at the same time though.
• */

+#define SCHED_STATE_NO_LOCK_ENABLED BIT(0) +static inline bool context_enabled(struct intel_context *ce) +{

• return (atomic_read(&ce->guc_sched_state_no_lock) &

• SCHED_STATE_NO_LOCK_ENABLED);
  

+}

+static inline void set_context_enabled(struct intel_context *ce) +{

• atomic_or(SCHED_STATE_NO_LOCK_ENABLED, &ce->guc_sched_state_no_lock);

+}

+static inline void clr_context_enabled(struct intel_context *ce) +{

• atomic_and((u32)~SCHED_STATE_NO_LOCK_ENABLED,

•    &ce->guc_sched_state_no_lock);
  

+}

• static inline struct i915_priolist *to_priolist(struct rb_node *rb) { return rb_entry(rb, struct i915_priolist, node);

@@ -122,37 +147,29 @@ static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id, xa_store_irq(&guc->context_lookup, id, ce, GFP_ATOMIC); }

-static void guc_add_request(struct intel_guc *guc, struct i915_request *rq) +static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) {

• /* Leaving stub as this function will be used in future patches */

-}

• int err;
• struct intel_context *ce = rq->context;
• u32 action[3];
• int len = 0;
• bool enabled = context_enabled(ce);

-/*

• • When we're doing submissions using regular execlists backend, writing to
• • ELSP from CPU side is enough to make sure that writes to ringbuffer pages
• • pinned in mappable aperture portion of GGTT are visible to command streamer.
• • Writes done by GuC on our behalf are not guaranteeing such ordering,
• • therefore, to ensure the flush, we're issuing a POSTING READ.
• */

-static void flush_ggtt_writes(struct i915_vma *vma) -{

• if (i915_vma_is_map_and_fenceable(vma))

• intel_uncore_posting_read_fw(vma->vm->gt->uncore,
  

• 			     GUC_STATUS);
  

-}

• if (!enabled) {

• action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET;
  

• action[len++] = ce->guc_id;
  

• action[len++] = GUC_CONTEXT_ENABLE;
  

• } else {

• action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT;
  

• action[len++] = ce->guc_id;
  

• }

-static void guc_submit(struct intel_engine_cs *engine,

•        struct i915_request **out,
  

•        struct i915_request **end)
  

-{

• struct intel_guc *guc = &engine->gt->uc.guc;

• err = intel_guc_send_nb(guc, action, len);

• do {

• struct i915_request *rq = *out++;
  

• if (!enabled && !err)

• set_context_enabled(ce);
  
  

• flush_ggtt_writes(rq->ring->vma);
  

• guc_add_request(guc, rq);
  

• } while (out != end);

• return err; }

  static inline int rq_prio(const struct i915_request *rq)

@@ -160,125 +177,88 @@ static inline int rq_prio(const struct i915_request *rq) return rq->sched.attr.priority; }

-static struct i915_request *schedule_in(struct i915_request *rq, int idx) +static int guc_dequeue_one_context(struct intel_guc *guc) {

• trace_i915_request_in(rq, idx);
• /*

• * Currently we are not tracking the rq->context being inflight
  

• * (ce->inflight = rq->engine). It is only used by the execlists
  

• * backend at the moment, a similar counting strategy would be
  

• * required if we generalise the inflight tracking.
  

• */
  

• __intel_gt_pm_get(rq->engine->gt);
• return i915_request_get(rq);

-}

-static void schedule_out(struct i915_request *rq) -{

• trace_i915_request_out(rq);
• intel_gt_pm_put_async(rq->engine->gt);
• i915_request_put(rq);

-}

-static void __guc_dequeue(struct intel_engine_cs *engine) -{

• struct intel_engine_execlists * const execlists = &engine->execlists;
• struct i915_sched_engine * const sched_engine = engine->sched_engine;
• struct i915_request **first = execlists->inflight;
• struct i915_request ** const last_port = first + execlists->port_mask;
• struct i915_request *last = first[0];
• struct i915_request **port;

• struct i915_sched_engine * const sched_engine = guc->sched_engine;

• struct i915_request *last = NULL; bool submit = false; struct rb_node *rb;

• int ret;

  lockdep_assert_held(&sched_engine->lock);

• if (last) {

• if (*++first)
  

• 	return;
  

• last = NULL;
  

• if (guc->stalled_request) {

• submit = true;
  

• last = guc->stalled_request;
  

• goto resubmit;
  

  }

• /*

• * We write directly into the execlists->inflight queue and don't use
  

• * the execlists->pending queue, as we don't have a distinct switch
  

• * event.
  

• */
  

• port = first; while ((rb = rb_first_cached(&sched_engine->queue))) { struct i915_priolist *p = to_priolist(rb); struct i915_request *rq, *rn;

  priolist_for_each_request_consume(rq, rn, p) {

• 	if (last && rq->context != last->context) {
  

• 		if (port == last_port)
  

• 			goto done;
  

• 		*port = schedule_in(last,
  

• 				    port - execlists->inflight);
  

• 		port++;
  

• 	}
  

• 	if (last && rq->context != last->context)
  

• 		goto done;
  
  list_del_init(&rq->sched.link);
  

• __i915_request_submit(rq);
  

• 	submit = true;
  

• 	trace_i915_request_in(rq, 0);
  last = rq;
  

• 	submit = true;
  

  }

  rb_erase_cached(&p->node, &sched_engine->queue); i915_priolist_free(p); } done:

• sched_engine->queue_priority_hint =

• rb ? to_priolist(rb)->priority : INT_MIN;
  

  if (submit) {

• *port = schedule_in(last, port - execlists->inflight);
  

• *++port = NULL;
  

• guc_submit(engine, first, port);
  

• last->context->lrc_reg_state[CTX_RING_TAIL] =
  

• 	intel_ring_set_tail(last->ring, last->tail);
  

+resubmit:

• /*
  

•  * We only check for -EBUSY here even though it is possible for
  

•  * -EDEADLK to be returned. If -EDEADLK is returned, the GuC has
  

•  * died and a full GT reset needs to be done. The hangcheck will
  

•  * eventually detect that the GuC has died and trigger this
  

•  * reset so no need to handle -EDEADLK here.
  

•  */
  

• ret = guc_add_request(guc, last);
  

• if (ret == -EBUSY) {
  

• 	tasklet_schedule(&sched_engine->tasklet);
  

• 	guc->stalled_request = last;
  

• 	return false;
  

• }
  

  }

• execlists->active = execlists->inflight;

• guc->stalled_request = NULL;

• return submit; }

  static void guc_submission_tasklet(struct tasklet_struct *t) { struct i915_sched_engine *sched_engine = from_tasklet(sched_engine, t, tasklet);

• struct intel_engine_cs * const engine = sched_engine->private_data;
• struct intel_engine_execlists * const execlists = &engine->execlists;
• struct i915_request **port, *rq; unsigned long flags;

• bool loop;

• spin_lock_irqsave(&engine->sched_engine->lock, flags);
• for (port = execlists->inflight; (rq = *port); port++) {

• if (!i915_request_completed(rq))
  

• 	break;
  

• schedule_out(rq);
  

• }
• if (port != execlists->inflight) {

• int idx = port - execlists->inflight;
  

• int rem = ARRAY_SIZE(execlists->inflight) - idx;
  

• memmove(execlists->inflight, port, rem * sizeof(*port));
  

• }

• spin_lock_irqsave(&sched_engine->lock, flags);

• __guc_dequeue(engine);

• do {

• loop = guc_dequeue_one_context(sched_engine->private_data);
  

• } while (loop);

• i915_sched_engine_reset_on_empty(engine->sched_engine);

• i915_sched_engine_reset_on_empty(sched_engine);

• spin_unlock_irqrestore(&engine->sched_engine->lock, flags);

• spin_unlock_irqrestore(&sched_engine->lock, flags); }

  static void cs_irq_handler(struct intel_engine_cs *engine, u16 iir) {

• if (iir & GT_RENDER_USER_INTERRUPT) {

• if (iir & GT_RENDER_USER_INTERRUPT) intel_engine_signal_breadcrumbs(engine);

• tasklet_hi_schedule(&engine->sched_engine->tasklet);
  

• } }

  static void guc_reset_prepare(struct intel_engine_cs *engine)

@@ -349,6 +329,10 @@ static void guc_reset_cancel(struct intel_engine_cs *engine) struct rb_node *rb; unsigned long flags;

• /* Can be called during boot if GuC fails to load */

• if (!engine->gt)

• return;
  

• ENGINE_TRACE(engine, "\n");

  /*

@@ -433,8 +417,11 @@ int intel_guc_submission_init(struct intel_guc *guc)

void intel_guc_submission_fini(struct intel_guc *guc) {

• if (guc->lrc_desc_pool)

• guc_lrc_desc_pool_destroy(guc);
  

• if (!guc->lrc_desc_pool)

• return;
  

• guc_lrc_desc_pool_destroy(guc);

• i915_sched_engine_put(guc->sched_engine); }

  static int guc_context_alloc(struct intel_context *ce)

@@ -499,32 +486,32 @@ static int guc_request_alloc(struct i915_request *request) return 0; }

-static inline void queue_request(struct intel_engine_cs *engine, +static inline void queue_request(struct i915_sched_engine *sched_engine, struct i915_request *rq, int prio) { GEM_BUG_ON(!list_empty(&rq->sched.link)); list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(engine->sched_engine, prio));
  

•       i915_sched_lookup_priolist(sched_engine, prio));
  

  set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); }

  static void guc_submit_request(struct i915_request *rq) {

• struct intel_engine_cs *engine = rq->engine;

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine; unsigned long flags;

  /* Will be called from irq-context when using foreign fences. */

• spin_lock_irqsave(&engine->sched_engine->lock, flags);

• spin_lock_irqsave(&sched_engine->lock, flags);

• queue_request(engine, rq, rq_prio(rq));

• queue_request(sched_engine, rq, rq_prio(rq));

• GEM_BUG_ON(i915_sched_engine_is_empty(engine->sched_engine));

• GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine)); GEM_BUG_ON(list_empty(&rq->sched.link));

• tasklet_hi_schedule(&engine->sched_engine->tasklet);

• tasklet_hi_schedule(&sched_engine->tasklet);

• spin_unlock_irqrestore(&engine->sched_engine->lock, flags);

• spin_unlock_irqrestore(&sched_engine->lock, flags); }

  static void sanitize_hwsp(struct intel_engine_cs *engine)

@@ -602,8 +589,6 @@ static void guc_release(struct intel_engine_cs *engine) { engine->sanitize = NULL; /* no longer in control, nothing to sanitize */

• tasklet_kill(&engine->sched_engine->tasklet);
• intel_engine_cleanup_common(engine); lrc_fini_wa_ctx(engine); }

@@ -674,6 +659,7 @@ static inline void guc_default_irqs(struct intel_engine_cs *engine) int intel_guc_submission_setup(struct intel_engine_cs *engine) { struct drm_i915_private *i915 = engine->i915;

• struct intel_guc *guc = &engine->gt->uc.guc;

  /*

  • The setup relies on several assumptions (e.g. irqs always enabled)

@@ -681,7 +667,18 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine) */ GEM_BUG_ON(GRAPHICS_VER(i915) < 11);

• tasklet_setup(&engine->sched_engine->tasklet, guc_submission_tasklet);

• if (!guc->sched_engine) {

• guc->sched_engine = i915_sched_engine_create(ENGINE_VIRTUAL);
  

• if (!guc->sched_engine)
  

• 	return -ENOMEM;
  

• guc->sched_engine->schedule = i915_schedule;
  

• guc->sched_engine->private_data = guc;
  

• tasklet_setup(&guc->sched_engine->tasklet,
  

• 	      guc_submission_tasklet);
  

• }

• i915_sched_engine_put(engine->sched_engine);

• engine->sched_engine = i915_sched_engine_get(guc->sched_engine);

  guc_default_vfuncs(engine); guc_default_irqs(engine);

On Mon, Jul 19, 2021 at 05:23:55PM -0700, John Harrison wrote:

On 7/16/2021 13:16, Matthew Brost wrote:

...

Implement GuC context operations which includes GuC specific operations alloc, pin, unpin, and destroy.

v2: (Daniel Vetter)

• Use msleep_interruptible rather than cond_resched in busy loop

(Michal)

• Remove C++ style comment

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_context.c | 5 + drivers/gpu/drm/i915/gt/intel_context_types.h | 22 +- drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 - drivers/gpu/drm/i915/gt/uc/intel_guc.h | 40 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 666 ++++++++++++++++-- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/i915_request.c | 1 + 8 files changed, 685 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index bd63813c8a80..32fd6647154b 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -384,6 +384,11 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine) mutex_init(&ce->pin_mutex);

• spin_lock_init(&ce->guc_state.lock);
• ce->guc_id = GUC_INVALID_LRC_ID;
• INIT_LIST_HEAD(&ce->guc_id_link);
• i915_active_init(&ce->active, __intel_context_active, __intel_context_retire, 0); }

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 6d99631d19b9..606c480aec26 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -96,6 +96,7 @@ struct intel_context { #define CONTEXT_BANNED 6 #define CONTEXT_FORCE_SINGLE_SUBMISSION 7 #define CONTEXT_NOPREEMPT 8 +#define CONTEXT_LRCA_DIRTY 9 struct { u64 timeout_us; @@ -138,14 +139,29 @@ struct intel_context { u8 wa_bb_page; /* if set, page num reserved for context workarounds */

• struct {

• /** lock: protects everything in guc_state */
  

• spinlock_t lock;
  

• /**
  

•  * sched_state: scheduling state of this context using GuC
  

•  * submission
  

•  */
  

• u8 sched_state;
  

• } guc_state;
• /* GuC scheduling state flags that do not require a lock. */ atomic_t guc_sched_state_no_lock;
• /* GuC LRC descriptor ID */
• u16 guc_id;
• /* GuC LRC descriptor reference count */
• atomic_t guc_id_ref;
• /*

• * GuC LRC descriptor ID - Not assigned in this patch but future patches
  

• * in the series will.
  

• * GuC ID link - in list when unpinned but guc_id still valid in GuC
  

  */

• u16 guc_id;

• struct list_head guc_id_link; }; #endif /* __INTEL_CONTEXT_TYPES__ */

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h index 41e5350a7a05..49d4857ad9b7 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h @@ -87,7 +87,6 @@ #define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0) #define MAX_CONTEXT_HW_ID (1 << 21) /* exclusive */ -#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */ #define GEN11_MAX_CONTEXT_HW_ID (1 << 11) /* exclusive */ /* in Gen12 ID 0x7FF is reserved to indicate idle */ #define GEN12_MAX_CONTEXT_HW_ID (GEN11_MAX_CONTEXT_HW_ID - 1) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 8c7b92f699f1..30773cd699f5 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -7,6 +7,7 @@ #define _INTEL_GUC_H_ #include <linux/xarray.h> +#include <linux/delay.h> #include "intel_uncore.h" #include "intel_guc_fw.h" @@ -44,6 +45,14 @@ struct intel_guc { void (*disable)(struct intel_guc *guc); } interrupts;

• /*

• * contexts_lock protects the pool of free guc ids and a linked list of
  

• * guc ids available to be stolen
  

• */
  

• spinlock_t contexts_lock;
• struct ida guc_ids;
• struct list_head guc_id_list;
• bool submission_selected; struct i915_vma *ads_vma;

@@ -101,6 +110,34 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, response_buf, response_buf_size, 0); } +static inline int intel_guc_send_busy_loop(struct intel_guc* guc,

• 			   const u32 *action,
  

• 			   u32 len,
  

• 			   bool loop)
  

+{

• int err;
• unsigned int sleep_period_ms = 1;
• bool not_atomic = !in_atomic() && !irqs_disabled();
• /* No sleeping with spin locks, just busy loop */
• might_sleep_if(loop && not_atomic);

+retry:

• err = intel_guc_send_nb(guc, action, len);
• if (unlikely(err == -EBUSY && loop)) {

• if (likely(not_atomic)) {
  

• 	if (msleep_interruptible(sleep_period_ms))
  

• 		return -EINTR;
  

• 	sleep_period_ms = sleep_period_ms << 1;
  

• } else {
  

• 	cpu_relax();
  

• }
  

• goto retry;
  

• }
• return err;

+}

• static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) { intel_guc_ct_event_handler(&guc->ct);

@@ -202,6 +239,9 @@ static inline void intel_guc_disable_msg(struct intel_guc *guc, u32 mask) int intel_guc_reset_engine(struct intel_guc *guc, struct intel_engine_cs *engine); +int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

• 			  const u32 *msg, u32 len);
  

• void intel_guc_load_status(struct intel_guc *guc, struct drm_printer *p); #endif

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 83ec60ea3f89..28ff82c5be45 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -928,6 +928,10 @@ static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *r case INTEL_GUC_ACTION_DEFAULT: ret = intel_guc_to_host_process_recv_msg(guc, payload, len); break;

• case INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE:

• ret = intel_guc_deregister_done_process_msg(guc, payload,
  

• 					    len);
  

• break;
  

  default: ret = -EOPNOTSUPP; break;

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 53b4a5eb4a85..a47b3813b4d0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -13,7 +13,9 @@ #include "gt/intel_gt.h" #include "gt/intel_gt_irq.h" #include "gt/intel_gt_pm.h" +#include "gt/intel_gt_requests.h" #include "gt/intel_lrc.h" +#include "gt/intel_lrc_reg.h" #include "gt/intel_mocs.h" #include "gt/intel_ring.h" @@ -85,6 +87,73 @@ static inline void clr_context_enabled(struct intel_context *ce) &ce->guc_sched_state_no_lock); } +/*

• • Below is a set of functions which control the GuC scheduling state which
• • require a lock, aside from the special case where the functions are called
• • from guc_lrc_desc_pin(). In that case it isn't possible for any other code
• • path to be executing on the context.
• */

+#define SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER BIT(0) +#define SCHED_STATE_DESTROYED BIT(1) +static inline void init_sched_state(struct intel_context *ce) +{

• /* Only should be called from guc_lrc_desc_pin() */
• atomic_set(&ce->guc_sched_state_no_lock, 0);
• ce->guc_state.sched_state = 0;

+}

+static inline bool +context_wait_for_deregister_to_register(struct intel_context *ce) +{

• return (ce->guc_state.sched_state &

• SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER);
  

+}

+static inline void +set_context_wait_for_deregister_to_register(struct intel_context *ce) +{

• /* Only should be called from guc_lrc_desc_pin() */
• ce->guc_state.sched_state |=

• SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER;
  

+}

+static inline void +clr_context_wait_for_deregister_to_register(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state =

• (ce->guc_state.sched_state &
  

•  ~SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER);
  

+}

+static inline bool +context_destroyed(struct intel_context *ce) +{

• return (ce->guc_state.sched_state & SCHED_STATE_DESTROYED);

+}

+static inline void +set_context_destroyed(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state |= SCHED_STATE_DESTROYED;

+}

+static inline bool context_guc_id_invalid(struct intel_context *ce) +{

• return (ce->guc_id == GUC_INVALID_LRC_ID);

+}

+static inline void set_context_guc_id_invalid(struct intel_context *ce) +{

• ce->guc_id = GUC_INVALID_LRC_ID;

+}

+static inline struct intel_guc *ce_to_guc(struct intel_context *ce) +{

• return &ce->engine->gt->uc.guc;

+}

• static inline struct i915_priolist *to_priolist(struct rb_node *rb) { return rb_entry(rb, struct i915_priolist, node);

@@ -155,6 +224,9 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) int len = 0; bool enabled = context_enabled(ce);

• GEM_BUG_ON(!atomic_read(&ce->guc_id_ref));
• GEM_BUG_ON(context_guc_id_invalid(ce));
• if (!enabled) { action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET; action[len++] = ce->guc_id;

@@ -417,6 +489,10 @@ int intel_guc_submission_init(struct intel_guc *guc) xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);

• spin_lock_init(&guc->contexts_lock);
• INIT_LIST_HEAD(&guc->guc_id_list);
• ida_init(&guc->guc_ids);
• return 0; }

@@ -429,9 +505,303 @@ void intel_guc_submission_fini(struct intel_guc *guc) i915_sched_engine_put(guc->sched_engine); } -static int guc_context_alloc(struct intel_context *ce) +static inline void queue_request(struct i915_sched_engine *sched_engine,

• 		 struct i915_request *rq,
  

• 		 int prio)
  

  {

• return lrc_alloc(ce, ce->engine);

• GEM_BUG_ON(!list_empty(&rq->sched.link));
• list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(sched_engine, prio));
  

• set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

+}

+static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

+{

• int ret;
• __i915_request_submit(rq);
• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

+}

+static void guc_submit_request(struct i915_request *rq) +{

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine;
• struct intel_guc *guc = &rq->engine->gt->uc.guc;
• unsigned long flags;
• /* Will be called from irq-context when using foreign fences. */
• spin_lock_irqsave(&sched_engine->lock, flags);
• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  

• spin_unlock_irqrestore(&sched_engine->lock, flags);

+}

+#define GUC_ID_START 64 /* First 64 guc_ids reserved */

Reserved for what?

A memory corruption which I found today. This is no longer needed.

...

+static int new_guc_id(struct intel_guc *guc) +{

• return ida_simple_get(&guc->guc_ids, GUC_ID_START,

• 	      GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL |
  

• 	      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
  

+}

+static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• if (!context_guc_id_invalid(ce)) {

• ida_simple_remove(&guc->guc_ids, ce->guc_id);
  

• reset_lrc_desc(guc, ce->guc_id);
  

• set_context_guc_id_invalid(ce);
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

+}

+static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• unsigned long flags;
• spin_lock_irqsave(&guc->contexts_lock, flags);
• __release_guc_id(guc, ce);
• spin_unlock_irqrestore(&guc->contexts_lock, flags);

+}

+static int steal_guc_id(struct intel_guc *guc) +{

• struct intel_context *ce;
• int guc_id;
• if (!list_empty(&guc->guc_id_list)) {

• ce = list_first_entry(&guc->guc_id_list,
  

• 		      struct intel_context,
  

• 		      guc_id_link);
  

• GEM_BUG_ON(atomic_read(&ce->guc_id_ref));
  

• GEM_BUG_ON(context_guc_id_invalid(ce));
  

• list_del_init(&ce->guc_id_link);
  

• guc_id = ce->guc_id;
  

• set_context_guc_id_invalid(ce);
  

• return guc_id;
  

• } else {

• return -EAGAIN;
  

• }

+}

+static int assign_guc_id(struct intel_guc *guc, u16 *out) +{

• int ret;
• ret = new_guc_id(guc);
• if (unlikely(ret < 0)) {

• ret = steal_guc_id(guc);
  

• if (ret < 0)
  

• 	return ret;
  

• }
• *out = ret;
• return 0;

+}

+#define PIN_GUC_ID_TRIES 4 +static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• int ret = 0;
• unsigned long flags, tries = PIN_GUC_ID_TRIES;
• GEM_BUG_ON(atomic_read(&ce->guc_id_ref));

+try_again:

• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (context_guc_id_invalid(ce)) {

• ret = assign_guc_id(guc, &ce->guc_id);
  

• if (ret)
  

• 	goto out_unlock;
  

• ret = 1;	/* Indidcates newly assigned guc_id */
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

• atomic_inc(&ce->guc_id_ref);

+out_unlock:

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
• /*

• * -EAGAIN indicates no guc_ids are available, let's retire any
  

• * outstanding requests to see if that frees up a guc_id. If the first
  

• * retire didn't help, insert a sleep with the timeslice duration before
  

• * attempting to retire more requests. Double the sleep period each
  

• * subsequent pass before finally giving up. The sleep period has max of
  

• * 100ms and minimum of 1ms.
  

• */
  

• if (ret == -EAGAIN && --tries) {

• if (PIN_GUC_ID_TRIES - tries > 1) {
  

• 	unsigned int timeslice_shifted =
  

• 		ce->engine->props.timeslice_duration_ms <<
  

• 		(PIN_GUC_ID_TRIES - tries - 2);
  

• 	unsigned int max = min_t(unsigned int, 100,
  

• 				 timeslice_shifted);
  

• 	msleep(max_t(unsigned int, max, 1));
  

• }
  

• intel_gt_retire_requests(guc_to_gt(guc));
  

• goto try_again;
  

• }
• return ret;

+}

+static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• unsigned long flags;
• GEM_BUG_ON(atomic_read(&ce->guc_id_ref) < 0);
• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (!context_guc_id_invalid(ce) && list_empty(&ce->guc_id_link) &&

•    !atomic_read(&ce->guc_id_ref))
  

• list_add_tail(&ce->guc_id_link, &guc->guc_id_list);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);

+}

+static int __guc_action_register_context(struct intel_guc *guc,

• 			 u32 guc_id,
  

• 			 u32 offset)
  

+{

• u32 action[] = {

• INTEL_GUC_ACTION_REGISTER_CONTEXT,
  

• guc_id,
  

• offset,
  

• };
• return intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), true);

+}

+static int register_context(struct intel_context *ce) +{

• struct intel_guc *guc = ce_to_guc(ce);
• u32 offset = intel_guc_ggtt_offset(guc, guc->lrc_desc_pool) +

• ce->guc_id * sizeof(struct guc_lrc_desc);
  

• return __guc_action_register_context(guc, ce->guc_id, offset);

+}

+static int __guc_action_deregister_context(struct intel_guc *guc,

• 			   u32 guc_id)
  

+{

• u32 action[] = {

• INTEL_GUC_ACTION_DEREGISTER_CONTEXT,
  

• guc_id,
  

• };
• return intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), true);

+}

+static int deregister_context(struct intel_context *ce, u32 guc_id) +{

• struct intel_guc *guc = ce_to_guc(ce);
• return __guc_action_deregister_context(guc, guc_id);

+}

+static intel_engine_mask_t adjust_engine_mask(u8 class, intel_engine_mask_t mask) +{

• switch (class) {
• case RENDER_CLASS:

• return mask >> RCS0;
  

• case VIDEO_ENHANCEMENT_CLASS:

• return mask >> VECS0;
  

• case VIDEO_DECODE_CLASS:

• return mask >> VCS0;
  

• case COPY_ENGINE_CLASS:

• return mask >> BCS0;
  

• default:

• GEM_BUG_ON("Invalid Class");
  

• return 0;
  

• }

+}

+static void guc_context_policy_init(struct intel_engine_cs *engine,

• 		    struct guc_lrc_desc *desc)
  

+{

• desc->policy_flags = 0;
• desc->execution_quantum = CONTEXT_POLICY_DEFAULT_EXECUTION_QUANTUM_US;
• desc->preemption_timeout = CONTEXT_POLICY_DEFAULT_PREEMPTION_TIME_US;

+}

+static int guc_lrc_desc_pin(struct intel_context *ce) +{

• struct intel_runtime_pm *runtime_pm =

• &ce->engine->gt->i915->runtime_pm;
  

• struct intel_engine_cs *engine = ce->engine;
• struct intel_guc *guc = &engine->gt->uc.guc;
• u32 desc_idx = ce->guc_id;
• struct guc_lrc_desc *desc;
• bool context_registered;
• intel_wakeref_t wakeref;
• int ret = 0;
• GEM_BUG_ON(!engine->mask);
• /*

• * Ensure LRC + CT vmas are is same region as write barrier is done
  

• * based on CT vma region.
  

• */
  

• GEM_BUG_ON(i915_gem_object_is_lmem(guc->ct.vma->obj) !=

•    i915_gem_object_is_lmem(ce->ring->vma->obj));
  

• context_registered = lrc_desc_registered(guc, desc_idx);
• reset_lrc_desc(guc, desc_idx);
• set_lrc_desc_registered(guc, desc_idx, ce);
• desc = __get_lrc_desc(guc, desc_idx);
• desc->engine_class = engine_class_to_guc_class(engine->class);
• desc->engine_submit_mask = adjust_engine_mask(engine->class,

• 				      engine->mask);
  

• desc->hw_context_desc = ce->lrc.lrca;
• desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
• desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
• guc_context_policy_init(engine, desc);
• init_sched_state(ce);
• /*

• * The context_lookup xarray is used to determine if the hardware
  

• * context is currently registered. There are two cases in which it
  

• * could be regisgered either the guc_id has been stole from from
  

'regisgered' and 'stole' should be 'stolen'

Yep.

...

• * another context or the lrc descriptor address of this context has
  

• * changed. In either case the context needs to be deregistered with the
  

• * GuC before registering this context.
  

• */
  

• if (context_registered) {

• set_context_wait_for_deregister_to_register(ce);
  

• intel_context_get(ce);
  

• /*
  

•  * If stealing the guc_id, this ce has the same guc_id as the
  

•  * context whos guc_id was stole.
  

whose guc_id was stolen

Yep.

...

•  */
  

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	ret = deregister_context(ce, ce->guc_id);
  

• } else {

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	ret = register_context(ce);
  

• }
• return ret; } static int guc_context_pre_pin(struct intel_context *ce,

@@ -443,36 +813,139 @@ static int guc_context_pre_pin(struct intel_context *ce, static int guc_context_pin(struct intel_context *ce, void *vaddr) {

• if (i915_ggtt_offset(ce->state) !=

•    (ce->lrc.lrca & CTX_GTT_ADDRESS_MASK))
  

• set_bit(CONTEXT_LRCA_DIRTY, &ce->flags);
  

• return lrc_pin(ce, ce->engine, vaddr); }

+static void guc_context_unpin(struct intel_context *ce) +{

• struct intel_guc *guc = ce_to_guc(ce);
• unpin_guc_id(guc, ce);
• lrc_unpin(ce);

+}

+static void guc_context_post_unpin(struct intel_context *ce) +{

• lrc_post_unpin(ce);

+}

+static inline void guc_lrc_desc_unpin(struct intel_context *ce) +{

• struct intel_engine_cs *engine = ce->engine;
• struct intel_guc *guc = &engine->gt->uc.guc;

Use ce_to_guc?

Yes.

...

• unsigned long flags;
• GEM_BUG_ON(!lrc_desc_registered(guc, ce->guc_id));
• GEM_BUG_ON(ce != __get_context(guc, ce->guc_id));
• spin_lock_irqsave(&ce->guc_state.lock, flags);
• set_context_destroyed(ce);
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• deregister_context(ce, ce->guc_id);

+}

+static void guc_context_destroy(struct kref *kref) +{

• struct intel_context *ce = container_of(kref, typeof(*ce), ref);
• struct intel_runtime_pm *runtime_pm = &ce->engine->gt->i915->runtime_pm;
• struct intel_guc *guc = &ce->engine->gt->uc.guc;

Also ce_to_guc?

Yes.

...

• intel_wakeref_t wakeref;
• unsigned long flags;
• /*

• * If the guc_id is invalid this context has been stolen and we can free
  

• * it immediately. Also can be freed immediately if the context is not
  

• * registered with the GuC.
  

• */
  

• if (context_guc_id_invalid(ce) ||

•    !lrc_desc_registered(guc, ce->guc_id)) {
  

• release_guc_id(guc, ce);
  

• lrc_destroy(kref);
  

• return;
  

• }
• /*

• * We have to acquire the context spinlock and check guc_id again, if it
  

• * is valid it hasn't been stolen and needs to be deregistered. We
  

• * delete this context from the list of unpinned guc_ids available to
  

• * stole to seal a race with guc_lrc_desc_pin(). When the G2H CTB
  

stole -> steal

...

• * returns indicating this context has been deregistered the guc_id is
  

• * returned to the pool of available guc_ids.
  

• */
  

• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (context_guc_id_invalid(ce)) {

• __release_guc_id(guc, ce);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
  

• lrc_destroy(kref);
  

• return;
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
• /*

• * We defer GuC context deregistration until the context is destroyed
  

• * in order to save on CTBs. With this optimization ideally we only need
  

• * 1 CTB to register the context during the first pin and 1 CTB to
  

• * deregister the context when the context is destroyed. Without this
  

• * optimization, a CTB would be needed every pin & unpin.
  

• *
  

• * XXX: Need to acqiure the runtime wakeref as this can be triggered
  

FIXME rather than XXX?

I think XXX is correct as it isn't broken. After this series gets merged have a patch in the follow on series that fixes this.

...

• * from context_free_worker when not runtime wakeref is held.
  

when runtime wakeref is not held

Yep.

...

• * guc_lrc_desc_unpin requires the runtime as a GuC register is written
  

• * in H2G CTB to deregister the context. A future patch may defer this
  

• * H2G CTB if the runtime wakeref is zero.
  

• */
  

• with_intel_runtime_pm(runtime_pm, wakeref)

• guc_lrc_desc_unpin(ce);
  

+}

+static int guc_context_alloc(struct intel_context *ce) +{

• return lrc_alloc(ce, ce->engine);

+}

• static const struct intel_context_ops guc_context_ops = { .alloc = guc_context_alloc, .pre_pin = guc_context_pre_pin, .pin = guc_context_pin,

• .unpin = lrc_unpin,
• .post_unpin = lrc_post_unpin,

• .unpin = guc_context_unpin,
• .post_unpin = guc_context_post_unpin, .enter = intel_context_enter_engine, .exit = intel_context_exit_engine, .reset = lrc_reset,

• .destroy = lrc_destroy,

• .destroy = guc_context_destroy, };

-static int guc_request_alloc(struct i915_request *request) +static bool context_needs_register(struct intel_context *ce, bool new_guc_id) {

• return new_guc_id || test_bit(CONTEXT_LRCA_DIRTY, &ce->flags) ||

• !lrc_desc_registered(ce_to_guc(ce), ce->guc_id);
  

+}

+static int guc_request_alloc(struct i915_request *rq) +{

• struct intel_context *ce = rq->context;
• struct intel_guc *guc = ce_to_guc(ce); int ret;

• GEM_BUG_ON(!intel_context_is_pinned(request->context));

• GEM_BUG_ON(!intel_context_is_pinned(rq->context)); /*
  • Flush enough space to reduce the likelihood of waiting after
  • we start building the request - in which case we will just
  • have to repeat work.
  */

• request->reserved_space += GUC_REQUEST_SIZE;

• rq->reserved_space += GUC_REQUEST_SIZE; /*
  • Note that after this point, we have committed to using

@@ -483,56 +956,47 @@ static int guc_request_alloc(struct i915_request *request) */ /* Unconditionally invalidate GPU caches and TLBs. */

• ret = request->engine->emit_flush(request, EMIT_INVALIDATE);

• ret = rq->engine->emit_flush(rq, EMIT_INVALIDATE); if (ret) return ret;

• request->reserved_space -= GUC_REQUEST_SIZE;
• return 0;

-}

• rq->reserved_space -= GUC_REQUEST_SIZE;

-static inline void queue_request(struct i915_sched_engine *sched_engine,

• 		 struct i915_request *rq,
  

• 		 int prio)
  

-{

• GEM_BUG_ON(!list_empty(&rq->sched.link));
• list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(sched_engine, prio));
  

• set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

-}

-static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

-{

• int ret;
• __i915_request_submit(rq);
• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

-}

-static void guc_submit_request(struct i915_request *rq) -{

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine;
• struct intel_guc *guc = &rq->engine->gt->uc.guc;
• unsigned long flags;

• /*

• * Call pin_guc_id here rather than in the pinning step as with
  

• * dma_resv, contexts can be repeatedly pinned / unpinned trashing the
  

• * guc_ids and creating horrible race conditions. This is especially bad
  

• * when guc_ids are being stolen due to over subscription. By the time
  

• * this function is reached, it is guaranteed that the guc_id will be
  

• * persistent until the generated request is retired. Thus, sealing these
  

• * race conditions. It is still safe to fail here if guc_ids are
  

• * exhausted and return -EAGAIN to the user indicating that they can try
  

• * again in the future.
  

• *
  

• * There is no need for a lock here as the timeline mutex ensures at
  

• * most one context can be executing this code path at once. The
  

• * guc_id_ref is incremented once for every request in flight and
  

• * decremented on each retire. When it is zero, a lock around the
  

• * increment (in pin_guc_id) is needed to seal a race with unpin_guc_id.
  

• */
  

• if (atomic_add_unless(&ce->guc_id_ref, 1, 0))

• return 0;
  

• /* Will be called from irq-context when using foreign fences. */
• spin_lock_irqsave(&sched_engine->lock, flags);

• ret = pin_guc_id(guc, ce); /* returns 1 if new guc_id assigned */
• if (unlikely(ret < 0))

• return ret;;
  

;;

...

• if (context_needs_register(ce, !!ret)) {

• ret = guc_lrc_desc_pin(ce);
  

• if (unlikely(ret)) {	/* unwind */
  

• 	atomic_dec(&ce->guc_id_ref);
  

• 	unpin_guc_id(guc, ce);
  

• 	return ret;
  

• }
  

• }

• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  

• clear_bit(CONTEXT_LRCA_DIRTY, &ce->flags);

• spin_unlock_irqrestore(&sched_engine->lock, flags);

• return 0; } static void sanitize_hwsp(struct intel_engine_cs *engine)

@@ -606,6 +1070,46 @@ static void guc_set_default_submission(struct intel_engine_cs *engine) engine->submit_request = guc_submit_request; } +static inline void guc_kernel_context_pin(struct intel_guc *guc,

• 			  struct intel_context *ce)
  

+{

• if (context_guc_id_invalid(ce))

• pin_guc_id(guc, ce);
  

• guc_lrc_desc_pin(ce);

+}

+static inline void guc_init_lrc_mapping(struct intel_guc *guc) +{

• struct intel_gt *gt = guc_to_gt(guc);
• struct intel_engine_cs *engine;
• enum intel_engine_id id;
• /* make sure all descriptors are clean... */
• xa_destroy(&guc->context_lookup);
• /*

• * Some contexts might have been pinned before we enabled GuC
  

• * submission, so we need to add them to the GuC bookeeping.
  

• * Also, after a reset the GuC we want to make sure that the information
  

reset of the GuC

Yep.

...

• * shared with GuC is properly reset. The kernel lrcs are not attached
  

LRCs

Yep.

...

• * to the gem_context, so they need to be added separately.
  

• *
  

• * Note: we purposely do not check the error return of
  

check the return of gldp for errors

Yep.

...

• * guc_lrc_desc_pin, because that function can only fail in two cases.
  

• * One, if there aren't enough free IDs, but we're guaranteed to have
  

• * enough here (we're either only pinning a handful of lrc on first boot
  

LRCs and same on line below

...

• * or we're re-pinning lrcs that were already pinned before the reset).
  

• * Two, if the GuC has died and CTBs can't make forward progress.
  

• * Presumably, the GuC should be alive as this function is called on
  

• * driver load or after a reset. Even if it is dead, another full GPU
  

GPU -> GT

This comment it stale. Need just update the whole thing.

...

• * reset will be triggered and this function would be called again.
  

• */
  

• for_each_engine(engine, gt, id)

• if (engine->kernel_context)
  

• 	guc_kernel_context_pin(guc, engine->kernel_context);
  

+}

• static void guc_release(struct intel_engine_cs *engine) { engine->sanitize = NULL; /* no longer in control, nothing to sanitize */

@@ -718,6 +1222,7 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine) void intel_guc_submission_enable(struct intel_guc *guc) {

• guc_init_lrc_mapping(guc); } void intel_guc_submission_disable(struct intel_guc *guc)

@@ -743,3 +1248,62 @@ void intel_guc_submission_init_early(struct intel_guc *guc) { guc->submission_selected = __guc_submission_selected(guc); }

+static inline struct intel_context * +g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) +{

• struct intel_context *ce;
• if (unlikely(desc_idx >= GUC_MAX_LRC_DESCRIPTORS)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm,
  

• 	"Invalid desc_idx %u", desc_idx);
  

Agree these should not be BUG_ONs or similar, but a drm_err seems appropriate. If either this or the error below occurs the something has gone quite wrong and it should be reported.

Sure.

...

• return NULL;
  

• }
• ce = __get_context(guc, desc_idx);
• if (unlikely(!ce)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm,
  

• 	"Context is NULL, desc_idx %u", desc_idx);
  

• return NULL;
  

• }
• return ce;

+}

+int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

• 			  const u32 *msg,
  

• 			  u32 len)
  

+{

• struct intel_context *ce;
• u32 desc_idx = msg[0];
• if (unlikely(len < 1)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm, "Invalid length %u", len);
  

As above, should be a drm_err I would say.

Sure.

Matt

...

• return -EPROTO;
  

• }
• ce = g2h_context_lookup(guc, desc_idx);
• if (unlikely(!ce))

• return -EPROTO;
  

• if (context_wait_for_deregister_to_register(ce)) {

• struct intel_runtime_pm *runtime_pm =
  

• 	&ce->engine->gt->i915->runtime_pm;
  

• intel_wakeref_t wakeref;
  

• /*
  

•  * Previous owner of this guc_id has been deregistered, now safe
  

•  * register this context.
  

•  */
  

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	register_context(ce);
  

• clr_context_wait_for_deregister_to_register(ce);
  

• intel_context_put(ce);
  

• } else if (context_destroyed(ce)) {

• /* Context has been destroyed */
  

• release_guc_id(guc, ce);
  

• lrc_destroy(&ce->ref);
  

• }
• return 0;

+} diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 943fe485c662..204c95c39353 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4142,6 +4142,7 @@ enum { FAULT_AND_CONTINUE /* Unsupported */ }; +#define CTX_GTT_ADDRESS_MASK GENMASK(31, 12) #define GEN8_CTX_VALID (1 << 0) #define GEN8_CTX_FORCE_PD_RESTORE (1 << 1) #define GEN8_CTX_FORCE_RESTORE (1 << 2) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 86b4c9f2613d..b48c4905d3fc 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -407,6 +407,7 @@ bool i915_request_retire(struct i915_request *rq) */ if (!list_empty(&rq->sched.link)) remove_from_engine(rq);

• atomic_dec(&rq->context->guc_id_ref); GEM_BUG_ON(!llist_empty(&rq->execute_cb)); __list_del_entry(&rq->link); /* poison neither prev/next (RCU walks) */

On Mon, Jul 19, 2021 at 05:51:46PM -0700, Daniele Ceraolo Spurio wrote:

On 7/16/2021 1:16 PM, Matthew Brost wrote:

...

Implement GuC context operations which includes GuC specific operations alloc, pin, unpin, and destroy.

v2: (Daniel Vetter)

• Use msleep_interruptible rather than cond_resched in busy loop

(Michal)

• Remove C++ style comment

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_context.c | 5 + drivers/gpu/drm/i915/gt/intel_context_types.h | 22 +- drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 - drivers/gpu/drm/i915/gt/uc/intel_guc.h | 40 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 666 ++++++++++++++++-- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/i915_request.c | 1 + 8 files changed, 685 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index bd63813c8a80..32fd6647154b 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -384,6 +384,11 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine) mutex_init(&ce->pin_mutex);

• spin_lock_init(&ce->guc_state.lock);
• ce->guc_id = GUC_INVALID_LRC_ID;
• INIT_LIST_HEAD(&ce->guc_id_link);
• i915_active_init(&ce->active, __intel_context_active, __intel_context_retire, 0); }

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 6d99631d19b9..606c480aec26 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -96,6 +96,7 @@ struct intel_context { #define CONTEXT_BANNED 6 #define CONTEXT_FORCE_SINGLE_SUBMISSION 7 #define CONTEXT_NOPREEMPT 8 +#define CONTEXT_LRCA_DIRTY 9 struct { u64 timeout_us; @@ -138,14 +139,29 @@ struct intel_context { u8 wa_bb_page; /* if set, page num reserved for context workarounds */

• struct {

• /** lock: protects everything in guc_state */
  

• spinlock_t lock;
  

• /**
  

•  * sched_state: scheduling state of this context using GuC
  

•  * submission
  

•  */
  

• u8 sched_state;
  

• } guc_state;
• /* GuC scheduling state flags that do not require a lock. */ atomic_t guc_sched_state_no_lock;
• /* GuC LRC descriptor ID */
• u16 guc_id;
• /* GuC LRC descriptor reference count */
• atomic_t guc_id_ref;
• /*

• * GuC LRC descriptor ID - Not assigned in this patch but future patches
  

• * in the series will.
  

• * GuC ID link - in list when unpinned but guc_id still valid in GuC
  

  */

• u16 guc_id;

• struct list_head guc_id_link; }; #endif /* __INTEL_CONTEXT_TYPES__ */

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h index 41e5350a7a05..49d4857ad9b7 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h @@ -87,7 +87,6 @@ #define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0) #define MAX_CONTEXT_HW_ID (1 << 21) /* exclusive */ -#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */ #define GEN11_MAX_CONTEXT_HW_ID (1 << 11) /* exclusive */ /* in Gen12 ID 0x7FF is reserved to indicate idle */ #define GEN12_MAX_CONTEXT_HW_ID (GEN11_MAX_CONTEXT_HW_ID - 1) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 8c7b92f699f1..30773cd699f5 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -7,6 +7,7 @@ #define _INTEL_GUC_H_ #include <linux/xarray.h> +#include <linux/delay.h> #include "intel_uncore.h" #include "intel_guc_fw.h" @@ -44,6 +45,14 @@ struct intel_guc { void (*disable)(struct intel_guc *guc); } interrupts;

• /*

• * contexts_lock protects the pool of free guc ids and a linked list of
  

• * guc ids available to be stolen
  

• */
  

• spinlock_t contexts_lock;
• struct ida guc_ids;
• struct list_head guc_id_list;
• bool submission_selected; struct i915_vma *ads_vma;

@@ -101,6 +110,34 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, response_buf, response_buf_size, 0); } +static inline int intel_guc_send_busy_loop(struct intel_guc* guc,

• 			   const u32 *action,
  

• 			   u32 len,
  

• 			   bool loop)
  

+{

• int err;
• unsigned int sleep_period_ms = 1;
• bool not_atomic = !in_atomic() && !irqs_disabled();
• /* No sleeping with spin locks, just busy loop */
• might_sleep_if(loop && not_atomic);

+retry:

• err = intel_guc_send_nb(guc, action, len);
• if (unlikely(err == -EBUSY && loop)) {

• if (likely(not_atomic)) {
  

• 	if (msleep_interruptible(sleep_period_ms))
  

• 		return -EINTR;
  

• 	sleep_period_ms = sleep_period_ms << 1;
  

• } else {
  

• 	cpu_relax();
  

Potentially something we can change later, but if we're in atomic context we can't keep looping without a timeout while we get -EBUSY, we need to bail out early.

Eventually intel_guc_send_nb will pop with a different return code after 1 second I think if the GuC is hung.

...

• }
  

• goto retry;
  

• }
• return err;

+}

• static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) { intel_guc_ct_event_handler(&guc->ct);

@@ -202,6 +239,9 @@ static inline void intel_guc_disable_msg(struct intel_guc *guc, u32 mask) int intel_guc_reset_engine(struct intel_guc *guc, struct intel_engine_cs *engine); +int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

• 			  const u32 *msg, u32 len);
  

• void intel_guc_load_status(struct intel_guc *guc, struct drm_printer *p); #endif

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 83ec60ea3f89..28ff82c5be45 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -928,6 +928,10 @@ static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *r case INTEL_GUC_ACTION_DEFAULT: ret = intel_guc_to_host_process_recv_msg(guc, payload, len); break;

• case INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE:

• ret = intel_guc_deregister_done_process_msg(guc, payload,
  

• 					    len);
  

• break;
  

  default: ret = -EOPNOTSUPP; break;

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 53b4a5eb4a85..a47b3813b4d0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -13,7 +13,9 @@ #include "gt/intel_gt.h" #include "gt/intel_gt_irq.h" #include "gt/intel_gt_pm.h" +#include "gt/intel_gt_requests.h" #include "gt/intel_lrc.h" +#include "gt/intel_lrc_reg.h" #include "gt/intel_mocs.h" #include "gt/intel_ring.h" @@ -85,6 +87,73 @@ static inline void clr_context_enabled(struct intel_context *ce) &ce->guc_sched_state_no_lock); } +/*

• • Below is a set of functions which control the GuC scheduling state which
• • require a lock, aside from the special case where the functions are called
• • from guc_lrc_desc_pin(). In that case it isn't possible for any other code
• • path to be executing on the context.
• */

Is there a reason to avoid taking the lock in guc_lrc_desc_pin, even if it isn't strictly needed? I'd prefer to avoid the asymmetry in the locking

If this code is called from releasing a fence a circular dependency is created. Once we move the DRM scheduler the locking becomes way easier and we clean this up then. We already have task for locking clean up.

scheme if possible, as that might case trouble if thing change in the future.

Again this is temporary code, so I think we can live with this until the DRM scheduler lands.

...

+#define SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER BIT(0) +#define SCHED_STATE_DESTROYED BIT(1) +static inline void init_sched_state(struct intel_context *ce) +{

• /* Only should be called from guc_lrc_desc_pin() */
• atomic_set(&ce->guc_sched_state_no_lock, 0);
• ce->guc_state.sched_state = 0;

+}

+static inline bool +context_wait_for_deregister_to_register(struct intel_context *ce) +{

• return (ce->guc_state.sched_state &

• SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER);
  

No need for (). Below as well.

Yep.

...

+}

+static inline void +set_context_wait_for_deregister_to_register(struct intel_context *ce) +{

• /* Only should be called from guc_lrc_desc_pin() */
• ce->guc_state.sched_state |=

• SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER;
  

+}

+static inline void +clr_context_wait_for_deregister_to_register(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state =

• (ce->guc_state.sched_state &
  

•  ~SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER);
  

nit: can also use

ce->guc_state.sched_state &= ~SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER

Yep.

...

+}

+static inline bool +context_destroyed(struct intel_context *ce) +{

• return (ce->guc_state.sched_state & SCHED_STATE_DESTROYED);

+}

+static inline void +set_context_destroyed(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state |= SCHED_STATE_DESTROYED;

+}

+static inline bool context_guc_id_invalid(struct intel_context *ce) +{

• return (ce->guc_id == GUC_INVALID_LRC_ID);

+}

+static inline void set_context_guc_id_invalid(struct intel_context *ce) +{

• ce->guc_id = GUC_INVALID_LRC_ID;

+}

+static inline struct intel_guc *ce_to_guc(struct intel_context *ce) +{

• return &ce->engine->gt->uc.guc;

+}

• static inline struct i915_priolist *to_priolist(struct rb_node *rb) { return rb_entry(rb, struct i915_priolist, node);

@@ -155,6 +224,9 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) int len = 0; bool enabled = context_enabled(ce);

• GEM_BUG_ON(!atomic_read(&ce->guc_id_ref));
• GEM_BUG_ON(context_guc_id_invalid(ce));
• if (!enabled) { action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET; action[len++] = ce->guc_id;

@@ -417,6 +489,10 @@ int intel_guc_submission_init(struct intel_guc *guc) xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);

• spin_lock_init(&guc->contexts_lock);
• INIT_LIST_HEAD(&guc->guc_id_list);
• ida_init(&guc->guc_ids);
• return 0; }

@@ -429,9 +505,303 @@ void intel_guc_submission_fini(struct intel_guc *guc) i915_sched_engine_put(guc->sched_engine); } -static int guc_context_alloc(struct intel_context *ce) +static inline void queue_request(struct i915_sched_engine *sched_engine,

• 		 struct i915_request *rq,
  

• 		 int prio)
  

  {

• return lrc_alloc(ce, ce->engine);

• GEM_BUG_ON(!list_empty(&rq->sched.link));
• list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(sched_engine, prio));
  

• set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

+}

+static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

+{

• int ret;
• __i915_request_submit(rq);
• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

+}

+static void guc_submit_request(struct i915_request *rq) +{

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine;
• struct intel_guc *guc = &rq->engine->gt->uc.guc;
• unsigned long flags;
• /* Will be called from irq-context when using foreign fences. */
• spin_lock_irqsave(&sched_engine->lock, flags);
• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  

• spin_unlock_irqrestore(&sched_engine->lock, flags);

+}

+#define GUC_ID_START 64 /* First 64 guc_ids reserved */ +static int new_guc_id(struct intel_guc *guc) +{

• return ida_simple_get(&guc->guc_ids, GUC_ID_START,

• 	      GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL |
  

• 	      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
  

+}

+static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• if (!context_guc_id_invalid(ce)) {

• ida_simple_remove(&guc->guc_ids, ce->guc_id);
  

• reset_lrc_desc(guc, ce->guc_id);
  

• set_context_guc_id_invalid(ce);
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

+}

+static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• unsigned long flags;
• spin_lock_irqsave(&guc->contexts_lock, flags);
• __release_guc_id(guc, ce);
• spin_unlock_irqrestore(&guc->contexts_lock, flags);

+}

+static int steal_guc_id(struct intel_guc *guc) +{

• struct intel_context *ce;
• int guc_id;
• if (!list_empty(&guc->guc_id_list)) {

• ce = list_first_entry(&guc->guc_id_list,
  

• 		      struct intel_context,
  

• 		      guc_id_link);
  

• GEM_BUG_ON(atomic_read(&ce->guc_id_ref));
  

• GEM_BUG_ON(context_guc_id_invalid(ce));
  

• list_del_init(&ce->guc_id_link);
  

• guc_id = ce->guc_id;
  

• set_context_guc_id_invalid(ce);
  

• return guc_id;
  

• } else {

• return -EAGAIN;
  

• }

+}

+static int assign_guc_id(struct intel_guc *guc, u16 *out) +{

• int ret;
• ret = new_guc_id(guc);
• if (unlikely(ret < 0)) {

• ret = steal_guc_id(guc);
  

• if (ret < 0)
  

• 	return ret;
  

• }
• *out = ret;
• return 0;

Is it worth adding spinlock_held asserts for guc->contexts_lock in these ID functions? Doubles up as a documentation of what locking we expect.

Never a bad idea to have asserts in the code. Will add.

...

+}

+#define PIN_GUC_ID_TRIES 4 +static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• int ret = 0;
• unsigned long flags, tries = PIN_GUC_ID_TRIES;
• GEM_BUG_ON(atomic_read(&ce->guc_id_ref));

+try_again:

• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (context_guc_id_invalid(ce)) {

• ret = assign_guc_id(guc, &ce->guc_id);
  

• if (ret)
  

• 	goto out_unlock;
  

• ret = 1;	/* Indidcates newly assigned guc_id */
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

• atomic_inc(&ce->guc_id_ref);

+out_unlock:

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
• /*

• * -EAGAIN indicates no guc_ids are available, let's retire any
  

• * outstanding requests to see if that frees up a guc_id. If the first
  

• * retire didn't help, insert a sleep with the timeslice duration before
  

• * attempting to retire more requests. Double the sleep period each
  

• * subsequent pass before finally giving up. The sleep period has max of
  

• * 100ms and minimum of 1ms.
  

• */
  

• if (ret == -EAGAIN && --tries) {

• if (PIN_GUC_ID_TRIES - tries > 1) {
  

• 	unsigned int timeslice_shifted =
  

• 		ce->engine->props.timeslice_duration_ms <<
  

• 		(PIN_GUC_ID_TRIES - tries - 2);
  

• 	unsigned int max = min_t(unsigned int, 100,
  

• 				 timeslice_shifted);
  

• 	msleep(max_t(unsigned int, max, 1));
  

• }
  

• intel_gt_retire_requests(guc_to_gt(guc));
  

• goto try_again;
  

• }
• return ret;

+}

+static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• unsigned long flags;
• GEM_BUG_ON(atomic_read(&ce->guc_id_ref) < 0);
• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (!context_guc_id_invalid(ce) && list_empty(&ce->guc_id_link) &&

•    !atomic_read(&ce->guc_id_ref))
  

• list_add_tail(&ce->guc_id_link, &guc->guc_id_list);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);

+}

+static int __guc_action_register_context(struct intel_guc *guc,

• 			 u32 guc_id,
  

• 			 u32 offset)
  

+{

• u32 action[] = {

• INTEL_GUC_ACTION_REGISTER_CONTEXT,
  

• guc_id,
  

• offset,
  

• };
• return intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), true);

+}

+static int register_context(struct intel_context *ce) +{

• struct intel_guc *guc = ce_to_guc(ce);
• u32 offset = intel_guc_ggtt_offset(guc, guc->lrc_desc_pool) +

• ce->guc_id * sizeof(struct guc_lrc_desc);
  

• return __guc_action_register_context(guc, ce->guc_id, offset);

+}

+static int __guc_action_deregister_context(struct intel_guc *guc,

• 			   u32 guc_id)
  

+{

• u32 action[] = {

• INTEL_GUC_ACTION_DEREGISTER_CONTEXT,
  

• guc_id,
  

• };
• return intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), true);

+}

+static int deregister_context(struct intel_context *ce, u32 guc_id) +{

• struct intel_guc *guc = ce_to_guc(ce);
• return __guc_action_deregister_context(guc, guc_id);

+}

+static intel_engine_mask_t adjust_engine_mask(u8 class, intel_engine_mask_t mask) +{

• switch (class) {
• case RENDER_CLASS:

• return mask >> RCS0;
  

• case VIDEO_ENHANCEMENT_CLASS:

• return mask >> VECS0;
  

• case VIDEO_DECODE_CLASS:

• return mask >> VCS0;
  

• case COPY_ENGINE_CLASS:

• return mask >> BCS0;
  

• default:

• GEM_BUG_ON("Invalid Class");
  

we usually use MISSING_CASE for this type of errors.

As soon as start talking in logical space (series immediatly after this gets merged) this function gets deleted but will fix.

...

• return 0;
  

• }

+}

+static void guc_context_policy_init(struct intel_engine_cs *engine,

• 		    struct guc_lrc_desc *desc)
  

+{

• desc->policy_flags = 0;
• desc->execution_quantum = CONTEXT_POLICY_DEFAULT_EXECUTION_QUANTUM_US;
• desc->preemption_timeout = CONTEXT_POLICY_DEFAULT_PREEMPTION_TIME_US;

+}

+static int guc_lrc_desc_pin(struct intel_context *ce) +{

• struct intel_runtime_pm *runtime_pm =

• &ce->engine->gt->i915->runtime_pm;
  

If you move this after the engine var below you can skip the ce->engine jump. Also, you can shorten the pointer chasing by using engine->uncore->rpm.

Sure.

...

• struct intel_engine_cs *engine = ce->engine;
• struct intel_guc *guc = &engine->gt->uc.guc;
• u32 desc_idx = ce->guc_id;
• struct guc_lrc_desc *desc;
• bool context_registered;
• intel_wakeref_t wakeref;
• int ret = 0;
• GEM_BUG_ON(!engine->mask);
• /*

• * Ensure LRC + CT vmas are is same region as write barrier is done
  

• * based on CT vma region.
  

• */
  

• GEM_BUG_ON(i915_gem_object_is_lmem(guc->ct.vma->obj) !=

•    i915_gem_object_is_lmem(ce->ring->vma->obj));
  

• context_registered = lrc_desc_registered(guc, desc_idx);
• reset_lrc_desc(guc, desc_idx);
• set_lrc_desc_registered(guc, desc_idx, ce);
• desc = __get_lrc_desc(guc, desc_idx);
• desc->engine_class = engine_class_to_guc_class(engine->class);
• desc->engine_submit_mask = adjust_engine_mask(engine->class,

• 				      engine->mask);
  

• desc->hw_context_desc = ce->lrc.lrca;
• desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
• desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
• guc_context_policy_init(engine, desc);
• init_sched_state(ce);
• /*

• * The context_lookup xarray is used to determine if the hardware
  

• * context is currently registered. There are two cases in which it
  

• * could be regisgered either the guc_id has been stole from from
  

typo regisgered

John got this one. Fixed.

...

• * another context or the lrc descriptor address of this context has
  

• * changed. In either case the context needs to be deregistered with the
  

• * GuC before registering this context.
  

• */
  

• if (context_registered) {

• set_context_wait_for_deregister_to_register(ce);
  

• intel_context_get(ce);
  

• /*
  

•  * If stealing the guc_id, this ce has the same guc_id as the
  

•  * context whos guc_id was stole.
  

•  */
  

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	ret = deregister_context(ce, ce->guc_id);
  

• } else {

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	ret = register_context(ce);
  

• }
• return ret; } static int guc_context_pre_pin(struct intel_context *ce,

@@ -443,36 +813,139 @@ static int guc_context_pre_pin(struct intel_context *ce, static int guc_context_pin(struct intel_context *ce, void *vaddr) {

• if (i915_ggtt_offset(ce->state) !=

•    (ce->lrc.lrca & CTX_GTT_ADDRESS_MASK))
  

• set_bit(CONTEXT_LRCA_DIRTY, &ce->flags);
  

Shouldn't this be set after lrc_pin()? I can see ce->lrc.lrca being re-assigned in there; it looks like it can only change if the context is new, but for future-proofing IMO better to re-order here.

No this is right. ce->state is assigned in lr_pre_pin and ce->lrc.lrca is assigned in lrc_pin. To catch the change you have to check between those two functions.

...

• return lrc_pin(ce, ce->engine, vaddr);

Could use a comment to say that the GuC context pinning call is delayed until request alloc and to look at the comment in the latter function for details (or move the explanation here and refer here from request alloc).

Yes.

...

} +static void guc_context_unpin(struct intel_context *ce) +{

• struct intel_guc *guc = ce_to_guc(ce);
• unpin_guc_id(guc, ce);
• lrc_unpin(ce);

+}

+static void guc_context_post_unpin(struct intel_context *ce) +{

• lrc_post_unpin(ce);

why do we need this function? we can just pass lrc_post_unpin to the ops (like we already do).

We don't before style reasons I think having a GuC specific wrapper is correct.

...

+}

+static inline void guc_lrc_desc_unpin(struct intel_context *ce) +{

• struct intel_engine_cs *engine = ce->engine;
• struct intel_guc *guc = &engine->gt->uc.guc;
• unsigned long flags;
• GEM_BUG_ON(!lrc_desc_registered(guc, ce->guc_id));
• GEM_BUG_ON(ce != __get_context(guc, ce->guc_id));
• spin_lock_irqsave(&ce->guc_state.lock, flags);
• set_context_destroyed(ce);
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• deregister_context(ce, ce->guc_id);

+}

+static void guc_context_destroy(struct kref *kref) +{

• struct intel_context *ce = container_of(kref, typeof(*ce), ref);
• struct intel_runtime_pm *runtime_pm = &ce->engine->gt->i915->runtime_pm;

same as above, going through engine->uncore->rpm is shorter

Sure.

...

• struct intel_guc *guc = &ce->engine->gt->uc.guc;
• intel_wakeref_t wakeref;
• unsigned long flags;
• /*

• * If the guc_id is invalid this context has been stolen and we can free
  

• * it immediately. Also can be freed immediately if the context is not
  

• * registered with the GuC.
  

• */
  

• if (context_guc_id_invalid(ce) ||

•    !lrc_desc_registered(guc, ce->guc_id)) {
  

• release_guc_id(guc, ce);
  

it feels a bit weird that we call release_guc_id in the case where the id is invalid. The code handles it fine, but still the flow doesn't feel clean. Not a blocker.

I understand. Let's see if this can get cleaned up at some point.

...

• lrc_destroy(kref);
  

• return;
  

• }
• /*

• * We have to acquire the context spinlock and check guc_id again, if it
  

• * is valid it hasn't been stolen and needs to be deregistered. We
  

• * delete this context from the list of unpinned guc_ids available to
  

• * stole to seal a race with guc_lrc_desc_pin(). When the G2H CTB
  

• * returns indicating this context has been deregistered the guc_id is
  

• * returned to the pool of available guc_ids.
  

• */
  

• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (context_guc_id_invalid(ce)) {

• __release_guc_id(guc, ce);
  

But here the call to __release_guc_id is unneded, right? the ce doesn't own the ID anymore and both the steal and the release function already clean ce->guc_id_link, so there should be nothing left to clean.

This is dead code. Will delete.

...

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
  

• lrc_destroy(kref);
  

• return;
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
• /*

• * We defer GuC context deregistration until the context is destroyed
  

• * in order to save on CTBs. With this optimization ideally we only need
  

• * 1 CTB to register the context during the first pin and 1 CTB to
  

• * deregister the context when the context is destroyed. Without this
  

• * optimization, a CTB would be needed every pin & unpin.
  

• *
  

• * XXX: Need to acqiure the runtime wakeref as this can be triggered
  

• * from context_free_worker when not runtime wakeref is held.
  

• * guc_lrc_desc_unpin requires the runtime as a GuC register is written
  

• * in H2G CTB to deregister the context. A future patch may defer this
  

• * H2G CTB if the runtime wakeref is zero.
  

• */
  

• with_intel_runtime_pm(runtime_pm, wakeref)

• guc_lrc_desc_unpin(ce);
  

+}

+static int guc_context_alloc(struct intel_context *ce) +{

• return lrc_alloc(ce, ce->engine);

+}

• static const struct intel_context_ops guc_context_ops = { .alloc = guc_context_alloc, .pre_pin = guc_context_pre_pin, .pin = guc_context_pin,

• .unpin = lrc_unpin,
• .post_unpin = lrc_post_unpin,

• .unpin = guc_context_unpin,
• .post_unpin = guc_context_post_unpin, .enter = intel_context_enter_engine, .exit = intel_context_exit_engine, .reset = lrc_reset,

• .destroy = lrc_destroy,

• .destroy = guc_context_destroy, };

-static int guc_request_alloc(struct i915_request *request) +static bool context_needs_register(struct intel_context *ce, bool new_guc_id) {

• return new_guc_id || test_bit(CONTEXT_LRCA_DIRTY, &ce->flags) ||

• !lrc_desc_registered(ce_to_guc(ce), ce->guc_id);
  

+}

+static int guc_request_alloc(struct i915_request *rq) +{

• struct intel_context *ce = rq->context;
• struct intel_guc *guc = ce_to_guc(ce); int ret;

• GEM_BUG_ON(!intel_context_is_pinned(request->context));

• GEM_BUG_ON(!intel_context_is_pinned(rq->context)); /*
  • Flush enough space to reduce the likelihood of waiting after
  • we start building the request - in which case we will just
  • have to repeat work.
  */

• request->reserved_space += GUC_REQUEST_SIZE;

• rq->reserved_space += GUC_REQUEST_SIZE; /*
  • Note that after this point, we have committed to using

@@ -483,56 +956,47 @@ static int guc_request_alloc(struct i915_request *request) */ /* Unconditionally invalidate GPU caches and TLBs. */

• ret = request->engine->emit_flush(request, EMIT_INVALIDATE);

• ret = rq->engine->emit_flush(rq, EMIT_INVALIDATE); if (ret) return ret;

• request->reserved_space -= GUC_REQUEST_SIZE;
• return 0;

-}

• rq->reserved_space -= GUC_REQUEST_SIZE;

-static inline void queue_request(struct i915_sched_engine *sched_engine,

• 		 struct i915_request *rq,
  

• 		 int prio)
  

-{

• GEM_BUG_ON(!list_empty(&rq->sched.link));
• list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(sched_engine, prio));
  

• set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

-}

-static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

-{

• int ret;
• __i915_request_submit(rq);
• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

-}

-static void guc_submit_request(struct i915_request *rq) -{

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine;
• struct intel_guc *guc = &rq->engine->gt->uc.guc;
• unsigned long flags;

• /*

• * Call pin_guc_id here rather than in the pinning step as with
  

• * dma_resv, contexts can be repeatedly pinned / unpinned trashing the
  

• * guc_ids and creating horrible race conditions. This is especially bad
  

• * when guc_ids are being stolen due to over subscription. By the time
  

• * this function is reached, it is guaranteed that the guc_id will be
  

• * persistent until the generated request is retired. Thus, sealing these
  

• * race conditions. It is still safe to fail here if guc_ids are
  

• * exhausted and return -EAGAIN to the user indicating that they can try
  

• * again in the future.
  

• *
  

• * There is no need for a lock here as the timeline mutex ensures at
  

• * most one context can be executing this code path at once. The
  

• * guc_id_ref is incremented once for every request in flight and
  

• * decremented on each retire. When it is zero, a lock around the
  

• * increment (in pin_guc_id) is needed to seal a race with unpin_guc_id.
  

• */
  

• if (atomic_add_unless(&ce->guc_id_ref, 1, 0))

• return 0;
  

• /* Will be called from irq-context when using foreign fences. */
• spin_lock_irqsave(&sched_engine->lock, flags);

• ret = pin_guc_id(guc, ce); /* returns 1 if new guc_id assigned */
• if (unlikely(ret < 0))

• return ret;;
  

typo ";;"

Yep.

...

• if (context_needs_register(ce, !!ret)) {

• ret = guc_lrc_desc_pin(ce);
  

• if (unlikely(ret)) {	/* unwind */
  

• 	atomic_dec(&ce->guc_id_ref);
  

• 	unpin_guc_id(guc, ce);
  

• 	return ret;
  

• }
  

• }

• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  

• clear_bit(CONTEXT_LRCA_DIRTY, &ce->flags);

Might be worth moving the pinning to a separate function to keep the request_alloc focused on the request. Can be done as a follow up.

Sure. Let me see how that looks in a follow up.

...

• spin_unlock_irqrestore(&sched_engine->lock, flags);

• return 0; } static void sanitize_hwsp(struct intel_engine_cs *engine)

@@ -606,6 +1070,46 @@ static void guc_set_default_submission(struct intel_engine_cs *engine) engine->submit_request = guc_submit_request; } +static inline void guc_kernel_context_pin(struct intel_guc *guc,

• 			  struct intel_context *ce)
  

+{

• if (context_guc_id_invalid(ce))

• pin_guc_id(guc, ce);
  

• guc_lrc_desc_pin(ce);

+}

+static inline void guc_init_lrc_mapping(struct intel_guc *guc) +{

• struct intel_gt *gt = guc_to_gt(guc);
• struct intel_engine_cs *engine;
• enum intel_engine_id id;
• /* make sure all descriptors are clean... */
• xa_destroy(&guc->context_lookup);
• /*

• * Some contexts might have been pinned before we enabled GuC
  

• * submission, so we need to add them to the GuC bookeeping.
  

• * Also, after a reset the GuC we want to make sure that the information
  

• * shared with GuC is properly reset. The kernel lrcs are not attached
  

• * to the gem_context, so they need to be added separately.
  

• *
  

• * Note: we purposely do not check the error return of
  

• * guc_lrc_desc_pin, because that function can only fail in two cases.
  

• * One, if there aren't enough free IDs, but we're guaranteed to have
  

• * enough here (we're either only pinning a handful of lrc on first boot
  

• * or we're re-pinning lrcs that were already pinned before the reset).
  

• * Two, if the GuC has died and CTBs can't make forward progress.
  

• * Presumably, the GuC should be alive as this function is called on
  

• * driver load or after a reset. Even if it is dead, another full GPU
  

• * reset will be triggered and this function would be called again.
  

• */
  

• for_each_engine(engine, gt, id)

• if (engine->kernel_context)
  

• 	guc_kernel_context_pin(guc, engine->kernel_context);
  

+}

• static void guc_release(struct intel_engine_cs *engine) { engine->sanitize = NULL; /* no longer in control, nothing to sanitize */

@@ -718,6 +1222,7 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine) void intel_guc_submission_enable(struct intel_guc *guc) {

• guc_init_lrc_mapping(guc); } void intel_guc_submission_disable(struct intel_guc *guc)

@@ -743,3 +1248,62 @@ void intel_guc_submission_init_early(struct intel_guc *guc) { guc->submission_selected = __guc_submission_selected(guc); }

+static inline struct intel_context * +g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) +{

• struct intel_context *ce;
• if (unlikely(desc_idx >= GUC_MAX_LRC_DESCRIPTORS)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm,
  

• 	"Invalid desc_idx %u", desc_idx);
  

• return NULL;
  

• }
• ce = __get_context(guc, desc_idx);
• if (unlikely(!ce)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm,
  

• 	"Context is NULL, desc_idx %u", desc_idx);
  

• return NULL;
  

• }
• return ce;

+}

+int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

• 			  const u32 *msg,
  

• 			  u32 len)
  

+{

• struct intel_context *ce;
• u32 desc_idx = msg[0];
• if (unlikely(len < 1)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm, "Invalid length %u", len);
  

• return -EPROTO;
  

• }
• ce = g2h_context_lookup(guc, desc_idx);
• if (unlikely(!ce))

• return -EPROTO;
  

• if (context_wait_for_deregister_to_register(ce)) {

• struct intel_runtime_pm *runtime_pm =
  

• 	&ce->engine->gt->i915->runtime_pm;
  

• intel_wakeref_t wakeref;
  

• /*
  

•  * Previous owner of this guc_id has been deregistered, now safe
  

•  * register this context.
  

•  */
  

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	register_context(ce);
  

• clr_context_wait_for_deregister_to_register(ce);
  

• intel_context_put(ce);
  

• } else if (context_destroyed(ce)) {

• /* Context has been destroyed */
  

• release_guc_id(guc, ce);
  

• lrc_destroy(&ce->ref);
  

• }
• return 0;

+} diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 943fe485c662..204c95c39353 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4142,6 +4142,7 @@ enum { FAULT_AND_CONTINUE /* Unsupported */ }; +#define CTX_GTT_ADDRESS_MASK GENMASK(31, 12) #define GEN8_CTX_VALID (1 << 0) #define GEN8_CTX_FORCE_PD_RESTORE (1 << 1) #define GEN8_CTX_FORCE_RESTORE (1 << 2) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 86b4c9f2613d..b48c4905d3fc 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -407,6 +407,7 @@ bool i915_request_retire(struct i915_request *rq) */ if (!list_empty(&rq->sched.link)) remove_from_engine(rq);

• atomic_dec(&rq->context->guc_id_ref);

Does this work/make sense  if GuC is disabled?