by Tvrtko Ursulin

[permalink] [raw]

Subject: Re: [Intel-gfx] [PATCH v2 06/21] drm/i915/gt: Batch TLB invalidations

On 28/07/2022 07:32, Mauro Carvalho Chehab wrote:
> On Wed, 27 Jul 2022 13:56:50 +0100
> Tvrtko Ursulin <[email protected]> wrote:
>
>>> Because vma_invalidate_tlb() basically stores a TLB seqno, but the
>>> actual invalidation is deferred to when the pages are unset, at
>>> __i915_gem_object_unset_pages().
>>>
>>> So, what happens is:
>>>
>>> - on VMA sync mode, the need to invalidate TLB is marked at
>>> __vma_put_pages(), before VMA unbind;
>>> - on async, this is deferred to happen at ppgtt_unbind_vma(), where
>>> it marks the need to invalidate TLBs.
>>>
>>> On both cases, __i915_gem_object_unset_pages() is called later,
>>> when the driver is ready to unmap the page.
>>
>> Sorry still not clear to me why is the patch moving marking of the need
>> to invalidate (regardless if it a bit like today, or a seqno like in
>> this patch) from bind to unbind?
>>
>> What if the seqno was stored in i915_vma_bind, where the bit is set
>> today, and all the hunks which touch the unbind and evict would
>> disappear from the patch. What wouldn't work in that case, if anything?
>
> Ah, now I see your point.
>
> I can't see any sense on having a sequence number at VMA bind, as the
> unbind order can be different. The need of doing a full TLB invalidation
> or not depends on the unbind order.

Sorry yes that was stupid from me.. What I was really thinking was the
approach I initially used for coalescing. Keeping the set_bit in bind
and then once the code enters intel_gt_invalidate_tlbs, takes a "ticket"
and waits on the mutex. Once it gets the mutex checks the ticket against
the GT copy and if two invalidations have passed since it was waiting on
the mutex it can immediately exit. That would seem like a minimal
improvement to batch things up.

But I guess it would still emit needless invalidations if there is no
contention, just a stream of serialized put pages. While the approach
from this patch can skip all but truly required.

Okay, go for it and thanks for the explanations.

Acked-by: Tvrtko Ursulin <[email protected]>

Regards,

Tvrtko

P.S. The last remaining "ugliness" is the 2nd call to invalidation from
evict. It would be nicer if there was a single common place to do it on
vma unbind but okay, I do not plan to dig into it so fine.

