[igt-dev] [PATCH igt] igt: Add gem_ctx_freq to exercise requesting freq on a ctx

[Sagar Arun Kamble]

On 3/13/2018 7:28 PM, Chris Wilson wrote:
> Exercise some new API that allows applications to request that
> individual contexts are executed within a desired frequency range.
>
> v2: Split single/continuous set_freq subtests
> v3: Do an up/down ramp for individual freq request, check nothing
> changes after each invalid request
> v4: Check the frequencies reported by the kernel across the entire
> range.
> v5: Rewrite sandwich to create a sandwich between multiple concurrent
> engines.
> v6: Exercise sysfs overrides.
> v7: Reset min/max of default context after independent(); don't ask
> about failure
> v8: Check transition beyond randomly chosen frequencies as well as
> up/down ramps.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Praveen Paneri <praveen.paneri at intel.com>
> Cc: Sagar A Kamble <sagar.a.kamble at intel.com>
> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> Reviewed-by: Antonio Argenziano <antonio.argenziano at intel.com> #v5
There are few stray whitespaces in __pmu_within_tolerance, pmu_assert.
Otherwise looks good to me.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>

Can you please clarify few things below:
> ---
<snip>
> +
> +static void sysfs_clamp(int fd, const struct intel_execution_engine *e)
> +{
> +#define N_STEPS 10
> +	const unsigned int engine = e->exec_id | e->flags;
> +	uint32_t ctx = gem_context_create(fd);
> +	uint32_t sys_min, sys_max;
> +	uint32_t min, max;
> +	double measured;
> +	igt_spin_t *spin;
> +	int pmu;
> +
> +	get_sysfs_freq(&sys_min, &sys_max);
> +	igt_info("System min freq: %dMHz; max freq: %dMHz\n", sys_min, sys_max);
> +
> +	get_freq(fd, ctx, &min, &max);
> +	igt_info("Context min freq: %dMHz; max freq: %dMHz\n", min, max);
> +
> +	pmu = perf_i915_open(I915_PMU_REQUESTED_FREQUENCY);
> +	igt_require(pmu >= 0);
> +
> +	for (int outer = 0; outer <= 2*N_STEPS; outer++) {
> +		int ofrac = outer > N_STEPS ? 2*N_STEPS - outer : outer;
> +		uint32_t ofreq = min + (max - min) * ofrac / N_STEPS;
> +		uint32_t cur, discard;
> +
> +		for (int inner = 0; inner <= 2*N_STEPS; inner++) {
> +			int ifrac = inner > N_STEPS ? 2*N_STEPS - inner : inner;
> +			uint32_t ifreq = min + (max - min) * ifrac / N_STEPS;
> +
> +			set_freq(fd, ctx, ifreq, ifreq);
> +
> +			gem_quiescent_gpu(fd);
> +			spin = __igt_spin_batch_new(fd, ctx, engine, 0);
> +			usleep(10000);
> +
> +			set_sysfs_freq(ofreq, ofreq);
> +			get_sysfs_freq(&cur, &discard);
We don't sleep here because we know that we set the frequency in sysfs?
> +
> +			measured = measure_frequency(pmu, SAMPLE_PERIOD);
> +			igt_debugfs_dump(fd, "i915_rps_boost_info");
> +
> +			set_sysfs_freq(sys_min, sys_max);
> +
> +			igt_spin_batch_free(fd, spin);
> +			igt_info("%s(sysfs): Measured %.1fMHz, context %dMhz, expected %dMhz\n",
> +					e->name, measured, ifreq, cur);
> +			pmu_assert(measured, cur);
> +		}
> +	}
> +	gem_quiescent_gpu(fd);
> +
> +	close(pmu);
> +	gem_context_destroy(fd, ctx);
> +
> +#undef N_STEPS
> +}
> +
...
> +static void disable_boost(int fd)
> +{
> +	char *value;
> +
> +	value = igt_sysfs_get(fd, "gt_RPn_freq_mhz");
> +	igt_sysfs_set(fd, "gt_boost_freq_mhz", value);
Why is this needed? kernel will not clamp boost freq as well within 
ctx_freq_min/max?
Kernel disabling boost seems more effective than setting boost_freq to Rpn.
> +	free(value);
> +}
> +
> +igt_main
> +{
> +	const struct intel_execution_engine *e;
> +	int fd = -1;
> +
> +	igt_fixture {
> +		fd = drm_open_driver(DRIVER_INTEL);
> +		igt_require_gem(fd);
> +
> +		igt_require(has_ctx_freq(fd));
> +
> +		sysfs = igt_sysfs_open(fd, NULL);
> +		igt_assert(sysfs != -1);
> +		igt_install_exit_handler(restore_sysfs_freq);
> +
> +		disable_boost(sysfs);
> +	}
> +
> +	igt_subtest("invalid")
> +		invalid(fd);
> +
> +	igt_subtest("idempotent")
> +		idempotent(fd);
> +
> +	igt_subtest("range")
> +		range(fd);
> +
> +	igt_subtest("independent")
> +		independent(fd);
> +
> +	igt_skip_on_simulation();
> +
> +	for (e = intel_execution_engines; e->name; e++) {
> +		igt_subtest_group {
> +			igt_fixture {
> +				gem_require_ring(fd, e->exec_id | e->flags);
> +			}
> +
> +			igt_subtest_f("%s-single", e->name)
> +				single(fd, e);
> +			igt_subtest_f("%s-continuous", e->name)
> +				continuous(fd, e);
> +			igt_subtest_f("%s-inflight", e->name)
> +				inflight(fd, e);
> +			igt_subtest_f("%s-sysfs", e->name)
> +				sysfs_clamp(fd, e);
> +		}
> +	}
> +
> +	igt_subtest("sandwich")
> +		sandwich(fd, 20);
> +
> +	igt_subtest("smoketest")
> +		smoketest(fd, 20);
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 1176463c..f07ad25e 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -36,6 +36,7 @@ test_progs = [
>   	'gem_ctx_create',
>   	'gem_ctx_exec',
>   	'gem_ctx_isolation',
> +	'gem_ctx_freq',
>   	'gem_ctx_param',
>   	'gem_ctx_switch',
>   	'gem_ctx_thrash',

-- 
Thanks,
Sagar