Re: [PATCH 2/3] drm/xe/xe_pmu: Add PMU support for per-function engine activity stats

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

Hi Umesh

On 3/7/2025 5:37 AM, Umesh Nerlige Ramappa wrote:
> On Tue, Mar 04, 2025 at 03:27:42PM +0530, Riana Tauro wrote:
>> Add PMU support for per-function engine activity stats.
>>
>> per-function engine activity is enabled when VF's are enabled.
>> If 2 VF's are enabled, then the applicable function values are
>>
>> 0 - PF engine activity
>> 1 - VF1 engine activity
>> 2 - VF2 engine activity
>>
>> This can be read from perf tool as shown below
>>
>> ./perf stat -e xe_<bdf>/engine-active-ticks,gt=0,engine_class=0,
>>              engine_instance=0,function=1/ -I 1000
>>
>> v2: fix documentation (Umesh)
>>    remove global for functions (Lucas, Michal)
>>
>> v3: fix commit message
>>    move function_id checks to same place (Michal)
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_pmu.c | 40 +++++++++++++++++++++++++++++--------
>> 1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> index f8f6ebb7c9c6..8922f53ebf93 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -13,6 +13,7 @@
>> #include "xe_hw_engine.h"
>> #include "xe_pm.h"
>> #include "xe_pmu.h"
>> +#include "xe_sriov_pf_helpers.h"
>>
>> /**
>>  * DOC: Xe PMU (Performance Monitoring Unit)
>> @@ -32,9 +33,10 @@
>>  *    gt[60:63]        Selects gt for the event
>>  *    engine_class[20:27]    Selects engine-class for event
>>  *    engine_instance[12:19]    Selects the engine-instance for the event
>> + *    function[44:59]        Selects the function of the event (SRIOV 
>> enabled)
>>  *
>>  * For engine specific events (engine-*), gt, engine_class and 
>> engine_instance parameters must be
>> - * set as populated by DRM_XE_DEVICE_QUERY_ENGINES.
>> + * set as populated by DRM_XE_DEVICE_QUERY_ENGINES and function if 
>> SRIOV is enabled.
>>  *
>>  * For gt specific events (gt-*) gt parameter must be passed. All 
>> other parameters will be 0.
>>  *
>> @@ -49,6 +51,7 @@
>>  */
>>
>> #define XE_PMU_EVENT_GT_MASK            GENMASK_ULL(63, 60)
>> +#define XE_PMU_EVENT_FUNCTION_MASK        GENMASK_ULL(59, 44)
>> #define XE_PMU_EVENT_ENGINE_CLASS_MASK        GENMASK_ULL(27, 20)
>> #define XE_PMU_EVENT_ENGINE_INSTANCE_MASK    GENMASK_ULL(19, 12)
>> #define XE_PMU_EVENT_ID_MASK            GENMASK_ULL(11, 0)
>> @@ -58,6 +61,11 @@ static unsigned int config_to_event_id(u64 config)
>>     return FIELD_GET(XE_PMU_EVENT_ID_MASK, config);
>> }
>>
>> +static unsigned int config_to_function_id(u64 config)
>> +{
>> +    return FIELD_GET(XE_PMU_EVENT_FUNCTION_MASK, config);
>> +}
>> +
>> static unsigned int config_to_engine_class(u64 config)
>> {
>>     return FIELD_GET(XE_PMU_EVENT_ENGINE_CLASS_MASK, config);
>> @@ -151,7 +159,7 @@ static bool event_supported(struct xe_pmu *pmu, 
>> unsigned int gt,
>> static bool event_param_valid(struct perf_event *event)
>> {
>>     struct xe_device *xe = container_of(event->pmu, typeof(*xe), 
>> pmu.base);
>> -    unsigned int engine_class, engine_instance;
>> +    unsigned int engine_class, engine_instance, function_id;
>>     u64 config = event->attr.config;
>>     struct xe_gt *gt;
>>
>> @@ -161,16 +169,27 @@ static bool event_param_valid(struct perf_event 
>> *event)
>>
>>     engine_class = config_to_engine_class(config);
>>     engine_instance = config_to_engine_instance(config);
>> +    function_id = config_to_function_id(config);
>>
>>     switch (config_to_event_id(config)) {
>>     case XE_PMU_EVENT_GT_C6_RESIDENCY:
>> -        if (engine_class || engine_instance)
>> +        if (engine_class || engine_instance || function_id)
>>             return false;
>>         break;
>>     case XE_PMU_EVENT_ENGINE_ACTIVE_TICKS:
>>     case XE_PMU_EVENT_ENGINE_TOTAL_TICKS:
>>         if (!event_to_hwe(event))
>>             return false;
>> +        /*
>> +         * PF(0) and total vfs when SRIOV is enabled
>> +         */
> 
> single line comment should be enough here ^.
> 
> with that:
> 
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Thank you for the review. Will fix the above review comment in next revision

Thanks
Riana
> 
> Thanks,
> Umesh
> 
>> +        if (IS_SRIOV_PF(xe)) {
>> +            if (function_id > xe_sriov_pf_get_totalvfs(xe))
>> +                return false;
>> +        } else if (function_id) {
>> +            return false;
>> +        }
>> +
>>         break;
>>     }
>>
>> @@ -242,14 +261,17 @@ static int xe_pmu_event_init(struct perf_event 
>> *event)
>> static u64 read_engine_events(struct xe_gt *gt, struct perf_event *event)
>> {
>>     struct xe_hw_engine *hwe;
>> -    u64 val = 0;
>> +    unsigned int function_id;
>> +    u64 config, val = 0;
>>
>> -    hwe = event_to_hwe(event);
>> +    config = event->attr.config;
>> +    function_id = config_to_function_id(config);
>>
>> -    if (config_to_event_id(event->attr.config) == 
>> XE_PMU_EVENT_ENGINE_ACTIVE_TICKS)
>> -        val = xe_guc_engine_activity_active_ticks(&gt->uc.guc, hwe, 0);
>> +    hwe = event_to_hwe(event);
>> +    if (config_to_event_id(config) == XE_PMU_EVENT_ENGINE_ACTIVE_TICKS)
>> +        val = xe_guc_engine_activity_active_ticks(&gt->uc.guc, hwe, 
>> function_id);
>>     else
>> -        val = xe_guc_engine_activity_total_ticks(&gt->uc.guc, hwe, 0);
>> +        val = xe_guc_engine_activity_total_ticks(&gt->uc.guc, hwe, 
>> function_id);
>>
>>     return val;
>> }
>> @@ -352,6 +374,7 @@ static void xe_pmu_event_del(struct perf_event 
>> *event, int flags)
>> }
>>
>> PMU_FORMAT_ATTR(gt,            "config:60-63");
>> +PMU_FORMAT_ATTR(function,        "config:44-59");
>> PMU_FORMAT_ATTR(engine_class,        "config:20-27");
>> PMU_FORMAT_ATTR(engine_instance,    "config:12-19");
>> PMU_FORMAT_ATTR(event,            "config:0-11");
>> @@ -360,6 +383,7 @@ static struct attribute *pmu_format_attrs[] = {
>>     &format_attr_event.attr,
>>     &format_attr_engine_class.attr,
>>     &format_attr_engine_instance.attr,
>> +    &format_attr_function.attr,
>>     &format_attr_gt.attr,
>>     NULL,
>> };
>> -- 
>> 2.47.1
>>