On 10/4/2021 15:06, Matthew Brost wrote:

Move guc_id allocation under submission state sub-struct as a future patch will reuse the spin lock as a global submission state lock. Moving this into sub-struct makes ownership of fields / lock clear.

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context_types.h | 6 +- drivers/gpu/drm/i915/gt/uc/intel_guc.h | 26 +++++---- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 56 ++++++++++--------- 3 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 12252c411159..e7e3984aab78 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -197,18 +197,18 @@ struct intel_context { struct { /** * @id: handle which is used to uniquely identify this context

•  * with the GuC, protected by guc->contexts_lock
  

•  * with the GuC, protected by guc->submission_state.lock
  

  */ u16 id; /**
  • @ref: the number of references to the guc_id, when
  • transitioning in and out of zero protected by

•  * guc->contexts_lock
  

•  * guc->submission_state.lock
  

  */ atomic_t ref; /**
  • @link: in guc->guc_id_list when the guc_id has no refs but is

•  * still valid, protected by guc->contexts_lock
  

•  * still valid, protected by guc->submission_state.lock
  

  */ struct list_head link; } guc_id;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 5dd174babf7a..65b5e8eeef96 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -70,17 +70,21 @@ struct intel_guc { void (*disable)(struct intel_guc *guc); } interrupts;

• /**

• * @contexts_lock: protects guc_ids, guc_id_list, ce->guc_id.id, and
  

• * ce->guc_id.ref when transitioning in and out of zero
  

• */
  

• spinlock_t contexts_lock;
• /** @guc_ids: used to allocate unique ce->guc_id.id values */
• struct ida guc_ids;
• /**

• * @guc_id_list: list of intel_context with valid guc_ids but no refs
  

• */
  

• struct list_head guc_id_list;

• struct {

• /**
  

•  * @lock: protects everything in submission_state
  

•  */
  

• spinlock_t lock;
  

The old version also mentioned 'ce->guc_id.ref'. Should this not also mention that transition? Or was the old comment inaccurate. I'm not seeing any actual behaviour changes in the patch.

• /**
  

•  * @guc_ids: used to allocate new guc_ids
  

•  */
  

• struct ida guc_ids;
  

• /**
  

•  * @guc_id_list: list of intel_context with valid guc_ids but no
  

•  * refs
  

•  */
  

• struct list_head guc_id_list;
  

• } submission_state;

  /**

  • @submission_supported: tracks whether we support GuC submission on

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 ba0de35f6323..ad5c18119d92 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -68,16 +68,16 @@

• fence is used to stall all requests associated with this guc_id until the
• corresponding G2H returns indicating the guc_id has been deregistered.

• • guc_ids:

• • submission_state.guc_ids:
  • Unique number associated with private GuC context data passed in during
  • context registration / submission / deregistration. 64k available. Simple ida
  • is used for allocation.

• • Stealing guc_ids:
• • If no guc_ids are available they can be stolen from another context at
• • request creation time if that context is unpinned. If a guc_id can't be found
• • we punt this problem to the user as we believe this is near impossible to hit
• • during normal use cases.

• • Stealing submission_state.guc_ids:
• • If no submission_state.guc_ids are available they can be stolen from another

I would abbreviate this instance as well, submission_state.guc_id is quite the mouthful. Unless this somehow magically links back to the structure entry in the kerneldoc output?

John.

• • context at request creation time if that context is unpinned. If a guc_id
• • can't be found we punt this problem to the user as we believe this is near
• • impossible to hit during normal use cases.
  • Locking:
  • In the GuC submission code we have 3 basic spin locks which protect

@@ -89,7 +89,7 @@

• sched_engine can be submitting at a time. Currently only one sched_engine is
• used for all of GuC submission but that could change in the future.

• • guc->contexts_lock

• • guc->submission_state.lock
  • Protects guc_id allocation for the given GuC, i.e. only one context can be
  • doing guc_id allocation operations at a time for each GuC in the system.

@@ -103,7 +103,7 @@

• Lock ordering rules:
• sched_engine->lock -> ce->guc_state.lock

• • guc->contexts_lock -> ce->guc_state.lock

• • guc->submission_state.lock -> ce->guc_state.lock
  • Reset races:
  • When a full GT reset is triggered it is assumed that some G2H responses to

@@ -1148,9 +1148,9 @@ 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);

• spin_lock_init(&guc->submission_state.lock);

• INIT_LIST_HEAD(&guc->submission_state.guc_id_list);

• ida_init(&guc->submission_state.guc_ids);

  return 0; }

@@ -1215,7 +1215,7 @@ static void guc_submit_request(struct i915_request *rq)

static int new_guc_id(struct intel_guc *guc) {

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

• return ida_simple_get(&guc->submission_state.guc_ids, 0, GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN); }

@@ -1223,7 +1223,8 @@ static int new_guc_id(struct intel_guc *guc) 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.id);
  

• ida_simple_remove(&guc->submission_state.guc_ids,
  

• 		  ce->guc_id.id);
  

  reset_lrc_desc(guc, ce->guc_id.id); set_context_guc_id_invalid(ce); }

@@ -1235,9 +1236,9 @@ static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) { unsigned long flags;

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

• spin_lock_irqsave(&guc->submission_state.lock, flags); __release_guc_id(guc, ce);

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags); }

  static int steal_guc_id(struct intel_guc *guc)

@@ -1245,10 +1246,10 @@ static int steal_guc_id(struct intel_guc *guc) struct intel_context *ce; int guc_id;

• lockdep_assert_held(&guc->contexts_lock);

• lockdep_assert_held(&guc->submission_state.lock);

• if (!list_empty(&guc->guc_id_list)) {

• ce = list_first_entry(&guc->guc_id_list,
  

• if (!list_empty(&guc->submission_state.guc_id_list)) {

• ce = list_first_entry(&guc->submission_state.guc_id_list,
  	      struct intel_context,
  	      guc_id.link);
  
  

@@ -1273,7 +1274,7 @@ static int assign_guc_id(struct intel_guc *guc, u16 *out) { int ret;

• lockdep_assert_held(&guc->contexts_lock);

• lockdep_assert_held(&guc->submission_state.lock);

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

@@ -1295,7 +1296,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) GEM_BUG_ON(atomic_read(&ce->guc_id.ref));

try_again:

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

• spin_lock_irqsave(&guc->submission_state.lock, flags);

  might_lock(&ce->guc_state.lock);

@@ -1310,7 +1311,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) atomic_inc(&ce->guc_id.ref);

out_unlock:

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags);

  /*

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

@@ -1346,11 +1347,12 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce) if (unlikely(context_guc_id_invalid(ce))) return;

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

• spin_lock_irqsave(&guc->submission_state.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);

• list_add_tail(&ce->guc_id.link,
  

• 	      &guc->submission_state.guc_id_list);
  

• spin_unlock_irqrestore(&guc->submission_state.lock, flags); }

  static int __guc_action_register_context(struct intel_guc *guc,

@@ -1921,16 +1923,16 @@ static void guc_context_destroy(struct kref *kref) * returns indicating this context has been deregistered the guc_id is * returned to the pool of available guc_id. */

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

• spin_lock_irqsave(&guc->submission_state.lock, flags); if (context_guc_id_invalid(ce)) {

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags);
  

  __guc_context_destroy(ce); return; }

  if (!list_empty(&ce->guc_id.link)) list_del_init(&ce->guc_id.link);

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags);

  /* Seal race with Reset */ spin_lock_irqsave(&ce->guc_state.lock, flags);

On 10/7/2021 08:05, Matthew Brost wrote:

On Wed, Oct 06, 2021 at 08:06:41PM -0700, John Harrison wrote:

...

On 10/4/2021 15:06, Matthew Brost wrote:

...

Move guc_id allocation under submission state sub-struct as a future patch will reuse the spin lock as a global submission state lock. Moving this into sub-struct makes ownership of fields / lock clear.

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context_types.h | 6 +- drivers/gpu/drm/i915/gt/uc/intel_guc.h | 26 +++++---- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 56 ++++++++++--------- 3 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 12252c411159..e7e3984aab78 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -197,18 +197,18 @@ struct intel_context { struct { /** * @id: handle which is used to uniquely identify this context

•  * with the GuC, protected by guc->contexts_lock
  

•  * with the GuC, protected by guc->submission_state.lock
   */
  u16 id;
  /**
   * @ref: the number of references to the guc_id, when
   * transitioning in and out of zero protected by
  

•  * guc->contexts_lock
  

•  * guc->submission_state.lock
   */
  atomic_t ref;
  /**
   * @link: in guc->guc_id_list when the guc_id has no refs but is
  

•  * still valid, protected by guc->contexts_lock
  

•  * still valid, protected by guc->submission_state.lock
   */
  struct list_head link;
  

  } guc_id;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 5dd174babf7a..65b5e8eeef96 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -70,17 +70,21 @@ struct intel_guc { void (*disable)(struct intel_guc *guc); } interrupts;

• /**

• * @contexts_lock: protects guc_ids, guc_id_list, ce->guc_id.id, and
  

• * ce->guc_id.ref when transitioning in and out of zero
  

• */
  

• spinlock_t contexts_lock;
• /** @guc_ids: used to allocate unique ce->guc_id.id values */
• struct ida guc_ids;
• /**

• * @guc_id_list: list of intel_context with valid guc_ids but no refs
  

• */
  

• struct list_head guc_id_list;

• struct {

• /**
  

•  * @lock: protects everything in submission_state
  

•  */
  

• spinlock_t lock;
  

The old version also mentioned 'ce->guc_id.ref'. Should this not also mention that transition? Or was the old comment inaccurate. I'm not seeing any actual behaviour changes in the patch.

Can add that back in.

...

...

• /**
  

•  * @guc_ids: used to allocate new guc_ids
  

•  */
  

• struct ida guc_ids;
  

• /**
  

•  * @guc_id_list: list of intel_context with valid guc_ids but no
  

•  * refs
  

•  */
  

• struct list_head guc_id_list;
  

• } submission_state; /**
  • @submission_supported: tracks whether we support GuC submission on

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 ba0de35f6323..ad5c18119d92 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -68,16 +68,16 @@ * fence is used to stall all requests associated with this guc_id until the * corresponding G2H returns indicating the guc_id has been deregistered. *

• • guc_ids:

• • submission_state.guc_ids:
    • Unique number associated with private GuC context data passed in during
    • context registration / submission / deregistration. 64k available. Simple ida
    • is used for allocation.

• • Stealing guc_ids:
• • If no guc_ids are available they can be stolen from another context at
• • request creation time if that context is unpinned. If a guc_id can't be found
• • we punt this problem to the user as we believe this is near impossible to hit
• • during normal use cases.

• • Stealing submission_state.guc_ids:
• • If no submission_state.guc_ids are available they can be stolen from another

I would abbreviate this instance as well, submission_state.guc_id is quite the mouthful. Unless this somehow magically links back to the structure entry in the kerneldoc output?

It might, not really sure but agree the submission_state should be dropped. Think changed because of global find replace.

Matt

Okay. With those nits fixed: Reviewed by: John Harrison John.C.Harrison@Intel.com

...

John.

...

• • context at request creation time if that context is unpinned. If a guc_id
• • can't be found we punt this problem to the user as we believe this is near
• • impossible to hit during normal use cases.
    • Locking:
    • In the GuC submission code we have 3 basic spin locks which protect

@@ -89,7 +89,7 @@ * sched_engine can be submitting at a time. Currently only one sched_engine is * used for all of GuC submission but that could change in the future. *

• • guc->contexts_lock

• • guc->submission_state.lock
    • Protects guc_id allocation for the given GuC, i.e. only one context can be
    • doing guc_id allocation operations at a time for each GuC in the system.

@@ -103,7 +103,7 @@ * * Lock ordering rules: * sched_engine->lock -> ce->guc_state.lock

• • guc->contexts_lock -> ce->guc_state.lock

• • guc->submission_state.lock -> ce->guc_state.lock
    • Reset races:
    • When a full GT reset is triggered it is assumed that some G2H responses to

@@ -1148,9 +1148,9 @@ 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);

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

@@ -1215,7 +1215,7 @@ static void guc_submit_request(struct i915_request *rq) static int new_guc_id(struct intel_guc *guc) {

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

• return ida_simple_get(&guc->submission_state.guc_ids, 0, GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN); }

@@ -1223,7 +1223,8 @@ static int new_guc_id(struct intel_guc *guc) 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.id);
  

• ida_simple_remove(&guc->submission_state.guc_ids,
  

• 		  ce->guc_id.id);
  reset_lrc_desc(guc, ce->guc_id.id);
  set_context_guc_id_invalid(ce);
  

  }

@@ -1235,9 +1236,9 @@ static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) { unsigned long flags;

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

• spin_lock_irqsave(&guc->submission_state.lock, flags); __release_guc_id(guc, ce);

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags); } static int steal_guc_id(struct intel_guc *guc)

@@ -1245,10 +1246,10 @@ static int steal_guc_id(struct intel_guc *guc) struct intel_context *ce; int guc_id;

• lockdep_assert_held(&guc->contexts_lock);

• lockdep_assert_held(&guc->submission_state.lock);

• if (!list_empty(&guc->guc_id_list)) {

• ce = list_first_entry(&guc->guc_id_list,
  

• if (!list_empty(&guc->submission_state.guc_id_list)) {

• ce = list_first_entry(&guc->submission_state.guc_id_list,
  		      struct intel_context,
  		      guc_id.link);
  

@@ -1273,7 +1274,7 @@ static int assign_guc_id(struct intel_guc *guc, u16 *out) { int ret;

• lockdep_assert_held(&guc->contexts_lock);

• lockdep_assert_held(&guc->submission_state.lock); ret = new_guc_id(guc); if (unlikely(ret < 0)) {

@@ -1295,7 +1296,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) GEM_BUG_ON(atomic_read(&ce->guc_id.ref)); try_again:

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

• spin_lock_irqsave(&guc->submission_state.lock, flags); might_lock(&ce->guc_state.lock);

@@ -1310,7 +1311,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) atomic_inc(&ce->guc_id.ref); out_unlock:

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags); /*
  • -EAGAIN indicates no guc_id are available, let's retire any

@@ -1346,11 +1347,12 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce) if (unlikely(context_guc_id_invalid(ce))) return;

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

• spin_lock_irqsave(&guc->submission_state.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);

• list_add_tail(&ce->guc_id.link,
  

• 	      &guc->submission_state.guc_id_list);
  

• spin_unlock_irqrestore(&guc->submission_state.lock, flags); } static int __guc_action_register_context(struct intel_guc *guc,

@@ -1921,16 +1923,16 @@ static void guc_context_destroy(struct kref *kref) * returns indicating this context has been deregistered the guc_id is * returned to the pool of available guc_id. */

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

• spin_lock_irqsave(&guc->submission_state.lock, flags); if (context_guc_id_invalid(ce)) {

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags);
  __guc_context_destroy(ce);
  return;
  

  } if (!list_empty(&ce->guc_id.link)) list_del_init(&ce->guc_id.link);

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

