Re: [PATCH v4 8/8] drm/xe: Avoid toggling schedule state to check LRC timestamp in TDR

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Fri, Nov 21, 2025 at 01:33:06PM -0800, Matthew Brost wrote:
>On Thu, Nov 20, 2025 at 12:33:12PM -0800, Umesh Nerlige Ramappa wrote:
>> On Wed, Nov 19, 2025 at 02:41:06PM -0800, Matthew Brost wrote:
>> > We now have proper infrastructure to accurately check the LRC timestamp
>> > without toggling the scheduling state for non-VFs. For VFs, it is still
>> > possible to get an inaccurate view if the context is on hardware. We
>> > guard against free-running contexts on VFs by banning jobs whose
>> > timestamps are not moving. In addition, VFs have a timeslice quantum
>> > that naturally triggers context switches when more than one VF is
>> > running, thus updating the LRC timestamp.
>> >
>> > For multi-queue, it is desirable to avoid scheduling toggling in the TDR
>> > because this scheduling state is shared among many queues. Furthermore,
>> > this change simplifies the GuC state machine. The trade-off for VF cases
>> > seems worthwhile.
>> >
>> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_guc_submit.c      | 100 ++++++------------------
>> > drivers/gpu/drm/xe/xe_sched_job.c       |   1 +
>> > drivers/gpu/drm/xe/xe_sched_job_types.h |   2 +
>> > 3 files changed, 28 insertions(+), 75 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > index 1f2afad1766e..7404716e979f 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > @@ -68,9 +68,7 @@ exec_queue_to_guc(struct xe_exec_queue *q)
>> > #define EXEC_QUEUE_STATE_KILLED			(1 << 7)
>> > #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
>> > #define EXEC_QUEUE_STATE_BANNED			(1 << 9)
>> > -#define EXEC_QUEUE_STATE_CHECK_TIMEOUT		(1 << 10)
>> > -#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 11)
>> > -#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 12)
>> > +#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 10)
>> >
>>
>> ... snip ...
>>
>> > static bool exec_queue_killed_or_banned_or_wedged(struct xe_exec_queue
>> > *q)
>> > {
>> > 	return (atomic_read(&q->guc->state) &
>> > @@ -996,7 +964,7 @@ static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
>> > 	u32 ctx_timestamp, ctx_job_timestamp;
>> > 	u32 timeout_ms = q->sched_props.job_timeout_ms;
>> > 	u32 diff;
>> > -	u64 running_time_ms;
>> > +	u64 running_time_ms, old_timestamp;
>> >
>> > 	if (!xe_sched_job_started(job)) {
>> > 		xe_gt_warn(gt, "Check job timeout: seqno=%u, lrc_seqno=%u, guc_id=%d, not started",
>> > @@ -1006,7 +974,17 @@ static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
>> > 		return xe_sched_invalidate_job(job, 2);
>> > 	}
>> >
>> > -	ctx_timestamp = lower_32_bits(xe_lrc_ctx_timestamp(q->lrc[0]));
>> > +	ctx_timestamp = lower_32_bits(xe_lrc_update_timestamp(q->lrc[0],
>> > +							      &old_timestamp));
>>
>> Reg: xe_lrc_update_timestamp()
>>
>> The way context utilization is using this helper is to accumulate the 'new -
>> old' values each time this function is called. In the below example, context
>> utilization will loose some ticks.
>>
>> Example:
>>
>> 1. This code calls xe_lrc_update_timestamp() to sample the timestamp for TDR
>> purposes. Say context/job is running, then the lrc->ctx_timestamp is updated
>> (moved forward).
>>
>> 2. The context utilization code calls xe_lrc_update_timestamp(). Within this
>> helper
>> - old_ts is sampled as lrc->ctx_timestamp
>> - new_ts is calculated based on whether the job/context is active
>> - lrc->ctx_timestamp is updated to the new value.
>>
>> The result is that we lost one chunk of utilization because of the previous
>> call from the TDR path. I think some refactor would be needed to fix that.
>>
>> The other comment you already mentioned offline is locking, which I think we
>> should add to protect lrc->ctx_timestamp. I don't know if a refactor will
>> avoid the lock though.
>>
>
>I agree with you analysis here - thanks for the help.
>
>How about - we extract the following code from
>xe_exec_queue_update_run_ticks into helper that also returns the current
>timestamp and is also protected by an queue spin lock:
>
>         new_ts = xe_lrc_update_timestamp(lrc, &old_ts);
>         q->xef->run_ticks[q->class] += (new_ts - old_ts) * q->width;

I was thinking something like below.

/**
  * xe_lrc_timestamp() - Current ctx timestamp
  * @lrc: Pointer to the lrc.
  *
  * Return latest ctx timestamp.
  *
  * Returns: New ctx timestamp value
  */
u64 xe_lrc_timestamp(struct xe_lrc *lrc)
{
         u64 lrc_ts, reg_ts, new_ts;
         u32 engine_id;

         lrc_ts = xe_lrc_ctx_timestamp(lrc);
         /* CTX_TIMESTAMP mmio read is invalid on VF, so return the LRC value */
         if (IS_SRIOV_VF(lrc_to_xe(lrc))) {
                 new_ts = lrc_ts;
                 goto done;
         }

         if (lrc_ts == CONTEXT_ACTIVE) {
                 engine_id = xe_lrc_engine_id(lrc);
                 if (!get_ctx_timestamp(lrc, engine_id, &reg_ts))
                         new_ts = reg_ts;

                 /* read lrc again to ensure context is still active */
                 lrc_ts = xe_lrc_ctx_timestamp(lrc);
         }

         /*
          * If context switched out, just use the lrc_ts. Note that this needs to
          * be a separate if condition.
          */
         if (lrc_ts != CONTEXT_ACTIVE)
                 new_ts = lrc_ts;

done:
         return new_ts;
}

/**
  * xe_lrc_update_timestamp() - Update ctx timestamp
  * @lrc: Pointer to the lrc.
  * @old_ts: Old timestamp value
  *
  * Populate @old_ts current saved ctx timestamp, read new ctx timestamp and
  * update saved value. With support for active contexts, the calculation may be
  * slightly racy, so follow a read-again logic to ensure that the context is
  * still active before returning the right timestamp.
  *
  * Returns: New ctx timestamp value
  */
u64 xe_lrc_update_timestamp(struct xe_lrc *lrc, u64 *old_ts)
{
         *old_ts = lrc->ctx_timestamp;
         lrc->ctx_timestamp = xe_lrc_timestamp(lrc);

         trace_xe_lrc_update_timestamp(lrc, *old_ts);

         return lrc->ctx_timestamp;
}

TDR logic could just use xe_lrc_timestamp() since it does not care about 
old_ts anyways. We could avoid the lock since xe_lrc_update_timestamp() 
is the only place where lrc->ctx_timestamp gets updated.

Thanks,
Umesh

>
>It harmless if the TDR also updates run_ticks when it samples the LRC
>timestamp, right? Also the helper just skips run_ticks if q->xef is
>NULL.
>
>Matt
>
>> Thanks,
>> Umesh
>>
>> > +	if (ctx_timestamp == job->sample_timestamp) {
>> > +		xe_gt_warn(gt, "Check job timeout: seqno=%u, lrc_seqno=%u, guc_id=%d, timestamp stuck",
>> > +			   xe_sched_job_seqno(job), xe_sched_job_lrc_seqno(job),
>> > +			   q->guc->id);
>> > +
>> > +		return xe_sched_invalidate_job(job, 2);
>> > +	}
>> > +
>> > +	job->sample_timestamp = ctx_timestamp;
>> > 	ctx_job_timestamp = xe_lrc_ctx_job_timestamp(q->lrc[0]);
>> >
>> > 	/*
>> > @@ -1135,16 +1113,17 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > 	}
>> >
>>
>>
>> ... snip ...
>>
>> > diff --git a/drivers/gpu/drm/xe/xe_sched_job_types.h
>> > b/drivers/gpu/drm/xe/xe_sched_job_types.h
>> > index d26612abb4ca..ad5eee8a8cdb 100644
>> > --- a/drivers/gpu/drm/xe/xe_sched_job_types.h
>> > +++ b/drivers/gpu/drm/xe/xe_sched_job_types.h
>> > @@ -59,6 +59,8 @@ struct xe_sched_job {
>> > 	u32 lrc_seqno;
>> > 	/** @migrate_flush_flags: Additional flush flags for migration jobs */
>> > 	u32 migrate_flush_flags;
> > >+	/** @sample_timestamp: Sampling of job timestamp in TDR */
>> > +	u64 sample_timestamp;
>> > 	/** @ring_ops_flush_tlb: The ring ops need to flush TLB before payload. */
>> > 	bool ring_ops_flush_tlb;
>> > 	/** @ggtt: mapped in ggtt. */
>> > --
>> > 2.34.1
>> >