Re: [Intel-xe] [PATCH v3 20/20] drm/xe: Initialize GuC earlier during probe

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Nov 14, 2023 at 02:02:31PM +0100, Michał Winiarski wrote:
> SR-IOV VF has limited access to MMIO registers. Fortunately, it is able
> to access a curated subset that is needed to initialize the driver by
> communicating with SR-IOV PF using GuC CT.
> Initialize GuC earlier in order to keep the unified probe ordering
> between VF and PF modes.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c | 11 +++++++++++
>  drivers/gpu/drm/xe/xe_gt.c     | 16 ++++------------
>  drivers/gpu/drm/xe/xe_guc.c    | 15 ++++++++++-----
>  drivers/gpu/drm/xe/xe_uc.c     | 35 ++++++++++++++++++++++++++--------
>  4 files changed, 52 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 531aca74e8eb4..ecbf1effecc41 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -32,6 +32,7 @@
>  #include "xe_pat.h"
>  #include "xe_pcode.h"
>  #include "xe_pm.h"
> +#include "xe_uc.h"
>  #include "xe_query.h"
>  #include "xe_tile.h"
>  #include "xe_ttm_stolen_mgr.h"
> @@ -414,6 +415,16 @@ int xe_device_probe(struct xe_device *xe)
>  			return err;
>  	}
>  
> +	for_each_gt(gt, xe, id) {
> +		err = xe_uc_init(&gt->uc);
> +		if (err)
> +			return err;
> +
> +		err = xe_uc_init_hwconfig(&gt->uc);
> +		if (err)
> +			return err;
> +	}
> +

I think this should be a GT level function rather touching the UC layer
here. Also maybe combine xe_force_wake_init_gt & xe_ggtt_init_early into
this new function too if it makes sense.

Also a comment of why this located wouldn't hurt either.

Other than these nits the patch itslef makes sense to me.

Matt

>  	err = drmm_add_action_or_reset(&xe->drm, xe_driver_flr_fini, xe);
>  	if (err)
>  		return err;
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 73665e4e66f22..bee661e1f164a 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -344,14 +344,6 @@ static int gt_fw_domain_init(struct xe_gt *gt)
>  			goto err_force_wake;
>  	}
>  
> -	err = xe_uc_init(&gt->uc);
> -	if (err)
> -		goto err_force_wake;
> -
> -	err = xe_uc_init_hwconfig(&gt->uc);
> -	if (err)
> -		goto err_force_wake;
> -
>  	xe_gt_idle_sysfs_init(&gt->gtidle);
>  
>  	/* XXX: Fake that we pull the engine mask from hwconfig blob */
> @@ -415,10 +407,6 @@ static int all_fw_domain_init(struct xe_gt *gt)
>  	if (err)
>  		goto err_force_wake;
>  
> -	err = xe_uc_init_post_hwconfig(&gt->uc);
> -	if (err)
> -		goto err_force_wake;
> -
>  	if (!xe_gt_is_media_type(gt)) {
>  		/*
>  		 * USM has its only SA pool to non-block behind user operations
> @@ -442,6 +430,10 @@ static int all_fw_domain_init(struct xe_gt *gt)
>  		}
>  	}
>  
> +	err = xe_uc_init_post_hwconfig(&gt->uc);
> +	if (err)
> +		goto err_force_wake;
> +
>  	err = xe_uc_init_hw(&gt->uc);
>  	if (err)
>  		goto err_force_wake;
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index e04b04be32b7d..895bcc24ae2c8 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -572,26 +572,31 @@ static int __xe_guc_upload(struct xe_guc *guc)
>   */
>  int xe_guc_min_load_for_hwconfig(struct xe_guc *guc)
>  {
> +	struct xe_gt *gt = guc_to_gt(guc);
>  	int ret;
>  
>  	xe_guc_ads_populate_minimal(&guc->ads);
>  
> +	ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (ret)
> +		return ret;
> +
>  	/* Raise GT freq to speed up HuC/GuC load */
>  	xe_guc_pc_init_early(&guc->pc);
>  
>  	ret = __xe_guc_upload(guc);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	ret = xe_guc_hwconfig_init(guc);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	ret = xe_guc_enable_communication(guc);
> -	if (ret)
> -		return ret;
> +out:
> +	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  int xe_guc_upload(struct xe_guc *guc)
> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index 13e76e6805ca1..069207a7f2f21 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -29,8 +29,16 @@ uc_to_xe(struct xe_uc *uc)
>  /* Should be called once at driver load only */
>  int xe_uc_init(struct xe_uc *uc)
>  {
> +	struct xe_device *xe = uc_to_xe(uc);
> +	struct xe_gt *gt = uc_to_gt(uc);
>  	int ret;
>  
> +	xe_device_mem_access_get(xe);
> +
> +	ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (ret)
> +		goto err;
> +
>  	/*
>  	 * We call the GuC/HuC init functions even if GuC submission is off to
>  	 * correctly move our tracking of the FW state to "disabled".
> @@ -38,26 +46,36 @@ int xe_uc_init(struct xe_uc *uc)
>  
>  	ret = xe_guc_init(&uc->guc);
>  	if (ret)
> -		goto err;
> +		goto err_fw;
>  
>  	ret = xe_huc_init(&uc->huc);
>  	if (ret)
> -		goto err;
> +		goto err_fw;
>  
>  	if (!xe_device_uc_enabled(uc_to_xe(uc)))
> -		return 0;
> +		goto out;
> +
>  
>  	ret = xe_wopcm_init(&uc->wopcm);
>  	if (ret)
> -		goto err;
> +		goto err_fw;
>  
>  	ret = xe_guc_submit_init(&uc->guc);
>  	if (ret)
> -		goto err;
> +		goto err_fw;
> +
> +out:
> +	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> +
> +	xe_device_mem_access_put(xe);
>  
>  	return 0;
>  
> +err_fw:
> +	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>  err:
> +	xe_device_mem_access_put(xe);
> +
>  	return ret;
>  }
>  
> @@ -117,17 +135,18 @@ int xe_uc_sanitize_reset(struct xe_uc *uc)
>   */
>  int xe_uc_init_hwconfig(struct xe_uc *uc)
>  {
> +	struct xe_device *xe = uc_to_xe(uc);
>  	int ret;
>  
>  	/* GuC submission not enabled, nothing to do */
>  	if (!xe_device_uc_enabled(uc_to_xe(uc)))
>  		return 0;
>  
> +	xe_device_mem_access_get(xe);
>  	ret = xe_guc_min_load_for_hwconfig(&uc->guc);
> -	if (ret)
> -		return ret;
> +	xe_device_mem_access_put(xe);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.42.1
>