• spin_unlock_irqrestore(&guc->submission_state.lock, flags); /* Seal race with Reset */ spin_lock_irqsave(&ce->guc_state.lock, flags);

On 10/4/2021 15:06, Matthew Brost wrote:

Taking a PM reference to prevent intel_gt_wait_for_idle from short circuiting while a deregister context H2G is in flight. To do this must issue the deregister H2G from a worker as context can be destroyed from an atomic context and taking GT PM ref blows up. Previously we took a runtime PM from this atomic context which worked but will stop working once runtime pm autosuspend in enabled.

So this patch is two fold, stop intel_gt_wait_for_idle from short circuting and fix runtime pm autosuspend.

v2: (John Harrison)

• Split structure changes out in different patch

(Tvrtko)

• Don't drop lock in deregister_destroyed_contexts

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context.c | 2 + drivers/gpu/drm/i915/gt/intel_context_types.h | 7 + drivers/gpu/drm/i915/gt/intel_engine_pm.h | 5 + drivers/gpu/drm/i915/gt/intel_gt_pm.h | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 11 ++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 146 +++++++++++------- 6 files changed, 121 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index e9a0cad5c34d..1076066f41e0 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -399,6 +399,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine) ce->guc_id.id = GUC_INVALID_LRC_ID; INIT_LIST_HEAD(&ce->guc_id.link);

• INIT_LIST_HEAD(&ce->destroyed_link);
• /*
  • Initialize fence to be complete as this is expected to be complete
  • unless there is a pending schedule disable outstanding.

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index e7e3984aab78..4613d027cbc3 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -213,6 +213,13 @@ struct intel_context { struct list_head link; } guc_id;

• /**

• * @destroyed_link: link in guc->submission_state.destroyed_contexts, in
  

• * list when context is pending to be destroyed (deregistered with the
  

• * GuC), protected by guc->submission_state.lock
  

• */
  

• struct list_head destroyed_link;
• #ifdef CONFIG_DRM_I915_SELFTEST /**
  • @drop_schedule_enable: Force drop of schedule enable G2H for selftest

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h index 8520c595f5e1..6fdeae668e6e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h @@ -16,6 +16,11 @@ intel_engine_pm_is_awake(const struct intel_engine_cs *engine) return intel_wakeref_is_active(&engine->wakeref); }

+static inline void __intel_engine_pm_get(struct intel_engine_cs *engine) +{

• __intel_wakeref_get(&engine->wakeref);

+}

• static inline void intel_engine_pm_get(struct intel_engine_cs *engine) { intel_wakeref_get(&engine->wakeref);

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h index d0588d8aaa44..05de6c1af25b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h @@ -41,6 +41,10 @@ static inline void intel_gt_pm_put_async(struct intel_gt *gt) intel_wakeref_put_async(&gt->wakeref); }

+#define with_intel_gt_pm(gt, tmp) \

• for (tmp = 1, intel_gt_pm_get(gt); tmp; \

•     intel_gt_pm_put(gt), tmp = 0)
  

• static inline int intel_gt_pm_wait_for_idle(struct intel_gt *gt) { return intel_wakeref_wait_for_idle(&gt->wakeref);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 65b5e8eeef96..25a598e2b6e8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -84,6 +84,17 @@ struct intel_guc { * refs */ struct list_head guc_id_list;

• /**
  

•  * @destroyed_contexts: list of contexts waiting to be destroyed
  

•  * (deregistered with the GuC)
  

•  */
  

• struct list_head destroyed_contexts;
  

• /**
  

•  * @destroyed_worker: worker to deregister contexts, need as we
  

•  * need to take a GT PM reference and can't from destroy
  

•  * function as it might be in an atomic context (no sleeping)
  

•  */
  

• struct work_struct destroyed_worker;
  

  } submission_state;

  /**

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 ad5c18119d92..17da2fea1bff 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -90,8 +90,8 @@

• used for all of GuC submission but that could change in the future.
• guc->submission_state.lock

• • Protects guc_id allocation for the given GuC, i.e. only one context can be
• • doing guc_id allocation operations at a time for each GuC in the system.

• • Global lock for GuC submission state. Protects guc_ids and destroyed contexts
• • list.

Feels like this should not be removing explanations, only adding to them. The patch itself is only adding new features not removing them. Either the details about id allocation are not worth mentioning and should not have been added in the previous patch. Or they are and should be kept rather than removed in this patch. Either way works for me. The comment was valid information but does maybe count as obvious from the guc_id member (and friends) are within a per GuC instance structure.

• ce->guc_state.lock
• Protects everything under ce->guc_state. Ensures that a context is in the

@@ -719,6 +719,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) if (deregister) guc_signal_context_fence(ce); if (destroyed) {

• 		intel_gt_pm_put_async(guc_to_gt(guc));
  	release_guc_id(guc, ce);
  	__guc_context_destroy(ce);
  }
  

@@ -797,6 +798,8 @@ static void guc_flush_submissions(struct intel_guc *guc) spin_unlock_irqrestore(&sched_engine->lock, flags); }

+static void guc_flush_destroyed_contexts(struct intel_guc *guc);

• void intel_guc_submission_reset_prepare(struct intel_guc *guc) { int i;

@@ -815,6 +818,7 @@ void intel_guc_submission_reset_prepare(struct intel_guc *guc) spin_unlock_irq(&guc_to_gt(guc)->irq_lock);

guc_flush_submissions(guc);

• guc_flush_destroyed_contexts(guc);

  /*

  • Handle any outstanding G2Hs before reset. Call IRQ handler directly

@@ -1126,6 +1130,8 @@ void intel_guc_submission_reset_finish(struct intel_guc *guc) intel_gt_unpark_heartbeats(guc_to_gt(guc)); }

+static void destroyed_worker_func(struct work_struct *w);

• /*
  • Set up the memory resources to be shared with the GuC (via the GGTT)
  • at firmware loading time.

@@ -1151,6 +1157,9 @@ int intel_guc_submission_init(struct intel_guc *guc) spin_lock_init(&guc->submission_state.lock); INIT_LIST_HEAD(&guc->submission_state.guc_id_list); ida_init(&guc->submission_state.guc_ids);

• INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);

• INIT_WORK(&guc->submission_state.destroyed_worker,

•   destroyed_worker_func);
  
  

  return 0; }

@@ -1161,6 +1170,7 @@ void intel_guc_submission_fini(struct intel_guc *guc) return;

guc_lrc_desc_pool_destroy(guc);

• guc_flush_destroyed_contexts(guc);

Seems like these lines should be reversed. We should destroy the higher level constructs before the lower level ones that they could be built on.

i915_sched_engine_put(guc->sched_engine); }

@@ -1859,11 +1869,30 @@ static void guc_context_sched_disable(struct intel_context *ce) static inline void guc_lrc_desc_unpin(struct intel_context *ce) { struct intel_guc *guc = ce_to_guc(ce);

• struct intel_gt *gt = guc_to_gt(guc);

• unsigned long flags;

• bool disabled;

• GEM_BUG_ON(!intel_gt_pm_is_awake(gt)); GEM_BUG_ON(!lrc_desc_registered(guc, ce->guc_id.id)); GEM_BUG_ON(ce != __get_context(guc, ce->guc_id.id)); GEM_BUG_ON(context_enabled(ce));

• /* Seal race with Reset */

• spin_lock_irqsave(&ce->guc_state.lock, flags);

• disabled = submission_disabled(guc);

• if (likely(!disabled)) {

• __intel_gt_pm_get(gt);
  

• set_context_destroyed(ce);
  

• clr_context_registered(ce);
  

• }

• spin_unlock_irqrestore(&ce->guc_state.lock, flags);

• if (unlikely(disabled)) {

• release_guc_id(guc, ce);
  

• __guc_context_destroy(ce);
  

• return;
  

• }

• deregister_context(ce, ce->guc_id.id); }

@@ -1891,78 +1920,86 @@ static void __guc_context_destroy(struct intel_context *ce) } }

+static void guc_flush_destroyed_contexts(struct intel_guc *guc) +{

• struct intel_context *ce, *cn;
• unsigned long flags;
• GEM_BUG_ON(!submission_disabled(guc) &&

•    guc_submission_initialized(guc));
  

• spin_lock_irqsave(&guc->submission_state.lock, flags);
• list_for_each_entry_safe(ce, cn,

• 		 &guc->submission_state.destroyed_contexts,
  

• 		 destroyed_link) {
  

• list_del_init(&ce->destroyed_link);
  

• __release_guc_id(guc, ce);
  

• __guc_context_destroy(ce);
  

• }
• spin_unlock_irqrestore(&guc->submission_state.lock, flags);

+}

+static void deregister_destroyed_contexts(struct intel_guc *guc) +{

• struct intel_context *ce, *cn;
• unsigned long flags;
• spin_lock_irqsave(&guc->submission_state.lock, flags);
• list_for_each_entry_safe(ce, cn,

• 		 &guc->submission_state.destroyed_contexts,
  

• 		 destroyed_link) {
  

• list_del_init(&ce->destroyed_link);
  

• guc_lrc_desc_unpin(ce);
  

• }
• spin_unlock_irqrestore(&guc->submission_state.lock, flags);

+}

+static void destroyed_worker_func(struct work_struct *w) +{

• struct intel_guc *guc = container_of(w, struct intel_guc,

• 			     submission_state.destroyed_worker);
  

• struct intel_gt *gt = guc_to_gt(guc);
• int tmp;
• with_intel_gt_pm(gt, tmp)

• deregister_destroyed_contexts(guc);
  

+}

• 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->uncore->rpm; struct intel_guc *guc = ce_to_guc(ce);
• intel_wakeref_t wakeref; unsigned long flags;
• bool disabled;

• bool destroy;

  /*

  • 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 or the GuC is in the middle of a reset.

  */

