Re: [PATCH v2] drm/xe/xe_guc: Dynamically decide g2g buffer owner

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 10/28/2025 9:00 AM, Jonathan Cavitt wrote:
> On today's driver, xe_device_get_gt(xe, 0); can never return NULL.
> Hardware-wise there's always at least one tile, and every tilie has a
> primary GT.  If something went wrong during init of tile or GT and we
> couldn't create/initialize the structures, then we already aborted the
> device probe immediately and we'll never get further on to places in the
> code that would be chasing a NULL pointer.
>
> However, there's currently ongoing work to allow the primary GT to be
> disabled via configfs for debugging purposes.  Once that lands, it will
> be possible for this query to return a NULL pointer.  This can cause
> problems in guc_g2g_alloc, as this process currently relies on the
> primary GT always being present.
>
> Instead of making the primary GT the g2g buffer owner, make the first
> GuC passed to guc_g2g_alloc the g2g buffer owner.  This requires keeping
> track of the g2g buffer owner in the xe device so each GuC can know if
> it's the owner or not during initialization.

I think we could still use a note here to mention that the kunit will 
always run with GT0 enabled and therefore we can directly use the root 
GT there.

>
> v2:
> - Update the kunit tests to clear and reset the g2g_owner variable as
>    needed (Daniele)
>
> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/xe/tests/xe_guc_g2g_test.c | 2 ++
>   drivers/gpu/drm/xe/xe_device_types.h       | 4 ++++
>   drivers/gpu/drm/xe/xe_guc.c                | 9 ++++++---
>   3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/tests/xe_guc_g2g_test.c b/drivers/gpu/drm/xe/tests/xe_guc_g2g_test.c
> index 3b213fcae916..6446232422e2 100644
> --- a/drivers/gpu/drm/xe/tests/xe_guc_g2g_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_guc_g2g_test.c
> @@ -358,6 +358,7 @@ static void g2g_distribute(struct kunit *test, struct xe_device *xe, struct xe_b
>   	root_gt = xe_device_get_gt(xe, 0);
>   	root_gt->uc.guc.g2g.bo = bo;
>   	root_gt->uc.guc.g2g.owned = true;
> +	xe->g2g_owner = &root_gt->uc.guc;
>   	kunit_info(test, "[%d.%d] Assigned 0x%p\n", gt_to_tile(root_gt)->id, root_gt->info.id, bo);
>   
>   	for_each_gt(gt, xe, i) {
> @@ -447,6 +448,7 @@ static void g2g_free(struct kunit *test, struct xe_device *xe)
>   
>   		gt->uc.guc.g2g.bo = NULL;
>   	}
> +	xe->g2g_owner = NULL;
>   }
>   
>   static void g2g_stop(struct kunit *test, struct xe_device *xe)
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index af0ce275b032..91ec9a295226 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -38,6 +38,7 @@ struct dram_info;
>   struct intel_display;
>   struct intel_dg_nvm_dev;
>   struct xe_ggtt;
> +struct xe_guc;
>   struct xe_i2c;
>   struct xe_pat_ops;
>   struct xe_pxp;
> @@ -628,6 +629,9 @@ struct xe_device {
>   	atomic_t g2g_test_count;
>   #endif
>   
> +	/** @g2g_owner: Pointer to the GuC that is the owner of the g2g buffer */
> +	struct xe_guc *g2g_owner;
> +
>   	/* private: */
>   
>   #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index ecc3e091b89e..a3a0961456d0 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -468,9 +468,8 @@ static int guc_g2g_alloc(struct xe_guc *guc)
>   	if (guc->g2g.bo)
>   		return 0;
>   
> -	if (gt->info.id != 0) {
> -		struct xe_gt *root_gt = xe_device_get_gt(xe, 0);
> -		struct xe_guc *root_guc = &root_gt->uc.guc;
> +	if (xe->g2g_owner) {
> +		struct xe_guc *root_guc = xe->g2g_owner;
>   		struct xe_bo *bo;
>   
>   		bo = xe_bo_get(root_guc->g2g.bo);

I think the name "root_guc" here might be a bit confusing if this can be 
a different GuC based on setup. Instead of renaming, IMO we can just 
drop the local root_guc variable and have:

         bo = xe_bo_get(xe->g2g_owner->g2g.bo);

With those nits addressed:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Note that the G2G test is not running in CI, so please make sure to run 
it locally and confirm there are no regressions before merging this patch.

Daniele

> @@ -495,6 +494,7 @@ static int guc_g2g_alloc(struct xe_guc *guc)
>   	xe_map_memset(xe, &bo->vmap, 0, 0, g2g_size);
>   	guc->g2g.bo = bo;
>   	guc->g2g.owned = true;
> +	xe->g2g_owner = guc;
>   
>   	return 0;
>   }
> @@ -507,6 +507,9 @@ static void guc_g2g_fini(struct xe_guc *guc)
>   	/* Unpinning the owned object is handled by generic shutdown */
>   	if (!guc->g2g.owned)
>   		xe_bo_put(guc->g2g.bo);
> +	/* g2g owner is no longer valid.  Mark as NULL in xe device */
> +	else
> +		guc_to_xe(guc)->g2g_owner = NULL;
>   
>   	guc->g2g.bo = NULL;
>   }