From: Matthew Brost <matthew.brost@intel.com>
To: John Harrison <john.c.harrison@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
daniele.ceraolospurio@intel.com
Subject: Re: [Intel-gfx] [PATCH 01/26] drm/i915/guc: Move GuC guc_id allocation under submission state sub-struct
Date: Thu, 7 Oct 2021 08:05:43 -0700 [thread overview]
Message-ID: <20211007150543.GA12560@jons-linux-dev-box> (raw)
In-Reply-To: <019f1cca-a971-f9ee-4eec-6450572ae580@intel.com>
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
> 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);
>
next prev parent reply other threads:[~2021-10-07 15:10 UTC|newest]
Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-04 22:06 [Intel-gfx] [PATCH 00/26] Parallel submission aka multi-bb execbuf Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 01/26] drm/i915/guc: Move GuC guc_id allocation under submission state sub-struct Matthew Brost
2021-10-07 3:06 ` John Harrison
2021-10-07 15:05 ` Matthew Brost [this message]
2021-10-07 18:13 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 02/26] drm/i915/guc: Take GT PM ref when deregistering context Matthew Brost
2021-10-07 3:37 ` John Harrison
2021-10-08 1:28 ` Matthew Brost
2021-10-08 18:23 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 03/26] drm/i915/guc: Take engine PM when a context is pinned with GuC submission Matthew Brost
2021-10-07 3:45 ` John Harrison
2021-10-07 15:19 ` Matthew Brost
2021-10-07 18:15 ` John Harrison
2021-10-08 1:23 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 04/26] drm/i915/guc: Don't call switch_to_kernel_context " Matthew Brost
2021-10-07 3:49 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 05/26] drm/i915: Add logical engine mapping Matthew Brost
2021-10-07 19:03 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 06/26] drm/i915: Expose logical engine instance to user Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 07/26] drm/i915/guc: Introduce context parent-child relationship Matthew Brost
2021-10-07 19:35 ` John Harrison
2021-10-08 18:33 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 08/26] drm/i915/guc: Add multi-lrc context registration Matthew Brost
2021-10-07 19:50 ` John Harrison
2021-10-08 1:31 ` Matthew Brost
2021-10-08 17:20 ` John Harrison
2021-10-08 17:29 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 09/26] drm/i915/guc: Ensure GuC schedule operations do not operate on child contexts Matthew Brost
2021-10-07 20:23 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 10/26] drm/i915/guc: Assign contexts in parent-child relationship consecutive guc_ids Matthew Brost
2021-10-07 22:03 ` John Harrison
2021-10-08 1:21 ` Matthew Brost
2021-10-08 16:40 ` John Harrison
2021-10-13 18:03 ` Matthew Brost
2021-10-13 19:11 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 11/26] drm/i915/guc: Implement parallel context pin / unpin functions Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 12/26] drm/i915/guc: Implement multi-lrc submission Matthew Brost
2021-10-05 7:55 ` kernel test robot
2021-10-05 10:37 ` kernel test robot
2021-10-08 17:20 ` John Harrison
2021-10-13 18:24 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 13/26] drm/i915/guc: Insert submit fences between requests in parent-child relationship Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 14/26] drm/i915/guc: Implement multi-lrc reset Matthew Brost
2021-10-08 17:39 ` John Harrison
2021-10-08 17:56 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 15/26] drm/i915/guc: Update debugfs for GuC multi-lrc Matthew Brost
2021-10-08 17:46 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 16/26] drm/i915: Fix bug in user proto-context creation that leaked contexts Matthew Brost
2021-10-08 17:49 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 17/26] drm/i915/guc: Connect UAPI to GuC multi-lrc interface Matthew Brost
2021-10-11 22:09 ` John Harrison
2021-10-11 22:59 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 18/26] drm/i915/doc: Update parallel submit doc to point to i915_drm.h Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 19/26] drm/i915/guc: Add basic GuC multi-lrc selftest Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 20/26] drm/i915/guc: Implement no mid batch preemption for multi-lrc Matthew Brost
2021-10-11 23:32 ` John Harrison
2021-10-13 1:52 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 21/26] drm/i915: Multi-BB execbuf Matthew Brost
2021-10-05 8:31 ` kernel test robot
2021-10-05 17:02 ` Matthew Brost
2021-10-06 20:46 ` Matthew Brost
2021-10-12 21:22 ` John Harrison
2021-10-13 0:37 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 22/26] drm/i915/guc: Handle errors in multi-lrc requests Matthew Brost
2021-10-12 21:56 ` John Harrison
2021-10-13 0:18 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 23/26] drm/i915: Make request conflict tracking understand parallel submits Matthew Brost
2021-10-12 22:08 ` John Harrison
2021-10-13 0:32 ` Matthew Brost
2021-10-13 19:35 ` John Harrison
2021-10-13 17:51 ` Matthew Brost
2021-10-13 19:25 ` John Harrison
2021-10-04 22:06 ` [Intel-gfx] [PATCH 24/26] drm/i915: Update I915_GEM_BUSY IOCTL to understand composite fences Matthew Brost
2021-10-11 22:15 ` Daniele Ceraolo Spurio
2021-10-12 7:53 ` Tvrtko Ursulin
2021-10-12 18:31 ` Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 25/26] drm/i915: Enable multi-bb execbuf Matthew Brost
2021-10-04 22:06 ` [Intel-gfx] [PATCH 26/26] drm/i915/execlists: Weak parallel submission support for execlists Matthew Brost
2021-10-04 22:21 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Parallel submission aka multi-bb execbuf (rev4) Patchwork
2021-10-12 22:15 ` John Harrison
2021-10-13 0:15 ` Matthew Brost
2021-10-13 19:24 ` John Harrison
2021-10-04 22:23 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-10-04 22:26 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2021-10-12 22:15 ` John Harrison
2021-10-13 0:12 ` Matthew Brost
2021-10-04 22:54 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-10-05 1:49 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Parallel submission aka multi-bb execbuf (rev5) Patchwork
2021-10-05 1:51 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-10-05 1:54 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2021-10-05 2:21 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-10-12 18:11 ` [Intel-gfx] [PATCH 02/26] drm/i915/guc: Take GT PM ref when deregistering context Matthew Brost
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211007150543.GA12560@jons-linux-dev-box \
--to=matthew.brost@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=john.c.harrison@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox