Re: [PATCH v2] drm/xe/guc: Request max GT freq during resume

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jun 04, 2024 at 04:37:51PM -0700, Vinay Belgaumkar wrote:
> We already request max freq in the load path, moving it
> to __xe_guc_upload will ensure this speeds up GuC load in
> the resume path as well.
> 
> v2: Rename xe_guc_pc_init_early since we now call it per
> GuC load (Michal W)
> 
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc.c    | 6 +++---
>  drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++--
>  drivers/gpu/drm/xe/xe_guc_pc.h | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 086a048876ba..dff9a9dfda87 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -696,6 +696,9 @@ static int __xe_guc_upload(struct xe_guc *guc)
>  {
>  	int ret;
>  
> +	/* Raise GT freq to speed up HuC/GuC load */
> +	xe_guc_pc_raise_unslice(&guc->pc);
> +
>  	guc_write_params(guc);
>  	guc_prepare_xfer(guc);
>  
> @@ -781,9 +784,6 @@ int xe_guc_min_load_for_hwconfig(struct xe_guc *guc)
>  
>  	xe_guc_ads_populate_minimal(&guc->ads);
>  
> -	/* 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;
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 8187dfb2ad6c..eb7950c36151 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -675,11 +675,11 @@ static void pc_init_fused_rp_values(struct xe_guc_pc *pc)
>  }
>  
>  /**
> - * xe_guc_pc_init_early - Initialize RPx values and request a higher GT
> + * xe_guc_pc_raise_unslice - Initialize RPx values and request a higher GT
>   * frequency to allow faster GuC load times
>   * @pc: Xe_GuC_PC instance
>   */
> -void xe_guc_pc_init_early(struct xe_guc_pc *pc)
> +void xe_guc_pc_raise_unslice(struct xe_guc_pc *pc)

I agree with the intention and the reasoning and the new name.
I just feel that it would be better to move pc_init_fused_rp_values
to some kind of init_early function that gets to run only once
at early stages of the probe.

>  {
>  	struct xe_gt *gt = pc_to_gt(pc);

I was skeptical about the forcewake assert that we have underneath here.
But I'm glad that it doesn't seem to be an issue :)

>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h
> index 532cac985a6d..ae000e66684f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
> @@ -28,6 +28,6 @@ int xe_guc_pc_set_max_freq(struct xe_guc_pc *pc, u32 freq);
>  enum xe_gt_idle_state xe_guc_pc_c_status(struct xe_guc_pc *pc);
>  u64 xe_guc_pc_rc6_residency(struct xe_guc_pc *pc);
>  u64 xe_guc_pc_mc6_residency(struct xe_guc_pc *pc);
> -void xe_guc_pc_init_early(struct xe_guc_pc *pc);
> +void xe_guc_pc_raise_unslice(struct xe_guc_pc *pc);
>  
>  #endif /* _XE_GUC_PC_H_ */
> -- 
> 2.38.1
>