Re: [PATCH v4 3/8] drm/xe/uapi: Add configs for Engine busyness

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

On 1/3/2024 12:32 PM, Aravind Iddamsetty wrote:
> 
> On 1/3/24 12:10, Riana Tauro wrote:
>>
>> Hi Aravind
>>
>> On 1/3/2024 10:56 AM, Aravind Iddamsetty wrote:
>>>
>>> On 12/22/23 13:15, Riana Tauro wrote:
>>>> GuC provides engine busyness ticks as a 64 bit counter which count
>>>> as clock ticks.
>>>>
>>>> Add configs to the uapi to expose Engine busyness via PMU.
>>>>
>>>> v2: add "__" prefix for internal helpers
>>>>       add a simple helper for application usage (Aravind)
>>>>
>>>> v3: rebase
>>>>       change internal uapi pmu config helpers (Umesh)
>>>>
>>>> Cc: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>>> ---
>>>>    include/uapi/drm/xe_drm.h | 41 +++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 41 insertions(+)
>>>>
>>>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>>>> index 9fa3ae324731..f8456cda5cda 100644
>>>> --- a/include/uapi/drm/xe_drm.h
>>>> +++ b/include/uapi/drm/xe_drm.h
>>>> @@ -1340,6 +1340,47 @@ struct drm_xe_wait_user_fence {
>>>>        __u64 reserved[2];
>>>>    };
>>>>    +/**
>>>> + * DOC: XE PMU event config IDs
>>>> + *
>>>> + * Check 'man perf_event_open' to use the ID's DRM_XE_PMU_XXXX listed in xe_drm.h
>>>> + * in 'struct perf_event_attr' as part of perf_event_open syscall to read a
>>>> + * particular event.
>>>> + *
>>>> + */
>>>> +
>>>> +/**
>>>> + * enum drm_xe_pmu_engine_sample - Supported PMU engine samples
>>>> + */
>>>> +enum drm_xe_pmu_engine_sample {
>>>> +    /** @DRM_XE_PMU_SAMPLE_BUSY_TICKS: Engine busy ticks */
>>>> +    DRM_XE_PMU_SAMPLE_BUSY_TICKS = 0,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Top bits of every counter are GT id.
>>>> + */
>>>> +#define __DRM_XE_PMU_GT_SHIFT (56)
>>>> +#define __DRM_XE_PMU_SAMPLE_BITS (4)
>>>> +#define __DRM_XE_PMU_SAMPLE_INSTANCE_BITS (8)
>>>> +#define __DRM_XE_PMU_CLASS_SHIFT \
>>>> +    (__DRM_XE_PMU_SAMPLE_BITS + __DRM_XE_PMU_SAMPLE_INSTANCE_BITS)
>>>> +
>>>> +#define __DRM_XE_PMU_GT_EVENT(gt, x) \
>>>> +    (((__u64)(x)) | ((__u64)(gt) << __DRM_XE_PMU_GT_SHIFT))
>>>> +
>>>> +#define __DRM_XE_PMU_ENGINE(class, instance, sample) \
>>>> +    (((class) << __DRM_XE_PMU_CLASS_SHIFT | \
>>>> +    (instance) << __DRM_XE_PMU_SAMPLE_BITS | \
>>>> +    (sample)))
>>>> +
>>>> +#define __DRM_XE_PMU_OTHER(gt, x) \
>>>> +    (__DRM_XE_PMU_GT_EVENT(gt, 0xfffff) + 1 + (x))
>>> Use __DRM_XE_PMU_ENGINE(0xff, 0xff, 0xf) instead of 0xfffff so that it will be clear
>>> that it is starting after the engine event.
>> Previous comments from Umesh suggested to use 0xfffff so changed it from rev3.
> looking back at the comment, Umesh corrected it to be 0xfffff, so using as suggested by me above
> looks to be more apt.
Initially Group busyness took the first few configs, so i had added a 
different offset.

Anyway ___DRM_XE_PMU_GT_EVENT(gt, 0xfffff) is same as 
__DRM_XE_PMU_ENGINE_EVENT(0xff, 0xff, 0xf). So will change it as you 
suggested.

>>>
>>> But __DRM_XE_PMU_OTHER is not used any where so why to introduce in this patch.
>> I added it here so that it can be used in the next patch to check if its engine event.
>>
>> Will move total active ticks config also to the same patch so that all configs are in one patch.
> 
> then you should define __DRM_XE_PMU_OTHER where active ticks is being introduced not viceversa.Okay will move it to the total active ticks patch.

Thanks
Riana
> 
> Thanks,
> Aravind.
>>
>> Thanks
>> Riana
>>
>>
>>>
>>> Thanks,
>>> Aravind.
>>>> +
>>>> +#define DRM_XE_PMU_ENGINE_BUSY_TICKS(gt, class, instance) \
>>>> +    __DRM_XE_PMU_GT_EVENT(gt, __DRM_XE_PMU_ENGINE(class, instance, \
>>>> +                              DRM_XE_PMU_SAMPLE_BUSY_TICKS))
>>>> +
>>>>    #if defined(__cplusplus)
>>>>    }
>>>>    #endif