-----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; > > } > > > >