• if (context_guc_id_invalid(ce)) {

• __guc_context_destroy(ce);
  

• return;
  

• } else if (submission_disabled(guc) ||

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

• release_guc_id(guc, ce);
  

• __guc_context_destroy(ce);
  

• 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_id available to
  

• * steal 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_id.
  

• */
  

  spin_lock_irqsave(&guc->submission_state.lock, flags);
• if (context_guc_id_invalid(ce)) {

• spin_unlock_irqrestore(&guc->submission_state.lock, flags);
  

• __guc_context_destroy(ce);
  

• return;
  

• destroy = submission_disabled(guc) || context_guc_id_invalid(ce) ||

• !lrc_desc_registered(guc, ce->guc_id.id);
  

• if (likely(!destroy)) {

• if (!list_empty(&ce->guc_id.link))
  

• 	list_del_init(&ce->guc_id.link);
  

• list_add_tail(&ce->destroyed_link,
  

• 	      &guc->submission_state.destroyed_contexts);
  

• } else {

• __release_guc_id(guc, ce);
  

'destroy' can be true if the guc_id is invalid. Is it good to call release on an invalid id?

John.

}

• if (!list_empty(&ce->guc_id.link))

• list_del_init(&ce->guc_id.link);
  

  spin_unlock_irqrestore(&guc->submission_state.lock, flags);
• /* Seal race with Reset */
• spin_lock_irqsave(&ce->guc_state.lock, flags);
• disabled = submission_disabled(guc);
• if (likely(!disabled)) {

• set_context_destroyed(ce);
  

• clr_context_registered(ce);
  

• }
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• if (unlikely(disabled)) {

• release_guc_id(guc, ce);
  

• if (unlikely(destroy)) { __guc_context_destroy(ce); return; }

  /*

• * 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 runtime wakeref is not 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.
  

• * We use a worker to issue the H2G to deregister the context as we can
  

• * take the GT PM for the first time which isn't allowed from an atomic
  

• * context.
  

  */

• with_intel_runtime_pm(runtime_pm, wakeref)

• guc_lrc_desc_unpin(ce);
  

• queue_work(system_unbound_wq, &guc->submission_state.destroyed_worker); }

  static int guc_context_alloc(struct intel_context *ce)

@@ -2798,6 +2835,7 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc, intel_context_put(ce); } else if (context_destroyed(ce)) { /* Context has been destroyed */

• intel_gt_pm_put_async(guc_to_gt(guc));
  

  release_guc_id(guc, ce); __guc_context_destroy(ce); }

On Wed, Oct 06, 2021 at 08:37:03PM -0700, John Harrison wrote:

On 10/4/2021 15:06, Matthew Brost wrote:

...

Taking a PM reference to prevent intel_gt_wait_for_idle from short circuiting while a deregister context H2G is in flight. To do this must issue the deregister H2G from a worker as context can be destroyed from an atomic context and taking GT PM ref blows up. Previously we took a runtime PM from this atomic context which worked but will stop working once runtime pm autosuspend in enabled.

So this patch is two fold, stop intel_gt_wait_for_idle from short circuting and fix runtime pm autosuspend.

v2: (John Harrison)

• Split structure changes out in different patch

(Tvrtko)

• Don't drop lock in deregister_destroyed_contexts

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context.c | 2 + drivers/gpu/drm/i915/gt/intel_context_types.h | 7 + drivers/gpu/drm/i915/gt/intel_engine_pm.h | 5 + drivers/gpu/drm/i915/gt/intel_gt_pm.h | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 11 ++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 146 +++++++++++------- 6 files changed, 121 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index e9a0cad5c34d..1076066f41e0 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -399,6 +399,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine) ce->guc_id.id = GUC_INVALID_LRC_ID; INIT_LIST_HEAD(&ce->guc_id.link);

• INIT_LIST_HEAD(&ce->destroyed_link);
• /*
  • Initialize fence to be complete as this is expected to be complete
  • unless there is a pending schedule disable outstanding.

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index e7e3984aab78..4613d027cbc3 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -213,6 +213,13 @@ struct intel_context { struct list_head link; } guc_id;

• /**

• * @destroyed_link: link in guc->submission_state.destroyed_contexts, in
  

• * list when context is pending to be destroyed (deregistered with the
  

• * GuC), protected by guc->submission_state.lock
  

• */
  

• struct list_head destroyed_link;
• #ifdef CONFIG_DRM_I915_SELFTEST /**
  • @drop_schedule_enable: Force drop of schedule enable G2H for selftest

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h index 8520c595f5e1..6fdeae668e6e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h @@ -16,6 +16,11 @@ intel_engine_pm_is_awake(const struct intel_engine_cs *engine) return intel_wakeref_is_active(&engine->wakeref); } +static inline void __intel_engine_pm_get(struct intel_engine_cs *engine) +{

• __intel_wakeref_get(&engine->wakeref);

+}

• static inline void intel_engine_pm_get(struct intel_engine_cs *engine) { intel_wakeref_get(&engine->wakeref);

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h index d0588d8aaa44..05de6c1af25b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h @@ -41,6 +41,10 @@ static inline void intel_gt_pm_put_async(struct intel_gt *gt) intel_wakeref_put_async(&gt->wakeref); } +#define with_intel_gt_pm(gt, tmp) \

• for (tmp = 1, intel_gt_pm_get(gt); tmp; \

•     intel_gt_pm_put(gt), tmp = 0)
  

• static inline int intel_gt_pm_wait_for_idle(struct intel_gt *gt) { return intel_wakeref_wait_for_idle(&gt->wakeref);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 65b5e8eeef96..25a598e2b6e8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -84,6 +84,17 @@ struct intel_guc { * refs */ struct list_head guc_id_list;

• /**
  

•  * @destroyed_contexts: list of contexts waiting to be destroyed
  

•  * (deregistered with the GuC)
  

•  */
  

• struct list_head destroyed_contexts;
  

• /**
  

•  * @destroyed_worker: worker to deregister contexts, need as we
  

•  * need to take a GT PM reference and can't from destroy
  

•  * function as it might be in an atomic context (no sleeping)
  

•  */
  

• struct work_struct destroyed_worker;
  

  } submission_state; /**

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 ad5c18119d92..17da2fea1bff 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -90,8 +90,8 @@

• used for all of GuC submission but that could change in the future.
• guc->submission_state.lock

• • Protects guc_id allocation for the given GuC, i.e. only one context can be
• • doing guc_id allocation operations at a time for each GuC in the system.

• • Global lock for GuC submission state. Protects guc_ids and destroyed contexts
• • list.

Feels like this should not be removing explanations, only adding to them. The patch itself is only adding new features not removing them. Either the details about id allocation are not worth mentioning and should not have been added in the previous patch. Or they are and should be kept rather than removed in this patch. Either way works for me. The comment was valid information but does maybe count as obvious from the guc_id member (and friends) are within a per GuC instance structure.

...

• ce->guc_state.lock
• Protects everything under ce->guc_state. Ensures that a context is in the

@@ -719,6 +719,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) if (deregister) guc_signal_context_fence(ce); if (destroyed) {

• 		intel_gt_pm_put_async(guc_to_gt(guc));
  	release_guc_id(guc, ce);
  	__guc_context_destroy(ce);
  }
  

@@ -797,6 +798,8 @@ static void guc_flush_submissions(struct intel_guc *guc) spin_unlock_irqrestore(&sched_engine->lock, flags); } +static void guc_flush_destroyed_contexts(struct intel_guc *guc);

• void intel_guc_submission_reset_prepare(struct intel_guc *guc) { int i;

@@ -815,6 +818,7 @@ void intel_guc_submission_reset_prepare(struct intel_guc *guc) spin_unlock_irq(&guc_to_gt(guc)->irq_lock); guc_flush_submissions(guc);

• guc_flush_destroyed_contexts(guc); /*
  • Handle any outstanding G2Hs before reset. Call IRQ handler directly

@@ -1126,6 +1130,8 @@ void intel_guc_submission_reset_finish(struct intel_guc *guc) intel_gt_unpark_heartbeats(guc_to_gt(guc)); } +static void destroyed_worker_func(struct work_struct *w);

• /*
  • Set up the memory resources to be shared with the GuC (via the GGTT)
  • at firmware loading time.

@@ -1151,6 +1157,9 @@ int intel_guc_submission_init(struct intel_guc *guc) spin_lock_init(&guc->submission_state.lock); INIT_LIST_HEAD(&guc->submission_state.guc_id_list); ida_init(&guc->submission_state.guc_ids);

• INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
• INIT_WORK(&guc->submission_state.destroyed_worker,

•   destroyed_worker_func);
  

  return 0; }

@@ -1161,6 +1170,7 @@ void intel_guc_submission_fini(struct intel_guc *guc) return; guc_lrc_desc_pool_destroy(guc);

• guc_flush_destroyed_contexts(guc);

Seems like these lines should be reversed. We should destroy the higher level constructs before the lower level ones that they could be built on.

Missed a few comments. Sure, will reverse.

...

i915_sched_engine_put(guc->sched_engine); } @@ -1859,11 +1869,30 @@ static void guc_context_sched_disable(struct intel_context *ce) static inline void guc_lrc_desc_unpin(struct intel_context *ce) { struct intel_guc *guc = ce_to_guc(ce);

• struct intel_gt *gt = guc_to_gt(guc);
• unsigned long flags;
• bool disabled;
• GEM_BUG_ON(!intel_gt_pm_is_awake(gt)); GEM_BUG_ON(!lrc_desc_registered(guc, ce->guc_id.id)); GEM_BUG_ON(ce != __get_context(guc, ce->guc_id.id)); GEM_BUG_ON(context_enabled(ce));
• /* Seal race with Reset */
• spin_lock_irqsave(&ce->guc_state.lock, flags);
• disabled = submission_disabled(guc);
• if (likely(!disabled)) {

• __intel_gt_pm_get(gt);
  

• set_context_destroyed(ce);
  

• clr_context_registered(ce);
  

• }
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• if (unlikely(disabled)) {

• release_guc_id(guc, ce);
  

• __guc_context_destroy(ce);
  

• return;
  

• }
• deregister_context(ce, ce->guc_id.id); }

@@ -1891,78 +1920,86 @@ static void __guc_context_destroy(struct intel_context *ce) } } +static void guc_flush_destroyed_contexts(struct intel_guc *guc) +{

• struct intel_context *ce, *cn;
• unsigned long flags;
• GEM_BUG_ON(!submission_disabled(guc) &&

•    guc_submission_initialized(guc));
  

• spin_lock_irqsave(&guc->submission_state.lock, flags);
• list_for_each_entry_safe(ce, cn,

• 		 &guc->submission_state.destroyed_contexts,
  

• 		 destroyed_link) {
  

• list_del_init(&ce->destroyed_link);
  

• __release_guc_id(guc, ce);
  

• __guc_context_destroy(ce);
  

• }
• spin_unlock_irqrestore(&guc->submission_state.lock, flags);

+}

+static void deregister_destroyed_contexts(struct intel_guc *guc) +{

• struct intel_context *ce, *cn;
• unsigned long flags;
• spin_lock_irqsave(&guc->submission_state.lock, flags);
• list_for_each_entry_safe(ce, cn,

• 		 &guc->submission_state.destroyed_contexts,
  

• 		 destroyed_link) {
  

• list_del_init(&ce->destroyed_link);
  

• guc_lrc_desc_unpin(ce);
  

• }
• spin_unlock_irqrestore(&guc->submission_state.lock, flags);

+}

+static void destroyed_worker_func(struct work_struct *w) +{

• struct intel_guc *guc = container_of(w, struct intel_guc,

• 			     submission_state.destroyed_worker);
  

• struct intel_gt *gt = guc_to_gt(guc);
• int tmp;
• with_intel_gt_pm(gt, tmp)

• deregister_destroyed_contexts(guc);
  

+}

• 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->uncore->rpm; struct intel_guc *guc = ce_to_guc(ce);
• intel_wakeref_t wakeref; unsigned long flags;
• bool disabled;

• bool destroy; /*
  • 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 or the GuC is in the middle of a reset.
  */

