Re: [PATCH v5 3/8] drm/xe/guc: Expose engine activity only for supported GuC version

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 06.02.2025 11:43, Riana Tauro wrote:
> Engine activity is supported only on GuC submission version >= 1.14.1
> Allow enabling/reading engine activity only on supported
> GuC versions. Warn once if not supported.
> 
> v2: use guc interface version (John)
> v3: do not use drm_WARN (Umesh)
> v4: use variable for supported and use gt logs
>     use a friendlier log message (Michal)
> 
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_engine_activity.c   | 42 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_guc_engine_activity.h   |  1 +
>  .../gpu/drm/xe/xe_guc_engine_activity_types.h |  3 ++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity.c b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
> index 9c08af273397..5d67fe38639a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.c
> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
> @@ -89,6 +89,22 @@ static int allocate_engine_activity_buffers(struct xe_guc *guc,
>  	return 0;
>  }
>  
> +static bool engine_activity_supported(struct xe_guc *guc)

maybe rename it a little to distinguish from other function that just
look at engine_activity->supported flag?

> +{
> +	struct xe_uc_fw_version *version = &guc->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
> +	struct xe_gt *gt = guc_to_gt(guc);
> +
> +	/* engine activity stats is supported from GuC interface version (1.14.1) */
> +	if (GUC_SUBMIT_VER(guc) >= MAKE_GUC_VER(1, 14, 1))
> +		return true;

it also doesn't work on VFs
so maybe it should be different logic:

	if (IS_SRIOV_VF) {
		xt_gt_into("EA not supported on VFs\n");
		return false;
	}

	if (GUC_SUBMIT_VER < 1.14.1) {
		xt_gt_into("EA not supported on vA, need vB+\n");
		return false;
	}

> +
> +	xe_gt_warn(gt,

does it really need to be 'warn' level?

> +		   "engine activity stats unsupported in GuC interface v%u.%u.%u, v%u.%u.%u or newer required\n",
> +		   version->major, version->minor, version->patch, 1, 14, 1);
> +
> +	return false;
> +}
> +
>  static struct engine_activity *hw_engine_to_engine_activity(struct xe_hw_engine *hwe)
>  {
>  	struct xe_guc *guc = &hwe->gt->uc.guc;
> @@ -250,6 +266,9 @@ u64 xe_guc_engine_activity_active_ticks(struct xe_hw_engine *hwe)
>  {
>  	struct xe_guc *guc =  &hwe->gt->uc.guc;
>  
> +	if (!xe_guc_engine_activity_supported(guc))
> +		return 0;
> +
>  	return get_engine_active_ticks(guc, hwe);
>  }
>  
> @@ -263,9 +282,27 @@ u64 xe_guc_engine_activity_total_ticks(struct xe_hw_engine *hwe)
>  {
>  	struct xe_guc *guc =  &hwe->gt->uc.guc;
>  
> +	if (!xe_guc_engine_activity_supported(guc))
> +		return 0;
> +
>  	return get_engine_total_ticks(guc, hwe);
>  }
>  
> +/**
> + * xe_guc_engine_activity_supported - Check support for engine activity stats
> + * @guc: The GuC object
> + *
> + * Engine activity stats is supported from GuC interface version (1.14.1)

what about VFs?

> + *
> + * Return: true if engine activity stats supported, false otherwise
> + */
> +bool xe_guc_engine_activity_supported(struct xe_guc *guc)
> +{
> +	struct xe_guc_engine_activity *engine_activity = &guc->engine_activity;
> +
> +	return engine_activity->supported;
> +}
> +
>  /**
>   * xe_guc_engine_activity_enable_stats - Enable engine activity stats
>   * @guc: The GuC object
> @@ -276,6 +313,9 @@ void xe_guc_engine_activity_enable_stats(struct xe_guc *guc)
>  {
>  	int ret;
>  
> +	if (!xe_guc_engine_activity_supported(guc))
> +		return;
> +
>  	ret = enable_engine_activity_stats(guc);
>  	if (ret)
>  		xe_gt_err(guc_to_gt(guc), "failed to enable activity stats%d\n", ret);
> @@ -302,6 +342,8 @@ int xe_guc_engine_activity_init(struct xe_guc *guc)
>  	struct xe_gt *gt = guc_to_gt(guc);
>  	int ret;
>  
> +	engine_activity->supported = engine_activity_supported(guc);

should we continue if not supported ?

> +
>  	ret = allocate_engine_activity_group(guc);
>  	if (ret) {
>  		xe_gt_err(gt, "failed to allocate activity group %d\n", ret);
> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity.h b/drivers/gpu/drm/xe/xe_guc_engine_activity.h
> index c00f3da5513d..9d3ea3f67b6a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.h
> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.h
> @@ -12,6 +12,7 @@ struct xe_hw_engine;
>  struct xe_guc;
>  
>  int xe_guc_engine_activity_init(struct xe_guc *guc);
> +bool xe_guc_engine_activity_supported(struct xe_guc *guc);
>  void xe_guc_engine_activity_enable_stats(struct xe_guc *guc);
>  u64 xe_guc_engine_activity_active_ticks(struct xe_hw_engine *hwe);
>  u64 xe_guc_engine_activity_total_ticks(struct xe_hw_engine *hwe);
> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h b/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h
> index a2ab327d3eec..81002c83d65e 100644
> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h
> @@ -79,6 +79,9 @@ struct xe_guc_engine_activity {
>  	/** @num_activity_group: number of activity groups */
>  	u32 num_activity_group;
>  
> +	/** @supported: checks if engine activity is supported */

indicates?

> +	bool supported;
> +
>  	/** @eag: holds the device level engine activity data */
>  	struct engine_activity_group *eag;
>