Re: [RFC PATCH] drm/xe/ggtt: Cut GGTT initialization from 900 ms to 37 ms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/ggtt: fs_reclaim lock inversion in ggtt_clear_gpu() under ggtt->lock
- [Low] drm/xe: missing kernel-parameters.txt entry for xe.ggtt_init_gpu
--

commit ed4c9843bc7f8d6478d283f57c556b54c7d8a262
Author: Daniel Colascione <dancol@dancol.org>

drm/xe/ggtt: Cut GGTT initialization from 900 ms to 37 ms

This patch uses the GPU copy engine to initialize GGTT page tables in batches
via MI_UPDATE_GTT commands on Lunar Lake, deferring the initial GGTT hole
clear to xe_ggtt_init_late() after GuC startup. It also adds the
xe.ggtt_init_gpu module parameter to toggle this behavior.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 9ef2ce743770f..47d12e5e95458 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
[ ... ]
> @@ -456,18 +461,119 @@ ALLOW_ERROR_INJECTION(xe_ggtt_init_early, ERRNO); /* See xe_pci_probe() */
[ ... ]
> +static int ggtt_clear_gpu(struct xe_exec_queue *q, struct xe_bb *bb,
> +			  u64 pte, u64 start, u64 end)
> +{
> +	while (start < end) {
> +		struct xe_sched_job *job;
> +		int err;
> +
> +		bb->len = 0;
> +		while (start < end && bb->len < GGTT_INIT_BATCH_DWORDS) {
> +			u32 count = min_t(u64, (end - start) >> XE_PTE_SHIFT,
> +					  GGTT_INIT_PTES_PER_PACKET);
> +
> +			bb->cs[bb->len++] = MI_UPDATE_GTT | (2 * count);
> +			bb->cs[bb->len++] = start;
> +			memset64((u64 *)&bb->cs[bb->len], pte, count);
> +			bb->len += 2 * count;
> +			start += (u64)count << XE_PTE_SHIFT;
> +		}
> +
> +		job = xe_bb_create_job(q, bb);
> +		if (IS_ERR(job))
> +			return PTR_ERR(job);
[ ... ]
> +static int xe_ggtt_initial_clear(struct xe_ggtt *ggtt, struct xe_hw_engine *hwe)
> +{
[ ... ]
> +	/* Preserve boot mappings and exclude allocator changes until completion. */
>  	mutex_lock(&ggtt->lock);
> -	drm_mm_for_each_hole(hole, &ggtt->mm, start, end)
> -		xe_ggtt_clear(ggtt, ggtt->start + start, end - start);
> +	drm_mm_for_each_hole(hole, &ggtt->mm, start, end) {
> +		if (q)
> +			err = ggtt_clear_gpu(q, bb, pte, ggtt->start + start, ggtt->start + end);
> +		else
> +			xe_ggtt_clear(ggtt, ggtt->start + start, end - start);
> +		if (err)
> +			goto unlock;
> +	}

[Severity: High]
Does calling ggtt_clear_gpu() while holding ggtt->lock cause an fs_reclaim
lock inversion?

During allocation in xe_ggtt_alloc(), primelockdep() in
drivers/gpu/drm/xe/xe_ggtt.c establishes the ordering fs_reclaim ->
ggtt->lock because BO destruction during memory reclaim acquires ggtt->lock:

    static void primelockdep(struct xe_ggtt *ggtt)
    {
        if (!IS_ENABLED(CONFIG_LOCKDEP))
            return;

        fs_reclaim_acquire(GFP_KERNEL);
        might_lock(&ggtt->lock);
        fs_reclaim_release(GFP_KERNEL);
    }

However, on Lunar Lake with xe.ggtt_init_gpu enabled,
gt_init_with_all_forcewake() -> xe_ggtt_init_late() -> xe_ggtt_initial_clear()
acquires ggtt->lock and calls ggtt_clear_gpu(), which invokes
xe_bb_create_job() -> __xe_bb_create_job() -> xe_sched_job_create().

Inside xe_sched_job_create(), several GFP_KERNEL allocations occur under
ggtt->lock, including job_alloc() in drivers/gpu/drm/xe/xe_sched_job.c:

    static struct xe_sched_job *job_alloc(bool parallel)
    {
        return kmem_cache_zalloc(parallel ? xe_sched_job_parallel_slab :
                                 xe_sched_job_slab, GFP_KERNEL);
    }

as well as drm_sched_job_init() and xe_lrc_alloc_seqno_fence() ->
xe_hw_fence_alloc() -> fence_alloc() in drivers/gpu/drm/xe/xe_hw_fence.c:

    return kmem_cache_zalloc(xe_hw_fence_slab, GFP_KERNEL);

Wouldn't this violate the fs_reclaim -> ggtt->lock hierarchy, triggering a
lockdep circular locking warning under CONFIG_PROVE_LOCKING during probe and
risking a reclaim deadlock if direct reclaim attempts to free a GGTT-mapped
BO?

> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> index 897724cb5cfb8..44cc0107cf53a 100644
> --- a/drivers/gpu/drm/xe/xe_module.c
> +++ b/drivers/gpu/drm/xe/xe_module.c
> @@ -22,6 +22,7 @@
>  #include "xe_sched_job.h"
>  
>  struct xe_modparam xe_modparam = {
> +	.ggtt_init_gpu =		true,
>  	.probe_display =	XE_DEFAULT_PROBE_DISPLAY,
>  	.guc_log_level =	XE_DEFAULT_GUC_LOG_LEVEL,
>  	.force_probe =		XE_DEFAULT_FORCE_PROBE,
> @@ -34,6 +35,10 @@ struct xe_modparam xe_modparam = {
>  	/* the rest are 0 by default */
>  };
>  
> +module_param_named(ggtt_init_gpu, xe_modparam.ggtt_init_gpu, bool, 0444);
> +MODULE_PARM_DESC(ggtt_init_gpu,
> +		 "GPU GGTT initialization on Lunar Lake (default: true)");

[Severity: Low]
Should this new xe.ggtt_init_gpu module parameter also be documented in
Documentation/admin-guide/kernel-parameters.txt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/87v76m1l9k.fsf@dancol.org?part=1