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

[Andi Shyti]

Hi John,

On Thu, Oct 12, 2023 at 06:08:11PM -0700, John Harrison wrote:
> On 10/12/2023 15:38, 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>
> > Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
> > Acked-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Acked-by: Nirmoy Das <nirmoy.das at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  1 +
> >   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 22 ++++++++++++-------
> >   drivers/gpu/drm/i915/gt/uc/intel_uc.c         |  7 ++++++
> >   3 files changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 0949628d69f8b..2b6dfe62c8f2a 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -537,4 +537,5 @@ int intel_guc_invalidate_tlb_engines(struct intel_guc *guc);
> >   int intel_guc_invalidate_tlb_guc(struct intel_guc *guc);
> >   int intel_guc_tlb_invalidation_done(struct intel_guc *guc,
> >   				    const u32 *payload, u32 len);
> > +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 1377398afcdfa..3a0d20064878a 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,24 @@ 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)) {
> Why the change from 'if(!is_available) return' to 'if(HAS_) {doStuff}'?

I believe the inversion of the logic is purely for aesthetic (I
preferred it how it was before so to save some level of
indentation, but this is pure taste).

While as for the statement, this was changed because at the
moment this function is called after reset, not all the
components have completed their reset. E.g., the GT is still in
the middle of it.

Another way to improve this is to call
wake_up_all_tlb_invalidate() at the end of gt_reset() in order to
clear up the queue of requests.

Andi