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

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

On Thu, 21 Apr 2022 10:00:51 -0700, Kamil Konieczny wrote:
>
> Hi Ashutosh,

Hi Kamil,

I have made some changes to this test so that it can get merged in
increments as the kernel patches are reviewed and merged. The new patch is
posted here:

https://patchwork.freedesktop.org/series/103175/

Some sysfs entries which are still under review are removed till they are
approved.

> On 2022-04-19 at 23:12:51 -0700, Ashutosh Dixit wrote:
> > 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
> -- ^
> I am not happy with that name here and with use of it in test
> name, there are many SoC with different clock dividers and/or
> multipliers and none of them is "disaggregated", they are just
> different IP blocks inside SoC which run with different
> frequncies. Maybe just drop this and do s/disag_// and
> s/"disaggregation"// and s/"disaggregated"// or use "media"
> instead (without colons).
> If you insist you can keep this and better describe this term.

Agreed, I have change the name of the test to "i915_pm_freq_mult" (in the
sense of frequency multiplier or factor) and discontinued using the
"disaggregated" term anywhere.

> > +IGT_TEST_DESCRIPTION(
> > +	"Tests for sysfs controls for \"disaggregated\" IP blocks, viz. IP "
> > +	"blocks which run at frequencies different from the main GT frequency."
>
> Imho this should be rewritten, those "disaggregated" should be
> dropped from description, so it will be like
>
>	"Tests for sysfs controls for IP blocks which run at frequencies "
>	"different from the main GT frequency."

Done, please take a look at the new text.

> > +#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
>
> Please add note that this is currently set to 0.1s.
>
> > + */
> > +#define wait_freq_set()	usleep(100000)

I'm skipping this since it is obvious that 0.1 seconds == 100000 us,
don't want to over-comment.

> > +
> > +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,
>
> What about only media engines here ? Or conversly, test without
> media engines ? This can go into separate test.

I have mentioned in reply to your other mail that at present the actual
media freq (after setting these factors) is not available on current
platforms but may be available on future platforms. We can make these
changes you suggest or add new tests later if needed after media freq is
available. At present just running the spinner to make sure GT is running
at max freq.

> > +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);
> > +	}
> > +
> > +	igt_describe("Tests for \"disaggregated\" media freq sysfs");
> -------------------------------- ^ -------------------- ^
> imho this "dis" word should be dropped and please
> s/freq/frequency/

Done.

> > diff --git a/tests/meson.build b/tests/meson.build
> > index 7261e9aa2950..cd89defbb418 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -356,6 +356,14 @@ test_executables += executable('gem_mmap_offset',
> >	   install : true)
> >  test_list += 'gem_mmap_offset'
> >
> > +test_executables += executable('i915_pm_disag_freq',
> > +	   join_paths('i915', 'i915_pm_disag_freq.c'),
>
> What about i915_pm_media_freq.c or i915_pm_freq.c or even
> i915_pm_ip_freq.c ? Looks better imho.

Changed to i915_pm_freq_mult.c, let me know if you're ok with this name.

Thanks.
--
Ashutosh