Re: [Intel-xe] [PATCH] drm/xe: Raise GT frequency before GuC load

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

On Fri, Oct 20, 2023 at 04:25:53PM -0700, Vinay Belgaumkar wrote:
> Starting GT freq is usually RPn. Raising freq to RP0 will
> help speed up GuC load times. As an example, this data was
> collected on DG2-
> 
> GuC Load time @RPn ~ 41 ms
> GuC Load time @RP0 ~ 11 ms
> 
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_gt_regs.h |  7 +++++
>  drivers/gpu/drm/xe/xe_guc.c          |  3 ++
>  drivers/gpu/drm/xe/xe_guc_pc.c       | 44 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/xe/xe_guc_pc.h       |  1 +
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index cd1821d96a5d..0e6fe2ee4a2c 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -244,6 +244,13 @@
>  
>  #define RPNSWREQ				XE_REG(0xa008)
>  #define   REQ_RATIO_MASK			REG_GENMASK(31, 23)
> +#define   SW_REQ_UNSLICE_RATIO_SHIFT		23

please avoid shift and use the REG_FIELD_PREP and REG_FIELD_GET

> +#define   IGNORE_SLICE_RATIO			(0 << 0)

x | 0 = x
so, please avoid it.

> +
> +#define RP_CONTROL				XE_REG(0xa024)
> +#define   RPSWCTL_SHIFT			9

same here

> +#define   RPSWCTL_ENABLE			(0x2 << RPSWCTL_SHIFT)
> +#define   RPSWCTL_DISABLE			(0x0 << RPSWCTL_SHIFT)

Please use REG_GENMASK and REG_FIELD_PREP

something like

#define   RPSWCTL_MASK		REG_GENMASK(something, 9)
#define   RPSWCTL_ENABLE	REG_FIELD_PREP(RPSWCTL_MASK, 2)
#define   RPSWCTL_DISABLE	REG_FIELD_PREP(RPSWCTL_MASK, 0)

>  #define RC_CONTROL				XE_REG(0xa090)
>  #define RC_STATE				XE_REG(0xa094)
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 84f0b5488783..5af97982564a 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -447,6 +447,9 @@ static int __xe_guc_upload(struct xe_guc *guc)
>  {
>  	int ret;
>  
> +	/* Raise GT freq to speed up GuC load */
> +	xe_guc_pc_request_rp0(&guc->pc);
> +

I have a strange feeling with this. Semantically...

guc_load asking guc_pc to do something...

we will need to have a xe_freq component that is detached from
the xe_guc_pc and then xe_freq exposes the sysfs and functions
like this and then it requests to guc_pc when it makes sense.

that can be done later, I agree, but we would need to at least
give this function a better name....

>  	guc_write_params(guc);
>  	guc_prepare_xfer(guc);
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index d9375d1d582f..e1543478c627 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -247,6 +247,12 @@ static u32 decode_freq(u32 raw)
>  				 GEN9_FREQ_SCALER);
>  }
>  
> +static u32 encode_freq(u32 freq)
> +{
> +	return DIV_ROUND_CLOSEST(freq * GEN9_FREQ_SCALER,
> +				 GT_FREQUENCY_MULTIPLIER);
> +}
> +
>  static u32 pc_get_min_freq(struct xe_guc_pc *pc)
>  {
>  	u32 freq;
> @@ -257,6 +263,28 @@ static u32 pc_get_min_freq(struct xe_guc_pc *pc)
>  	return decode_freq(freq);
>  }
>  
> +static void pc_set_manual_rp_ctrl(struct xe_guc_pc *pc, bool enable)
> +{
> +	struct xe_gt *gt = pc_to_gt(pc);
> +	u32 state = enable ? RPSWCTL_ENABLE : RPSWCTL_DISABLE;
> +
> +	/* Allow/Disallow punit to process software freq requests */
> +	xe_mmio_write32(gt, RP_CONTROL, state);
> +}
> +
> +static void pc_set_cur_freq(struct xe_guc_pc *pc, u32 freq)
> +{
> +	struct xe_gt *gt = pc_to_gt(pc);
> +
> +	pc_set_manual_rp_ctrl(pc, true);
> +
> +	/* Req freq is in units of 16.66 Mhz */
> +	xe_mmio_write32(gt, RPNSWREQ, (encode_freq(freq) << SW_REQ_UNSLICE_RATIO_SHIFT)
> +			| IGNORE_SLICE_RATIO);
> +
> +	pc_set_manual_rp_ctrl(pc, false);
> +}
> +
>  static int pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
>  {
>  	/*
> @@ -685,6 +713,20 @@ static void pc_init_fused_rp_values(struct xe_guc_pc *pc)
>  	else
>  		tgl_init_fused_rp_values(pc);
>  }
> +
> +/**
> + * xe_guc_pc_request_rp0 - Request RP0
> + * @pc: Xe_GuC_PC instance

some more information here on the purpose and usage of this function
is needed. Specially because it should be avoided on regular cases.

> + */
> +void xe_guc_pc_request_rp0(struct xe_guc_pc *pc)
> +{
> +	struct xe_gt *gt = pc_to_gt(pc);
> +
> +	xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
> +	pc_init_fused_rp_values(pc);
> +	pc_set_cur_freq(pc, pc->rp0_freq);
> +}
> +
>  static int pc_adjust_freq_bounds(struct xe_guc_pc *pc)
>  {
>  	int ret;
> @@ -918,8 +960,6 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>  
>  	pc->bo = bo;
>  
> -	pc_init_fused_rp_values(pc);
> -

hmmm... this also shows that we are then calling a function
of this component before the component is initialized!!

so, maybe we need a function called xe_guc_pc_init_early()
and then this function by itself sets this pc->rp* stuff
and manually force the freq to rp0. And we document that
the pre_init or init_early needs to be called before GuC
is loaded for faster loading of GuC.

>  	err = sysfs_create_files(gt->sysfs, pc_attrs);
>  	if (err)
>  		return err;
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h
> index 43ea582545b5..8bed5d9fc12a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
> @@ -17,4 +17,5 @@ int xe_guc_pc_gucrc_disable(struct xe_guc_pc *pc);
>  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_request_rp0(struct xe_guc_pc *pc);
>  #endif /* _XE_GUC_PC_H_ */
> -- 
> 2.38.1
>