Re: [PATCH] drm/xe/guc: Reset GuC before unmapping its post-hwconfig buffers

["Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>]

On 19.10.2026 15:34, Sujaritha Sundaresan wrote:
> guc_fini_hw() resets the GuC on teardown, but it is registered before
> xe_guc_init_post_hwconfig() (re)allocates most of the GuC buffers. As
> devm actions run in reverse order, these buffers get unmapped from the
> GGTT while the GuC is still running and may still access them. When
> unmapped GGTT entries are left invalid, this results in a GuC page
> fault on unbind or probe failure.
> 
> Re-register guc_fini_hw() after all the GuC buffer allocations, so the
> GuC is reset before any of them is unmapped. Keep GuC submit and RC
> teardown running before the reset, as they still need to stop or talk
> to the GuC.
> 
> Assisted-by: LLM
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 6c35474ab843..14ae73844d76 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -882,9 +882,10 @@ static u32 guc_additional_cache_size(struct xe_device *xe)
>   */
>  int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>  {
> +	struct xe_device *xe = guc_to_xe(guc);
>  	int ret;
>  
> -	if (IS_SRIOV_VF(guc_to_xe(guc)))
> +	if (IS_SRIOV_VF(xe))
>  		return vf_guc_init_post_hwconfig(guc);
>  
>  	ret = xe_guc_realloc_post_hwconfig(guc);
> @@ -897,28 +898,41 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>  
>  	guc_init_params_post_hwconfig(guc);
>  
> -	ret = xe_guc_submit_init(guc);

The teardown function guc_submit_fini, registered by xe_guc_submit_init,
is invoked before releasing the buffers shared with GuC.

guc_submit_fini stops guc submission and also does a soft reset using
XE_GUC_ACTION_CLIENT_SOFT_RESET.
The GuC specification says, on XE_GUC_ACTION_CLIENT_SOFT_RESET, the GuC
revokes the log and CTB buffers.

So I believe, after that GuC shouldn't be accessing any buffers shared
by XeKMD. It must be safe to release the buffers after sending
XE_GUC_ACTION_CLIENT_SOFT_RESET.

Did we understand why are seeing this issue now but not earlier?
Is it a regresssion from a patch, either in KMD or GuC?

Regards,
Bala

> +	ret = xe_guc_pc_init(&guc->pc);
>  	if (ret)
>  		return ret;
>  
> -	ret = xe_guc_db_mgr_init(&guc->dbm, ~0);
> +	ret = xe_guc_engine_activity_init(guc);
>  	if (ret)
>  		return ret;
>  
> -	ret = xe_guc_pc_init(&guc->pc);
> +	ret = xe_guc_buf_cache_init_with_size(&guc->buf,
> +					      guc_additional_cache_size(xe));
>  	if (ret)
>  		return ret;
>  
> -	ret = xe_guc_rc_init(guc);
> +	/*
> +	 * The GuC keeps accessing its GGTT-mapped buffers until it is reset,
> +	 * so move guc_fini_hw() after all GuC allocations above to have the
> +	 * reset done before any of them is unpinned and its GGTT PTEs cleared.
> +	 * Teardown actions that must run before the reset, either because they
> +	 * still talk to the GuC (like GuC RC) or because they stop the CT and
> +	 * submission (like GuC submit), must be registered after this point.
> +	 */
> +	devm_remove_action(xe->drm.dev, guc_fini_hw, guc);
> +	ret = devm_add_action_or_reset(xe->drm.dev, guc_fini_hw, guc);
>  	if (ret)
>  		return ret;
>  
> -	ret = xe_guc_engine_activity_init(guc);
> +	ret = xe_guc_submit_init(guc);
>  	if (ret)
>  		return ret;
>  
> -	ret = xe_guc_buf_cache_init_with_size(&guc->buf,
> -					      guc_additional_cache_size(guc_to_xe(guc)));
> +	ret = xe_guc_db_mgr_init(&guc->dbm, ~0);
> +	if (ret)
> +		return ret;
> +
> +	ret = xe_guc_rc_init(guc);
>  	if (ret)
>  		return ret;
>  
> 
> base-commit: 5bc00022b24f8a7bc6630609e9fd93dd977a87d3
> -- 
> 2.34.1
>