• if (context_guc_id_invalid(ce)) {

• __guc_context_destroy(ce);
  

• return;
  

• } else if (submission_disabled(guc) ||

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

• release_guc_id(guc, ce);
  

• __guc_context_destroy(ce);
  

• 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_id available to
  

• * steal 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_id.
  

• */
  

  spin_lock_irqsave(&guc->submission_state.lock, flags);
• if (context_guc_id_invalid(ce)) {

• spin_unlock_irqrestore(&guc->submission_state.lock, flags);
  

• __guc_context_destroy(ce);
  

• return;
  

• destroy = submission_disabled(guc) || context_guc_id_invalid(ce) ||

• !lrc_desc_registered(guc, ce->guc_id.id);
  

• if (likely(!destroy)) {

• if (!list_empty(&ce->guc_id.link))
  

• 	list_del_init(&ce->guc_id.link);
  

• list_add_tail(&ce->destroyed_link,
  

• 	      &guc->submission_state.destroyed_contexts);
  

• } else {

• __release_guc_id(guc, ce);
  

'destroy' can be true if the guc_id is invalid. Is it good to call release on an invalid id?

__release_guc_id protects again that, it is harmless to call.

Matt

John.

...

}

• if (!list_empty(&ce->guc_id.link))

• list_del_init(&ce->guc_id.link);
  

  spin_unlock_irqrestore(&guc->submission_state.lock, flags);
• /* Seal race with Reset */
• spin_lock_irqsave(&ce->guc_state.lock, flags);
• disabled = submission_disabled(guc);
• if (likely(!disabled)) {

• set_context_destroyed(ce);
  

• clr_context_registered(ce);
  

• }
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• if (unlikely(disabled)) {

• release_guc_id(guc, ce);
  

• if (unlikely(destroy)) { __guc_context_destroy(ce); return; } /*

• * 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 runtime wakeref is not 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.
  

• * We use a worker to issue the H2G to deregister the context as we can
  

• * take the GT PM for the first time which isn't allowed from an atomic
  

• * context.
  

  */

• with_intel_runtime_pm(runtime_pm, wakeref)

• guc_lrc_desc_unpin(ce);
  

• queue_work(system_unbound_wq, &guc->submission_state.destroyed_worker); } static int guc_context_alloc(struct intel_context *ce)

@@ -2798,6 +2835,7 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc, intel_context_put(ce); } else if (context_destroyed(ce)) { /* Context has been destroyed */

• intel_gt_pm_put_async(guc_to_gt(guc));
  

  release_guc_id(guc, ce); __guc_context_destroy(ce); }

On 10/4/2021 15:06, Matthew Brost wrote:

Taking a PM reference to prevent intel_gt_wait_for_idle from short circuiting while a scheduling of user context could be enabled.

I'm not sure what 'while a scheduling of user context could be enabled' means.

John.

Returning GT idle when it is not can cause all sorts of issues throughout the stack.

v2: (Daniel Vetter)

• Add might_lock annotations to pin / unpin function

v3: (CI)

• Drop intel_engine_pm_might_put from unpin path as an async put is used

v4: (John Harrison)

• Make intel_engine_pm_might_get/put work with GuC virtual engines
• Update commit message

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context.c | 2 ++ drivers/gpu/drm/i915/gt/intel_engine_pm.h | 32 +++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_pm.h | 10 ++++++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 36 +++++++++++++++++-- drivers/gpu/drm/i915/intel_wakeref.h | 12 +++++++ 5 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 1076066f41e0..f601323b939f 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -240,6 +240,8 @@ int __intel_context_do_pin_ww(struct intel_context *ce, if (err) goto err_post_unpin;

• intel_engine_pm_might_get(ce->engine);
• if (unlikely(intel_context_is_closed(ce))) { err = -ENOENT; goto err_unlock;

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h index 6fdeae668e6e..d68675925b79 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h @@ -6,9 +6,11 @@ #ifndef INTEL_ENGINE_PM_H #define INTEL_ENGINE_PM_H

+#include "i915_drv.h" #include "i915_request.h" #include "intel_engine_types.h" #include "intel_wakeref.h" +#include "intel_gt_pm.h"

static inline bool intel_engine_pm_is_awake(const struct intel_engine_cs *engine) @@ -31,6 +33,21 @@ static inline bool intel_engine_pm_get_if_awake(struct intel_engine_cs *engine) return intel_wakeref_get_if_active(&engine->wakeref); }

+static inline void intel_engine_pm_might_get(struct intel_engine_cs *engine) +{

• if (!intel_engine_is_virtual(engine)) {

• intel_wakeref_might_get(&engine->wakeref);
  

• } else {

• struct intel_gt *gt = engine->gt;
  

• struct intel_engine_cs *tengine;
  

• intel_engine_mask_t tmp, mask = engine->mask;
  

• for_each_engine_masked(tengine, gt, mask, tmp)
  

• 	intel_wakeref_might_get(&tengine->wakeref);
  

• }
• intel_gt_pm_might_get(engine->gt);

+}

• static inline void intel_engine_pm_put(struct intel_engine_cs *engine) { intel_wakeref_put(&engine->wakeref);

@@ -52,6 +69,21 @@ static inline void intel_engine_pm_flush(struct intel_engine_cs *engine) intel_wakeref_unlock_wait(&engine->wakeref); }

+static inline void intel_engine_pm_might_put(struct intel_engine_cs *engine) +{

• if (!intel_engine_is_virtual(engine)) {

• intel_wakeref_might_put(&engine->wakeref);
  

• } else {

• struct intel_gt *gt = engine->gt;
  

• struct intel_engine_cs *tengine;
  

• intel_engine_mask_t tmp, mask = engine->mask;
  

• for_each_engine_masked(tengine, gt, mask, tmp)
  

• 	intel_wakeref_might_put(&tengine->wakeref);
  

• }
• intel_gt_pm_might_put(engine->gt);

+}

• static inline struct i915_request * intel_engine_create_kernel_request(struct intel_engine_cs *engine) {

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h index 05de6c1af25b..bc898df7a48c 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h @@ -31,6 +31,11 @@ static inline bool intel_gt_pm_get_if_awake(struct intel_gt *gt) return intel_wakeref_get_if_active(&gt->wakeref); }

+static inline void intel_gt_pm_might_get(struct intel_gt *gt) +{

• intel_wakeref_might_get(&gt->wakeref);

+}

• static inline void intel_gt_pm_put(struct intel_gt *gt) { intel_wakeref_put(&gt->wakeref);

@@ -41,6 +46,11 @@ static inline void intel_gt_pm_put_async(struct intel_gt *gt) intel_wakeref_put_async(&gt->wakeref); }

+static inline void intel_gt_pm_might_put(struct intel_gt *gt) +{

• intel_wakeref_might_put(&gt->wakeref);

+}

• #define with_intel_gt_pm(gt, tmp) \ for (tmp = 1, intel_gt_pm_get(gt); tmp; \ intel_gt_pm_put(gt), tmp = 0)

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 17da2fea1bff..8b82da50c2bc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1571,7 +1571,12 @@ static int guc_context_pre_pin(struct intel_context *ce,

static int guc_context_pin(struct intel_context *ce, void *vaddr) {

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

• int ret = __guc_context_pin(ce, ce->engine, vaddr);

• if (likely(!ret && !intel_context_is_barrier(ce)))

• intel_engine_pm_get(ce->engine);
  

• return ret; }

  static void guc_context_unpin(struct intel_context *ce)

@@ -1580,6 +1585,9 @@ static void guc_context_unpin(struct intel_context *ce)

unpin_guc_id(guc, ce); lrc_unpin(ce);

• if (likely(!intel_context_is_barrier(ce)))

• intel_engine_pm_put_async(ce->engine);
  

  }

  static void guc_context_post_unpin(struct intel_context *ce)

@@ -2341,8 +2349,30 @@ static int guc_virtual_context_pre_pin(struct intel_context *ce, static int guc_virtual_context_pin(struct intel_context *ce, void *vaddr) { struct intel_engine_cs *engine = guc_virtual_get_sibling(ce->engine, 0);

• int ret = __guc_context_pin(ce, engine, vaddr);
• intel_engine_mask_t tmp, mask = ce->engine->mask;
• if (likely(!ret))

• for_each_engine_masked(engine, ce->engine->gt, mask, tmp)
  

• 	intel_engine_pm_get(engine);
  
  

• return __guc_context_pin(ce, engine, vaddr);

• return ret;

+}

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

• intel_engine_mask_t tmp, mask = ce->engine->mask;

• struct intel_engine_cs *engine;

• struct intel_guc *guc = ce_to_guc(ce);

• GEM_BUG_ON(context_enabled(ce));

• GEM_BUG_ON(intel_context_is_barrier(ce));

• unpin_guc_id(guc, ce);

• lrc_unpin(ce);

• for_each_engine_masked(engine, ce->engine->gt, mask, tmp)

• intel_engine_pm_put_async(engine);
  

  }

  static void guc_virtual_context_enter(struct intel_context *ce)

@@ -2379,7 +2409,7 @@ static const struct intel_context_ops virtual_guc_context_ops = {

.pre_pin = guc_virtual_context_pre_pin, .pin = guc_virtual_context_pin,

• .unpin = guc_context_unpin,

• .unpin = guc_virtual_context_unpin, .post_unpin = guc_context_post_unpin,

  .ban = guc_context_ban,

diff --git a/drivers/gpu/drm/i915/intel_wakeref.h b/drivers/gpu/drm/i915/intel_wakeref.h index 545c8f277c46..4f4c2e15e736 100644 --- a/drivers/gpu/drm/i915/intel_wakeref.h +++ b/drivers/gpu/drm/i915/intel_wakeref.h @@ -123,6 +123,12 @@ enum { __INTEL_WAKEREF_PUT_LAST_BIT__ };

+static inline void +intel_wakeref_might_get(struct intel_wakeref *wf) +{

• might_lock(&wf->mutex);

+}

• /**
  • intel_wakeref_put_flags: Release the wakeref
  • @wf: the wakeref

@@ -170,6 +176,12 @@ intel_wakeref_put_delay(struct intel_wakeref *wf, unsigned long delay) FIELD_PREP(INTEL_WAKEREF_PUT_DELAY, delay)); }

+static inline void +intel_wakeref_might_put(struct intel_wakeref *wf) +{

• might_lock(&wf->mutex);

+}

• /**
  • intel_wakeref_lock: Lock the wakeref (mutex)
  • @wf: the wakeref

On 10/7/2021 08:19, Matthew Brost wrote:

On Wed, Oct 06, 2021 at 08:45:42PM -0700, John Harrison wrote:

...

On 10/4/2021 15:06, Matthew Brost wrote:

...

Taking a PM reference to prevent intel_gt_wait_for_idle from short circuiting while a scheduling of user context could be enabled.

I'm not sure what 'while a scheduling of user context could be enabled' means.

Not really sure how this isn't clear.

It means if a user context has scheduling enabled this function cannot short circuit returning idle.

Matt

Okay. The 'a scheduling' was throwing me off. And I was reading 'could be enabled' as saying something that might happen in the future. English is great at being ambiguous ;). Maybe 'while any user context has scheduling enabled' would be simpler?

John.

...

John.

...

Returning GT idle when it is not can cause all sorts of issues throughout the stack.

v2: (Daniel Vetter) - Add might_lock annotations to pin / unpin function v3: (CI) - Drop intel_engine_pm_might_put from unpin path as an async put is used v4: (John Harrison) - Make intel_engine_pm_might_get/put work with GuC virtual engines - Update commit message

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context.c | 2 ++ drivers/gpu/drm/i915/gt/intel_engine_pm.h | 32 +++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_pm.h | 10 ++++++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 36 +++++++++++++++++-- drivers/gpu/drm/i915/intel_wakeref.h | 12 +++++++ 5 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 1076066f41e0..f601323b939f 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -240,6 +240,8 @@ int __intel_context_do_pin_ww(struct intel_context *ce, if (err) goto err_post_unpin;

• intel_engine_pm_might_get(ce->engine);
• if (unlikely(intel_context_is_closed(ce))) { err = -ENOENT; goto err_unlock;

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h index 6fdeae668e6e..d68675925b79 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h @@ -6,9 +6,11 @@ #ifndef INTEL_ENGINE_PM_H #define INTEL_ENGINE_PM_H +#include "i915_drv.h" #include "i915_request.h" #include "intel_engine_types.h" #include "intel_wakeref.h" +#include "intel_gt_pm.h" static inline bool intel_engine_pm_is_awake(const struct intel_engine_cs *engine) @@ -31,6 +33,21 @@ static inline bool intel_engine_pm_get_if_awake(struct intel_engine_cs *engine) return intel_wakeref_get_if_active(&engine->wakeref); } +static inline void intel_engine_pm_might_get(struct intel_engine_cs *engine) +{

• if (!intel_engine_is_virtual(engine)) {

• intel_wakeref_might_get(&engine->wakeref);
  

• } else {

• struct intel_gt *gt = engine->gt;
  

• struct intel_engine_cs *tengine;
  

• intel_engine_mask_t tmp, mask = engine->mask;
  

• for_each_engine_masked(tengine, gt, mask, tmp)
  

• 	intel_wakeref_might_get(&tengine->wakeref);
  

• }
• intel_gt_pm_might_get(engine->gt);

+}

• static inline void intel_engine_pm_put(struct intel_engine_cs *engine) { intel_wakeref_put(&engine->wakeref);

@@ -52,6 +69,21 @@ static inline void intel_engine_pm_flush(struct intel_engine_cs *engine) intel_wakeref_unlock_wait(&engine->wakeref); } +static inline void intel_engine_pm_might_put(struct intel_engine_cs *engine) +{

• if (!intel_engine_is_virtual(engine)) {

• intel_wakeref_might_put(&engine->wakeref);
  

• } else {

• struct intel_gt *gt = engine->gt;
  

• struct intel_engine_cs *tengine;
  

• intel_engine_mask_t tmp, mask = engine->mask;
  

• for_each_engine_masked(tengine, gt, mask, tmp)
  

• 	intel_wakeref_might_put(&tengine->wakeref);
  

• }
• intel_gt_pm_might_put(engine->gt);

+}

• static inline struct i915_request * intel_engine_create_kernel_request(struct intel_engine_cs *engine) {

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h index 05de6c1af25b..bc898df7a48c 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h @@ -31,6 +31,11 @@ static inline bool intel_gt_pm_get_if_awake(struct intel_gt *gt) return intel_wakeref_get_if_active(&gt->wakeref); } +static inline void intel_gt_pm_might_get(struct intel_gt *gt) +{

• intel_wakeref_might_get(&gt->wakeref);

+}

• static inline void intel_gt_pm_put(struct intel_gt *gt) { intel_wakeref_put(&gt->wakeref);

@@ -41,6 +46,11 @@ static inline void intel_gt_pm_put_async(struct intel_gt *gt) intel_wakeref_put_async(&gt->wakeref); } +static inline void intel_gt_pm_might_put(struct intel_gt *gt) +{

• intel_wakeref_might_put(&gt->wakeref);

+}

• #define with_intel_gt_pm(gt, tmp) \ for (tmp = 1, intel_gt_pm_get(gt); tmp; \ intel_gt_pm_put(gt), tmp = 0)

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 17da2fea1bff..8b82da50c2bc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1571,7 +1571,12 @@ static int guc_context_pre_pin(struct intel_context *ce, static int guc_context_pin(struct intel_context *ce, void *vaddr) {

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

• int ret = __guc_context_pin(ce, ce->engine, vaddr);
• if (likely(!ret && !intel_context_is_barrier(ce)))

• intel_engine_pm_get(ce->engine);
  

• return ret; } static void guc_context_unpin(struct intel_context *ce)

@@ -1580,6 +1585,9 @@ static void guc_context_unpin(struct intel_context *ce) unpin_guc_id(guc, ce); lrc_unpin(ce);

• if (likely(!intel_context_is_barrier(ce)))

• intel_engine_pm_put_async(ce->engine);
  

  } static void guc_context_post_unpin(struct intel_context *ce)

@@ -2341,8 +2349,30 @@ static int guc_virtual_context_pre_pin(struct intel_context *ce, static int guc_virtual_context_pin(struct intel_context *ce, void *vaddr) { struct intel_engine_cs *engine = guc_virtual_get_sibling(ce->engine, 0);

• int ret = __guc_context_pin(ce, engine, vaddr);
• intel_engine_mask_t tmp, mask = ce->engine->mask;
• if (likely(!ret))

• for_each_engine_masked(engine, ce->engine->gt, mask, tmp)
  

• 	intel_engine_pm_get(engine);
  

• return __guc_context_pin(ce, engine, vaddr);

• return ret;

+}

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

• intel_engine_mask_t tmp, mask = ce->engine->mask;
• struct intel_engine_cs *engine;
• struct intel_guc *guc = ce_to_guc(ce);
• GEM_BUG_ON(context_enabled(ce));
• GEM_BUG_ON(intel_context_is_barrier(ce));
• unpin_guc_id(guc, ce);
• lrc_unpin(ce);
• for_each_engine_masked(engine, ce->engine->gt, mask, tmp)

• intel_engine_pm_put_async(engine);
  

  } static void guc_virtual_context_enter(struct intel_context *ce)

@@ -2379,7 +2409,7 @@ static const struct intel_context_ops virtual_guc_context_ops = { .pre_pin = guc_virtual_context_pre_pin, .pin = guc_virtual_context_pin,

• .unpin = guc_context_unpin,

• .unpin = guc_virtual_context_unpin, .post_unpin = guc_context_post_unpin, .ban = guc_context_ban,

diff --git a/drivers/gpu/drm/i915/intel_wakeref.h b/drivers/gpu/drm/i915/intel_wakeref.h index 545c8f277c46..4f4c2e15e736 100644 --- a/drivers/gpu/drm/i915/intel_wakeref.h +++ b/drivers/gpu/drm/i915/intel_wakeref.h @@ -123,6 +123,12 @@ enum { __INTEL_WAKEREF_PUT_LAST_BIT__ }; +static inline void +intel_wakeref_might_get(struct intel_wakeref *wf) +{

• might_lock(&wf->mutex);

+}

• /**
  • intel_wakeref_put_flags: Release the wakeref
  • @wf: the wakeref

@@ -170,6 +176,12 @@ intel_wakeref_put_delay(struct intel_wakeref *wf, unsigned long delay) FIELD_PREP(INTEL_WAKEREF_PUT_DELAY, delay)); } +static inline void +intel_wakeref_might_put(struct intel_wakeref *wf) +{

• might_lock(&wf->mutex);

+}

• /**
  • intel_wakeref_lock: Lock the wakeref (mutex)
  • @wf: the wakeref

On 10/4/2021 15:06, Matthew Brost wrote:

Introduce context parent-child relationship. Once this relationship is created all pinning / unpinning operations are directed to the parent context. The parent context is responsible for pinning all of its'

No need for an apostrophe.

children and itself.

This is a precursor to the full GuC multi-lrc implementation but aligns to how GuC mutli-lrc interface is defined - a single H2G is used register / deregister all of the contexts simultaneously.

Subsequent patches in the series will implement the pinning / unpinning operations for parent / child contexts.

v2: (Daniel Vetter)

• Add kernel doc, add wrapper to access parent to ensure safety

v3: (John Harrison)

• Fix comment explaing GEM_BUG_ON in to_parent()
• Make variable names generic (non-GuC specific)

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context.c | 29 +++++++++++++ drivers/gpu/drm/i915/gt/intel_context.h | 41 +++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_context_types.h | 21 ++++++++++ 3 files changed, 91 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index f601323b939f..c5bb7ccfb3f8 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -403,6 +403,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)

INIT_LIST_HEAD(&ce->destroyed_link);

• INIT_LIST_HEAD(&ce->parallel.child_list);
• /*
  • Initialize fence to be complete as this is expected to be complete
  • unless there is a pending schedule disable outstanding.

@@ -417,10 +419,17 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)

void intel_context_fini(struct intel_context *ce) {

• struct intel_context *child, *next;

• if (ce->timeline) intel_timeline_put(ce->timeline); i915_vm_put(ce->vm);

• /* Need to put the creation ref for the children */

• if (intel_context_is_parent(ce))

• for_each_child_safe(ce, child, next)
  

• 	intel_context_put(child);
  

• mutex_destroy(&ce->pin_mutex); i915_active_fini(&ce->active); i915_sw_fence_fini(&ce->guc_state.blocked);

@@ -537,6 +546,26 @@ struct i915_request *intel_context_find_active_request(struct intel_context *ce) return active; }

+void intel_context_bind_parent_child(struct intel_context *parent,

• 		     struct intel_context *child)
  

+{

• /*

• * Callers responsibility to validate that this function is used
  

• * correctly but we use GEM_BUG_ON here ensure that they do.
  

• */
  

• GEM_BUG_ON(!intel_engine_uses_guc(parent->engine));
• GEM_BUG_ON(intel_context_is_pinned(parent));
• GEM_BUG_ON(intel_context_is_child(parent));
• GEM_BUG_ON(intel_context_is_pinned(child));
• GEM_BUG_ON(intel_context_is_child(child));
• GEM_BUG_ON(intel_context_is_parent(child));
• parent->parallel.number_children++;
• list_add_tail(&child->parallel.child_link,

•       &parent->parallel.child_list);
  

• child->parallel.parent = parent;

+}

• #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftest_context.c" #endif

diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h index c41098950746..b63c10a144af 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.h +++ b/drivers/gpu/drm/i915/gt/intel_context.h @@ -44,6 +44,47 @@ void intel_context_free(struct intel_context *ce); int intel_context_reconfigure_sseu(struct intel_context *ce, const struct intel_sseu sseu);

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

• return !!ce->parallel.parent;

+}

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

• return !!ce->parallel.number_children;

+}

