Re: [PATCH v7 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: memory leak of pt_job_ops and dma_fence when PT jobs are canceled
- [High] drm/xe: GPU hang and memory leak due to missing CPU bind support in Execlist backend
--

commit 6471c15e2149c2517458e891e7dceef8c5573f97
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Enable CPU binds for jobs

This commit updates the migration layer to formulate page table jobs that
issue CPU binds in the submission backend. All code related to GPU-based
binding is removed to lower latency and avoid PTE migration limits.

> 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,
>  {
>  	const struct xe_migrate_pt_update_ops *ops = pt_update->ops;
>  	struct xe_tile *tile = m->tile;
>  	struct xe_sched_job *job;
>  	struct dma_fence *fence;
>  	bool is_migrate = is_migrate_queue(m, pt_update_ops->q);
> +	int err;
>  
> -	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);

[Severity: High]
Does the Execlist backend properly handle these new PT jobs?

Since xe_sched_job_create() is now called with a NULL batch address, these
jobs rely on backend interception. However, looking at execlist_run_job() in
the Execlist backend:

drivers/gpu/drm/xe/xe_execlist.c:execlist_run_job() {
    ...
    q->ring_ops->emit_job(job);
    xe_execlist_make_active(exl);

    return job->fence;
}

If GuC submission is disabled, it appears the backend receives this job and
directly emits it to the ring without an is_pt_job() check. Could this result
in pushing an uninitialized batch address to the hardware, causing a GPU hang
and leaking the pt_job_ops structures?

>  	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);
>  
>  	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: High]
Is there a memory leak if this PT job gets canceled before execution?

We acquire a reference to pt_job_ops here, and xe_sched_job_arm() takes an
extra dma_fence reference. If the job is canceled (for example, from an
execution queue kill or GPU reset), the DRM scheduler calls the free_job
callback which ultimately calls xe_sched_job_destroy():

drivers/gpu/drm/xe/xe_sched_job.c:xe_sched_job_destroy() {
    ...
    xe_sched_job_free_fences(job);
    dma_fence_put(job->fence);
    drm_sched_job_cleanup(&job->drm);
    job_free(job);
}

Since xe_sched_job_destroy() doesn't check job->is_pt_job, it seems we never
drop the pt_job_ops or the extra dma_fence reference. This would permanently
leak the deferred actions and their associated page table BO references. Does
the job destruction path need updating to clean up canceled PT jobs?

>  	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/20260925045320.1325860-1-matthew.brost@intel.com?part=13