Re: [PATCH v3 09/10] drm/xe/pmu: Add PMU support for per-engine-class activity

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

Hi Umesh

On 1/17/2025 6:48 AM, Umesh Nerlige Ramappa wrote:
> Hi Riana,
> 
> This is part 2 of the review:
Thank you for the review comments
> 
> On Mon, Jan 06, 2025 at 01:25:58PM +0530, Riana Tauro wrote:
>> PMU provides two counters (engine-active-ticks, total-ticks)
>> to calculate engine acitivity. When querying engine busyness,
>> user must group these 2 counters using the perf_event
>> group mechanism to ensure both counters are sampled together.
>>
>> To list the events
>>
>>     ./perf list
>>       xe_0000_03_00.0/engine-active-ticks/        [Kernel PMU event]
>>       xe_0000_03_00.0/total-ticks/            [Kernel PMU event]
> 
> total ticks is also engine specific, so maybe we can use engine-total- 
> ticks as the name.
Sure will use engine-total-ticks
> 
>>
>> The formats to be used with the above are
>>
>>     engine_class    - config:12-19
>>     engine_instance    - config:20-27
>>     gt_id        - config:60-63
>>
>> The events can then be read using perf tool
>>
>> ./perf stat -e xe_0000_03_00.0/engine-active-ticks,gt_id=0,
>>                    engine_class=0,engine_instance=0/,
>>            xe_0000_03_00.0/total-ticks,gt_id=0,
>>                    engine_class=0,engine_instance=0/ -I 1000
>>
> 
> I am also wondering how a user knows what format bits are/are-not 
> applicable to each events.
As of now, its mentioned in the documentation. But that is the reason i
was thinking adding the engine class in the name would be better

  For example, if I pass engine class and
> instance to a frequency event, it shows unsupported. How does the 
> implementation check for valid config?

The current approach first checks if it is engine event by checking
if the event type is active ticks or total-ticks

return ((sample == XE_PMU_ENGINE_ACTIVITY_TICKS) || (sample == 
XE_PMU_TOTAL_TICKS));

else it enters the config_status. config_status then checks all bits 
apart from gt_id to check the sample. So it returns
unsupported

But that seems to have changed in the latest patches sent by Lucas
https://patchwork.freedesktop.org/patch/632764/?series=139121&rev=13
Approach might have to be tweaked

