| Message ID | 20250127223301.2819108-2-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tests/intel/xe_pmu: Add PMU tests | expand |
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. 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 >
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 >>
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 >>
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 + * @param: Parameter for which you need the format start/end bits + * Returns: Start/end bit positions for a event parameter format + */ +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); + 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 */
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(+)