Re: [igt-dev] [PATCH i-g-t 2/2] tests/i915_pm_disag_freq: New test for media freq factor

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 14 Apr 2022 08:40:21 -0700, Kamil Konieczny wrote:
>
> Hi Ashutosh,

Hi Kamil,

> Dnia 2022-04-13 at 10:27:47 -0700, Ashutosh Dixit napisał(a):
> > XEHPSDV and DG2/ATS-M allow media IP blocks to run at frequencies different
> > from the GT frequency. i915 exposes sysfs controls for this frequency
> > "disaggregation". IGT's introduced in this patch exercise and verify these
> > per-gt (gt/gtN) sysfs attributes.
> >
> > Further, RPS defaults exposed in gt/gtN/.defaults sysfs directory are used
> > in the test to start and complete in the known default state.
> >
> > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  tests/i915/i915_pm_disag_freq.c | 182 ++++++++++++++++++++++++++++++++
>
> imho test should be named i915_pm_media_freq.c

That would not be a correct name. The key word is "disag" as explained in
the commit message above. There is other IP where media freq is same as the
GT freq, we only want "disag" tests here. i915_pm_disag_media_freq will be
fine but we are planning to add more tests for "disaggregated" stuff here
later. So I can change to i915_pm_disag_media_freq if you insist but we
will need to change it back later. So that's why I've left it as is.

>
> >  tests/meson.build               |   8 ++
> >  2 files changed, 190 insertions(+)
> >  create mode 100644 tests/i915/i915_pm_disag_freq.c
> >
> > diff --git a/tests/i915/i915_pm_disag_freq.c b/tests/i915/i915_pm_disag_freq.c
> > new file mode 100644
> > index 000000000000..596ac9e8fd24
> > --- /dev/null
> > +++ b/tests/i915/i915_pm_disag_freq.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2022 Intel Corporation
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +
> > +#include "i915/gem.h"
> > +#include "igt.h"
> > +#include "igt_sysfs.h"
> > +
> > +#define FREQ_SCALE_FACTOR	0.00390625f	/* 1.0f / 256 */
> > +
> > +/*
> > + * Firmware interfaces are not completely synchronous, a delay is needed
> > + * before the requested freq is actually set.
> > + * Media ratio read back after set will mismatch if this value is too small
> > + */
> > +#define wait_freq_set()	usleep(100000)
>
> Isn't it a little too big ? 10^5 is about 0.1s ?

Leaving as is too, as explained in the comment above this has given me
enough trouble as it is. The issue is the media ratio is read back after
GuC firmware has initialize it and the delays there are completely
unpredictable.