>
> The way the current algorithm works is that drm_i915_gem_object can be
> created on any order, and, at unbind/evict, they receive a seqno.
>
> The seqno is incremented at intel_gt_invalidate_tlb():
>
> void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
> {
> with_intel_gt_pm_if_awake(gt, wakeref) {
> mutex_lock(&gt->tlb.invalidate_lock);
> if (tlb_seqno_passed(gt, seqno))
> goto unlock;
>
> mmio_invalidate_full(gt);
>
> write_seqcount_invalidate(&gt->tlb.seqno); // increment seqno
>
>
> So, let's say 3 objects were created, on this order:
>
> obj1
> obj2
> obj3
>
> They would be unbind/evict on a different order. On that time,
> the mm.tlb will be stamped with a seqno, using the number from the
> last TLB flush, plus 1.
>
> As different threads can be used to handle TLB flushes, let's imagine
> two threads (just for the sake of having an example). On such case,
> what we would have is:
>
> seqno Thread 0 Thread 1
>
> seqno=2 unbind/evict event
> obj3.mm.tlb = seqno | 1
> seqno=2 unbind/evict event
> obj1.mm.tlb = seqno | 1
> __i915_gem_object_unset_pages()
> called for obj3, TLB flush happened,
> invalidating both obj1 and obj2.
> seqno += 2
> seqno=4 unbind/evict event
> obj1.mm.tlb = seqno | 1
> __i915_gem_object_unset_pages()
> called for obj1, don't flush.
> ...
> __i915_gem_object_unset_pages() called for obj2, TLB flush happened
> seqno += 2
> seqno=6
>
> So, basically the seqno is used to track when the object data stopped
> being updated, because of an unbind/evict event, being later used by
> intel_gt_invalidate_tlb() when called from __i915_gem_object_unset_pages(),
> in order to check if a previous invalidation call was enough to invalidate
> the object, or if a new call is needed.
>
> Now, if seqno is stored at bind, data can still leak, as the assumption
> made by intel_gt_invalidate_tlb() that the data stopped being used at
> seqno is not true anymore.
>
> Still, I agree that this logic is complex and should be better
> documented. So, if you're now OK with this patch, I'll add the above
> explanation inside a kernel-doc comment.
>
> Regards,
> Mauro

2022-08-02 07:54:29

by Mauro Carvalho Chehab

[permalink] [raw]

Subject: Re: [Intel-gfx] [PATCH v2 09/21] drm/i915/guc: Define CTB based TLB invalidation routines

On Thu, 14 Jul 2022 16:06:28 +0200
Michal Wajdeczko <[email protected]> wrote:

> On 14.07.2022 14:06, Mauro Carvalho Chehab wrote:
> > From: Prathap Kumar Valsan <[email protected]>
> >
> > Add routines to interface with GuC firmware for TLB invalidation.
> >
> > Signed-off-by: Prathap Kumar Valsan <[email protected]>
> > Cc: Bruce Chang <[email protected]>
> > Cc: Michal Wajdeczko <[email protected]>
> > Cc: Matthew Brost <[email protected]>
> > Cc: Chris Wilson <[email protected]>
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > ---
> >
> > To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> > See [PATCH v2 00/21] at: https://lore.kernel.org/all/[email protected]/
> >
> > .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 35 +++++++
> > drivers/gpu/drm/i915/gt/uc/intel_guc.c | 90 ++++++++++++++++++
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 13 +++
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 24 ++++-
> > drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 6 ++
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++++++++++++++++++-
> > 6 files changed, 253 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
> > index 4ef9990ed7f8..2e39d8df4c82 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
> > @@ -134,6 +134,10 @@ enum intel_guc_action {
> > INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC = 0x4601,
> > INTEL_GUC_ACTION_CLIENT_SOFT_RESET = 0x5507,
> > INTEL_GUC_ACTION_SET_ENG_UTIL_BUFF = 0x550A,
> > + INTEL_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR = 0x6000,
>
> should this be part of this patch ?

No, I'll drop...
>
> > + INTEL_GUC_ACTION_PAGE_FAULT_NOTIFICATION = 0x6001,

... and also drop this one.

> > + INTEL_GUC_ACTION_TLB_INVALIDATION = 0x7000,
> > + INTEL_GUC_ACTION_TLB_INVALIDATION_DONE = 0x7001,
>
> can we document layout of these actions ?

Where should we document it? At intel_guc_invalidate_tlb_guc()
function & friends, or are you thinking on something else, at this
header file?

>
> > INTEL_GUC_ACTION_STATE_CAPTURE_NOTIFICATION = 0x8002,
> > INTEL_GUC_ACTION_NOTIFY_FLUSH_LOG_BUFFER_TO_FILE = 0x8003,
> > INTEL_GUC_ACTION_NOTIFY_CRASH_DUMP_POSTED = 0x8004,
> > @@ -177,4 +181,35 @@ enum intel_guc_state_capture_event_status {
> >
> > #define INTEL_GUC_STATE_CAPTURE_EVENT_STATUS_MASK 0x000000FF
> >
> > +#define INTEL_GUC_TLB_INVAL_TYPE_SHIFT 0
> > +#define INTEL_GUC_TLB_INVAL_MODE_SHIFT 8
>
> can we stop using SHIFT-based definitions and start using MASK-based
> instead ? then we will be able to use FIELD_PREP/GET like we do for i915_reg

Ok.

>
> > +/* Flush PPC or SMRO caches along with TLB invalidation request */
> > +#define INTEL_GUC_TLB_INVAL_FLUSH_CACHE (1 << 31)
> > +
> > +enum intel_guc_tlb_invalidation_type {
> > + INTEL_GUC_TLB_INVAL_GUC = 0x3,
> > +};
> > +
> > +/*
> > + * 0: Heavy mode of Invalidation:
> > + * The pipeline of the engine(s) for which the invalidation is targeted to is
> > + * blocked, and all the in-flight transactions are guaranteed to be Globally
> > + * Observed before completing the TLB invalidation
> > + * 1: Lite mode of Invalidation:
> > + * TLBs of the targeted engine(s) are immediately invalidated.
> > + * In-flight transactions are NOT guaranteed to be Globally Observed before
> > + * completing TLB invalidation.
> > + * Light Invalidation Mode is to be used only when
> > + * it can be guaranteed (by SW) that the address translations remain invariant
> > + * for the in-flight transactions across the TLB invalidation. In other words,
> > + * this mode can be used when the TLB invalidation is intended to clear out the
> > + * stale cached translations that are no longer in use. Light Invalidation Mode
> > + * is much faster than the Heavy Invalidation Mode, as it does not wait for the
> > + * in-flight transactions to be GOd.
> > + */
>
> either drop this comment or squash with patch 10/21 to fix it

Ok.

>
> > +enum intel_guc_tlb_inval_mode {
> > + INTEL_GUC_TLB_INVAL_MODE_HEAVY = 0x0,
> > + INTEL_GUC_TLB_INVAL_MODE_LITE = 0x1,
> > +};
> > +
> > #endif /* _ABI_GUC_ACTIONS_ABI_H */
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > index 2706a8c65090..5c59f9b144a3 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > @@ -855,6 +855,96 @@ int intel_guc_self_cfg64(struct intel_guc *guc, u16 key, u64 value)
> > return __guc_self_cfg(guc, key, 2, value);
> > }
> >
> > +static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 *action, u32 size)
>
> nit: maybe since MMIO TLB has moved to dedicated file, we can do the
> same with GUC TLB code like "intel_guc_tlb.c" ?

I'll add a patch at the end of this series moving the code.

> > +{
> > + struct intel_guc_tlb_wait _wq, *wq = &_wq;
> > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > + int err = 0;
> > + u32 seqno;
> > +
> > + init_waitqueue_head(&_wq.wq);
> > +
> > + if (xa_alloc_cyclic_irq(&guc->tlb_lookup, &seqno, wq,
> > + xa_limit_32b, &guc->next_seqno,
> > + GFP_ATOMIC | __GFP_NOWARN) < 0) {
> > + /* Under severe memory pressure? Serialise TLB allocations */
> > + xa_lock_irq(&guc->tlb_lookup);
> > + wq = xa_load(&guc->tlb_lookup, guc->serial_slot);
> > + wait_event_lock_irq(wq->wq,
> > + !READ_ONCE(wq->status),
> > + guc->tlb_lookup.xa_lock);
> > + /*
> > + * Update wq->status under lock to ensure only one waiter can
> > + * issue the tlb invalidation command using the serial slot at a
> > + * time. The condition is set to false before releasing the lock
> > + * so that other caller continue to wait until woken up again.
> > + */
> > + wq->status = 1;
> > + xa_unlock_irq(&guc->tlb_lookup);
> > +
> > + seqno = guc->serial_slot;
> > + }
> > +
> > + action[1] = seqno;
>
> it's sad that we need to update in blind this action message
>
> if you don't want to expose seqno allocation in a helper function that
> each caller would use, then maybe assert that this action message is
> expected one

I'll encapsulate the code that allocates the seqno on a new helper function:

static int intel_guc_alloc_tlb_seqno(struct intel_guc *guc,
struct intel_guc_tlb_wait *wq)
{
u32 seqno;

if (xa_alloc_cyclic_irq(&guc->tlb_lookup, &seqno, wq,
xa_limit_32b, &guc->next_seqno,
GFP_ATOMIC | __GFP_NOWARN) >= 0)
return seqno;

/* Under severe memory pressure? Serialise TLB allocations */

xa_lock_irq(&guc->tlb_lookup);
wq = xa_load(&guc->tlb_lookup, guc->serial_slot);
wait_event_lock_irq(wq->wq,
!READ_ONCE(wq->status),
guc->tlb_lookup.xa_lock);
/*
* Update wq->status under lock to ensure only one waiter can
* issue the TLB invalidation command using the serial slot at a
* time. The condition is set to false before releasing the lock
* so that other caller continue to wait until woken up again.
*/
wq->status = 1;
xa_unlock_irq(&guc->tlb_lookup);

return guc->serial_slot;
}

This should improve the readability of the invalidate function:

static int guc_send_invalidate_tlb(struct intel_guc *guc, u32 *action, u32 size)
{
struct intel_guc_tlb_wait _wq, *wq = &_wq;
DEFINE_WAIT_FUNC(wait, woken_wake_function);
int err = 0;
u32 seqno;

init_waitqueue_head(&_wq.wq);

seqno = intel_guc_alloc_tlb_seqno(guc, wq);
action[1] = seqno;
...

> > +
> > + add_wait_queue(&wq->wq, &wait);
> > +
> > + err = intel_guc_send_busy_loop(guc, action, size, G2H_LEN_DW_INVALIDATE_TLB, true);
> > + if (err) {
> > + /*
> > + * XXX: Failure of tlb invalidation is critical and would
>
> s/tlb/TLB
>
> > + * warrant a gt reset.
> > + */
> > + goto out;
> > + }
> > +/*
> > + * GuC has a timeout of 1ms for a tlb invalidation response from GAM. On a
>
> ditto
>
> > + * timeout GuC drops the request and has no mechanism to notify the host about
> > + * the timeout. So keep a larger timeout that accounts for this individual
> > + * timeout and max number of outstanding invalidation requests that can be
> > + * queued in CT buffer.
> > + */
> > +#define OUTSTANDING_GUC_TIMEOUT_PERIOD (HZ)
> > + if (!wait_woken(&wait, TASK_UNINTERRUPTIBLE,
>
> IIRC there was some discussion if we can rely on this in our scenario
> can you sync with Chris on that?

I'll check.

>
> > + OUTSTANDING_GUC_TIMEOUT_PERIOD)) {
> > + /*
> > + * XXX: Failure of tlb invalidation is critical and would
>
> s/tlb/TLB
>
> > + * warrant a gt reset.
> > + */
> > + drm_err(&guc_to_gt(guc)->i915->drm,
> > + "tlb invalidation response timed out for seqno %u\n", seqno);
>
> s/tlb/TLB
>
> btw, should we care here about G2H_LEN_DW_INVALIDATE_TLB space that we
> reserved in send_busy_loop() ?

Good question. The logic at intel_guc_tlb_invalidation_done() has
something to handle timeouts:

/* We received a response after the waiting task did exit with a timeout */
if (unlikely(!wait))
drm_err(&guc_to_gt(guc)->i915->drm,
"Stale TLB invalidation response with seqno %d\n", seqno);

It sounds to me that this is already covered there.

>
> > + err = -ETIME;
> > + }
> > +out:
> > + remove_wait_queue(&wq->wq, &wait);
> > + if (seqno != guc->serial_slot)
> > + xa_erase_irq(&guc->tlb_lookup, seqno);
> > +
> > + return err;
> > +}
> > +
> > +/*
> > + * Guc TLB Invalidation: Invalidate the TLB's of GuC itself.
> > + */
> > +int intel_guc_invalidate_tlb_guc(struct intel_guc *guc,
> > + enum intel_guc_tlb_inval_mode mode)
> > +{
> > + u32 action[] = {
> > + INTEL_GUC_ACTION_TLB_INVALIDATION,
> > + 0,
> > + INTEL_GUC_TLB_INVAL_GUC << INTEL_GUC_TLB_INVAL_TYPE_SHIFT |
> > + mode << INTEL_GUC_TLB_INVAL_MODE_SHIFT |
> > + INTEL_GUC_TLB_INVAL_FLUSH_CACHE,
> > + };
> > +
> > + if (!INTEL_GUC_SUPPORTS_TLB_INVALIDATION(guc)) {
> > + DRM_ERROR("Tlb invalidation: Operation not supported in this platform!\n");
>
> you should use drm_err() instead

Ok.

> but wondering if maybe this should be treated as a coding error (and
> then use GEM_BUG/WARN_ON instead) but then not sure how to interpret the
> check for the intel_guc_ct_enabled() embedded in above macro ...
> note that intel_guc_ct_send() will return -ENODEV if CTB is down

Good point. Still, I don't think that the driver should crash with
GEM_BUG/WARN_ON() if TLB invalidation is not available, as this may
cause regressions.

I mean, assuming that someone is currently using the driver on a
firmware that doesn't support such actions, after this patch, the
driver will stop working for him.

So, I think the right thing to do is to just report it as an
error.

> > + return 0;
> > + }
> > +
> > + return guc_send_invalidate_tlb(guc, action, ARRAY_SIZE(action));
> > +}
> > +
> > /**
> > * intel_guc_load_status - dump information about GuC load status
> > * @guc: the GuC
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index d0d99f178f2d..f82a121b0838 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -77,6 +77,10 @@ struct intel_guc {
> > atomic_t outstanding_submission_g2h;
> >
> > /** @interrupts: pointers to GuC interrupt-managing functions. */
> > + struct xarray tlb_lookup;
> > + u32 serial_slot;
> > + u32 next_seqno;
>
> wrong place - above kernel-doc is for the struct below

Ok.

> > +
> > struct {
> > void (*reset)(struct intel_guc *guc);
> > void (*enable)(struct intel_guc *guc);
> > @@ -248,6 +252,11 @@ struct intel_guc {
> > #endif
> > };
> >
> > +struct intel_guc_tlb_wait {
> > + struct wait_queue_head wq;
> > + u8 status;
> > +} __aligned(4);
> > +
> > static inline struct intel_guc *log_to_guc(struct intel_guc_log *log)
> > {
> > return container_of(log, struct intel_guc, log);
> > @@ -363,6 +372,9 @@ int intel_guc_allocate_and_map_vma(struct intel_guc *guc, u32 size,
> > int intel_guc_self_cfg32(struct intel_guc *guc, u16 key, u32 value);
> > int intel_guc_self_cfg64(struct intel_guc *guc, u16 key, u64 value);
> >
> > +int intel_guc_invalidate_tlb_guc(struct intel_guc *guc,
> > + enum intel_guc_tlb_inval_mode mode);
> > +
> > static inline bool intel_guc_is_supported(struct intel_guc *guc)
> > {
> > return intel_uc_fw_is_supported(&guc->fw);
> > @@ -440,6 +452,7 @@ int intel_guc_engine_failure_process_msg(struct intel_guc *guc,
> > const u32 *msg, u32 len);
> > int intel_guc_error_capture_process_msg(struct intel_guc *guc,
> > const u32 *msg, u32 len);
> > +void intel_guc_tlb_invalidation_done(struct intel_guc *guc, u32 seqno);
> >
> > struct intel_engine_cs *
> > intel_guc_lookup_engine(struct intel_guc *guc, u8 guc_class, u8 instance);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > index f01325cd1b62..c1ce542b7855 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -1023,7 +1023,7 @@ static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *r
> > return 0;
> > }
> >
> > -static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
> > +static bool ct_process_incoming_requests(struct intel_guc_ct *ct, struct list_head *incoming)
> > {
> > unsigned long flags;
> > struct ct_incoming_msg *request;
> > @@ -1031,11 +1031,11 @@ static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
> > int err;
> >
> > spin_lock_irqsave(&ct->requests.lock, flags);
> > - request = list_first_entry_or_null(&ct->requests.incoming,
> > + request = list_first_entry_or_null(incoming,
> > struct ct_incoming_msg, link);
> > if (request)
> > list_del(&request->link);
> > - done = !!list_empty(&ct->requests.incoming);
> > + done = !!list_empty(incoming);
> > spin_unlock_irqrestore(&ct->requests.lock, flags);
> >
> > if (!request)
> > @@ -1058,7 +1058,7 @@ static void ct_incoming_request_worker_func(struct work_struct *w)
> > bool done;
> >
> > do {
> > - done = ct_process_incoming_requests(ct);
> > + done = ct_process_incoming_requests(ct, &ct->requests.incoming);
> > } while (!done);
> > }
> >
> > @@ -1078,14 +1078,30 @@ static int ct_handle_event(struct intel_guc_ct *ct, struct ct_incoming_msg *requ
> > switch (action) {
> > case INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_DONE:
> > case INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE:
> > + case INTEL_GUC_ACTION_TLB_INVALIDATION_DONE:
> > g2h_release_space(ct, request->size);
> > }
> > + /* Handle tlb invalidation response in interrupt context */
>
> since it breaks layering, can you add more comments why this is done in
> such way ?
>
> > + if (action == INTEL_GUC_ACTION_TLB_INVALIDATION_DONE) {

I'll improve the comment here. I guess something like this would
be enough:

/*
* Handle tlb invalidation response in interrupt context
*
* As TLB invalidation is needed to avoid leaking data, wait until
* TLB invalidation is completed before returning.
*/

> > + const u32 *payload;
> > + u32 hxg_len, len;
> > +
> > + hxg_len = request->size - GUC_CTB_MSG_MIN_LEN;
> > + len = hxg_len - GUC_HXG_MSG_MIN_LEN;
> > + if (unlikely(len < 1))
> > + return -EPROTO;
> > + payload = &hxg[GUC_HXG_MSG_MIN_LEN];
>
> if we still need to handle this at this level, can we at least move this
> message decomposition to the handler (in other words: just pass hxg
> pointer instead of single dword payload)

Yeah, makes sense.

>
> > + intel_guc_tlb_invalidation_done(ct_to_guc(ct), payload[0]);
> > + ct_free_msg(request);
> > + return 0;
> > + }
> >
> > spin_lock_irqsave(&ct->requests.lock, flags);
> > list_add_tail(&request->link, &ct->requests.incoming);
> > spin_unlock_irqrestore(&ct->requests.lock, flags);
> >
> > queue_work(system_unbound_wq, &ct->requests.worker);
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > index b3c9a9327f76..3edf567b3f65 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > @@ -22,6 +22,7 @@
> > /* Payload length only i.e. don't include G2H header length */
> > #define G2H_LEN_DW_SCHED_CONTEXT_MODE_SET 2
> > #define G2H_LEN_DW_DEREGISTER_CONTEXT 1
> > +#define G2H_LEN_DW_INVALIDATE_TLB 1
> >
> > #define GUC_CONTEXT_DISABLE 0
> > #define GUC_CONTEXT_ENABLE 1
> > @@ -431,4 +432,9 @@ enum intel_guc_recv_message {
> > INTEL_GUC_RECV_MSG_EXCEPTION = BIT(30),
> > };
> >
> > +#define INTEL_GUC_SUPPORTS_TLB_INVALIDATION(guc) \
> > + ((intel_guc_ct_enabled(&(guc)->ct)) && \

This basically does:
static inline bool intel_guc_ct_enabled(struct intel_guc_ct *ct)
{
return ct->enabled;
}

>
> do we need this check ?
> CTB is prerequisite for submission that is required below
>
> > + (intel_guc_submission_is_used(guc)) && \

If I understood the code right, this checks the status machine,
by looking at __intel_uc_fw_status(&guc->fw), but it doesn't look
at ct->enabled. Without knowing exactly how the status is updated,
is hard to tell if ct enabled flag will be always true here.

So, I would keep the check.

> > + (GRAPHICS_VER(guc_to_gt((guc))->i915) >= 12))
> > +
> > #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 40f726c61e95..6888ea1bc7c1 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -1653,11 +1653,20 @@ static void __guc_reset_context(struct intel_context *ce, intel_engine_mask_t st
> > intel_context_put(parent);
> > }
> >
> > +static void wake_up_tlb_invalidate(struct intel_guc_tlb_wait *wait)
> > +{
> > + /* Barrier to ensure the store is observed by the woken thread */
> > + smp_store_mb(wait->status, 0);
> > + wake_up(&wait->wq);
> > +}
> > +
> > void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stalled)
> > {
> > + struct intel_guc_tlb_wait *wait;
> > 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! */
> > @@ -1683,6 +1692,13 @@ void intel_guc_submission_reset(struct intel_guc *guc, intel_engine_mask_t stall
> >
> > /* GuC is blown away, drop all references to contexts */
> > xa_destroy(&guc->context_lookup);
> > +
> > + /*
> > + * The full GT reset will have cleared the TLB caches and flushed the
> > + * G2H message queue; we can release all the blocked waiters.
> > + */
> > + xa_for_each(&guc->tlb_lookup, i, wait)
> > + wake_up_tlb_invalidate(wait);
>
> shouldn't this be closer to intel_guc_invalidate_tlb_guc()
> then we can avoid spreading code across many files
>
> same for the init/fini_tlb_lookup() functions below

I'll address that on a final patch moving GuC-based TLB patch to a
separate file.

> > }
> >
> > static void guc_cancel_context_requests(struct intel_context *ce)
> > @@ -1805,6 +1821,41 @@ 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;
> > +
> > + xa_init_flags(&guc->tlb_lookup, XA_FLAGS_ALLOC);
> > +
> > + wait = kzalloc(sizeof(*wait), GFP_KERNEL);
> > + if (!wait)
> > + return -ENOMEM;
> > +
> > + init_waitqueue_head(&wait->wq);
> > + err = xa_alloc_cyclic_irq(&guc->tlb_lookup, &guc->serial_slot, wait,
> > + xa_limit_32b, &guc->next_seqno, GFP_KERNEL);
> > + if (err == -ENOMEM) {
> > + kfree(wait);
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void fini_tlb_lookup(struct intel_guc *guc)
> > +{
> > + struct intel_guc_tlb_wait *wait;
> > +
> > + wait = xa_load(&guc->tlb_lookup, guc->serial_slot);
> > + if (wait) {
> > + GEM_BUG_ON(wait->status);
> > + 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.
> > @@ -1812,20 +1863,31 @@ static void reset_fail_worker_func(struct work_struct *w);
> > int intel_guc_submission_init(struct intel_guc *guc)
> > {
> > struct intel_gt *gt = guc_to_gt(guc);
> > + int ret;
> >
> > if (guc->submission_initialized)
> > return 0;
> >
> > + ret = init_tlb_lookup(guc);
>
> if we promote guc_tlb to own file/functions then maybe it could be
> init/fini directly from __uc_init_hw() ?

I'll look on it at the new patch to be added at the end.

> > + if (ret)
> > + return ret;
> > +
> > guc->submission_state.guc_ids_bitmap =
> > bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
> > - if (!guc->submission_state.guc_ids_bitmap)
> > - return -ENOMEM;
> > + if (!guc->submission_state.guc_ids_bitmap) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> >
> > guc->timestamp.ping_delay = (POLL_TIME_CLKS / gt->clock_frequency + 1) * HZ;
> > guc->timestamp.shift = gpm_timestamp_shift(gt);
> > guc->submission_initialized = true;
> >
> > return 0;
> > +
> > +err:
> > + fini_tlb_lookup(guc);
> > + return ret;
> > }
> >
> > void intel_guc_submission_fini(struct intel_guc *guc)
> > @@ -1836,6 +1898,7 @@ void intel_guc_submission_fini(struct intel_guc *guc)
> > guc_flush_destroyed_contexts(guc);
> > i915_sched_engine_put(guc->sched_engine);
> > bitmap_free(guc->submission_state.guc_ids_bitmap);
> > + fini_tlb_lookup(guc);
> > guc->submission_initialized = false;
> > }
> >
> > @@ -4027,6 +4090,30 @@ g2h_context_lookup(struct intel_guc *guc, u32 ctx_id)
> > return ce;
> > }
> >
> > +static void wait_wake_outstanding_tlb_g2h(struct intel_guc *guc, u32 seqno)
> > +{
> > + struct intel_guc_tlb_wait *wait;
> > + unsigned long flags;
> > +
> > + xa_lock_irqsave(&guc->tlb_lookup, flags);
> > + wait = xa_load(&guc->tlb_lookup, seqno);
> > +
> > + /* We received a response after the waiting task did exit with a timeout */
> > + if (unlikely(!wait))
> > + drm_dbg(&guc_to_gt(guc)->i915->drm,
> > + "Stale tlb invalidation response with seqno %d\n", seqno);
>
> hmm, this sounds like a problem as we shouldn't get any late
> notifications - do we really want to hide it under drm_dbg ?

Agreed. I'll change it to drm_err().

> > +
> > + if (wait)
> > + wake_up_tlb_invalidate(wait);
> > +
> > + xa_unlock_irqrestore(&guc->tlb_lookup, flags);
> > +}
> > +
> > +void intel_guc_tlb_invalidation_done(struct intel_guc *guc, u32 seqno)
> > +{
> > + wait_wake_outstanding_tlb_g2h(guc, seqno);
> > +}
> > +
> > int intel_guc_deregister_done_process_msg(struct intel_guc *guc,
> > const u32 *msg,
> > u32 len)
>
> ,Michal