+static inline bool intel_context_is_pinned(struct intel_context *ce);

+static inline struct intel_context * +intel_context_to_parent(struct intel_context *ce) +{

• if (intel_context_is_child(ce)) {

• /*
  

•  * The parent holds ref count to the child so it is always safe
  

•  * for the parent to access the child, but the child has a
  

•  * pointer to the parent without a ref. To ensure this is safe
  

•  * the child should only access the parent pointer while the
  

•  * parent is pinned.
  

•  */
  

• GEM_BUG_ON(!intel_context_is_pinned(ce->parallel.parent));
  

• return ce->parallel.parent;
  

• } else {

• return ce;
  

• }

+}

+void intel_context_bind_parent_child(struct intel_context *parent,

• 		     struct intel_context *child);
  

+#define for_each_child(parent, ce)\

• list_for_each_entry(ce, &(parent)->parallel.child_list,\

• 	    parallel.child_link)
  

+#define for_each_child_safe(parent, ce, cn)\

• list_for_each_entry_safe(ce, cn, &(parent)->parallel.child_list,\

• 		 parallel.child_link)
  

• /**
  • intel_context_lock_pinned - Stablises the 'pinned' status of the HW context
  • @ce - the context

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 4613d027cbc3..76dfca57cb45 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -220,6 +220,27 @@ struct intel_context { */ struct list_head destroyed_link;

• /** @parallel: sub-structure for parallel submission members */
• struct {

• union {
  

• 	/**
  

• 	 * @child_list: parent's list of children
  

• 	 * contexts, no protection as immutable after context
  

• 	 * creation
  

• 	 */
  

• 	struct list_head child_list;
  

• 	/**
  

• 	 * @child_link: child's link into parent's list of
  

• 	 * children
  

• 	 */
  

• 	struct list_head child_link;
  

• };
  

• /** @parent: pointer to parent if child */
  

• struct intel_context *parent;
  

• /** @number_children: number of children if parent */
  

• u8 number_children;
  

Is there any particular reason for using 'u8'? A simple 'int' can be much more efficient depending upon the host CPU architecture.

Not a blocker though. So with the typo above fixed: Reviewed-by: John Harrison John.C.Harrison@Intel.com

• } parallel;
• #ifdef CONFIG_DRM_I915_SELFTEST /**
  • @drop_schedule_enable: Force drop of schedule enable G2H for selftest

On Thu, Oct 07, 2021 at 12:50:28PM -0700, John Harrison wrote:

On 10/4/2021 15:06, Matthew Brost wrote:

...

Add multi-lrc context registration H2G. In addition a workqueue and process descriptor are setup during multi-lrc context registration as these data structures are needed for multi-lrc submission.

v2: (John Harrison)

• Move GuC specific fields into sub-struct
• Clean up WQ defines
• Add comment explaining math to derive WQ / PD address

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context_types.h | 12 ++ drivers/gpu/drm/i915/gt/intel_lrc.c | 5 + .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 2 - .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 114 +++++++++++++++++- 5 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 76dfca57cb45..48decb5ee954 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -239,6 +239,18 @@ struct intel_context { struct intel_context *parent; /** @number_children: number of children if parent */ u8 number_children;

• /** @guc: GuC specific members for parallel submission */
  

• struct {
  

• 	/** @wqi_head: head pointer in work queue */
  

• 	u16 wqi_head;
  

• 	/** @wqi_tail: tail pointer in work queue */
  

• 	u16 wqi_tail;
  

• 	/**
  

• 	 * @parent_page: page in context state (ce->state) used
  

• 	 * by parent for work queue, process descriptor
  

• 	 */
  

• 	u8 parent_page;
  

• } guc;
  

  } parallel; #ifdef CONFIG_DRM_I915_SELFTEST

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 3ef9eaf8c50e..57339d5c1fc8 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -942,6 +942,11 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine) context_size += PAGE_SIZE; }

• if (intel_context_is_parent(ce) && intel_engine_uses_guc(engine)) {

• ce->parallel.guc.parent_page = context_size / PAGE_SIZE;
  

• context_size += PAGE_SIZE;
  

• }
• obj = i915_gem_object_create_lmem(engine->i915, context_size, I915_BO_ALLOC_PM_VOLATILE); if (IS_ERR(obj))

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 8ff582222aff..ba10bd374cee 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -142,6 +142,7 @@ enum intel_guc_action { INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505, INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506, INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE = 0x4600,

• INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC = 0x4601, INTEL_GUC_ACTION_RESET_CLIENT = 0x5507, INTEL_GUC_ACTION_LIMIT };

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h index fa4be13c8854..0eeb2a9feeed 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h @@ -52,8 +52,6 @@ #define GUC_DOORBELL_INVALID 256 -#define GUC_WQ_SIZE (PAGE_SIZE * 2)

• /* Work queue item header definitions */ #define WQ_STATUS_ACTIVE 1 #define WQ_STATUS_SUSPENDED 2

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 451d9ae861a6..ab6d7fc1b0b1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -344,6 +344,45 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb) return rb_entry(rb, struct i915_priolist, node); } +/*

• • When using multi-lrc submission an extra page in the context state is
• • reserved for the process descriptor and work queue.
• • The layout of this page is below:
• • 0 guc_process_desc
• • ... unused
• • PAGE_SIZE / 2 work queue start
• • ... work queue
• • PAGE_SIZE - 1 work queue end
• */

+#define WQ_SIZE (PAGE_SIZE / 2) +#define WQ_OFFSET (PAGE_SIZE - WQ_SIZE)

I thought you were going with '#define PARENT_SCRATCH SIZE PAGE_SIZE' and then using that everywhere else? Unless there is a fundamental reason why the above must be exactly a page in size then I think the size should be defined once and re-used rather than assumed in multiple places (including in the description comment).

Right, forgot about that. Will fix.

...

+static u32 __get_process_desc_offset(struct intel_context *ce) +{

• GEM_BUG_ON(!ce->parallel.guc.parent_page);
• return ce->parallel.guc.parent_page * PAGE_SIZE;

+}

+static u32 __get_wq_offset(struct intel_context *ce) +{

• return __get_process_desc_offset(ce) + WQ_OFFSET;

+}

+static struct guc_process_desc * +__get_process_desc(struct intel_context *ce) +{

• /*

• * Need to subtract LRC_STATE_OFFSET here as the
  

• * parallel.guc.parent_page is the offset into ce->state while
  

• * ce->lrc_reg_reg is ce->state + LRC_STATE_OFFSET.
  

• */
  

• return (struct guc_process_desc *)

• (ce->lrc_reg_state +
  

•  ((__get_process_desc_offset(ce) -
  

•    LRC_STATE_OFFSET) / sizeof(u32)));
  

+}

• static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index) { struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;

@@ -1365,6 +1404,30 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce) spin_unlock_irqrestore(&guc->submission_state.lock, flags); } +static int __guc_action_register_multi_lrc(struct intel_guc *guc,

• 			   struct intel_context *ce,
  

• 			   u32 guc_id,
  

• 			   u32 offset,
  

• 			   bool loop)
  

+{

• struct intel_context *child;
• u32 action[4 + MAX_ENGINE_INSTANCE];
• int len = 0;
• GEM_BUG_ON(ce->parallel.number_children > MAX_ENGINE_INSTANCE);
• action[len++] = INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC;
• action[len++] = guc_id;
• action[len++] = ce->parallel.number_children + 1;
• action[len++] = offset;
• for_each_child(ce, child) {

• offset += sizeof(struct guc_lrc_desc);
  

• action[len++] = offset;
  

• }
• return guc_submission_send_busy_loop(guc, action, len, 0, loop);

+}

