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

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

On 1/28/2025 5:00 AM, 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:
> 
> /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
> 
> 
>> + */
>> +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.
this was to avoid numbers and use existing macros instead.
NAME_MAX or PATH_MAX anything should be good based on the size required

Thanks
Riana
> 
> 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).
> 
> 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
>>