Re: [PATCH i-g-t v6 1/2] lib/igt_perf: Add utils to extract PMU event info

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 1/27/2025 3:30 PM, Lucas De Marchi wrote:
> On Mon, Jan 27, 2025 at 02:33:00PM -0800, Vinay Belgaumkar wrote:
>> Functions to parse event ID and GT bit shift for PMU events.
>>
>> v2: Review comments (Riana)
>>
>> Cc: Riana Tauro <riana.tauro@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>> lib/igt_perf.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> lib/igt_perf.h |  2 ++
>> 2 files changed, 72 insertions(+)
>>
>> diff --git a/lib/igt_perf.c b/lib/igt_perf.c
>> index 3866c6d77..7e81d5516 100644
>> --- a/lib/igt_perf.c
>> +++ b/lib/igt_perf.c
>> @@ -92,6 +92,76 @@ const char *xe_perf_device(int xe, char *buf, int 
>> buflen)
>>     return buf;
>> }
>>
>> +/**
>> + * perf_event_format: Returns the start/end positions of an event 
>> format param
>> + * @device: Device string in driver:pci format
>
> driver:pci seems wrong and is not true neither for i915 or xe.
> This is actually the pmu_device:
sure. Meant to say it is in the format of device_pci.
>
> /sys/bus/event_source/devices/{pmu_device}/events/{event_name}
>
> and
>
> /sys/bus/event_source/devices/{pmu_device}/format/{field}
>
>
>
>> + * @param: Parameter for which you need the format start/end bits
>> + * Returns: Start/end bit positions for a event parameter format
>
>     Returns 0 on success, or a negative error code on failure.
> would be more accurate to the int return of this function
ok.
>
>
>> + */
>> +int perf_event_format(const char *device, const char *param, 
>> uint32_t *start, uint32_t *end)
>> +{
>> +    char buf[NAME_MAX];
>
> it seems this was part of a previous review, but I don't understand how
> NAME_MAX is related to the buffer size here. You use it for the entire
> **path** size and then re-use it for the buffer content.
meant to replace a hardcoded length value.
>
> Well... don´t really care much: as long as we don't overflow and when we
> do we fail accordingly, should be good enough **for IGT**
>
>> +    ssize_t bytes;
>> +    int ret;
>> +    int fd;
>> +
>> +    snprintf(buf, sizeof(buf),
>> +         "/sys/bus/event_source/devices/%s/format/%s",
>> +         device, param);
>> +
>> +    fd = open(buf, O_RDONLY);
>
> O_CLOEXEC
>
> we have igt_sysfs_read() that could be used here (note dirfd is ignored
> when path is absolute).

I will have to make igt_perf lib dependent on igt_sysfs for this to 
work. So, chose the easy route.

Thanks,

Vinay.

>
> not really blocking this as is though.
>
> Lucas De Marchi
>
>> +    if (fd < 0)
>> +        return -EINVAL;
>> +
>> +    bytes = read(fd, buf, sizeof(buf) - 1);
>> +    close(fd);
>> +    if (bytes < 1)
>> +        return -EINVAL;
>> +
>> +    buf[bytes] = '\0';
>> +    ret = sscanf(buf, "config:%u-%u", start, end);
>> +    if (ret != 2)
>> +        return -EINVAL;
>> +
>> +    return ret;
>> +}
>> +
>> +/**
>> + * perf_event_config:
>> + * @device: Device string in driver:pci format
>> + * @event: The event name
>> + * @config: Pointer to the config
>> + * Returns: 0 for success, negative value on error
>> + */
>> +int perf_event_config(const char *device, const char *event, 
>> uint64_t *config)
>> +{
>> +    char buf[NAME_MAX];
>> +    ssize_t bytes;
>> +    int ret;
>> +    int fd;
>> +
>> +    snprintf(buf, sizeof(buf),
>> +         "/sys/bus/event_source/devices/%s/events/%s",
>> +         device,
>> +         event);
>> +
>> +    fd = open(buf, O_RDONLY);
>> +    if (fd < 0)
>> +        return -EINVAL;
>> +
>> +    bytes = read(fd, buf, sizeof(buf) - 1);
>> +    close(fd);
>> +    if (bytes < 1)
>> +        return ret;
>> +
>> +    buf[bytes] = '\0';
>> +    ret = sscanf(buf, "event=0x%lx", config);
>> +    if (ret != 1)
>> +        return -EINVAL;
>> +
>> +    return 0;
>> +}
>> +
>> uint64_t xe_perf_type_id(int xe)
>> {
>>     char buf[80];
>> diff --git a/lib/igt_perf.h b/lib/igt_perf.h
>> index 3d9ba2917..69f7a3d74 100644
>> --- a/lib/igt_perf.h
>> +++ b/lib/igt_perf.h
>> @@ -71,5 +71,7 @@ int perf_i915_open(int i915, uint64_t config);
>> int perf_i915_open_group(int i915, uint64_t config, int group);
>>
>> int perf_xe_open(int xe, uint64_t config);
>> +int perf_event_config(const char *device, const char *event, 
>> uint64_t *config);
>> +int perf_event_format(const char *device, const char *param, 
>> uint32_t *start, uint32_t *end);
>>
>> #endif /* I915_PERF_H */
>> -- 
>> 2.38.1
>>