Re: [PATCH v4 6/8] drm/xe: Add support for per-function engine activity

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

Hi Umesh

Thank you for the review comments

On 2/1/2025 5:22 AM, Umesh Nerlige Ramappa wrote:
> On Wed, Jan 29, 2025 at 03:46:49PM +0530, Riana Tauro wrote:
>> Add support for function level per-engine-class activity stats.
>> This is enabled when sriov_numvfs is set and disabled when vf's
>> are disabled.
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> drivers/gpu/drm/xe/abi/guc_actions_abi.h      |   1 +
>> drivers/gpu/drm/xe/xe_guc_engine_activity.c   | 200 +++++++++++++++---
>> drivers/gpu/drm/xe/xe_guc_engine_activity.h   |   5 +-
>> .../gpu/drm/xe/xe_guc_engine_activity_types.h |   8 +-
>> drivers/gpu/drm/xe/xe_pmu.c                   |   4 +-
>> 5 files changed, 186 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h b/drivers/gpu/ 
>> drm/xe/abi/guc_actions_abi.h
>> index ec516e838ee8..448afb86e05c 100644
>> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> @@ -141,6 +141,7 @@ enum xe_guc_action {
>>     XE_GUC_ACTION_CLIENT_SOFT_RESET = 0x5507,
>>     XE_GUC_ACTION_SET_ENG_UTIL_BUFF = 0x550A,
>>     XE_GUC_ACTION_SET_DEVICE_ENGINE_ACTIVITY_BUFFER = 0x550C,
>> +    XE_GUC_ACTION_SET_FUNCTION_ENGINE_ACTIVITY_BUFFER = 0x550D,
>>     XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR = 0x6000,
>>     XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC = 0x6002,
>>     XE_GUC_ACTION_PAGE_FAULT_RES_DESC = 0x6003,
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity.c b/drivers/ 
>> gpu/drm/xe/xe_guc_engine_activity.c
>> index 4d720afd12ac..0ab716a58d5c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> @@ -15,35 +15,61 @@
>> #include "xe_hw_engine.h"
>> #include "xe_map.h"
>> #include "xe_mmio.h"
>> +#include "xe_sriov_pf_helpers.h"
>> #include "xe_trace_guc.h"
>>
>> #define TOTAL_QUANTA 0x8000
>>
>> -static struct iosys_map engine_activity_map(struct xe_guc *guc, 
>> struct xe_hw_engine *hwe)
>> +static struct iosys_map engine_activity_map(struct xe_guc *guc, 
>> struct xe_hw_engine *hwe,
>> +                        unsigned int index)
>> {
>>     struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>>     struct engine_activity_buffer *buffer = &engine_activity- 
>> >device_buffer;
> You can drop the initialization here since it is being set below.
yeah missed to do this. will fix it
> 
>>     u16 guc_class = xe_engine_class_to_guc_class(hwe->class);
>>     size_t offset = 0;
> For readability, you could set offset = 0 in the else part instead of here.
okay will add it below
>>
>> -    offset = offsetof(struct guc_engine_activity_data,
>> +    if (index) {
>> +        buffer = &engine_activity->function_buffer;
>> +        offset = sizeof(struct guc_engine_activity_data) * (index - 1);
>> +    } else {
>> +        buffer = &engine_activity->device_buffer;
>> +    }
>> +
>> +    offset += offsetof(struct guc_engine_activity_data,
>>               engine_activity[guc_class][hwe->logical_instance]);
>>
>>     return IOSYS_MAP_INIT_OFFSET(&buffer->activity_bo->vmap, offset);
>> }
>>
>> -static struct iosys_map engine_metadata_map(struct xe_guc *guc)
>> +static struct iosys_map engine_metadata_map(struct xe_guc *guc,
>> +                        unsigned int index)
>> {
>>     struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> -    struct engine_activity_buffer *buffer = &engine_activity- 
>> >device_buffer;
>> +    struct engine_activity_buffer *buffer;
>> +    size_t offset = 0;
> Same here ^
> 
>> +
>> +    if (index) {
>> +        buffer = &engine_activity->function_buffer;
>> +        offset = sizeof(struct guc_engine_activity_metadata) * (index 
>> - 1);
>> +    } else {
>> +        buffer = &engine_activity->device_buffer;
>> +    }
>>
>> -    return buffer->metadata_bo->vmap;
>> +    return IOSYS_MAP_INIT_OFFSET(&buffer->metadata_bo->vmap, offset);
>> }
>>
>> static int allocate_engine_activity_group(struct xe_guc *guc)
>> {
>>     struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> -    u32 num_activity_group = 1;
>> +    struct xe_device *xe = guc_to_xe(guc);
>> +    u32 num_activity_group;
>> +
>> +    /*
>> +     * Two additional activity groups are allocated one for global
>> +     * and one for PF engine activity when SRIOV is enabled
>> +     */
>> +    num_activity_group = IS_SRIOV_PF(xe) ? 
>> xe_sriov_pf_get_totalvfs(xe) + 2 : 1;
>> +
>>
>>     engine_activity->eag = kmalloc_array(num_activity_group,
>>                          sizeof(struct engine_activity_group),
>> @@ -59,10 +85,11 @@ static int allocate_engine_activity_group(struct 
>> xe_guc *guc)
>> }
>>
>> static int allocate_engine_activity_buffers(struct xe_guc *guc,
>> -                        struct engine_activity_buffer *buffer)
>> +                        struct engine_activity_buffer *buffer,
>> +                        int count)
>> {
>> -    u32 metadata_size = sizeof(struct guc_engine_activity_metadata);
>> -    u32 size = sizeof(struct guc_engine_activity_data);
>> +    u32 metadata_size = sizeof(struct guc_engine_activity_metadata) * 
>> count;
>> +    u32 size = sizeof(struct guc_engine_activity_data) * count;
>>     struct xe_gt *gt = guc_to_gt(guc);
>>     struct xe_tile *tile = gt_to_tile(gt);
>>     struct xe_bo *bo, *metadata_bo;
>> @@ -89,10 +116,17 @@ static int 
>> allocate_engine_activity_buffers(struct xe_guc *guc,
>>     return 0;
>> }
>>
>> -static struct engine_activity *hw_engine_to_engine_activity(struct 
>> xe_hw_engine *hwe)
>> +static void free_engine_activity_buffers(struct 
>> engine_activity_buffer *buffer)
>> +{
>> +    xe_bo_unpin_map_no_vm(buffer->metadata_bo);
> +    xe_bo_unpin_map_no_vm(buffer->activity_bo);
>> +}
>> +
>> +static struct engine_activity *hw_engine_to_engine_activity(struct 
>> xe_hw_engine *hwe,
>> +                                unsigned int index)
>> {
>>     struct xe_guc *guc = &hwe->gt->uc.guc;
>> -    struct engine_activity_group *eag = &guc->engine_activity.eag[0];
>> +    struct engine_activity_group *eag = &guc- 
>> >engine_activity.eag[index];
>>     u16 guc_class = xe_engine_class_to_guc_class(hwe->class);
>>
>>     return &eag->engine[guc_class][hwe->logical_instance];
>> @@ -109,9 +143,10 @@ static u64 cpu_ns_to_guc_tsc_tick(ktime_t ns, u32 
>> freq)
>> #define read_metadata_record(xe_, map_, field_) \
>>     xe_map_rd_field(xe_, map_, 0, struct guc_engine_activity_metadata, 
>> field_)
>>
>> -static u64 get_engine_active_ticks(struct xe_guc *guc, struct 
>> xe_hw_engine *hwe)
>> +static u64 get_engine_active_ticks(struct xe_guc *guc, struct 
>> xe_hw_engine *hwe,
>> +                   unsigned int index)
>> {
>> -    struct engine_activity *ea = hw_engine_to_engine_activity(hwe);
>> +    struct engine_activity *ea = hw_engine_to_engine_activity(hwe, 
>> index);
>>     struct guc_engine_activity *cached_activity = &ea->activity;
>>     struct guc_engine_activity_metadata *cached_metadata = &ea->metadata;
>>     struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> @@ -122,8 +157,8 @@ static u64 get_engine_active_ticks(struct xe_guc 
>> *guc, struct xe_hw_engine *hwe)
>>     u64 active_ticks, gpm_ts;
>>     u16 change_num;
>>
>> -    activity_map = engine_activity_map(guc, hwe);
>> -    metadata_map = engine_metadata_map(guc);
>> +    activity_map = engine_activity_map(guc, hwe, index);
>> +    metadata_map = engine_metadata_map(guc, index);
>>     global_change_num = read_metadata_record(xe, &metadata_map, 
>> global_change_num);
>>
>>     /* GuC has not initialized activity data yet, return 0 */
>> @@ -166,9 +201,9 @@ static u64 get_engine_active_ticks(struct xe_guc 
>> *guc, struct xe_hw_engine *hwe)
>>     return ea->total + ea->active;
>> }
>>
>> -static u64 get_engine_total_ticks(struct xe_guc *guc, struct 
>> xe_hw_engine *hwe)
>> +static u64 get_engine_total_ticks(struct xe_guc *guc, struct 
>> xe_hw_engine *hwe, unsigned int index)
>> {
>> -    struct engine_activity *ea = hw_engine_to_engine_activity(hwe);
>> +    struct engine_activity *ea = hw_engine_to_engine_activity(hwe, 
>> index);
>>     struct guc_engine_activity_metadata *cached_metadata = &ea->metadata;
>>     struct guc_engine_activity *cached_activity = &ea->activity;
>>     struct iosys_map activity_map, metadata_map;
>> @@ -177,8 +212,8 @@ static u64 get_engine_total_ticks(struct xe_guc 
>> *guc, struct xe_hw_engine *hwe)
>>     u64 numerator;
>>     u16 quanta_ratio;
>>
>> -    activity_map = engine_activity_map(guc, hwe);
>> -    metadata_map = engine_metadata_map(guc);
>> +    activity_map = engine_activity_map(guc, hwe, index);
>> +    metadata_map = engine_metadata_map(guc, index);
>>
>>     if (!cached_metadata->guc_tsc_frequency_hz)
>>         cached_metadata->guc_tsc_frequency_hz = 
>> read_metadata_record(xe, &metadata_map,
>> @@ -220,10 +255,35 @@ static int enable_engine_activity_stats(struct 
>> xe_guc *guc)
>>     return xe_guc_ct_send_block(&guc->ct, action, ARRAY_SIZE(action));
>> }
>>
>> -static void engine_activity_set_cpu_ts(struct xe_guc *guc)
>> +static int enable_function_engine_activity_stats(struct xe_guc *guc, 
>> bool enable)
>> {
>>     struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> -    struct engine_activity_group *eag = &engine_activity->eag[0];
>> +    u32 metadata_ggtt_addr = 0, ggtt_addr = 0, num_functions = 0;
>> +    struct engine_activity_buffer *buffer = &engine_activity- 
>> >function_buffer;
>> +    u32 action[6];
>> +    int len = 0;
>> +
>> +    if (enable) {
>> +        metadata_ggtt_addr = xe_bo_ggtt_addr(buffer->metadata_bo);
>> +        ggtt_addr = xe_bo_ggtt_addr(buffer->activity_bo);
>> +        num_functions = engine_activity->num_functions;
>> +    }
>> +
>> +    action[len++] = XE_GUC_ACTION_SET_FUNCTION_ENGINE_ACTIVITY_BUFFER;
>> +    action[len++] = num_functions;
>> +    action[len++] = metadata_ggtt_addr;
>> +    action[len++] = 0;
>> +    action[len++] = ggtt_addr;
>> +    action[len++] = 0;
>> +
>> +    /* Blocking here to ensure the buffers are ready before reading 
>> them */
>> +    return xe_guc_ct_send_block(&guc->ct, action, ARRAY_SIZE(action));
>> +}
>> +
>> +static void engine_activity_set_cpu_ts(struct xe_guc *guc, unsigned 
>> int index)
>> +{
>> +    struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> +    struct engine_activity_group *eag = &engine_activity->eag[index];
>>     int i, j;
>>
>>     for (i = 0; i < GUC_MAX_ENGINE_CLASSES; i++)
>> @@ -240,36 +300,103 @@ static u32 gpm_timestamp_shift(struct xe_gt *gt)
>>     return 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);
>> }
>>
>> +static bool is_function_valid(struct xe_guc *guc, unsigned int fn_id)
>> +{
>> +    struct xe_device *xe = guc_to_xe(guc);
>> +    struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> +
>> +    if (!IS_SRIOV(xe) && fn_id)
>> +        return false;
>> +
>> +    if (fn_id > engine_activity->num_functions)
> 
> This ^ should be 'else if'. Consider the Native case where fn_id = 0. I 
> believe num_functions will be 0 for native, so the check would fail 
> always for native. Right?
Only if it's greater it would fail. So for native it will not.

In case of SRIOV too. It'll fail only if num_vfs is not set.
So there won't be two counters one global and one PF. Only global will 
be present.

Thanks
Riana
> 
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
> 
> Thanks,
> Umesh