Re: [PATCH i-g-t] tests/perf_pmu: Restore sysfs freq in exit handler
On 1/5/2024 3:33 AM, Kamil Konieczny wrote:
Hi Vinay,
On 2024-01-04 at 17:10:00 -0800, Vinay Belgaumkar wrote:
looks good, there are some nits, first about subject:
[PATCH i-g-t] tests/perf_pmu: Restore sysfs freq in exit handler
s!tests/perf_pmu:!tests/intel/perf_pmu:!
Also you can drop "sysfs", so it will look:
[PATCH i-g-t] tests/intel/perf_pmu: Restore freq in exit handler
Seeing random issues where this test starts with invalid values.
Btw if issue is it starts with invalid values maybe culprit is in
some previous test, not this one? What about setting freq values
to defaults first? This can be done in separate patch.
I looked into log from test here:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1438/bat-dg2-11/igt_runner10.txt
and here:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1438/bat-dg2-11/igt@perf_pmu@freque...@gt0.html
One more thing, why is boost < max? Is it allowed? What about
just restore it to max (or other value?) before testing and
skipping only when min == max? But even then it seems like
restoring defaults should be first step before freq checks.
The only freq related test in that log is gem_ctx_freq which never
modifies boost freq. AFAICS, this is the only test that modifies boost
freq to be below RP0. I am thinking a previous iteration of this test
left it in this state, not impossible I guess. Boost freq can be < max,
it is allowed. We could "restore" to default, but if we have exit
handlers in place, that should never be needed.
For more nits see below.
Ensure that we restore the frequencies in case test exits early
due to some system issues.
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9432
Signed-off-by: Vinay Belgaumkar
---
tests/intel/perf_pmu.c | 53 +-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/tests/intel/perf_pmu.c b/tests/intel/perf_pmu.c
index c6e6a8b77..ceacc1d3d 100644
--- a/tests/intel/perf_pmu.c
+++ b/tests/intel/perf_pmu.c
@@ -2454,12 +2454,59 @@ static void pmu_read(int i915)
for_each_if((e)->class == I915_ENGINE_CLASS_RENDER) \
igt_dynamic_f("%s", e->name)
+int fd = -1;
+uint32_t *stash_min, *stash_max, *stash_boost;
+
+static void save_sysfs_freq(int i915)
+{
+ int gt, num_gts, sysfs, tmp;
+
+ num_gts = igt_sysfs_get_num_gt(i915);
+
+ stash_min = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
+ stash_max = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
+ stash_boost = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
+
+ /* Save boost, min and max across GTs */
+ i915_for_each_gt(i915, tmp, gt) {
+ sysfs = igt_sysfs_gt_open(i915, gt);
+ igt_require(sysfs >= 0);
+
+ stash_min[gt] = igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz");
+ stash_max[gt] = igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz");
+ stash_boost[gt] = igt_sysfs_get_u32(sysfs,
"rps_boost_freq_mhz");
+ igt_debug("GT: %d, min: %d, max: %d, boost:%d\n",
+ gt, stash_min[gt], stash_max[gt], stash_boost[gt]);
+
+ close(sysfs);
+ }
+}
+
+static void restore_sysfs_freq(int sig)
+{
+ int sysfs, gt, tmp;
+
+ /* Restore frequencies */
+ i915_for_each_gt(fd, tmp, gt) {
+ sysfs = igt_sysfs_gt_open(fd, gt);
+ igt_require(sysfs >= 0);
^
Don't use require at exit handler, better use continue.
Not sure about this. If we cannot restore, doesn't it mean there is an
issue writing to sysfs and we should fail?
+
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
stash_max[gt]));
^
Same here.
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
stash_min[gt]));
^
Same.
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
stash_boost[gt]));
^
Same.
+
+ close(sysfs);
+ }
+ free(stash_min);
+ free(stash_max);
Free also stash_boost.
ok.
+}
+
igt_main
{
const struct intel_execution_engine2 *e;
unsigned int num_engines = 0;
const intel_ctx_t *ctx = NULL;
- int gt, tmp, fd = -1;
+ int gt, tmp;
int num_gt = 0;
/**
@@ -2482,6 +2529,7 @@ igt_main
i915_for_each_gt(fd, tmp, gt)
num_gt++;
+
Remove this empty line.
ok, thanks,
Vinay,
Regards,
Kamil
}
igt_describe("Verify i915 pmu dir exists and read all events");
@@ -2664,6 +2712,9 @@ igt_main
* Test GPU frequency.
*/
igt_subtest_with_dynamic("frequency") {
+ save_sysfs_freq(fd);
+ igt_install_exit_handler(restore_sysfs_freq);
+
i915_for_each_gt(fd, tmp, gt) {
igt_dynamic_f("gt%u", gt)
test_frequency(fd, gt);
--
2.38.1
Re: [PATCH i-g-t] tests/perf_pmu: Restore sysfs freq in exit handler
Hi Vinay,
On 2024-01-04 at 17:10:00 -0800, Vinay Belgaumkar wrote:
looks good, there are some nits, first about subject:
[PATCH i-g-t] tests/perf_pmu: Restore sysfs freq in exit handler
s!tests/perf_pmu:!tests/intel/perf_pmu:!
Also you can drop "sysfs", so it will look:
[PATCH i-g-t] tests/intel/perf_pmu: Restore freq in exit handler
> Seeing random issues where this test starts with invalid values.
Btw if issue is it starts with invalid values maybe culprit is in
some previous test, not this one? What about setting freq values
to defaults first? This can be done in separate patch.
I looked into log from test here:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1438/bat-dg2-11/igt_runner10.txt
and here:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1438/bat-dg2-11/igt@perf_pmu@freque...@gt0.html
One more thing, why is boost < max? Is it allowed? What about
just restore it to max (or other value?) before testing and
skipping only when min == max? But even then it seems like
restoring defaults should be first step before freq checks.
For more nits see below.
> Ensure that we restore the frequencies in case test exits early
> due to some system issues.
>
> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9432
> Signed-off-by: Vinay Belgaumkar
> ---
> tests/intel/perf_pmu.c | 53 +-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/tests/intel/perf_pmu.c b/tests/intel/perf_pmu.c
> index c6e6a8b77..ceacc1d3d 100644
> --- a/tests/intel/perf_pmu.c
> +++ b/tests/intel/perf_pmu.c
> @@ -2454,12 +2454,59 @@ static void pmu_read(int i915)
> for_each_if((e)->class == I915_ENGINE_CLASS_RENDER) \
> igt_dynamic_f("%s", e->name)
>
> +int fd = -1;
> +uint32_t *stash_min, *stash_max, *stash_boost;
> +
> +static void save_sysfs_freq(int i915)
> +{
> + int gt, num_gts, sysfs, tmp;
> +
> + num_gts = igt_sysfs_get_num_gt(i915);
> +
> + stash_min = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
> + stash_max = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
> + stash_boost = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
> +
> + /* Save boost, min and max across GTs */
> + i915_for_each_gt(i915, tmp, gt) {
> + sysfs = igt_sysfs_gt_open(i915, gt);
> + igt_require(sysfs >= 0);
> +
> + stash_min[gt] = igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz");
> + stash_max[gt] = igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz");
> + stash_boost[gt] = igt_sysfs_get_u32(sysfs,
> "rps_boost_freq_mhz");
> + igt_debug("GT: %d, min: %d, max: %d, boost:%d\n",
> + gt, stash_min[gt], stash_max[gt], stash_boost[gt]);
> +
> + close(sysfs);
> + }
> +}
> +
> +static void restore_sysfs_freq(int sig)
> +{
> + int sysfs, gt, tmp;
> +
> + /* Restore frequencies */
> + i915_for_each_gt(fd, tmp, gt) {
> + sysfs = igt_sysfs_gt_open(fd, gt);
> + igt_require(sysfs >= 0);
^
Don't use require at exit handler, better use continue.
> +
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
> stash_max[gt]));
^
Same here.
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
> stash_min[gt]));
^
Same.
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
> stash_boost[gt]));
^
Same.
> +
> + close(sysfs);
> + }
> + free(stash_min);
> + free(stash_max);
Free also stash_boost.
> +}
> +
> igt_main
> {
> const struct intel_execution_engine2 *e;
> unsigned int num_engines = 0;
> const intel_ctx_t *ctx = NULL;
> - int gt, tmp, fd = -1;
> + int gt, tmp;
> int num_gt = 0;
>
> /**
> @@ -2482,6 +2529,7 @@ igt_main
>
> i915_for_each_gt(fd, tmp, gt)
> num_gt++;
> +
Remove this empty line.
Regards,
Kamil
> }
>
> igt_describe("Verify i915 pmu dir exists and read all events");
> @@ -2664,6 +2712,9 @@ igt_main
>* Test GPU frequency.
>*/
> igt_subtest_with_dynamic("frequency") {
> + save_sysfs_freq(fd);
> + igt_install_exit_handler(restore_sysfs_freq);
> +
> i915_for_each_gt(fd, tmp, gt) {
> igt_dynamic_f("gt%u", gt)
> test_frequency(fd, gt);
> --
> 2.38.1
>
[PATCH i-g-t] tests/perf_pmu: Restore sysfs freq in exit handler
Seeing random issues where this test starts with invalid values.
Ensure that we restore the frequencies in case test exits early
due to some system issues.
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9432
Signed-off-by: Vinay Belgaumkar
---
tests/intel/perf_pmu.c | 53 +-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/tests/intel/perf_pmu.c b/tests/intel/perf_pmu.c
index c6e6a8b77..ceacc1d3d 100644
--- a/tests/intel/perf_pmu.c
+++ b/tests/intel/perf_pmu.c
@@ -2454,12 +2454,59 @@ static void pmu_read(int i915)
for_each_if((e)->class == I915_ENGINE_CLASS_RENDER) \
igt_dynamic_f("%s", e->name)
+int fd = -1;
+uint32_t *stash_min, *stash_max, *stash_boost;
+
+static void save_sysfs_freq(int i915)
+{
+ int gt, num_gts, sysfs, tmp;
+
+ num_gts = igt_sysfs_get_num_gt(i915);
+
+ stash_min = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
+ stash_max = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
+ stash_boost = (uint32_t *)malloc(sizeof(uint32_t) * num_gts);
+
+ /* Save boost, min and max across GTs */
+ i915_for_each_gt(i915, tmp, gt) {
+ sysfs = igt_sysfs_gt_open(i915, gt);
+ igt_require(sysfs >= 0);
+
+ stash_min[gt] = igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz");
+ stash_max[gt] = igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz");
+ stash_boost[gt] = igt_sysfs_get_u32(sysfs,
"rps_boost_freq_mhz");
+ igt_debug("GT: %d, min: %d, max: %d, boost:%d\n",
+ gt, stash_min[gt], stash_max[gt], stash_boost[gt]);
+
+ close(sysfs);
+ }
+}
+
+static void restore_sysfs_freq(int sig)
+{
+ int sysfs, gt, tmp;
+
+ /* Restore frequencies */
+ i915_for_each_gt(fd, tmp, gt) {
+ sysfs = igt_sysfs_gt_open(fd, gt);
+ igt_require(sysfs >= 0);
+
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
stash_max[gt]));
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
stash_min[gt]));
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
stash_boost[gt]));
+
+ close(sysfs);
+ }
+ free(stash_min);
+ free(stash_max);
+}
+
igt_main
{
const struct intel_execution_engine2 *e;
unsigned int num_engines = 0;
const intel_ctx_t *ctx = NULL;
- int gt, tmp, fd = -1;
+ int gt, tmp;
int num_gt = 0;
/**
@@ -2482,6 +2529,7 @@ igt_main
i915_for_each_gt(fd, tmp, gt)
num_gt++;
+
}
igt_describe("Verify i915 pmu dir exists and read all events");
@@ -2664,6 +2712,9 @@ igt_main
* Test GPU frequency.
*/
igt_subtest_with_dynamic("frequency") {
+ save_sysfs_freq(fd);
+ igt_install_exit_handler(restore_sysfs_freq);
+
i915_for_each_gt(fd, tmp, gt) {
igt_dynamic_f("gt%u", gt)
test_frequency(fd, gt);
--
2.38.1
