[PATCH v4 6/8] drm/xe: Add support for per-function engine activity
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Wed Feb 5 01:30:55 UTC 2025
On Mon, Feb 03, 2025 at 10:56:03AM +0530, Riana Tauro wrote:
>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 at 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.
ah, sorry my bad. Looks fine
Thanks,
Umesh
>
>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
>
More information about the Intel-xe
mailing list