From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4A5D510E9AB for ; Fri, 27 Jan 2023 16:17:00 +0000 (UTC) Date: Fri, 27 Jan 2023 08:13:14 -0800 Message-ID: <87mt64ez1h.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Ashutosh Dixit , Riana Tauro In-Reply-To: <20230127122945.yrlacxyfjhqihlua@kamilkon-desk1> References: <20230127062502.134917-1-ashutosh.dixit@intel.com> <20230127062502.134917-2-ashutosh.dixit@intel.com> <20230127122945.yrlacxyfjhqihlua@kamilkon-desk1> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [igt-dev] [PATCH i-g-t 1/2] i915/i915_power: Sanity check power measurement List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Fri, 27 Jan 2023 04:29:45 -0800, Kamil Konieczny wrote: > > Hi Ashutosh, Hi Kamil, I have addressed all comments. Please take a look at the latest version: https://patchwork.freedesktop.org/series/113436/ (not sure why Patchwork is creating new series each time). Thanks. -- Ashutosh > > On 2023-01-26 at 22:25:01 -0800, Ashutosh Dixit wrote: > > Add a test to sanity check power measurement on integrated/discrete by > > requiring power under load to be > 0 and >=3D idle power. > > > > v2: Code reorganization (Kamil K) > > > > Signed-off-by: Ashutosh Dixit > > Reviewed-by: Riana Tauro > > --- > > tests/i915/i915_power.c | 80 +++++++++++++++++++++++++++++++++++++++++ > > tests/meson.build | 1 + > > 2 files changed, 81 insertions(+) > > create mode 100644 tests/i915/i915_power.c > > > > diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c > > new file mode 100644 > > index 00000000000..00993cdc846 > > --- /dev/null > > +++ b/tests/i915/i915_power.c > > @@ -0,0 +1,80 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright =A9 2023 Intel Corporation > > + */ > > + > > +#include "igt.h" > > +#include "i915/gem.h" > > +#include "igt_power.h" > > + > > +IGT_TEST_DESCRIPTION("i915 power measurement tests"); > > + > > +static double get_power(struct igt_power *pwr, const char *domain) > ------------------------------------------------------------- ^ > You are not using this so delete it. Done. > > > +{ > > + const int sleep_duration_sec =3D 2; > ----------------- ^ > Make this a parameter for function. Done. > > > + struct power_sample sample[2]; > > + > > + igt_power_get_energy(pwr, &sample[0]); > > + usleep(sleep_duration_sec * USEC_PER_SEC); > > + igt_power_get_energy(pwr, &sample[1]); > > + > > + return igt_power_get_mW(pwr, &sample[0], &sample[1]); > > +} > > + > > +static double measure_power(int i915, struct igt_power *pwr, > > + const char *domain, bool load) > ----------------------------------------------- ^ > This is confusing, delete this. Please avoid creating functions > with boolean parameter, just do its work in place you will use > true or false. Done. > > > +{ > > + const intel_ctx_t *ctx =3D intel_ctx_create_all_physical(i915); > > + uint64_t ahnd =3D get_reloc_ahnd(i915, ctx->id); > > + igt_spin_t *spin; > > + double mW; > > + > > + gem_quiescent_gpu(i915); > > + if (load) { > ------- ^ > Delete this if() and move spin creation after idle see below. Done. > > > + spin =3D igt_spin_new(i915, .ahnd =3D ahnd, .ctx =3D ctx, > > + .engine =3D ALL_ENGINES, > > + .flags =3D IGT_SPIN_POLL_RUN); > > + /* Wait till at least one spinner starts */ > > + igt_spin_busywait_until_started(spin); > > + } > > + > > + mW =3D get_power(pwr, domain); > > + igt_info("Domain: %s, Load: %d, Measured power: %g mW\n", domain, loa= d, mW); > > + > > + igt_free_spins(i915); > > + put_ahnd(ahnd); > > + intel_ctx_destroy(i915, ctx); > > + > > + return mW; > > +} > > + > > +static void sanity(int i915, const char *domain) > > +{ > > + struct igt_power pwr; > > + double idle, busy; > > + > > + igt_require(!igt_power_open(i915, &pwr, domain)); > > + idle =3D measure_power(i915, &pwr, domain, false); > > + busy =3D measure_power(i915, &pwr, domain, true); > > + igt_power_close(&pwr); > > + > > + igt_assert(idle >=3D 0 && busy > 0 && busy >=3D idle); > ------------------------------------------------ ^ > imho this should be "busy > idle" ? Done. > > Merge sanity() with measure_power() so it will be simple. > This should look like: > // check require > // prepare ctx and other > idle =3D get_power(pwr, 2 * USEC_PER_SEC); /* 2 seconds */ > // create spin and wait until it will run > busy =3D get_power(pwr, 2 * USEC_PER_SEC); /* 2 seconds */ > // cleanup after spin, ctx, and other > // print info what was measured, domain, power and time used > // assert Done. > > > +} > > + > > +igt_main > > +{ > > + int i915; > > + > > + igt_fixture { > > + i915 =3D drm_open_driver_master(DRIVER_INTEL); > > + } > > + > > + igt_describe("Sanity check power measurement"); > > + igt_subtest("sanity") { > > + sanity(i915, "gpu"); > ----------------------------- ^ > Do you plan other domains, maybe multi-gpu like gpu-#number ? Future patch. > > Regards, > Kamil > > > + } > > + > > + igt_fixture { > > + close(i915); > > + } > > +} > > diff --git a/tests/meson.build b/tests/meson.build > > index cce9d89e1c8..d93a07c9ed6 100644 > > --- a/tests/meson.build > > +++ b/tests/meson.build > > @@ -213,6 +213,7 @@ i915_progs =3D [ > > 'i915_pm_dc', > > 'i915_pm_rps', > > 'i915_pm_sseu', > > + 'i915_power', > > 'i915_query', > > 'i915_selftest', > > 'i915_suspend', > > -- > > 2.38.0 > >