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

[Riana Tauro <riana.tauro@intel.com>]

Hi Umesh

Thank you for the review
On 12/21/2023 6:22 AM, Umesh Nerlige Ramappa wrote:
> 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.
Sure. Will remove the comments.

Thanks
Riana
> 
> 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
>>