Re: [PATCH 5/7] drm/xe: Add single engine busyness support

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

Hi Umesh

Thank you for the review comments

On 11/15/2024 5:38 AM, Umesh Nerlige Ramappa wrote:
> On Wed, Nov 13, 2024 at 10:25:47AM +0530, Riana Tauro wrote:
>> GuC provides support to read engine active counters to calculate the
>> engine utilization. KMD exposes two counters via the PMU interface to
>> calculate engine busyness
>>
>> Engine Active Ticks(<engine>-busy-ticks-gt<n>) - number of active ticks
>>                          for engine
>> Total Ticks (<engine>-total-ticks-gt<n>) - total ticks GT has been active
>>
>> Busyness percentage can be calculated as below
>> busyness % = (engine active ticks/total ticks) * 100.
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> 
> Some minor comments below,
>> ---
>> drivers/gpu/drm/xe/Makefile                   |   1 +
>> drivers/gpu/drm/xe/abi/guc_actions_abi.h      |   1 +
>> drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   2 +
>> drivers/gpu/drm/xe/xe_engine_activity.c       | 317 ++++++++++++++++++
>> drivers/gpu/drm/xe/xe_engine_activity.h       |  18 +
>> drivers/gpu/drm/xe/xe_engine_activity_types.h |  85 +++++
>> drivers/gpu/drm/xe/xe_guc_fwif.h              |  19 ++
>> drivers/gpu/drm/xe/xe_guc_types.h             |   4 +
>> 8 files changed, 447 insertions(+)
>> create mode 100644 drivers/gpu/drm/xe/xe_engine_activity.c
>> create mode 100644 drivers/gpu/drm/xe/xe_engine_activity.h
>> create mode 100644 drivers/gpu/drm/xe/xe_engine_activity_types.h
>>
> 
> [snip]
> 
>> +static u64 get_engine_active_ticks(struct xe_guc *guc, struct 
>> xe_hw_engine *hwe)
>> +{
>> +    struct engine_activity *ea = hw_engine_to_engine_activity(hwe);
>> +    struct guc_engine_activity *cached_activity = &ea->activity;
>> +    struct guc_engine_activity_metadata *cached_metadata = &ea- 
>> >metadata;
>> +    struct xe_engine_activity *engine_busy = &guc->engine_busy;
>> +    struct activity_buffer *device_buffer = &engine_busy->device_buffer;
>> +    struct xe_device *xe =  guc_to_xe(guc);
>> +    struct xe_gt *gt = guc_to_gt(guc);
>> +
>> +    u16 guc_class = xe_engine_class_to_guc_class(hwe->class);
>> +    size_t offset = offsetof(struct guc_engine_activity_data,
>> +                 engine_activity[guc_class][hwe->logical_instance]);
>> +    struct iosys_map engine_activity_map = 
>> IOSYS_MAP_INIT_OFFSET(activity_to_map(device_buffer),
>> +                                     offset);
>> +    u32 last_update_tick, global_change_num;
>> +    u64 active_ticks, gpm_ts;
>> +    u16 change_num;
>> +
>> +    global_change_num = read_metadata_record(xe, device_buffer, 
>> global_change_num);
>> +
>> +    /* GuC has not initialized activity data yet, return 0 */
>> +    if (!global_change_num)
>> +        goto update;
>> +
>> +    if (global_change_num == cached_metadata->global_change_num)
>> +        goto update;
>> +    else
>> +        cached_metadata->global_change_num = global_change_num;
>> +
>> +    change_num = read_engine_activity_record(xe, 
>> &engine_activity_map, change_num);
>> +
>> +    if (!change_num || change_num == cached_activity->change_num)
>> +        goto update;
>> +
>> +    /* read engine activity values */
>> +    last_update_tick = read_engine_activity_record(xe, 
>> &engine_activity_map, last_update_tick);
>> +    active_ticks = read_engine_activity_record(xe, 
>> &engine_activity_map, active_ticks);
> 
> Whatever we read from the metadata record and the activity record could 
> be added to a trace call so that we can debug issues using ftrace when 
> needed.
Sure will add this
> 
>> +
>> +    /* activity calculations */
>> +    ea->running = !!last_update_tick;
>> +    ea->total += active_ticks - cached_activity->active_ticks;
>> +    ea->active = 0;
>> +
>> +    /* cache the counter */
>> +    cached_activity->change_num = change_num;
>> +    cached_activity->last_update_tick = last_update_tick;
>> +    cached_activity->active_ticks = active_ticks;
>> +
>> +update:
>> +    if (ea->running) {
>> +        gpm_ts  = xe_mmio_read64_2x32(&gt->mmio, MISC_STATUS_0) >>
> 
> extra space after gpm_ts ^.
> 
> Also, does the mmio read require a forcewake here?
> 
Thanks for catching this. This needs a forcewake
>> +              engine_busy->gpm_timestamp_shift;
>> +        ea->active = lower_32_bits(gpm_ts) - cached_activity- 
>> >last_update_tick;
>> +    }
>> +
>> +    return ea->total + ea->active;
>> +}
>> +
> 
> [snip]
> 
>> +static u32 gpm_timestamp_shift(struct xe_gt *gt)
>> +{
>> +    u32 reg;
>> +
>> +    reg = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
> 
> Does the mmio read require a forcewake here?
This doesn't require a forcewake
> 
>> +
>> +    return 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);
>> +}
> 
> [snip]
> 
>> +struct engine_activity {
>> +    /** @active: current activity */
>> +    u64 active;
>> +
>> +    /** @last_cpu_ts: cpu timestamp in nsec of previous sample */
>> +    u64 last_cpu_ts;
>> +
>> +    /** @quanta: total quanta used on HW */
>> +    u64 quanta;
>> +
>> +    /** @quanta_ns: total quanta_ns used on HW */
>> +    u64 quanta_ns;
>> +
>> +    /**
>> +     * @quanta_remainder_ns: remainder when the CPU time is scaled as
>> +     * per the quanta_ratio. This remainder is used insubsequent
> 
> s/insubsequent/in subsequent/
Will fix this.

Thanks
Riana Tauro
> 
> Thanks,
> Umesh