From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 72E69E7719A for ; Thu, 9 Jan 2025 22:28:23 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3393F10E4A2; Thu, 9 Jan 2025 22:28:23 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="Cm1x3qqy"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2543610E4A2 for ; Thu, 9 Jan 2025 22:28:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1736461701; x=1767997701; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=K2lP9OLAE4ldE4x8i/D8Ch3DnG0MjGZuDOd+PpbHkwA=; b=Cm1x3qqyUIvsC88Bj3bjOEMm+eKprGlyXGlvVeXBLdWGl/oLOSMdjGN5 K8tdtPnCSbjWCxzEK8fL+oo51HpuFmQAJ4P0f9Kt3AHi8YO4iNd82zLHR 5PQTh4xTC3qvu72fswe0MS16EyJgvGcrz4TBZpXHoxvIg0cVBjG/qrsad 08SetatSnuUgZCDJI5KJIaxEk5L2yk96L+p3c1i+7Stqp3b+LuGZCuxqX v1e4qb+92bGqiCTH8T/a4bGug5WWxNwozh+N+soL7rk/8I3uztgFXMVob 7m43REYB2wQolQ7L6zMkzL7B/z2IEmhTpzwAcw6idfQKmRRchf3+OczoT Q==; X-CSE-ConnectionGUID: dYuAJtGTRz+KVrmDzGIqGw== X-CSE-MsgGUID: 5Keam509Q/ejE1EciLePyw== X-IronPort-AV: E=McAfee;i="6700,10204,11310"; a="36905068" X-IronPort-AV: E=Sophos;i="6.12,302,1728975600"; d="scan'208";a="36905068" Received: from orviesa004.jf.intel.com ([10.64.159.144]) by fmvoesa109.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jan 2025 14:28:20 -0800 X-CSE-ConnectionGUID: +vofqpxNRHmDWnwSWtcBwg== X-CSE-MsgGUID: znsofS4DRbCBiXz4VcwkqQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,302,1728975600"; d="scan'208";a="108539020" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by orviesa004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jan 2025 14:28:20 -0800 Date: Thu, 09 Jan 2025 14:28:20 -0800 Message-ID: <855xmnvczf.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Sai Teja Pottumuttu Cc: , , Subject: Re: [PATCH i-g-t v2 2/2] tests/intel/xe_oa: Remove hardcoded time heuristics In-Reply-To: <20250106034638.1746051-3-sai.teja.pottumuttu@intel.com> References: <20250106034638.1746051-1-sai.teja.pottumuttu@intel.com> <20250106034638.1746051-3-sai.teja.pottumuttu@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-redhat-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-BeenThere: igt-dev@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development mailing list for IGT GPU Tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" On Sun, 05 Jan 2025 19:46:38 -0800, Sai Teja Pottumuttu wrote: > Hi Sai Teja, Thanks for the patch, mostly looks good but I have some usual nits. > Some tests in xe_oa tests have hardcoded timing heuristics. Refactor it to > make it more robust and reliable. The patch extends the wait time logically > but usually it would take a single iteration for the required reports to be > available so wait time doesn't change much. > > v2: > - Extend commit message [Lucas] > - Make wait function more generic [Lucas] > > Signed-off-by: Sai Teja Pottumuttu > --- > tests/intel/xe_oa.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c > index ad3526406..b271278d6 100644 > --- a/tests/intel/xe_oa.c > +++ b/tests/intel/xe_oa.c > @@ -4367,6 +4367,29 @@ static void map_oa_buffer_forked_access(const struct drm_xe_engine_class_instanc > munmap(vaddr, size); > } > > +static void wait_for_periodic_reports(void *oa_vaddr, Let's change this name to 'mmap_wait_for_periodic_reports', to highlight it only applies to the mmap'd OA buffer case. > + uint32_t n, optional nit: move this to the previous line, I generally try to to optimize vertical real estate. > + const struct drm_xe_engine_class_instance *hwe) > +{ > + uint32_t period_us = oa_exponent_to_ns(oa_exp_1_millisec) / 1000; > + struct intel_xe_perf_metric_set *test_set = metric_set(hwe); > + uint64_t fmt = test_set->perf_oa_format; > + struct oa_format format = get_oa_format(fmt); optional nit again, but delete the two temporary variables above, see below. > + uint32_t num_periodic_reports = 0; > + uint32_t *reports; > + > + while (num_periodic_reports < n) { > + usleep(100 * period_us); I think this should be something like: usleep(2 * n * period_us); So the wait time should be a function of n (not constant like 100). Here I'm assuming if we wait for '2 * n' periods, we should probably have n periodic reports. > + num_periodic_reports = 0; > + for (reports = (uint32_t *)oa_vaddr; > + reports[0] && oa_timestamp(reports, fmt); > + reports += format.size) { > + if (oa_report_is_periodic(reports)) > + num_periodic_reports++; > + } optional nit: I think this entire loop can just be: num_periodic_reports = 0; for (reports = (uint32_t *)oa_vaddr; reports[0] && oa_timestamp(reports, fmt) && oa_report_is_periodic(reports); reports += get_oa_format(test_set->perf_oa_format).size) num_periodic_reports++; Just a couple of general comments below, no need to change anything: * The loop is a little 'funky' in that it goes over the mapped OA buffer multiple times, even over previoulsy found reports. I think that is ok for now. * The other thing is that OA periodic reports should be generated as long as DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT has been used in OA properties. Both places where this function is called from has that, so that should be ok. * If HW somehow doesn't generate periodic reports we'll hang here. But that is not expected, so leave as is and we'll deal with it later if we ever hit that. > + } > +} > + > static void check_reports(void *oa_vaddr, uint32_t oa_size, > const struct drm_xe_engine_class_instance *hwe) > { > @@ -4396,12 +4419,10 @@ static void check_reports_from_mapped_buffer(const struct drm_xe_engine_class_in > { > void *vaddr; > uint32_t size; > - uint32_t period_us = oa_exponent_to_ns(oa_exp_1_millisec) / 1000; > > vaddr = map_oa_buffer(&size); > > - /* wait for approx 100 reports */ > - usleep(100 * period_us); > + wait_for_periodic_reports(vaddr, 20, hwe); I am wondering if we should make this 10 instead of 20, and also change 20 to 10 in check_reports(). > check_reports(vaddr, size, hwe); > > munmap(vaddr, size); > @@ -4426,12 +4447,11 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta > }; > void *vaddr; > uint32_t size; > - uint32_t period_us = oa_exponent_to_ns(oa_exp_1_millisec) / 1000; > > stream_fd = __perf_open(drm_fd, ¶m, false); > vaddr = map_oa_buffer(&size); > > - usleep(100 * period_us); > + wait_for_periodic_reports(vaddr, 20, hwe); Here too. > check_reports(vaddr, size, hwe); > > munmap(vaddr, size); > -- > 2.34.1 > Thanks. -- Ashutosh