-----Original Message-----
From: Harrison, John C <john.c.harrison@xxxxxxxxx>
Sent: Tuesday, October 10, 2023 2:57 PM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; chris.p.wilson@xxxxxxxxxxxxxxx; Iddamsetty, Aravind <aravind.iddamsetty@xxxxxxxxx>; Yang, Fei <fei.yang@xxxxxxxxx>; Shyti, Andi <andi.shyti@xxxxxxxxx>; Das, Nirmoy <nirmoy.das@xxxxxxxxx>; Krzysztofik, Janusz <janusz.krzysztofik@xxxxxxxxx>; Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; tvrtko.ursulin@xxxxxxxxxxxxxxx; jani.nikula@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v10 4/7] drm/i915: No TLB invalidation on suspended GT
>
> On 10/10/2023 08:02, Jonathan Cavitt wrote:
> > In case of GT is suspended, don't allow submission of new TLB invalidation
> > request and cancel all pending requests. The TLB entries will be
> > invalidated either during GuC reload or on system resume.
> >
> > Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > CC: John Harrison <john.c.harrison@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 +
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 21 +++++++++++++------
> > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 +++++++
> > 3 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 06c44f5c28776..ff7e7b90fd49b 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -536,4 +536,5 @@ void intel_guc_dump_time_info(struct intel_guc *guc, struct drm_printer *p);
> >
> > int intel_guc_sched_disable_gucid_threshold_max(struct intel_guc *guc);
> >
> > +void wake_up_all_tlb_invalidate(struct intel_guc *guc);
> > #endif
> > 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 e9854652c2b52..b9c168ea57270 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -1796,13 +1796,25 @@ static void __guc_reset_context(struct intel_context *ce, intel_engine_mask_t st
> > intel_context_put(parent);
> > }
> >
> > -void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled)
> > +void wake_up_all_tlb_invalidate(struct intel_guc *guc)
> > {
> > struct intel_guc_tlb_wait *wait;
> > + unsigned long i;
> > +
> > + if (!HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915))
> > + return;
> > +
> > + xa_lock_irq(&guc->tlb_lookup);
> > + xa_for_each(&guc->tlb_lookup, i, wait)
> > + wake_up(&wait->wq);
> > + xa_unlock_irq(&guc->tlb_lookup);
> > +}
> > +
> > +void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled)
> What is changed on this line? Or is it just diff being confused and
> seeing the move of the 'wait' declaration as being the anchor point
> rather than the function declaration?
It's the latter, yes. Diff is confused.
-Jonathan Cavitt
>
> John.
>
>
> > +{
> > struct intel_context *ce;
> > unsigned long index;
> > unsigned long flags;
> > - unsigned long i;
> >
> > if (unlikely(!guc_submission_initialized(guc))) {
> > /* Reset called during driver load? GuC not yet initialised! */
> > @@ -1833,10 +1845,7 @@ void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stall
> > * The full GT reset will have cleared the TLB caches and flushed the
> > * G2H message queue; we can release all the blocked waiters.
> > */
> > - xa_lock_irq(&guc->tlb_lookup);
> > - xa_for_each(&guc->tlb_lookup, i, wait)
> > - wake_up(&wait->wq);
> > - xa_unlock_irq(&guc->tlb_lookup);
> > + wake_up_all_tlb_invalidate(guc);
> > }
> >
> > static void guc_cancel_context_requests(struct intel_context *ce)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > index 98b103375b7ab..750cb63503dd7 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > @@ -688,6 +688,8 @@ void intel_uc_suspend(struct intel_uc *uc)
> > /* flush the GSC worker */
> > intel_gsc_uc_flush_work(&uc->gsc);
> >
> > + wake_up_all_tlb_invalidate(guc);
> > +
> > if (!intel_guc_is_ready(guc)) {
> > guc->interrupts.enabled = false;
> > return;
> > @@ -736,6 +738,11 @@ static int __uc_resume(struct intel_uc *uc, bool enable_communication)
> >
> > intel_gsc_uc_resume(&uc->gsc);
> >
> > + if (HAS_GUC_TLB_INVALIDATION(gt->i915)) {
> > + intel_guc_invalidate_tlb_engines(guc);
> > + intel_guc_invalidate_tlb_guc(guc);
> > + }
> > +
> > return 0;
> > }
> >
>
>
