Re: [PATCH v2 3/8] RFC drm/xe/guc: Expose engine busyness only for supported GuC version

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

On Thu, Dec 07, 2023 at 06:27:57PM +0530, Riana Tauro wrote:
>Guc version numbers are 8 bits only so convert to 32 bit 8.8.8
>to allow version comparisions. use compatibility version
>for the same.
>
>Engine busyness is supported only on GuC versions >= 70.11.1.
>Allow enabling/reading engine busyness only on supported
>GuC versions. Warn once if not supported.
>
>v2: rebase
>    fix guc comparison error (Matthew Brost)
>    add a macro for guc version comparison
>
>Cc: John Harrison <John.C.Harrison@Intel.com>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>---
> drivers/gpu/drm/xe/xe_guc_engine_busyness.c | 29 +++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_guc_engine_busyness.c b/drivers/gpu/drm/xe/xe_guc_engine_busyness.c
>index 287429e31e6c..431d1ca59d2f 100644
>--- a/drivers/gpu/drm/xe/xe_guc_engine_busyness.c
>+++ b/drivers/gpu/drm/xe/xe_guc_engine_busyness.c
>@@ -23,6 +23,23 @@
>  * engine busyness ticks (ticks_engine) : clock ticks for which engine was active
>  */
>
>+/* GuC version number components are only 8-bit, so converting to a 32bit 8.8.8 */
>+#define GUC_VER(maj, min, pat)	(((maj) << 16) | ((min) << 8) | (pat))
>+
>+static bool guc_engine_busyness_supported(struct xe_guc *guc)
>+{
>+	struct xe_uc_fw *uc_fw = &guc->fw;
>+	struct xe_uc_fw_version *version = &uc_fw->versions.found[XE_UC_FW_VER_COMPATIBILITY];
>+
>+	if (GUC_VER(version->major, version->minor, version->patch) >= GUC_VER(1, 3, 1))
>+		return true;
>+
>+	drm_WARN_ON_ONCE(&guc_to_xe(guc)->drm,
>+			 "Engine busyness not supported in this GuC version\n");
>+
>+	return false;
>+}
>+
> static void guc_engine_busyness_usage_map(struct xe_guc *guc,
> 					  struct xe_hw_engine *hwe,
> 					  struct iosys_map *engine_map)
>@@ -80,6 +97,10 @@ static void guc_engine_busyness_enable_stats(struct xe_guc *guc)
> 	struct xe_device *xe = guc_to_xe(guc);
> 	int ret;
>
>+	/* Engine busyness supported only on GuC >= 70.11.1 */
>+	if (!guc_engine_busyness_supported(guc))
>+		return;
>+
> 	ret = xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
> 	if (ret)
> 		drm_err(&xe->drm, "Failed to enable usage stats %pe", ERR_PTR(ret));
>@@ -104,6 +125,10 @@ u64 xe_guc_engine_busyness_ticks(struct xe_guc *guc, struct xe_hw_engine *hwe)
> {
> 	u64 ticks_engine;
>
>+	/* Engine busyness supported only on GuC >= 70.11.1 */
>+	if (!guc_engine_busyness_supported(guc))
>+		return 0;
>+
> 	guc_engine_busyness_get_usage(guc, hwe, &ticks_engine);
>
> 	return ticks_engine;
>@@ -127,6 +152,10 @@ int xe_guc_engine_busyness_init(struct xe_guc *guc)
> 	u32 size;
> 	int err;
>
>+	/* Engine busyness supported only on GuC >= 70.11.1 */

I think the comment can reside in the helper function and you can delete 
is here and other places where this is called.

With that, this is
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>


>+	if (!guc_engine_busyness_supported(guc))
>+		return 0;
>+
> 	/* Initialization already done */
> 	if (guc->busy.bo)
> 		return 0;
>-- 
>2.40.0
>