[Intel-gfx] [PATCH v10 4/7] drm/i915: No TLB invalidation on suspended GT

[Cavitt, Jonathan]

-----Original Message-----
From: Harrison, John C <john.c.harrison at intel.com> 
Sent: Tuesday, October 10, 2023 2:57 PM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; chris.p.wilson at linux.intel.com; Iddamsetty, Aravind <aravind.iddamsetty at intel.com>; Yang, Fei <fei.yang at intel.com>; Shyti, Andi <andi.shyti at intel.com>; Das, Nirmoy <nirmoy.das at intel.com>; Krzysztofik, Janusz <janusz.krzysztofik at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; tvrtko.ursulin at linux.intel.com; jani.nikula at linux.intel.com
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 at intel.com>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > CC: John Harrison <john.c.harrison at intel.com>
> > ---
> >   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;
> >   }
> >   
> 
>