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

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

On 1/3/2024 12:36 PM, Riana Tauro wrote:
> 
> 
> 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.
> 
  0xfffff is same as __DRM_XE_PMU_ENGINE_EVENT(0xff, 0xff, 0xf).
So will add it as

  ___DRM_XE_PMU_GT_EVENT(gt, __DRM_XE_PMU_ENGINE_EVENT(0xff, 0xff, 0xf))

>>>>
>>>> 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