Re: [PATCH 6/7] drm/xe/guc: Expose engine busyness only for supported GuC version

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Nov 13, 2024 at 10:25:48AM +0530, Riana Tauro wrote:
>Engine busyness is supported only on GuC versions >= 70.32.0
>Allow enabling/reading engine busyness only on supported
>GuC versions. Warn once if not supported.
>
>Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>---
> drivers/gpu/drm/xe/xe_engine_activity.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_engine_activity.c b/drivers/gpu/drm/xe/xe_engine_activity.c
>index 7a2ae09d23de..fc34dcf3057a 100644
>--- a/drivers/gpu/drm/xe/xe_engine_activity.c
>+++ b/drivers/gpu/drm/xe/xe_engine_activity.c
>@@ -236,6 +236,16 @@ static u32 gpm_timestamp_shift(struct xe_gt *gt)
> 	return 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);
> }
>
>+static bool guc_engine_activity_supported(struct xe_guc *guc)
>+{
>+	if (GUC_FIRMWARE_VER(guc) >= MAKE_GUC_VER(70, 32, 0))
>+		return true;
>+
>+	drm_WARN_ON_ONCE(&guc_to_xe(guc)->drm,
>+			 "Engine busyness not supported in this GuC version\n");
>+	return false;
>+}
>+
> /**
>  * xe_engine_activity_get_active_ticks - Get engine active ticks
>  * @hwe: The hw_engine object
>@@ -246,6 +256,9 @@ u64 xe_engine_activity_get_active_ticks(struct xe_hw_engine *hwe)
> {
> 	struct xe_guc *guc =  &hwe->gt->uc.guc;
>
>+	if (!guc_engine_activity_supported(guc))
>+		return 0;
>+
> 	return get_engine_active_ticks(guc, hwe);
> }
>
>@@ -259,6 +272,9 @@ u64 xe_engine_activity_get_total_ticks(struct xe_hw_engine *hwe)
> {
> 	struct xe_guc *guc =  &hwe->gt->uc.guc;
>
>+	if (!guc_engine_activity_supported(guc))
Since total ticks is used in the denominator, UMDs would run into 
divide-by-zero if they implemented this feature, but do not have an 
updated GuC. We ran into that issue with some older version of this 
feature and UMDs patched it up in their code. Do you think we can return 
the cpu time here converted to gpu ticks? That way with unsupported GuC, 
UMDs still get 0 busyness.

Thanks,
Umesh
>+		return 0;
>+
> 	return get_engine_total_ticks(guc, hwe);
> }
>
>@@ -272,6 +288,9 @@ void xe_engine_activity_enable_stats(struct xe_guc *guc)
> {
> 	int ret;
>
>+	if (!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);
>@@ -298,6 +317,9 @@ int xe_engine_activity_init(struct xe_guc *guc)
> 	struct xe_gt *gt = guc_to_gt(guc);
> 	int ret;
>
>+	if (!guc_engine_activity_supported(guc))
>+		return 0;
>+
> 	ret = allocate_activity_group(engine_busy);
> 	if (ret) {
> 		xe_gt_err(gt, "failed to allocate activity group %d\n", ret);
>-- 
>2.40.0
>