From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2347A10E6F2 for ; Tue, 24 Jan 2023 18:02:21 +0000 (UTC) Date: Tue, 24 Jan 2023 10:01:35 -0800 Message-ID: <87lelrlslc.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Ashutosh Dixit , Badal Nilawar , Riana Tauro , Anshuman Gupta In-Reply-To: <20230124170023.uxpgwmec635djm7t@kamilkon-desk1> References: <20230121054153.410979-1-ashutosh.dixit@intel.com> <20230121054153.410979-2-ashutosh.dixit@intel.com> <20230124170023.uxpgwmec635djm7t@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 Tue, 24 Jan 2023 09:00:23 -0800, Kamil Konieczny wrote: > > Hi Ashutosh, > > one nit, see below. Hi Kamil, > > On 2023-01-20 at 21:41:52 -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. > > > > Signed-off-by: Ashutosh Dixit > > --- > > tests/i915/i915_power.c | 73 +++++++++++++++++++++++++++++++++++++++++ > > tests/meson.build | 1 + > > 2 files changed, 74 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..e7d5c32f1af > > --- /dev/null > > +++ b/tests/i915/i915_power.c > > @@ -0,0 +1,73 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright =A9 2022 Intel Corporation > > + */ > > + > > +#include "igt.h" > > +#include "i915/gem.h" > > +#include "igt_power.h" > > + > > +IGT_TEST_DESCRIPTION("i915 power measurement tests"); > > + > > +static double power(int i915, const char *domain, bool load) > > +{ > > + const intel_ctx_t *ctx =3D intel_ctx_create_all_physical(i915); > > + uint64_t ahnd =3D get_reloc_ahnd(i915, ctx->id); > > + struct power_sample sample[2]; > > + int sleep_duration_sec =3D 2; > > + struct igt_power pwr; > > + igt_spin_t *spin; > > + double mW; > > + > > + gem_quiescent_gpu(i915); > > + if (load) { > > + 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); > > + } > > + > > + igt_require(!igt_power_open(i915, &pwr, domain)); > > Please move this before igt_spin_new, Sure, I can move this to the top of the function. > also rewrite this so you will free context before Sorry, before what? I don't want to free the context before igt_free_spins because that requires context persistence which is not fully supported on GuC based systems. > or move context creation to igt_fixture and give it as param for this > function. Here I consider the context internal to the power measurement so I prefer to keep it in this function. But if you explain the reason why you are asking for these changes maybe I can figure out what to do. Do you just want to create the context after igt_require? Thanks. -- Ashutosh > > Regards, > Kamil > > > + igt_power_get_energy(&pwr, &sample[0]); > > + usleep(sleep_duration_sec * USEC_PER_SEC); > > + igt_power_get_energy(&pwr, &sample[1]); > > + > > + mW =3D igt_power_get_mW(&pwr, &sample[0], &sample[1]); > > + igt_info("Domain: %s, Load: %d, Measured power: %g mW\n", domain, loa= d, mW); > > + > > + igt_power_close(&pwr); > > + igt_free_spins(i915); > > + put_ahnd(ahnd); > > + intel_ctx_destroy(i915, ctx); > > + > > + return mW; > > +} > > + > > +static void sanity(int i915, const char *domain) > > +{ > > + double idle, busy; > > + > > + idle =3D power(i915, domain, false); > > + busy =3D power(i915, domain, true); > > + > > + igt_assert(busy > 0 && busy >=3D idle); > > +} > > + > > +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"); > > + } > > + > > + igt_fixture { > > + close(i915); > > + } > > +} > > diff --git a/tests/meson.build b/tests/meson.build > > index e20a864035b..e0f41e9e6a1 100644 > > --- a/tests/meson.build > > +++ b/tests/meson.build > > @@ -212,6 +212,7 @@ i915_progs =3D [ > > 'i915_pm_dc', > > 'i915_pm_rps', > > 'i915_pm_sseu', > > + 'i915_power', > > 'i915_query', > > 'i915_selftest', > > 'i915_suspend', > > -- > > 2.38.0 > >