Re: [PATCH v8 13/24] drm/xe: Enable CPU binds for jobs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: xe_execlist submission backend crashes on CPU-bound PT jobs
- [Medium] drm/xe: memory leak of xe_pt_job_ops on canceled PT jobs
--

commit 9de8f05bf053b28de6fdac64d9ee437e1c0df6f7
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/xe: Enable CPU binds for jobs

This commit transitions the driver to formulating PT jobs that issue CPU
binds in the submission backend instead of using the GPU. This approach
aims to lower latency and avoid migration PTE exhaustion for large arrays
of binds.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c

[ ... ]

> @@ -1937,162 +1866,18 @@ __xe_migrate_update_pgtables(struct xe_migrate *m,
>  {

[ ... ]

> -	job = xe_bb_create_migration_job(pt_update_ops->q, bb,
> -					 xe_migrate_batch_base(m, usm),
> -					 update_idx);
> +	job = xe_sched_job_create(pt_update_ops->q, NULL);
>  	if (IS_ERR(job)) {
>  		err = PTR_ERR(job);
> -		goto err_sa;
> +		goto err_out;
>  	}
>  
> -	xe_sched_job_add_migrate_flush(job, MI_INVALIDATE_TLB);
> +	xe_tile_assert(tile, job->is_pt_job);

[Severity: High]
Does this cause a crash or hang when the driver operates in execlist mode
(e.g., when GuC is disabled)? In drivers/gpu/drm/xe/xe_execlist.c, the
execlist_run_job() function unconditionally emits the job to the GPU
ringbuffer without checking if is_pt_job is true. Because the job uses a
union for ptrs and pt_update, the backend reads the newly populated
job->pt_update as a batch address, emitting garbage into the hardware ring.

>  
>  	if (ops->pre_commit) {
>  		pt_update->job = job;

[ ... ]

> @@ -2103,6 +1888,12 @@ __xe_migrate_update_pgtables(struct xe_migrate *m,
>  	if (is_migrate)
>  		mutex_lock(&m->job_mutex);
>  
> +	job->pt_update[0].vm = pt_update->vops->vm;
> +	job->pt_update[0].tile = tile;
> +	job->pt_update[0].ops = ops;
> +	job->pt_update[0].pt_job_ops =
> +		xe_pt_job_ops_get(pt_update_ops->pt_job_ops);
> +

[Severity: Medium]
Does this leak the dynamically allocated xe_pt_job_ops structure if the PT
job is canceled or dropped before execution? The scheduler directly invokes
the free_job callback when a process is killed and jobs are aborted, which
calls xe_sched_job_destroy() in drivers/gpu/drm/xe/xe_sched_job.c. Since
xe_sched_job_destroy() does not check for job->is_pt_job, the reference
acquired here is never released.

>  	xe_sched_job_arm(job);
>  	fence = dma_fence_get(&job->drm.s_fence->finished);
>  	xe_sched_job_push(job);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925201526.1355279-1-matthew.brost@intel.com?part=13