• static int __guc_action_register_context(struct intel_guc *guc, u32 guc_id, u32 offset,

@@ -1387,9 +1450,15 @@ static int register_context(struct intel_context *ce, bool loop) ce->guc_id.id * sizeof(struct guc_lrc_desc); int ret;

• GEM_BUG_ON(intel_context_is_child(ce)); trace_intel_context_register(ce);

• ret = __guc_action_register_context(guc, ce->guc_id.id, offset, loop);

• if (intel_context_is_parent(ce))

• ret = __guc_action_register_multi_lrc(guc, ce, ce->guc_id.id,
  

• 				      offset, loop);
  

• else

• ret = __guc_action_register_context(guc, ce->guc_id.id, offset,
  

• 				    loop);
  

  if (likely(!ret)) { unsigned long flags;

@@ -1418,6 +1487,7 @@ static int deregister_context(struct intel_context *ce, u32 guc_id) { struct intel_guc *guc = ce_to_guc(ce);

• GEM_BUG_ON(intel_context_is_child(ce)); trace_intel_context_deregister(ce); return __guc_action_deregister_context(guc, guc_id);

@@ -1445,6 +1515,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) struct guc_lrc_desc *desc; bool context_registered; intel_wakeref_t wakeref;

• struct intel_context *child; int ret = 0; GEM_BUG_ON(!engine->mask);

@@ -1470,6 +1541,41 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop) desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD; guc_context_policy_init(engine, desc);

• /*

• * Context is a parent, we need to register a process descriptor
  

• * describing a work queue and register all child contexts.
  

• */
  

This was now meant to say 'If the context is a parent...'?

Yep, that is what it should say. Will fix.

Matt

John.

...

• if (intel_context_is_parent(ce)) {

• struct guc_process_desc *pdesc;
  

• ce->parallel.guc.wqi_tail = 0;
  

• ce->parallel.guc.wqi_head = 0;
  

• desc->process_desc = i915_ggtt_offset(ce->state) +
  

• 	__get_process_desc_offset(ce);
  

• desc->wq_addr = i915_ggtt_offset(ce->state) +
  

• 	__get_wq_offset(ce);
  

• desc->wq_size = WQ_SIZE;
  

• pdesc = __get_process_desc(ce);
  

• memset(pdesc, 0, sizeof(*(pdesc)));
  

• pdesc->stage_id = ce->guc_id.id;
  

• pdesc->wq_base_addr = desc->wq_addr;
  

• pdesc->wq_size_bytes = desc->wq_size;
  

• pdesc->wq_status = WQ_STATUS_ACTIVE;
  

• for_each_child(ce, child) {
  

• 	desc = __get_lrc_desc(guc, child->guc_id.id);
  

• 	desc->engine_class =
  

• 		engine_class_to_guc_class(engine->class);
  

• 	desc->hw_context_desc = child->lrc.lrca;
  

• 	desc->priority = ce->guc_state.prio;
  

• 	desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
  

• 	guc_context_policy_init(engine, desc);
  

• }
  

• }
• /*
  • The context_lookup xarray is used to determine if the hardware
  • context is currently registered. There are two cases in which it

@@ -2804,6 +2910,12 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) return NULL; }

• if (unlikely(intel_context_is_child(ce))) {

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

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

• return NULL;
  

• }
• return ce; }

On Fri, Oct 08, 2021 at 10:20:16AM -0700, John Harrison wrote:

On 10/7/2021 12:50, John Harrison wrote:

...

On 10/4/2021 15:06, Matthew Brost wrote:

...

Add multi-lrc context registration H2G. In addition a workqueue and process descriptor are setup during multi-lrc context registration as these data structures are needed for multi-lrc submission.

v2:   (John Harrison)    - Move GuC specific fields into sub-struct    - Clean up WQ defines    - Add comment explaining math to derive WQ / PD address

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/intel_context_types.h |  12 ++   drivers/gpu/drm/i915/gt/intel_lrc.c           |   5 +   .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   2 -   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 114 +++++++++++++++++-   5 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index 76dfca57cb45..48decb5ee954 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -239,6 +239,18 @@ struct intel_context {           struct intel_context *parent;           /** @number_children: number of children if parent */           u8 number_children; +        /** @guc: GuC specific members for parallel submission */ +        struct { +            /** @wqi_head: head pointer in work queue */ +            u16 wqi_head; +            /** @wqi_tail: tail pointer in work queue */ +            u16 wqi_tail;

PS: As per comments on previous rev, something somewhere needs to explicitly state what WQI means. One suggestion was to do that here, ideally with maybe a brief description of what the queue is, how it is used, etc. Although probably it would be better kept in a GuC specific file. E.g. added to guc_fwif.h in patch #12.

I think this should just be in the main GuC kernel doc. I can include an update to the kernel DoC in a patch at the end of the next rev of the series. That patch doesn't necessarily have to included in the initial merge of parallel submission if it takes a bit more time to review.

Matt

John.

...

...

+            /** +             * @parent_page: page in context state (ce->state) used +             * by parent for work queue, process descriptor +             */ +            u8 parent_page; +        } guc;       } parallel;     #ifdef CONFIG_DRM_I915_SELFTEST diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 3ef9eaf8c50e..57339d5c1fc8 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -942,6 +942,11 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine)           context_size += PAGE_SIZE;       }   +    if (intel_context_is_parent(ce) && intel_engine_uses_guc(engine)) { +        ce->parallel.guc.parent_page = context_size / PAGE_SIZE; +        context_size += PAGE_SIZE; +    }

obj = i915_gem_object_create_lmem(engine->i915, context_size,                         I915_BO_ALLOC_PM_VOLATILE);       if (IS_ERR(obj)) diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 8ff582222aff..ba10bd374cee 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -142,6 +142,7 @@ enum intel_guc_action {       INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,       INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,       INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE = 0x4600, +    INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC = 0x4601,       INTEL_GUC_ACTION_RESET_CLIENT = 0x5507,       INTEL_GUC_ACTION_LIMIT   }; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h index fa4be13c8854..0eeb2a9feeed 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h @@ -52,8 +52,6 @@     #define GUC_DOORBELL_INVALID        256   -#define GUC_WQ_SIZE            (PAGE_SIZE * 2)

/* Work queue item header definitions */   #define WQ_STATUS_ACTIVE        1   #define WQ_STATUS_SUSPENDED        2 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 451d9ae861a6..ab6d7fc1b0b1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -344,6 +344,45 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)       return rb_entry(rb, struct i915_priolist, node);   }   +/*

• • When using multi-lrc submission an extra page in the context

state is

• • reserved for the process descriptor and work queue.
• • The layout of this page is below:
• • 0                        guc_process_desc
• • ...                        unused
• • PAGE_SIZE / 2                work queue start
• • ...                        work queue
• • PAGE_SIZE - 1                work queue end
• */

+#define WQ_SIZE            (PAGE_SIZE / 2) +#define WQ_OFFSET        (PAGE_SIZE - WQ_SIZE)

I thought you were going with '#define PARENT_SCRATCH SIZE PAGE_SIZE' and then using that everywhere else? Unless there is a fundamental reason why the above must be exactly a page in size then I think the size should be defined once and re-used rather than assumed in multiple places (including in the description comment).

...

+static u32 __get_process_desc_offset(struct intel_context *ce) +{ +    GEM_BUG_ON(!ce->parallel.guc.parent_page);

+    return ce->parallel.guc.parent_page * PAGE_SIZE; +}

+static u32 __get_wq_offset(struct intel_context *ce) +{ +    return __get_process_desc_offset(ce) + WQ_OFFSET; +}

+static struct guc_process_desc * +__get_process_desc(struct intel_context *ce) +{ +    /* +     * Need to subtract LRC_STATE_OFFSET here as the +     * parallel.guc.parent_page is the offset into ce->state while +     * ce->lrc_reg_reg is ce->state + LRC_STATE_OFFSET. +     */ +    return (struct guc_process_desc *) +        (ce->lrc_reg_state + +         ((__get_process_desc_offset(ce) - +           LRC_STATE_OFFSET) / sizeof(u32))); +}

static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)   {       struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr; @@ -1365,6 +1404,30 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce)       spin_unlock_irqrestore(&guc->submission_state.lock, flags);   }   +static int __guc_action_register_multi_lrc(struct intel_guc *guc, +                       struct intel_context *ce, +                       u32 guc_id, +                       u32 offset, +                       bool loop) +{ +    struct intel_context *child; +    u32 action[4 + MAX_ENGINE_INSTANCE]; +    int len = 0;

+    GEM_BUG_ON(ce->parallel.number_children > MAX_ENGINE_INSTANCE);

+    action[len++] = INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC; +    action[len++] = guc_id; +    action[len++] = ce->parallel.number_children + 1; +    action[len++] = offset; +    for_each_child(ce, child) { +        offset += sizeof(struct guc_lrc_desc); +        action[len++] = offset; +    }

+    return guc_submission_send_busy_loop(guc, action, len, 0, loop); +}

