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

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

On 29.01.2025 11:16, 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 submission version (John)
> v3: use drm_warn_once to avoid stacktrace (Umesh)
> 
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_engine_activity.c | 32 +++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_guc_engine_activity.h |  1 +
>  2 files changed, 33 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..4d720afd12ac 100644
> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.c
> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
> @@ -250,6 +250,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 +266,32 @@ 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 submission version
> + * (1.14.1)
> + *
> + * Return: true if engine activity stats supported, false otherwise
> + */
> +bool xe_guc_engine_activity_supported(struct xe_guc *guc)
> +{
> +	if (GUC_SUBMIT_VER(guc) >= MAKE_GUC_VER(1, 14, 1))
> +		return true;

this patch seems to be redundant as patch:

[PATCH v4 5/8] drm/xe/guc: Bump minimum required GuC version to v70.36.0

now requires at least submit version 1.17.1

> +
> +	drm_warn_once(&guc_to_xe(guc)->drm,

btw, for GuC related logs we prefer xe_gt_info() logs

and _once() variant is n/a here as it will hide any other messages from
drivers running on other devices that might be present on the system

better option seems to be check/log once in the init() and then just set
a flag that could be used later

> +		      "per-engine-class activity not supported for this GuC version\n");

and maybe we should be more friendly and instead "this" also print
actual version used to save the user looking for it elsewhere

> +
> +	return false;
> +}
> +
>  /**
>   * xe_guc_engine_activity_enable_stats - Enable engine activity stats
>   * @guc: The GuC object
> @@ -276,6 +302,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 +331,9 @@ int xe_guc_engine_activity_init(struct xe_guc *guc)
>  	struct xe_gt *gt = guc_to_gt(guc);
>  	int ret;
>  
> +	if (!xe_guc_engine_activity_supported(guc))
> +		return 0;
> +
>  	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);