Re: [PATCH 2/2] drm/xe: Set job credits based on LRC size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: scheduler credits under-account 8-byte padded ring writes
- [High] drm/xe: setting credit limit to ring capacity causes full/empty aliasing hang
--

commit 1784ce8c172d03c0fac8aec6a5070336e0615926
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/xe: Set job credits based on LRC size

This commit changes the scheduler credit limit in the xe driver to be based
on xe_lrc_ring_size() and sets the job credits to the actual job size. This
allows filling the entire LRC without throttling.

> diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
> index a36db39dcda8a..94d4d982f79b3 100644
> --- a/drivers/gpu/drm/xe/xe_execlist.c
> +++ b/drivers/gpu/drm/xe/xe_execlist.c
> @@ -329,7 +329,7 @@ static int execlist_exec_queue_init(struct xe_exec_queue *q)
>  	const struct drm_sched_init_args args = {
>  		.ops = &drm_sched_ops,
>  		.num_rqs = 1,
> -		.credit_limit = xe_lrc_ring_size() / MAX_JOB_SIZE_BYTES,
> +		.credit_limit = xe_lrc_ring_size(),

[Severity: High]
Does setting the credit limit to exactly xe_lrc_ring_size() cause a
full/empty aliasing hang?

When a user submits a series of jobs whose total size perfectly hits the
xe_lrc_ring_size() limit, the scheduler permits all these jobs to be queued.
The tail pointer advances by the full ring capacity and wraps around to
precisely equal the head pointer.

Since a condition of tail == head indicates an empty ring, the hardware
stops fetching commands, permanently hanging the queue and triggering a TDR
reset. Should the limit be strictly less than the capacity (e.g.,
xe_lrc_ring_size() - 8) to prevent this?

>  		.hang_limit = XE_SCHED_HANG_LIMIT,
>  		.timeout = XE_SCHED_JOB_TIMEOUT,
>  		.name = q->hwe->name,

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index f3ba8abfc228e..98fbc72af9f3d 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2089,7 +2089,7 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
>  	}
>  
>  	err = xe_sched_init(&ge->sched, &drm_sched_ops, &xe_sched_ops,
> -			    submit_wq, xe_lrc_ring_size() / MAX_JOB_SIZE_BYTES, 64,
> +			    submit_wq, xe_lrc_ring_size(), 64,

[Severity: High]
Does configuring the GuC submit scheduler's credit limit exactly to the
ring size expose GuC submissions to the same full/empty aliasing hang as in
xe_execlist.c?

>  			    timeout, guc_to_gt(guc)->ordered_wq, NULL,
>  			    ge->name, gt_to_xe(q->gt)->drm.dev);
>  	if (err)

> diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c
> index 57ed6734d31d5..be99d9cd16ad3 100644
> --- a/drivers/gpu/drm/xe/xe_sched_job.c
> +++ b/drivers/gpu/drm/xe/xe_sched_job.c

[ ... ]

> @@ -295,8 +295,10 @@ void xe_sched_job_push(struct xe_sched_job *job)
>  	xe_sched_job_get(job);
>  	trace_xe_sched_job_exec(job);
>  
> -	if (!job->restore_replay)
> +	if (!job->restore_replay) {
>  		job->q->ring_ops->prepare_job(job);
> +		job->drm.credits = job->ptrs[0].job_size;

[Severity: High]
Can assigning the unaligned job_size to the scheduler credits lead to a
buffer overflow of the hardware ring buffer?

If a user submits multiple jobs whose sizes are not perfectly 8-byte
aligned, the scheduler deducts the unaligned job_size (e.g., 68 bytes) from
its credit limit. However, xe_lrc_write_ring() aligns each job write to 8
bytes by padding with MI_NOOP (e.g., writing 72 bytes):

drivers/gpu/drm/xe/xe_lrc.c:xe_lrc_write_ring() {
	...
	aligned_size = ALIGN(size, 8);
	...
	if (aligned_size > size) {
		u32 noop = MI_NOOP;

		__xe_lrc_write_ring(lrc, ring, &noop, sizeof(noop));
	}
	...
}

Because credit_limit is increased to the absolute maximum
xe_lrc_ring_size(), the accumulation of unaccounted padding bytes allows
the total written bytes to physically exceed the ring's size while the
scheduler believes there is still space.

The tail pointer could then overtake the head pointer, overwriting active
commands.

> +	}
>  
>  	drm_sched_entity_push_job(&job->drm);
>  	xe_sched_job_put(job);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918081609.518988-1-dev@lankhorst.se?part=2