static int __guc_action_register_context(struct intel_guc *guc,                        u32 guc_id,                        u32 offset, @@ -1387,9 +1450,15 @@ static int register_context(struct intel_context *ce, bool loop)           ce->guc_id.id * sizeof(struct guc_lrc_desc);       int ret;   +    GEM_BUG_ON(intel_context_is_child(ce));       trace_intel_context_register(ce);   -    ret = __guc_action_register_context(guc, ce->guc_id.id, offset, loop); +    if (intel_context_is_parent(ce)) +        ret = __guc_action_register_multi_lrc(guc, ce, ce->guc_id.id, +                              offset, loop); +    else +        ret = __guc_action_register_context(guc, ce->guc_id.id, offset, +                            loop);       if (likely(!ret)) {           unsigned long flags;   @@ -1418,6 +1487,7 @@ static int deregister_context(struct intel_context *ce, u32 guc_id)   {       struct intel_guc *guc = ce_to_guc(ce);   +    GEM_BUG_ON(intel_context_is_child(ce));       trace_intel_context_deregister(ce);         return __guc_action_deregister_context(guc, guc_id); @@ -1445,6 +1515,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)       struct guc_lrc_desc *desc;       bool context_registered;       intel_wakeref_t wakeref; +    struct intel_context *child;       int ret = 0;         GEM_BUG_ON(!engine->mask); @@ -1470,6 +1541,41 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)       desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;       guc_context_policy_init(engine, desc);   +    /* +     * Context is a parent, we need to register a process descriptor +     * describing a work queue and register all child contexts. +     */

This was now meant to say 'If the context is a parent...'?

John.

...

+    if (intel_context_is_parent(ce)) { +        struct guc_process_desc *pdesc;

+        ce->parallel.guc.wqi_tail = 0; +        ce->parallel.guc.wqi_head = 0;

+        desc->process_desc = i915_ggtt_offset(ce->state) + +            __get_process_desc_offset(ce); +        desc->wq_addr = i915_ggtt_offset(ce->state) + +            __get_wq_offset(ce); +        desc->wq_size = WQ_SIZE;

+        pdesc = __get_process_desc(ce); +        memset(pdesc, 0, sizeof(*(pdesc))); +        pdesc->stage_id = ce->guc_id.id; +        pdesc->wq_base_addr = desc->wq_addr; +        pdesc->wq_size_bytes = desc->wq_size; +        pdesc->wq_status = WQ_STATUS_ACTIVE;

+        for_each_child(ce, child) { +            desc = __get_lrc_desc(guc, child->guc_id.id);

+            desc->engine_class = +                engine_class_to_guc_class(engine->class); +            desc->hw_context_desc = child->lrc.lrca; +            desc->priority = ce->guc_state.prio; +            desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD; +            guc_context_policy_init(engine, desc); +        } +    }

/*        * The context_lookup xarray is used to determine if the hardware        * context is currently registered. There are two cases in which it @@ -2804,6 +2910,12 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx)           return NULL;       }   +    if (unlikely(intel_context_is_child(ce))) { +        drm_err(&guc_to_gt(guc)->i915->drm, +            "Context is child, desc_idx %u", desc_idx); +        return NULL; +    }

return ce;   }

On 10/4/2021 15:06, Matthew Brost wrote:

In GuC parent-child contexts the parent context controls the scheduling, ensure only the parent does the scheduling operations.

Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: John Harrison John.C.Harrison@Intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

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 ab6d7fc1b0b1..1f2809187513 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -324,6 +324,12 @@ static inline void decr_context_committed_requests(struct intel_context *ce) GEM_BUG_ON(ce->guc_state.number_committed_requests < 0); }

+static struct intel_context * +request_to_scheduling_context(struct i915_request *rq) +{

• return intel_context_to_parent(rq->context);

+}

• static inline bool context_guc_id_invalid(struct intel_context *ce) { return ce->guc_id.id == GUC_INVALID_LRC_ID;

@@ -1710,6 +1716,7 @@ static void __guc_context_sched_disable(struct intel_guc *guc,

GEM_BUG_ON(guc_id == GUC_INVALID_LRC_ID);

• GEM_BUG_ON(intel_context_is_child(ce)); trace_intel_context_sched_disable(ce);

  guc_submission_send_busy_loop(guc, action, ARRAY_SIZE(action),

@@ -1935,6 +1942,8 @@ static void guc_context_sched_disable(struct intel_context *ce) intel_wakeref_t wakeref; u16 guc_id;

• GEM_BUG_ON(intel_context_is_child(ce));

• spin_lock_irqsave(&ce->guc_state.lock, flags);

  /*

@@ -2303,6 +2312,8 @@ static void guc_signal_context_fence(struct intel_context *ce) { unsigned long flags;

• GEM_BUG_ON(intel_context_is_child(ce));
• spin_lock_irqsave(&ce->guc_state.lock, flags); clr_context_wait_for_deregister_to_register(ce); __guc_signal_context_fence(ce);

@@ -2333,7 +2344,7 @@ static void guc_context_init(struct intel_context *ce)

static int guc_request_alloc(struct i915_request *rq) {

• struct intel_context *ce = rq->context;

• struct intel_context *ce = request_to_scheduling_context(rq); struct intel_guc *guc = ce_to_guc(ce); unsigned long flags; int ret;

On 10/4/2021 15:06, Matthew Brost wrote:

Assign contexts in parent-child relationship consecutive guc_ids. This is accomplished by partitioning guc_id space between ones that need to be consecutive (1/16 available guc_ids) and ones that do not (15/16 of available guc_ids). The consecutive search is implemented via the bitmap API.

This is a precursor to the full GuC multi-lrc implementation but aligns to how GuC mutli-lrc interface is defined - guc_ids must be consecutive when using the GuC multi-lrc interface.

v2: (Daniel Vetter)

• Explicitly state why we assign consecutive guc_ids

v3: (John Harrison)

• Bring back in spin lock

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 6 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 104 ++++++++++++++---- 2 files changed, 86 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 25a598e2b6e8..a9f4ec972bfb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -76,9 +76,13 @@ struct intel_guc { */ spinlock_t lock; /**

•  * @guc_ids: used to allocate new guc_ids
  

•  * @guc_ids: used to allocate new guc_ids, single-lrc
  

  */ struct ida guc_ids;

• /**
  

•  * @guc_ids_bitmap: used to allocate new guc_ids, multi-lrc
  

•  */
  

• unsigned long *guc_ids_bitmap;
  

  /**
  • @guc_id_list: list of intel_context with valid guc_ids but no
  • refs

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 1f2809187513..79e7732e83b2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -128,6 +128,16 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count);

#define GUC_REQUEST_SIZE 64 /* bytes */

+/*

• • We reserve 1/16 of the guc_ids for multi-lrc as these need to be contiguous
• • per the GuC submission interface. A different allocation algorithm is used
• • (bitmap vs. ida) between multi-lrc and single-lrc hence the reason to
• • partition the guc_id space. We believe the number of multi-lrc contexts in
• • use should be low and 1/16 should be sufficient. Minimum of 32 guc_ids for
• • multi-lrc.
• */

+#define NUMBER_MULTI_LRC_GUC_ID (GUC_MAX_LRC_DESCRIPTORS / 16)

• /*
  • Below is a set of functions which control the GuC scheduling state which
  • require a lock.

@@ -1206,6 +1216,11 @@ int intel_guc_submission_init(struct intel_guc *guc) INIT_WORK(&guc->submission_state.destroyed_worker, destroyed_worker_func);

• guc->submission_state.guc_ids_bitmap =

• bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID, GFP_KERNEL);
  

• if (!guc->submission_state.guc_ids_bitmap)

• return -ENOMEM;
  

• return 0; }

@@ -1217,6 +1232,7 @@ void intel_guc_submission_fini(struct intel_guc *guc) guc_lrc_desc_pool_destroy(guc); guc_flush_destroyed_contexts(guc); i915_sched_engine_put(guc->sched_engine);

• bitmap_free(guc->submission_state.guc_ids_bitmap); }

  static inline void queue_request(struct i915_sched_engine *sched_engine,

@@ -1268,18 +1284,43 @@ static void guc_submit_request(struct i915_request *rq) spin_unlock_irqrestore(&sched_engine->lock, flags); }

-static int new_guc_id(struct intel_guc *guc) +static int new_guc_id(struct intel_guc *guc, struct intel_context *ce) {

• return ida_simple_get(&guc->submission_state.guc_ids, 0,

• 	      GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL |
  

• 	      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
  

• int ret;

• GEM_BUG_ON(intel_context_is_child(ce));

• if (intel_context_is_parent(ce))

• ret = bitmap_find_free_region(guc->submission_state.guc_ids_bitmap,
  

• 			      NUMBER_MULTI_LRC_GUC_ID,
  

• 			      order_base_2(ce->parallel.number_children
  

• 					   + 1));
  

• else

• ret = ida_simple_get(&guc->submission_state.guc_ids,
  

• 		     NUMBER_MULTI_LRC_GUC_ID,
  

• 		     GUC_MAX_LRC_DESCRIPTORS,
  

• 		     GFP_KERNEL | __GFP_RETRY_MAYFAIL |
  

• 		     __GFP_NOWARN);
  

• if (unlikely(ret < 0))

• return ret;
  

• ce->guc_id.id = ret;

• return 0; }

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

• GEM_BUG_ON(intel_context_is_child(ce));

• if (!context_guc_id_invalid(ce)) {

• ida_simple_remove(&guc->submission_state.guc_ids,
  

• 		  ce->guc_id.id);
  

• if (intel_context_is_parent(ce))
  

• 	bitmap_release_region(guc->submission_state.guc_ids_bitmap,
  

• 			      ce->guc_id.id,
  

• 			      order_base_2(ce->parallel.number_children
  

• 					   + 1));
  

There was a discussion on the previous revision about adding a BUG_ON to ensure that number_children cannot change between the bitmap alloc and the bitmap release. I'm not seeing the new BUG_ON mentioned in this patch.

John.

• else
  

• 	ida_simple_remove(&guc->submission_state.guc_ids,
  

• 			  ce->guc_id.id);
  

  reset_lrc_desc(guc, ce->guc_id.id); set_context_guc_id_invalid(ce); }

@@ -1296,49 +1337,64 @@ static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) spin_unlock_irqrestore(&guc->submission_state.lock, flags); }

-static int steal_guc_id(struct intel_guc *guc) +static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) {

• struct intel_context *ce;
• int guc_id;

• struct intel_context *cn;

  lockdep_assert_held(&guc->submission_state.lock);

• GEM_BUG_ON(intel_context_is_child(ce));

• GEM_BUG_ON(intel_context_is_parent(ce));

  if (!list_empty(&guc->submission_state.guc_id_list)) {

• ce = list_first_entry(&guc->submission_state.guc_id_list,
  

• cn = list_first_entry(&guc->submission_state.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));
  

• GEM_BUG_ON(atomic_read(&cn->guc_id.ref));
  

• GEM_BUG_ON(context_guc_id_invalid(cn));
  

• GEM_BUG_ON(intel_context_is_child(cn));
  

• GEM_BUG_ON(intel_context_is_parent(cn));
  
  

• list_del_init(&ce->guc_id.link);
  

• guc_id = ce->guc_id.id;
  

• list_del_init(&cn->guc_id.link);
  

• ce->guc_id = cn->guc_id;
  
  

  spin_lock(&ce->guc_state.lock);

• clr_context_registered(ce);
  

• clr_context_registered(cn);
  

  spin_unlock(&ce->guc_state.lock);

• set_context_guc_id_invalid(ce);
  

• return guc_id;
  

• set_context_guc_id_invalid(cn);
  

• return 0;
  

  } else { return -EAGAIN; } }

-static int assign_guc_id(struct intel_guc *guc, u16 *out) +static int assign_guc_id(struct intel_guc *guc, struct intel_context *ce) { int ret;

lockdep_assert_held(&guc->submission_state.lock);

• GEM_BUG_ON(intel_context_is_child(ce));

• ret = new_guc_id(guc);

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

• ret = steal_guc_id(guc);
  

• if (intel_context_is_parent(ce))
  

• 	return -ENOSPC;
  

• ret = steal_guc_id(guc, ce);
  

  if (ret < 0) return ret; }

• *out = ret;

• if (intel_context_is_parent(ce)) {

• struct intel_context *child;
  

• int i = 1;
  

• for_each_child(ce, child)
  

• 	child->guc_id.id = ce->guc_id.id + i++;
  

• }
• return 0; }

@@ -1356,7 +1412,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) might_lock(&ce->guc_state.lock);

if (context_guc_id_invalid(ce)) {

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

• ret = assign_guc_id(guc, ce);
  

  if (ret) goto out_unlock; ret = 1; /* Indidcates newly assigned guc_id */

@@ -1398,8 +1454,10 @@ 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);

• GEM_BUG_ON(intel_context_is_child(ce));

• if (unlikely(context_guc_id_invalid(ce)))

• if (unlikely(context_guc_id_invalid(ce) ||

•      intel_context_is_parent(ce)))
  

  return;

  spin_lock_irqsave(&guc->submission_state.lock, flags);

On 10/7/2021 18:21, Matthew Brost wrote:

On Thu, Oct 07, 2021 at 03:03:04PM -0700, John Harrison wrote:

...

On 10/4/2021 15:06, Matthew Brost wrote:

...

Assign contexts in parent-child relationship consecutive guc_ids. This is accomplished by partitioning guc_id space between ones that need to be consecutive (1/16 available guc_ids) and ones that do not (15/16 of available guc_ids). The consecutive search is implemented via the bitmap API.

This is a precursor to the full GuC multi-lrc implementation but aligns to how GuC mutli-lrc interface is defined - guc_ids must be consecutive when using the GuC multi-lrc interface.

v2: (Daniel Vetter) - Explicitly state why we assign consecutive guc_ids v3: (John Harrison) - Bring back in spin lock

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 6 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 104 ++++++++++++++---- 2 files changed, 86 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 25a598e2b6e8..a9f4ec972bfb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -76,9 +76,13 @@ struct intel_guc { */ spinlock_t lock; /**

•  * @guc_ids: used to allocate new guc_ids
  

•  * @guc_ids: used to allocate new guc_ids, single-lrc
   */
  struct ida guc_ids;
  

• /**
  

•  * @guc_ids_bitmap: used to allocate new guc_ids, multi-lrc
  

•  */
  

• unsigned long *guc_ids_bitmap;
  /**
   * @guc_id_list: list of intel_context with valid guc_ids but no
   * refs
  

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 1f2809187513..79e7732e83b2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -128,6 +128,16 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count); #define GUC_REQUEST_SIZE 64 /* bytes */ +/*

• • We reserve 1/16 of the guc_ids for multi-lrc as these need to be contiguous
• • per the GuC submission interface. A different allocation algorithm is used
• • (bitmap vs. ida) between multi-lrc and single-lrc hence the reason to
• • partition the guc_id space. We believe the number of multi-lrc contexts in
• • use should be low and 1/16 should be sufficient. Minimum of 32 guc_ids for
• • multi-lrc.
• */

+#define NUMBER_MULTI_LRC_GUC_ID (GUC_MAX_LRC_DESCRIPTORS / 16)

• /*
  • Below is a set of functions which control the GuC scheduling state which
  • require a lock.

@@ -1206,6 +1216,11 @@ int intel_guc_submission_init(struct intel_guc *guc) INIT_WORK(&guc->submission_state.destroyed_worker, destroyed_worker_func);

• guc->submission_state.guc_ids_bitmap =

• bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID, GFP_KERNEL);
  

• if (!guc->submission_state.guc_ids_bitmap)

• return -ENOMEM;
  

• return 0; }

@@ -1217,6 +1232,7 @@ void intel_guc_submission_fini(struct intel_guc *guc) guc_lrc_desc_pool_destroy(guc); guc_flush_destroyed_contexts(guc); i915_sched_engine_put(guc->sched_engine);

• bitmap_free(guc->submission_state.guc_ids_bitmap); } static inline void queue_request(struct i915_sched_engine *sched_engine,

@@ -1268,18 +1284,43 @@ static void guc_submit_request(struct i915_request *rq) spin_unlock_irqrestore(&sched_engine->lock, flags); } -static int new_guc_id(struct intel_guc *guc) +static int new_guc_id(struct intel_guc *guc, struct intel_context *ce) {

• return ida_simple_get(&guc->submission_state.guc_ids, 0,

• 	      GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL |
  

• 	      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
  

• int ret;
• GEM_BUG_ON(intel_context_is_child(ce));
• if (intel_context_is_parent(ce))

• ret = bitmap_find_free_region(guc->submission_state.guc_ids_bitmap,
  

• 			      NUMBER_MULTI_LRC_GUC_ID,
  

• 			      order_base_2(ce->parallel.number_children
  

• 					   + 1));
  

• else

• ret = ida_simple_get(&guc->submission_state.guc_ids,
  

• 		     NUMBER_MULTI_LRC_GUC_ID,
  

• 		     GUC_MAX_LRC_DESCRIPTORS,
  

• 		     GFP_KERNEL | __GFP_RETRY_MAYFAIL |
  

• 		     __GFP_NOWARN);
  

• if (unlikely(ret < 0))

• return ret;
  

• ce->guc_id.id = ret;
• return 0; } static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce) {
• GEM_BUG_ON(intel_context_is_child(ce));
• if (!context_guc_id_invalid(ce)) {

• ida_simple_remove(&guc->submission_state.guc_ids,
  

• 		  ce->guc_id.id);
  

• if (intel_context_is_parent(ce))
  

• 	bitmap_release_region(guc->submission_state.guc_ids_bitmap,
  

• 			      ce->guc_id.id,
  

• 			      order_base_2(ce->parallel.number_children
  

• 					   + 1));
  

There was a discussion on the previous revision about adding a BUG_ON to ensure that number_children cannot change between the bitmap alloc and the bitmap release. I'm not seeing the new BUG_ON mentioned in this patch.

I thought you meant to add a BUG_ON to ensure before we release a region / id it is occupied? I looked in both the bitmap API and ida API and neither have a function that checks if region / id is occupied so can't really add a BUG_ON for that.

How much you add BUG_ON to ensure the number of children canoot change between alloc and release? I don't follow how that would work.

Matt

I was thinking that where number_children is modified, you have a BUG_ON(guc_id_is_valid). That would ensure that the release has to match the alloc. Hmm, you already have a BUG_ON about the parent/child not being pinned in intel_context_bind_parent_child(), which I guess covers it because you shouldn't have a guc_id if you aren't pinned, right? And that is the only function which can modify number_children, yes? So maybe it's all good?

John.

...

John.

...

• else
  

• 	ida_simple_remove(&guc->submission_state.guc_ids,
  

• 			  ce->guc_id.id);
  reset_lrc_desc(guc, ce->guc_id.id);
  set_context_guc_id_invalid(ce);
  

  }

@@ -1296,49 +1337,64 @@ static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) spin_unlock_irqrestore(&guc->submission_state.lock, flags); } -static int steal_guc_id(struct intel_guc *guc) +static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) {

• struct intel_context *ce;
• int guc_id;

• struct intel_context *cn; lockdep_assert_held(&guc->submission_state.lock);
• GEM_BUG_ON(intel_context_is_child(ce));
• GEM_BUG_ON(intel_context_is_parent(ce)); if (!list_empty(&guc->submission_state.guc_id_list)) {

• ce = list_first_entry(&guc->submission_state.guc_id_list,
  

• cn = list_first_entry(&guc->submission_state.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));
  

• GEM_BUG_ON(atomic_read(&cn->guc_id.ref));
  

• GEM_BUG_ON(context_guc_id_invalid(cn));
  

• GEM_BUG_ON(intel_context_is_child(cn));
  

• GEM_BUG_ON(intel_context_is_parent(cn));
  

• list_del_init(&ce->guc_id.link);
  

• guc_id = ce->guc_id.id;
  

• list_del_init(&cn->guc_id.link);
  

• ce->guc_id = cn->guc_id;
  spin_lock(&ce->guc_state.lock);
  

• clr_context_registered(ce);
  

• clr_context_registered(cn);
  spin_unlock(&ce->guc_state.lock);
  

• set_context_guc_id_invalid(ce);
  

• return guc_id;
  

• set_context_guc_id_invalid(cn);
  

• return 0;
  

  } else { return -EAGAIN; } }

-static int assign_guc_id(struct intel_guc *guc, u16 *out) +static int assign_guc_id(struct intel_guc *guc, struct intel_context *ce) { int ret; lockdep_assert_held(&guc->submission_state.lock);

• GEM_BUG_ON(intel_context_is_child(ce));

• ret = new_guc_id(guc);

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

• ret = steal_guc_id(guc);
  

• if (intel_context_is_parent(ce))
  

• 	return -ENOSPC;
  

• ret = steal_guc_id(guc, ce);
  if (ret < 0)
  	return ret;
  

  }

• *out = ret;

• if (intel_context_is_parent(ce)) {

• struct intel_context *child;
  

• int i = 1;
  

• for_each_child(ce, child)
  

• 	child->guc_id.id = ce->guc_id.id + i++;
  

• }
• return 0; }

@@ -1356,7 +1412,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) might_lock(&ce->guc_state.lock); if (context_guc_id_invalid(ce)) {

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

• ret = assign_guc_id(guc, ce);
  if (ret)
  	goto out_unlock;
  ret = 1;	/* Indidcates newly assigned guc_id */
  

@@ -1398,8 +1454,10 @@ 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);

