[Intel-gfx] [PATCH v14 3/7] drm/i915: Define and use GuC and CTB TLB invalidation routines

[Andi Shyti]

Hi John,

...

> > @@ -560,6 +562,17 @@ static int __uc_init_hw(struct intel_uc *uc)
> >   	guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc)));
> >   	guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc)));
> > +	/*
> > +	 * The full GT reset will have cleared the TLB caches and flushed the
> > +	 * G2H message queue; we can release all the blocked waiters.
> > +	 */
> > +	if (intel_guc_tlb_invalidation_is_available(guc)) {
> > +		xa_lock_irq(&guc->tlb_lookup);
> > +		xa_for_each(&guc->tlb_lookup, i, wait)
> > +			wake_up(&wait->wq);
> > +		xa_unlock_irq(&guc->tlb_lookup);
> > +	}
> > +
> This is not the right place for this. This is an init function but this
> comment is talking about resets and is doing things that assume the system
> has been running for some time.
> 
> Yes, hw init might happen as part of a reset but this is reset only
> processing and it should be in a reset specific code path.
> 
> What was wrong with the previous version that had the code in
> intel_guc_submission_reset?

It was wrongly placed there because at that point the gt reset
was not totally complete but it was still mid-way through.

The threads needed a bit more time in order to wait for the GT to
be completely alive after the reset.

The works need to be woken up at the end of the gt reset, where,
besides, we need to clear up the xa_array with work queues.

First Jonathan added it in driectly at the end of the
intel_gt_reset(), but that's not the right place as this is a UC
related operation, rather than GT.

Following the reset flow this looked like the right place, called
after the reset in the UC part.

What's wrong with placing it it here?

Andi