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

[Kamil Konieczny]

Hi Riana,
On 2025-02-12 at 15:28:27 +0530, Riana Tauro wrote:
> From: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> 
> Functions to parse event ID and GT bit shift for PMU events.
> 
> v2: Review comments (Riana)
> v3: Review comments (Lucas)
> 
> Cc: Riana Tauro <riana.tauro at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Reviewed-by: Riana Tauro <riana.tauro at intel.com>
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at 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..f021fc3ec 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: PMU device
> + * @param: Parameter for which you need the format start/end bits

You should describe here all params. Add also one empty line before ' * Returns'
like:

 * @end: last bit position
 *
 * Returns: 0 on success or negative error code

> + * Returns: 0 on success or negative error code
> + */
> +int perf_event_format(const char *device, const char *param, uint32_t *start, uint32_t *end)
> +{
> +	char buf[NAME_MAX];
> +	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);
> +	if (fd < 0)
> +		return -EINVAL;

imho here you could return -errno

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

>From function description this should be zero?
s/ret/0/

Regards,
Kamil

> +}
> +
> +/**
> + * 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.47.1
>