• GEM_BUG_ON(intel_context_is_child(ce));

• if (unlikely(context_guc_id_invalid(ce)))

• if (unlikely(context_guc_id_invalid(ce) ||

•      intel_context_is_parent(ce)))
  return;
  

  spin_lock_irqsave(&guc->submission_state.lock, flags);

On 10/13/2021 11:03, Matthew Brost wrote:

On Fri, Oct 08, 2021 at 09:40:43AM -0700, John Harrison wrote:

...

On 10/7/2021 18:21, Matthew Brost wrote:

...

On Thu, Oct 07, 2021 at 03:03:04PM -0700, John Harrison wrote:

...

On 10/4/2021 15:06, Matthew Brost wrote:

...

Assign contexts in parent-child relationship consecutive guc_ids. This is accomplished by partitioning guc_id space between ones that need to be consecutive (1/16 available guc_ids) and ones that do not (15/16 of available guc_ids). The consecutive search is implemented via the bitmap API.

This is a precursor to the full GuC multi-lrc implementation but aligns to how GuC mutli-lrc interface is defined - guc_ids must be consecutive when using the GuC multi-lrc interface.

v2: (Daniel Vetter) - Explicitly state why we assign consecutive guc_ids v3: (John Harrison) - Bring back in spin lock

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   6 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 104 ++++++++++++++----
2 files changed, 86 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 25a598e2b6e8..a9f4ec972bfb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -76,9 +76,13 @@ struct intel_guc { */ spinlock_t lock; /**

•  * @guc_ids: used to allocate new guc_ids
  

•  * @guc_ids: used to allocate new guc_ids, single-lrc
  	 */
  	struct ida guc_ids;
  

• /**
  

•  * @guc_ids_bitmap: used to allocate new guc_ids, multi-lrc
  

•  */
  

• unsigned long *guc_ids_bitmap;
  	/**
  	 * @guc_id_list: list of intel_context with valid guc_ids but no
  	 * refs
  

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 1f2809187513..79e7732e83b2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -128,6 +128,16 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count); #define GUC_REQUEST_SIZE 64 /* bytes */ +/*

• • We reserve 1/16 of the guc_ids for multi-lrc as these need to be contiguous
• • per the GuC submission interface. A different allocation algorithm is used
• • (bitmap vs. ida) between multi-lrc and single-lrc hence the reason to
• • partition the guc_id space. We believe the number of multi-lrc contexts in
• • use should be low and 1/16 should be sufficient. Minimum of 32 guc_ids for
• • multi-lrc.
• */

+#define NUMBER_MULTI_LRC_GUC_ID (GUC_MAX_LRC_DESCRIPTORS / 16)

• /*
  • Below is a set of functions which control the GuC scheduling state which
  • require a lock.

@@ -1206,6 +1216,11 @@ int intel_guc_submission_init(struct intel_guc *guc) INIT_WORK(&guc->submission_state.destroyed_worker, destroyed_worker_func);

• guc->submission_state.guc_ids_bitmap =

• bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID, GFP_KERNEL);
  

• if (!guc->submission_state.guc_ids_bitmap)

• return -ENOMEM;
  

• return 0;
  

  }

@@ -1217,6 +1232,7 @@ void intel_guc_submission_fini(struct intel_guc *guc) guc_lrc_desc_pool_destroy(guc); guc_flush_destroyed_contexts(guc); i915_sched_engine_put(guc->sched_engine);

• bitmap_free(guc->submission_state.guc_ids_bitmap); } static inline void queue_request(struct i915_sched_engine *sched_engine,

@@ -1268,18 +1284,43 @@ static void guc_submit_request(struct i915_request *rq) spin_unlock_irqrestore(&sched_engine->lock, flags); } -static int new_guc_id(struct intel_guc *guc) +static int new_guc_id(struct intel_guc *guc, struct intel_context *ce) {

• return ida_simple_get(&guc->submission_state.guc_ids, 0,

• 	      GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL |
  

• 	      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
  

• int ret;
• GEM_BUG_ON(intel_context_is_child(ce));
• if (intel_context_is_parent(ce))

• ret = bitmap_find_free_region(guc->submission_state.guc_ids_bitmap,
  

• 			      NUMBER_MULTI_LRC_GUC_ID,
  

• 			      order_base_2(ce->parallel.number_children
  

• 					   + 1));
  

• else

• ret = ida_simple_get(&guc->submission_state.guc_ids,
  

• 		     NUMBER_MULTI_LRC_GUC_ID,
  

• 		     GUC_MAX_LRC_DESCRIPTORS,
  

• 		     GFP_KERNEL | __GFP_RETRY_MAYFAIL |
  

• 		     __GFP_NOWARN);
  

• if (unlikely(ret < 0))

• return ret;
  

• ce->guc_id.id = ret;
• return 0; } static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce) {
• GEM_BUG_ON(intel_context_is_child(ce));

• if (!context_guc_id_invalid(ce)) {
  

• ida_simple_remove(&guc->submission_state.guc_ids,
  

• 		  ce->guc_id.id);
  

• if (intel_context_is_parent(ce))
  

• 	bitmap_release_region(guc->submission_state.guc_ids_bitmap,
  

• 			      ce->guc_id.id,
  

• 			      order_base_2(ce->parallel.number_children
  

• 					   + 1));
  

There was a discussion on the previous revision about adding a BUG_ON to ensure that number_children cannot change between the bitmap alloc and the bitmap release. I'm not seeing the new BUG_ON mentioned in this patch.

I thought you meant to add a BUG_ON to ensure before we release a region / id it is occupied? I looked in both the bitmap API and ida API and neither have a function that checks if region / id is occupied so can't really add a BUG_ON for that.

How much you add BUG_ON to ensure the number of children canoot change between alloc and release? I don't follow how that would work.

Matt

I was thinking that where number_children is modified, you have a BUG_ON(guc_id_is_valid). That would ensure that the release has to match the alloc. Hmm, you already have a BUG_ON about the parent/child not being pinned in intel_context_bind_parent_child(), which I guess covers it because you shouldn't have a guc_id if you aren't pinned, right? And that is the only function which can modify number_children, yes? So maybe it's all good?

I think we are all good.

We are all awesome ;)

Reviewed-by: John Harrison John.C.Harrison@Intel.com

Matt

...

John.

...

...

John.

...

• else
  

• 	ida_simple_remove(&guc->submission_state.guc_ids,
  

• 			  ce->guc_id.id);
  	reset_lrc_desc(guc, ce->guc_id.id);
  	set_context_guc_id_invalid(ce);
  }
  

@@ -1296,49 +1337,64 @@ static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) spin_unlock_irqrestore(&guc->submission_state.lock, flags); } -static int steal_guc_id(struct intel_guc *guc) +static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) {

• struct intel_context *ce;
• int guc_id;

• struct intel_context *cn; lockdep_assert_held(&guc->submission_state.lock);
• GEM_BUG_ON(intel_context_is_child(ce));
• GEM_BUG_ON(intel_context_is_parent(ce)); if (!list_empty(&guc->submission_state.guc_id_list)) {

• ce = list_first_entry(&guc->submission_state.guc_id_list,
  

• cn = list_first_entry(&guc->submission_state.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));
  

• GEM_BUG_ON(atomic_read(&cn->guc_id.ref));
  

• GEM_BUG_ON(context_guc_id_invalid(cn));
  

• GEM_BUG_ON(intel_context_is_child(cn));
  

• GEM_BUG_ON(intel_context_is_parent(cn));
  

• list_del_init(&ce->guc_id.link);
  

• guc_id = ce->guc_id.id;
  

• list_del_init(&cn->guc_id.link);
  

• ce->guc_id = cn->guc_id;
  	spin_lock(&ce->guc_state.lock);
  

• clr_context_registered(ce);
  

• clr_context_registered(cn);
  	spin_unlock(&ce->guc_state.lock);
  

• set_context_guc_id_invalid(ce);
  

• return guc_id;
  

• set_context_guc_id_invalid(cn);
  

• return 0;
  } else {
  	return -EAGAIN;
  }
  

  }

-static int assign_guc_id(struct intel_guc *guc, u16 *out) +static int assign_guc_id(struct intel_guc *guc, struct intel_context *ce) { int ret; lockdep_assert_held(&guc->submission_state.lock);

• GEM_BUG_ON(intel_context_is_child(ce));

• ret = new_guc_id(guc);

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

• ret = steal_guc_id(guc);
  

• if (intel_context_is_parent(ce))
  

• 	return -ENOSPC;
  

• ret = steal_guc_id(guc, ce);
  	if (ret < 0)
  		return ret;
  }
  

• *out = ret;

• if (intel_context_is_parent(ce)) {

• struct intel_context *child;
  

• int i = 1;
  

• for_each_child(ce, child)
  

• 	child->guc_id.id = ce->guc_id.id + i++;
  

• }

• return 0;
  

  }

@@ -1356,7 +1412,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) might_lock(&ce->guc_state.lock); if (context_guc_id_invalid(ce)) {

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

• ret = assign_guc_id(guc, ce);
  	if (ret)
  		goto out_unlock;
  	ret = 1;	/* Indidcates newly assigned guc_id */
  

@@ -1398,8 +1454,10 @@ 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);

• GEM_BUG_ON(intel_context_is_child(ce));

• if (unlikely(context_guc_id_invalid(ce)))

• if (unlikely(context_guc_id_invalid(ce) ||

•      intel_context_is_parent(ce)))
  	return;
  spin_lock_irqsave(&guc->submission_state.lock, flags);