Hi,
...
> > @@ -131,11 +132,23 @@ void intel_gt_invalidate_tlb_full(struct intel_gt *gt, u32 seqno)
> > return;
> > with_intel_gt_pm_if_awake(gt, wakeref) {
> > + struct intel_guc *guc = >->uc.guc;
> > +
> > mutex_lock(>->tlb.invalidate_lock);
> > if (tlb_seqno_passed(gt, seqno))
> > goto unlock;
> > - mmio_invalidate_full(gt);
> > + if (HAS_GUC_TLB_INVALIDATION(gt->i915)) {
> > + /*
> > + * Only perform GuC TLB invalidation if GuC is ready.
> > + * If GuC is not ready, then there are no TLBs to
> > + * invalidate. Ergo, skip invalidation.
> > + */
> > + if (intel_guc_is_ready(guc))
> > + intel_guc_invalidate_tlb_engines(guc);
>
> What was the answer to John's question on why it is okay to just skip and
> not maybe fall back to mmio?
this maybe can be written as:
if (HAS_GUC_TLB_INVALIDATION(gt->i915) &&
intel_guc_is_ready(guc))
intel_guc_invalidate_tlb_engines(guc);
else
mmio_invalidate_full(gt);
> > + } else {
> > + mmio_invalidate_full(gt);
> > + }
> > write_seqcount_invalidate(>->tlb.seqno);
> > unlock:
...
> > + /*
> > + * The full GT reset will have cleared the TLB caches and flushed the
> > + * G2H message queue; we can release all the blocked waiters.
> > + *
> > + * This is safe to do unlocked because the xarray is not dependent
> > + * on the GT reset, and there's a separate execution path for TLB
> > + * invalidations on GT reset, and there's a large window of time
> > + * between the GT reset and GuC becoming available.
> > + */
> > + xa_for_each(&guc->tlb_lookup, i, wait)
> > + wake_up(&wait->wq);
>
> If you are confident there can be no failures to wake up someone, who maybe
> just added themselves to the xarray (via put pages for instance), while
> reset in ongoing. Or even removed themselves after say timing out the wait
> and so freed their entry...
I guess yuo are suggesting here to spinlock around this. The
reset is protected by the uncore->spinlock, but I don't really
see it colliding with reset, to be honest.
> > }
> > static void guc_cancel_context_requests(struct intel_context *ce)
> > @@ -1948,6 +1962,50 @@ void intel_guc_submission_reset_finish(struct intel_guc *guc)
> > static void destroyed_worker_func(struct work_struct *w);
> > static void reset_fail_worker_func(struct work_struct *w);
> > +static int init_tlb_lookup(struct intel_guc *guc)
> > +{
> > + struct intel_guc_tlb_wait *wait;
> > + int err;
> > +
> > + if (!HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915))
> > + return 0;
> > +
> > + xa_init_flags(&guc->tlb_lookup, XA_FLAGS_ALLOC);
> > +
> > + wait = kzalloc(sizeof(*wait), GFP_KERNEL);
> > + if (!wait)
> > + return -ENOMEM;
> > +
> > + init_waitqueue_head(&wait->wq);
> > +
> > + /* Preallocate a shared id for use under memory pressure. */
> > + err = xa_alloc_cyclic_irq(&guc->tlb_lookup, &guc->serial_slot, wait,
> > + xa_limit_32b, &guc->next_seqno, GFP_KERNEL);
> > + /* Only error if out of memory, not when busy (list full)*/
> > + if (err == -ENOMEM) {
> > + kfree(wait);
> > + return err;
> > + }
>
> I agreed with John here that only looking at ENOMEM reads odd and I did not
> see that answered. Did I miss it?
xa_alloc_cyclic_irq() can also fail with -EBUSY... so that I
think this is a matter...
> Otherwise, I _know_ it is not likely to get any other error having *just*
> created a new xarray, but still, why not simply catch all? It is not like
> the slot fallback code at runtime would handle guc->serial_slot being
> empty?! It appears it would just explode in guc_send_invalidate_tlb if it
> would hit it..
... of what we want to do for such errors. E.g. Jonathan decided
here not to fail, but ignore the error. Should we fail for every
error?
> > +
> > + return 0;
> > +}
> > +
> > +static void fini_tlb_lookup(struct intel_guc *guc)
> > +{
> > + struct intel_guc_tlb_wait *wait;
> > +
> > + if (!HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915))
> > + return;
> > +
> > + wait = xa_load(&guc->tlb_lookup, guc->serial_slot);
> > + if (wait) {
> > + guc_dbg(guc, "fini_tlb_lookup: Unexpected item in tlb_lookup\n");
>
> Hm wait, why is this unexpected when init_tlb_lookup() pre-allocated that
> entry? Who frees it? guc_send_invalidate_tlb() does not appear to.
I think this links with my answer above, right? With th
refactoring of the if's for not skipping tlb invalidation.
> > + kfree(wait);
> > + }
> > +
> > + xa_destroy(&guc->tlb_lookup);
> > +}
> > +
> > /*
> > * Set up the memory resources to be shared with the GuC (via the GGTT)
> > * at firmware loading time.
...
> > +int intel_guc_tlb_invalidation_done(struct intel_guc *guc, u32 size, u32 len, u32 seqno)
> > +{
> > + /* Check for underflow */
> > + if (unlikely(len < 1 || len > size))
> > + return -EPROTO;
>
> These check are not valid for any message/action type ct_process_request()
> can receive?
You mean discriminating by payload? Jonathan... you konw the
details here?