> 
>> Engine activity can then be calculated as below
>> engine activity % = (engine active ticks/total ticks) * 100
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc.c       |   5 ++
>> drivers/gpu/drm/xe/xe_pmu.c       | 139 +++++++++++++++++++++++++-----
>> drivers/gpu/drm/xe/xe_pmu_types.h |   7 ++
>> drivers/gpu/drm/xe/xe_uc.c        |   3 +
>> 4 files changed, 131 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 408365dfe4ee..f229745b78b9 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -26,6 +26,7 @@
>> #include "xe_guc_capture.h"
>> #include "xe_guc_ct.h"
>> #include "xe_guc_db_mgr.h"
>> +#include "xe_guc_engine_activity.h"
>> #include "xe_guc_hwconfig.h"
>> #include "xe_guc_log.h"
>> #include "xe_guc_pc.h"
>> @@ -743,6 +744,10 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>>     if (ret)
>>         return ret;
>>
>> +    ret = xe_guc_engine_activity_init(guc);
>> +    if (ret)
>> +        return ret;
>> +
>>     return xe_guc_ads_init_post_hwconfig(&guc->ads);
>> }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> index bae8eb38fddd..5bd312b6b8f6 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -12,7 +12,9 @@
>> #include "xe_force_wake.h"
>> #include "xe_gt_clock.h"
>> #include "xe_gt_idle.h"
>> +#include "xe_guc_engine_activity.h"
>> #include "xe_guc_pc.h"
>> +#include "xe_hw_engine.h"
>> #include "xe_mmio.h"
>> #include "xe_macros.h"
>> #include "xe_module.h"
>> @@ -90,6 +92,17 @@ static unsigned int xe_pmu_target_cpu = -1;
>>  *    1950
>>  *    1950
>>  *    1950
>> + *
>> + * Engine Activity: PMU provides two counters (engine-active-ticks, 
>> total-ticks) to calculate
>> + * engine activity. While querying the engine activity the user 
>> should group these two counters
>> + * using the perf_event group mechanism to ensure both counters are 
>> sampled together.
>> + *
>> + * To read a engine specific event for a GT of class 1 and instance 0
>> + *
>> + * perf stat -e xe_0000_03_00.0/engine-active- 
>> ticks,gt_id=0,engine_class=1,engine_instance=0/,
>> + *        xe_0000_03_00.0/total- 
>> ticks,gt_id=0,engine_class=1,engine_instance=0/ -I 1000
>> + *
>> + * engine active % = (engine active ticks/total ticks) * 100
>>  */
>>
>> static struct xe_pmu *event_to_pmu(struct perf_event *event)
>> @@ -107,6 +120,33 @@ static u64 config_counter(const u64 config)
>>     return config & ~(~0ULL << __XE_PMU_GT_SHIFT);
>> }
>>
>> +static u64 engine_event_sample(const u64 config)
>> +{
>> +    return config_counter(config) & 0xfff;
>> +}
>> +
>> +static u8 engine_event_class(const u64 config)
>> +{
>> +    return (config_counter(config) >> XE_PMU_CLASS_SHIFT) & 0xff;
>> +}
>> +
>> +static u8 engine_event_instance(const u64 config)
>> +{
>> +    return (config_counter(config) >> XE_PMU_INSTANCE_SHIFT) & 0xff;
>> +}
>> +
>> +static bool is_engine_event(struct xe_device *xe, const u64 config)
>> +{
>> +    const u64 gt_id = config >> __XE_PMU_GT_SHIFT;
>> +    struct xe_gt *gt = xe_device_get_gt(xe, gt_id);
>> +    u64 sample = engine_event_sample(config);
>> +
>> +    if (!xe_guc_engine_activity_supported(&gt->uc.guc))
>> +        return false;
>> +
>> +    return ((sample == XE_PMU_ENGINE_ACTIVITY_TICKS) || (sample == 
>> XE_PMU_TOTAL_TICKS));
>> +}
>> +
>> static unsigned int pm_bit(const u64 config)
>> {
>>     unsigned int val;
>> @@ -192,6 +232,23 @@ config_status(struct xe_device *xe, u64 config)
>>     return 0;
>> }
>>
>> +static int engine_event_init(struct xe_device *xe, u64 config)
>> +{
>> +    const unsigned int gt_id = config_gt_id(config);
>> +    struct drm_xe_engine_class_instance eci;
>> +    struct xe_hw_engine *hwe;
>> +
>> +    eci.engine_class = engine_event_class(config);
>> +    eci.engine_instance = engine_event_instance(config);
>> +    eci.gt_id = gt_id;
>> +
>> +    hwe = xe_hw_engine_lookup(xe, eci);
> 
> Getting hwe from the config could be a helper since you have similar 
> logic in __xe_pmu_event_read()
Will add an helper
> 
>> +    if (!hwe || xe_hw_engine_is_reserved(hwe))
>> +        return -ENOENT;
>> +
>> +    return 0;
>> +}
>> +
>> static int xe_pmu_event_init(struct perf_event *event)
>> {
>>     struct xe_device *xe =
>> @@ -221,7 +278,12 @@ static int xe_pmu_event_init(struct perf_event 
>> *event)
>>         return -EINVAL;
>>
>>     event_config = event->attr.config;
>> -    ret = config_status(xe, event_config);
>> +
>> +    if (is_engine_event(xe, event_config))
>> +        ret = engine_event_init(xe, event_config);
>> +    else
>> +        ret = config_status(xe, event_config);
>> +
>>     if (ret)
>>         return ret;
>>
>> @@ -300,24 +362,49 @@ static u64 __xe_pmu_event_read(struct perf_event 
>> *event)
>>     struct xe_gt *gt = xe_device_get_gt(xe, gt_id);
>>     u64 val = 0;
>>
>> -    switch (config_counter(config)) {
>> -    case XE_PMU_C6_RESIDENCY:
>> -        val = get_c6(gt);
>> -        break;
>> -    case XE_PMU_ACTUAL_FREQUENCY:
>> -        val =
>> -           div_u64(read_sample(pmu, gt_id,
>> -                       __XE_SAMPLE_FREQ_ACT),
>> -               USEC_PER_SEC /* to MHz */);
>> -        break;
>> -    case XE_PMU_REQUESTED_FREQUENCY:
>> -        val =
>> -           div_u64(read_sample(pmu, gt_id,
>> -                       __XE_SAMPLE_FREQ_REQ),
>> -               USEC_PER_SEC /* to MHz */);
>> -        break;
>> -    default:
>> -        drm_warn(&gt->tile->xe->drm, "unknown pmu event\n");
>> +    if (is_engine_event(xe, config)) {
>> +        struct drm_xe_engine_class_instance eci;
>> +        struct xe_hw_engine *hwe;
>> +        u64 sample = engine_event_sample(config);
>> +
>> +        eci.engine_class = engine_event_class(config);
>> +        eci.engine_instance = engine_event_instance(config);
>> +        eci.gt_id = gt_id;
>> +
>> +        hwe = xe_hw_engine_lookup(xe, eci);
>> +        if (!hwe)
>> +            drm_WARN_ON_ONCE(&xe->drm, "unknown engine\n");
>> +
>> +        if (xe_pm_runtime_suspended(xe))
>> +            return 0;
>> +
>> +        if (sample == XE_PMU_ENGINE_ACTIVITY_TICKS)
>> +            val = xe_guc_engine_activity_active_ticks(hwe);
>> +        else if (sample == XE_PMU_TOTAL_TICKS)
>> +            val = xe_guc_engine_activity_total_ticks(hwe);
>> +        else
>> +            drm_warn(&xe->drm, "unknown pmu engine event\n");
> 
> Maybe also print the unknown sample/value for debug. Same for the else 
> section below.
Sure will add it.

Thanks
Riana
> 
> Rest looks good.
> 
> Thanks,
> Umesh
>