Re: [PATCH i-g-t v6 7/9] tests/perf_pmu: Tests for i915 PMU API

Chris Wilson
2017-10-10T16:41:35Z

Quoting Tvrtko Ursulin (2017-10-10 15:17:54)
> +static void
> +busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
> + const unsigned int num_engines)
> +{
> + const struct intel_execution_engine2 *e_;
> + uint64_t val[num_engines];
> + int fd[num_engines];
> + igt_spin_t *spin;
> + unsigned int busy_idx, i;
> +
> + i = 0;
> + fd[0] = -1;
> + for_each_engine_class_instance(fd, e_) {
> + if (!gem_has_engine(gem_fd, e_->class, e_->instance))
> + continue;
> + else if (e == e_)
> + busy_idx = i;
> +
> + fd[i++] = open_group(I915_PMU_ENGINE_BUSY(e_->class,
> + e_->instance),
> + fd[0]);
> + }
igt_assert(i == num_engines);

Feels like a bug waiting to happen; a trap.

> +static void
> +test_frequency(int gem_fd)
> +{
> + const uint64_t duration_ns = 2e9;
> + uint32_t min_freq, max_freq, boost_freq;
> + uint64_t min[2], max[2], start[2];
> + igt_spin_t *spin;
> + int fd, sysfs;
> +
> + sysfs = igt_sysfs_open(gem_fd, NULL);
> + igt_require(sysfs >= 0);
> +
> + min_freq = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> + max_freq = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> + boost_freq = igt_sysfs_get_u32(sysfs, "gt_boost_freq_mhz");
> + igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
> + igt_require(max_freq > min_freq);
> + igt_require(boost_freq > min_freq);
> +
> + fd = open_group(I915_PMU_REQUESTED_FREQUENCY, -1);
> + open_group(I915_PMU_ACTUAL_FREQUENCY, fd);
> +
> + /*
> + * Set GPU to min frequency and read PMU counters.
> + */
> + igt_require(igt_sysfs_set_u32(sysfs, "gt_max_freq_mhz", min_freq));
> + igt_require(igt_sysfs_get_u32(sysfs, "gt_max_freq_mhz") == min_freq);
> + igt_require(igt_sysfs_set_u32(sysfs, "gt_boost_freq_mhz", min_freq));
> + igt_require(igt_sysfs_get_u32(sysfs, "gt_boost_freq_mhz") == min_freq);
> +
> + pmu_read_multi(fd, 2, start);
> +
> + spin = igt_spin_batch_new(gem_fd, 0, I915_EXEC_RENDER, 0);
> + igt_spin_batch_set_timeout(spin, duration_ns);
> + gem_sync(gem_fd, spin->handle);
> +
> + pmu_read_multi(fd, 2, min);
> + min[0] -= start[0];
> + min[1] -= start[1];
> +
> + igt_spin_batch_free(gem_fd, spin);
> +
> + usleep(1e6);
> +
> + /*
> + * Set GPU to max frequency and read PMU counters.
> + */
> + igt_require(igt_sysfs_set_u32(sysfs, "gt_max_freq_mhz", max_freq));
> + igt_require(igt_sysfs_get_u32(sysfs, "gt_max_freq_mhz") == max_freq);
> + igt_require(igt_sysfs_set_u32(sysfs, "gt_boost_freq_mhz", boost_freq));
> + igt_require(igt_sysfs_get_u32(sysfs, "gt_boost_freq_mhz") == boost_freq);
> +
> + igt_require(igt_sysfs_set_u32(sysfs, "gt_min_freq_mhz", max_freq));
> + igt_require(igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz") == max_freq);
> +
> + pmu_read_multi(fd, 2, start);
> +
> + spin = igt_spin_batch_new(gem_fd, 0, I915_EXEC_RENDER, 0);
> + igt_spin_batch_set_timeout(spin, duration_ns);
> + gem_sync(gem_fd, spin->handle);
> +
> + pmu_read_multi(fd, 2, max);
> + max[0] -= start[0];
> + max[1] -= start[1];
> +
> + igt_spin_batch_free(gem_fd, spin);
> +
> + /*
> + * Restore min/max.
> + */
> + igt_require(igt_sysfs_set_u32(sysfs, "gt_min_freq_mhz", min_freq));
> + igt_require(igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz") == min_freq);

The test is done at this point, and you are just being neat and tidy for
the next user. We don't need to do anything but warn:

igt_sysfs_set_u32(sysfs, "gt_min_freq_mhz");
if (igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz") != min_freq)
igt_warn("Unable to restore min frequency to save value [%d MHz], now %d MHz\n",
min_freq, igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz"));
> +
> + close(fd);

Add to the list of subtests that want a destructor (for clean error
paths).

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris