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 1AFC9CD342C for ; Wed, 6 May 2026 04:58:20 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A195D10EC9E; Wed, 6 May 2026 04:58:19 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="jabsqoKX"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) by gabe.freedesktop.org (Postfix) with ESMTPS id 98D2E10EC9E for ; Wed, 6 May 2026 04:58:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1778043484; x=1809579484; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=B0FAdYGXculKll3aESwYKOOHKyn91UW6zQZ9gnlCT6o=; b=jabsqoKXqs1sUqBYeQ6QWJCJ1k7wQulG8X9lBKAaZwqOdI9p7l4iIrdW w2+DWoBOnW/V7yzMKPxLGT8z2p7iNMSl+Ek5WQYxy2Z8oAyO5uRUOkNZC pXBfQ0g022ZYDqceNJUXCYySmNvstxPcZkVNkrty/eCA5jVvR8eIo2zC7 S7RQ1wcjV9aKrvoheHDECwcC3H9ocaNfZd9C3JVQdj4jCxKaJsSzFojys byMZ/GkoU3dP09PlF+/XqtQiEzOnV0Y7cTMdBdMq9N4lMoQxE9utxtuTI maLOq4bUpgmnf4+3ipeN89DKHfL4xBeSC2RfiOx2hyJiqP2xmxjrBK/Sq A==; X-CSE-ConnectionGUID: D5z58OFrQaS310guOAxEBQ== X-CSE-MsgGUID: vfMawtZkSOqQBGxdBqLjKA== X-IronPort-AV: E=McAfee;i="6800,10657,11777"; a="78083491" X-IronPort-AV: E=Sophos;i="6.23,218,1770624000"; d="scan'208";a="78083491" Received: from orviesa007.jf.intel.com ([10.64.159.147]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 May 2026 21:58:03 -0700 X-CSE-ConnectionGUID: fKna3AvRTcaQPiFAz+YKXA== X-CSE-MsgGUID: MghzglpTQ5SQvpXPxK4dJA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,218,1770624000"; d="scan'208";a="236264222" Received: from maduranp-mobl1.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.34.22]) by orviesa007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 May 2026 21:58:03 -0700 Date: Tue, 05 May 2026 21:58:02 -0700 Message-ID: <87ecjpgmn9.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Shekhar Chauhan Cc: Subject: Re: [PATCH v5 2/2] tests/intel/xe_oa: Wa_14026633728 In-Reply-To: <9c0f2a57-dece-4d71-b8bc-5333cbff015d@intel.com> References: <20260506024443.612636-1-shekhar.chauhan@intel.com> <20260506024443.612636-3-shekhar.chauhan@intel.com> <9c0f2a57-dece-4d71-b8bc-5333cbff015d@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/30.2 (x86_64-pc-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 Tue, 05 May 2026 19:55:14 -0700, Shekhar Chauhan wrote: > > > On 5/6/2026 8:14, Shekhar Chauhan wrote: > > For MERTOA in CRI, oa buffer can be in device memory. Because of slower > > device mem access, OA exponent values lower than 8 can result in buffer > > overflows. Bump the OA exponent value. > > > > Signed-off-by: Shekhar Chauhan > > --- > > tests/intel/xe_oa.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c > > index 988c46df6..67db20168 100644 > > --- a/tests/intel/xe_oa.c > > +++ b/tests/intel/xe_oa.c > > @@ -24,6 +24,7 @@ > > #include "igt_device.h" > > #include "igt_syncobj.h" > > #include "igt_sysfs.h" > > +#include "intel_wa.h" > > #include "xe/xe_ioctl.h" > > #include "xe/xe_query.h" > > #include "xe/xe_oa.h" > > @@ -2643,8 +2644,15 @@ test_buffer_fill(const struct drm_xe_oa_unit *oau) > > static void > > test_non_zero_reason(const struct drm_xe_oa_unit *oau, size_t oa_buffer_size) > > { > > - /* ~20 micro second period */ > > - int oa_exponent = max_oa_exponent_for_period_lte(20000); > > + /* > > + * Wa_14026633728: For MERTOA in CRI, oa buffer can be in device memory. > > + * Because of slower device mem access, OA exponent values lower than 8 can > > + * result in buffer overflows. > > + * By default, use ~20 micro second period. > > + */ > > + int oa_exponent = max(max_oa_exponent_for_period_lte(20000), > > + (oau->oa_unit_type == DRM_XE_OA_UNIT_TYPE_MERT && > > + igt_has_intel_wa(drm_fd, "14026633728")) ? 8 : 0); > > I agree that the v2 was more readable. But, the issue in using that again > is the C90 rule violation. If at all I declare here and then later modify > it, properties[] uses oa_exponent, and I believe it's value should be > finalised before declaration. If I place this max condition before that, > again a C90 rule violation. The max() condition in the initializer avoids > this which is why I didn't make that change. If you have any other ideas, > I'm open to suggestions. Add the if statement after the declarations and modify the property array too (there are several examples of this) with the new exponent value. Note max etc. is not needed, as it was in v2. > > As for the dropping > 0, I still have mixed feelings about that > change. igt_has_intel_wa has 3 return values: 0 if no WA is present, 1 if > the WA is present and -1 on error (maybe couldn't load from debugfs or some > other issue). When I explicitly write it as > 0, it signifies that the > workaround was definitely found and it exists. Of course, the return value > would always be 1 when the workaround is found, so, technically, the code > is still right (and cleaner, I agree). OK, you are right. But the function is *very* badly written. A function returning int should generally return 0 on success and non-zero on failure. There is no point distinguishing between error and no_wa cases. Moreover, error case should just be an assert, rather than a return value. If you want to clean all this up, add a 3rd patch, since you already have a R-b from Gustavo on Patch 1. So retain Patch 1 as is for now. There are no users/callers of the function except for oa, so it's a good time to change it now before new callers appear. > > Let me know what you (Ashutosh) think about the oa_exponent declaration vs > initialization vs usage thing (in regards to C90) and I can send another > patch if a change is required. > > -shekhar > > > struct intel_xe_perf_metric_set *test_set = oa_unit_metric_set(oau); > > uint64_t fmt = test_set->perf_oa_format; > > size_t report_size = get_oa_format(fmt).size; > > -- > Shekhar Chauhan > Linux Graphics Software Engineer > Intel Corporation >