[PATCH v2 1/2] drm/xe/pmu: Add event tracking

[Belgaumkar, Vinay]

On 2/4/2025 2:30 PM, Lucas De Marchi wrote:
> On Tue, Feb 04, 2025 at 10:09:04AM -0800, Vinay Belgaumkar wrote:
>> Keep track of which events are enabled and number of instances
>> as preparation for the PMU frequency events. Also add locks where
>> we update these structures.
>>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Riana Tauro <riana.tauro at intel.com>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_pmu.c       | 44 ++++++++++++++++++++++++++++++-
>> drivers/gpu/drm/xe/xe_pmu_types.h | 14 ++++++++++
>> 2 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> index 3910a82328ee..10603e7c3eff 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -8,6 +8,7 @@
>>
>> #include "xe_device.h"
>> #include "xe_gt_idle.h"
>> +#include "xe_macros.h"
>> #include "xe_pm.h"
>> #include "xe_pmu.h"
>>
>> @@ -38,6 +39,11 @@
>> #define XE_PMU_EVENT_GT_MASK        GENMASK_ULL(63, 60)
>> #define XE_PMU_EVENT_ID_MASK        GENMASK_ULL(11, 0)
>>
>> +static struct xe_pmu *event_to_pmu(struct perf_event *event)
>> +{
>> +    return container_of(event->pmu, struct xe_pmu, base);
>> +}
>> +
>> static unsigned int config_to_event_id(u64 config)
>> {
>>     return FIELD_GET(XE_PMU_EVENT_ID_MASK, config);
>> @@ -156,6 +162,21 @@ static void xe_pmu_event_read(struct perf_event 
>> *event)
>>
>> static void xe_pmu_enable(struct perf_event *event)
>> {
>> +    struct xe_pmu *pmu = event_to_pmu(event);
>> +    struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
>> +    const unsigned int event_bit = 
>> config_to_event_id(event->attr.config);
>> +    unsigned long flags;
>> +
>> +    raw_spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +    XE_WARN_ON(event_bit >= XE_PMU_MAX_EVENT_TYPES);
>
> ^ not compatible with raw_spin_lock... if the warn triggers you are
> likely to bring the machine down. it's also called event_id, not
> event_bit.
> Please do not copy and paste from i915. It's not a good reference.
Not meant to be a copy paste, wanted to add protection to ensure the bit 
index doesn't go out of bounds. The config-0:11 bits should match the 
array size. Maybe check should be before spin_lock().
>
>> + XE_WARN_ON(pmu->enable_count[event_bit] == ~0);
Same here, should be before the spin_lock() in case it is not compatible.
>>
>> +
>> +    /* Save the event config */
>> +    pmu->enable |= event->attr.config;
>
> what does "enable" have to do with config?
We need this info while toggling/using the internal timer for frequency 
stats (next patch).
>
> we can't simply OR any event config since there are more things than the
> event_id. Suppose userspace did this:
>
> proc1:
> perf_event_open(gt-c6, gt=3)
>
>     pmu->enable = 3 << 60 | GT_C6_ID
>
> proc2:
> perf_event_open(freq, gt=1)
>
>     pmu->enable =  1 << 60 | FREQ_ID
>
> now when you close the fd in proc2, your pmu->enable field will have a
> gt == 2.

Hmm, true. We need a per GT event bitmask along with the count.

Thanks,

Vinay.

>
>
> it's not clear at all why you need to save the config. I will try to
> figure out from next patch.
>
>> +    pmu->enable_count[event_bit]++;
>> +
>> +    raw_spin_unlock_irqrestore(&pmu->lock, flags);
>>     /*
>>      * Store the current counter value so we can report the correct 
>> delta
>>      * for all listeners. Even when the event was already enabled and 
>> has
>> @@ -176,15 +197,34 @@ static void xe_pmu_event_start(struct 
>> perf_event *event, int flags)
>>     event->hw.state = 0;
>> }
>>
>> +static void xe_pmu_disable(struct perf_event *event)
>> +{
>> +    struct xe_device *xe = container_of(event->pmu, typeof(*xe), 
>> pmu.base);
>> +    struct xe_pmu *pmu = &xe->pmu;
>> +    unsigned long flags;
>> +    unsigned int event_bit = config_to_event_id(event->attr.config);
>> +
>> +    XE_WARN_ON(event_bit >= XE_PMU_MAX_EVENT_TYPES);
>> +    raw_spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +    if (--pmu->enable_count[event_bit] == 0)
>> +        pmu->enable &=  ~event->attr.config;
>> +
>> +    raw_spin_unlock_irqrestore(&pmu->lock, flags);
>> +}
>> +
>> static void xe_pmu_event_stop(struct perf_event *event, int flags)
>> {
>>     struct xe_device *xe = container_of(event->pmu, typeof(*xe), 
>> pmu.base);
>>     struct xe_pmu *pmu = &xe->pmu;
>>
>> -    if (pmu->registered)
>> +    if (pmu->registered) {
>>         if (flags & PERF_EF_UPDATE)
>>             xe_pmu_event_update(event);
>>
>> +        xe_pmu_disable(event);
>> +    }
>> +
>>     event->hw.state = PERF_HES_STOPPED;
>> }
>>
>> @@ -334,6 +374,8 @@ int xe_pmu_register(struct xe_pmu *pmu)
>>     if (IS_SRIOV_VF(xe))
>>         return 0;
>>
>> +    raw_spin_lock_init(&pmu->lock);
>> +
>>     name = kasprintf(GFP_KERNEL, "xe_%s",
>>              dev_name(xe->drm.dev));
>>     if (!name)
>> diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h 
>> b/drivers/gpu/drm/xe/xe_pmu_types.h
>> index f5ba4d56622c..b15858d4d4a9 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu_types.h
>> +++ b/drivers/gpu/drm/xe/xe_pmu_types.h
>> @@ -10,6 +10,8 @@
>> #include <linux/spinlock_types.h>
>>
>> #define XE_PMU_MAX_GT 2
>> +/* Event config is defined as 0:11 */
>> +#define XE_PMU_MAX_EVENT_TYPES 12
>>
>> /**
>>  * struct xe_pmu - PMU related data per Xe device
>> @@ -34,6 +36,18 @@ struct xe_pmu {
>>      * @supported_events: Bitmap of supported events, indexed by 
>> event id
>>      */
>>     u64 supported_events;
>> +    /**
>> +     * @lock: Lock protecting enable mask and sampling
>> +     */
>> +    raw_spinlock_t lock;
>> +    /**
>> +     * @enable: Store requested PMU event config.
>> +     */
>> +    u64 enable;
>> +    /**
>> +     * @enable_count: Reference counts for the enabled events.
>> +     */
>> +    unsigned int enable_count[XE_PMU_MAX_EVENT_TYPES];
>> };
>>
>> #endif
>> -- 
>> 2.38.1
>>