>
> > +
> > +static int i915 = -1;
> > +const intel_ctx_t *ctx;
> > +uint64_t ahnd;
> > +
> > +static void spin_all(void)
> > +{
> > +	igt_spin_t *spin = igt_spin_new(i915, .ahnd = ahnd, .ctx = ctx, .engine = ALL_ENGINES,
> > +					.flags = IGT_SPIN_POLL_RUN);
> > +
> > +	/* Wait till at least one spinner starts */
> > +	igt_spin_busywait_until_started(spin);
> > +}
> > +
> > +static void restore_rps_defaults(int dir)
> > +{
> > +	int def, min, max, media;
> > +
> > +	/* Read from gt/gtN/.defaults/ write to gt/gtN/ */
> > +	def = openat(dir, ".defaults", O_RDONLY);
> > +	if (def <= 0)
> > +		return;
> > +
> > +	max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
> > +	igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
> > +
> > +	min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> > +	igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> > +
> > +	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> > +		media = igt_sysfs_get_u32(def, "media_freq_factor");
> > +		igt_sysfs_set_u32(dir, "media_freq_factor", media);
> > +	}
> > +
> > +	close(def);
> > +}
> > +
> > +static void __restore_rps_defaults(int sig)
> > +{
> > +	int dir, gt;
> > +
> > +	for_each_sysfs_gt_dirfd(i915, dir, gt)
> > +		restore_rps_defaults(dir);
> > +}
> > +
> > +static void setup_freq(int gt, int dir)
> > +{
> > +	int rp0, rp1, rpn, min, max, act, media;
> > +
> > +	ctx = intel_ctx_create_all_physical(i915);
> > +	ahnd = get_reloc_ahnd(i915, ctx->id);
> > +
> > +	/* Reset to known state */
> > +	restore_rps_defaults(dir);
> > +
> > +	/* Spin on all engines to jack freq up to max */
> > +	spin_all();
> > +	wait_freq_set();
> > +
> > +	/* Print some debug information */
> > +	rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
> > +	rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
> > +	rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
> > +	min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
> > +	max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
> > +	act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> > +
> > +	igt_info("RP0 mhz: %d, RP1 mhz: %d, RPn mhz: %d, min mhz: %d, max mhz: %d, act mhz: %d\n", rp0, rp1, rpn, min, max, act);
> > +
> > +	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> > +		media = igt_sysfs_get_u32(dir, "media_freq_factor");
> > +		igt_info("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
> > +	}
> > +}
> > +
> > +static void cleanup(int dir)
> > +{
> > +	igt_free_spins(i915);
> > +	put_ahnd(ahnd);
> > +	intel_ctx_destroy(i915, ctx);
> > +	restore_rps_defaults(dir);
> > +	gem_quiescent_gpu(i915);
> > +}
> > +
> > +static void media_freq(int gt, int dir)
> > +{
> > +	float scale;
> > +
> > +	igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
> > +
> > +	igt_sysfs_scanf(dir, "media_freq_factor.scale", "%g", &scale);
> > +	igt_assert_eq(scale, FREQ_SCALE_FACTOR);
> > +
> > +	setup_freq(gt, dir);
> > +
> > +	igt_info("media RP0 mhz: %d, media RPn mhz: %d\n",
> > +		 igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
> > +		 igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
> > +	igt_info("media ratio value 0.0 represents dynamic mode\n");
> > +
> > +	/*
> > +	 * Media freq ratio modes supported are: dynamic (0), 1:2 (128) and
> > +	 * 1:1 (256). Setting dynamic (0) can return any of the three
> > +	 * modes. Fixed ratio modes should return the same value.
> > +	 */
> > +	for (int v = 256; v >= 0; v -= 64) {
> > +		int getv, ret;
> > +
> > +		/*
> > +		 * Check that we can set the mode. Ratios other than 1:2
> > +		 * and 1:1 are not supported.
> > +		 */
> > +		ret = igt_sysfs_printf(dir, "media_freq_factor", "%u", v);
> > +		if (ret <= 0) {
> > +			igt_info("Media ratio %.2f is not supported\n", v * scale);
> > +			continue;
> > +		}
> > +
> > +		wait_freq_set();
> > +
> > +		getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> > +
> > +		igt_info("media ratio set: %.2f, media ratio get: %.2f\n",
> > +			 v * scale, getv * scale);
> > +
> > +		/*
> > +		 * Skip validation in dynamic mode since the returned media
> > +		 * ratio and freq are platform dependent and not clearly defined
> > +		 */
> > +		if (!v)
> > +			continue;
> ----------------------- ^
>
> This is last iteration, so maybe just "break;" here or
>		if (v)
>			igt_assert_eq(getv, v);
>
> If you change then also change comment appropriatly.

Done in v2.

> Do we need all that igt_info above ? I would change all or
> almost all of them into igt_debug.

Done in v2. I want to see all the values if there's a failure but I think
igt_debug gets dumped to stderr too so it should be ok.

> > +
> > +		igt_assert_eq(getv, v);
> > +	}
> > +
> > +	cleanup(dir);
> > +}
> > +
> > +igt_main
> > +{
> > +	int dir, gt;
> > +
> > +	igt_fixture {
> > +		i915 = drm_open_driver(DRIVER_INTEL);
> > +
> > +		/* Disag multipliers (aka "frequency factors") are not simulated. */
> > +		igt_require(!igt_run_in_simulation());
> > +		igt_install_exit_handler(__restore_rps_defaults);
> > +	}
> > +
>
> Please put description before subtest, check with --describe
> what needs to be described. Write also global description at
> begin of file.

Done in v2.

Thanks for reviewing.
--
Ashutosh