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 16846CD4F3C for ; Sat, 16 May 2026 19:11:35 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9239910E1FB; Sat, 16 May 2026 19:11:35 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="hlSsujzM"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.9]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8DEAF10E1FB for ; Sat, 16 May 2026 19:11:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1778958675; x=1810494675; h=date:message-id:from:to:subject:in-reply-to:references: mime-version; bh=k1YwkmJUQRm60OeIjn0A0LLctVMRmhV9ge1LFXezL9g=; b=hlSsujzMxND7ABnC2mbslRfB8GkbliAOVm2rouVIzO4s/35wmjMeDtfm 5HWpH8RvJMouflwJsMnl1aezxm22oQuicEHeLdAKkfrtugmFllx3poufe OoAymvubjtpmDnvW6yxbUjc3onuPZ9IpvNXTQ6S95w/3tzahjOsQ07m6u JcTVkxJ27NwAbGwFEWVW9gCS2t1B13RZgP6TCLtpVFSS6A61/f7yeOcGX jGqrmZ6edSf8+vA1XRYDkbGtvMbpK9L4EffK2kWE4K15j+lEZJk5V3pGU m1JcbhXysGoCXcnRS6BeKqj0bGXa9JStZtbNYifNhd4M+TkHYJ2peWFLQ Q==; X-CSE-ConnectionGUID: iylXYWvLSyStz+KdIH8c9A== X-CSE-MsgGUID: MIqTZl6LSWOE1XYCDhjNkg== X-IronPort-AV: E=McAfee;i="6800,10657,11788"; a="90573718" X-IronPort-AV: E=Sophos;i="6.23,238,1770624000"; d="scan'208";a="90573718" Received: from fmviesa009.fm.intel.com ([10.60.135.149]) by fmvoesa103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 May 2026 12:11:15 -0700 X-CSE-ConnectionGUID: jyy+MwegQBe9EyU4ZKRBkQ== X-CSE-MsgGUID: wvrQwgVkQUCexigCYLHVig== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,238,1770624000"; d="scan'208";a="232625783" Received: from bwrockwe-mobl.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.70.219]) by fmviesa009-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 May 2026 12:11:15 -0700 Date: Sat, 16 May 2026 12:11:13 -0700 Message-ID: <87ecjbgofi.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Kamil Konieczny , Shekhar Chauhan , igt-dev@lists.freedesktop.org, ashutosh.dixit@intel.com Subject: Re: [PATCH v2] lib/intel_wa: Assert on error instead of returning -1 In-Reply-To: <20260515122408.zd564l5tnhaprha3@kamilkon-DESK.igk.intel.com> References: <20260514063655.1683322-1-shekhar.chauhan@intel.com> <20260515122408.zd564l5tnhaprha3@kamilkon-DESK.igk.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 Fri, 15 May 2026 05:24:08 -0700, Kamil Konieczny wrote: > Hi Kamil, > Hi Shekhar, > On 2026-05-14 at 12:06:55 +0530, Shekhar Chauhan wrote: > > igt_has_intel_wa() returns 0, 1, or -1, but there is no point > > distinguishing the error and no-workaround cases for callers. Replace > > the error return with igt_assert() and simplify debugfs_file_has_wa() by > > dropping the igt_debugfs_exists() check, since igt_sysfs_get() returns > > NULL for missing files anyway. > > > > v2: Make the func as bool instead of int. > > > > Signed-off-by: Shekhar Chauhan > > --- > > lib/intel_wa.c | 19 +++++++++---------- > > lib/intel_wa.h | 2 +- > > 2 files changed, 10 insertions(+), 11 deletions(-) > > > > diff --git a/lib/intel_wa.c b/lib/intel_wa.c > > index 727dd6c98..e4fe7b0ee 100644 > > --- a/lib/intel_wa.c > > +++ b/lib/intel_wa.c > > @@ -7,18 +7,18 @@ > > #include > > #include > > > > +#include "igt_core.h" > > #include "igt_debugfs.h" > > #include "igt_sysfs.h" > > #include "intel_wa.h" > > #include "xe/xe_query.h" > > > > -static int debugfs_file_has_wa(int drm_fd, int debugfs_fd, > > +static bool debugfs_file_has_wa(int drm_fd, int debugfs_fd, > > const char *debugfs_name, const char *wa) > > { > > char *debugfs_dump; > > > > - if (!igt_debugfs_exists(drm_fd, debugfs_name, O_RDONLY)) > > - return -1; > > + igt_assert(igt_debugfs_exists(drm_fd, debugfs_name, O_RDONLY)); > > > > debugfs_dump = igt_sysfs_get(debugfs_fd, debugfs_name); > > if (debugfs_dump) { > > @@ -27,10 +27,10 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd, > > free(debugfs_dump); > > > > if (has_wa) > > - return 1; > > + return true; > > } > > > > - return 0; > > + return false; > > } > > > > /** > > @@ -38,18 +38,17 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd, > > * @drm_fd: A drm file descriptor > > * @check_wa: Workaround to be checked > > * > > - * Returns: 0 if no WA, 1 if WA present, -1 on error > > + * Returns: true if WA present, false otherwise > > */ > > -int igt_has_intel_wa(int drm_fd, const char *check_wa) > > +bool igt_has_intel_wa(int drm_fd, const char *check_wa) > > { > > - int ret = 0; > > + bool ret = 0; > > int debugfs_fd; > > unsigned int xe; > > char name[256]; > > > > debugfs_fd = igt_debugfs_dir(drm_fd); > > - if (debugfs_fd == -1) > > - return -1; > > + igt_assert(debugfs_fd >= 0); > > This will mean that all other users needs to be updated. To date there are no other users of this function, except one in OA. > The way of adding any igt_assert/igt_require into lib/ > make it impossible to reuse them in tools/ and require > to write same functionality twice, one in lib/ and one > for any tools which will want to use them. Or to write > __function() without igt_assert/igt_require. Well it was I who suggested doing this. Can you explain your point a bit? Any tools which use a lib function which uses e.g. igt_assert/igt_require can also link with lib_igt (which provides igt_assert/igt_require). See e.g. tools/xe-perf/meson.build. It is not unreasonable for IGT tools to link with lib_igt. I thought not linking with igt_lib was an unreasonabe requirement and I removed it for tools/xe-perf. If you are saying tools should be able to run without debugfs mounted, maybe you have a point. In that case we can drop this patch. But even here maybe IGT tools should run only with debugfs mounted? Thanks. -- Ashutosh > > xe_for_each_gt(drm_fd, xe) { > > sprintf(name, "gt%d/workarounds", xe); > > diff --git a/lib/intel_wa.h b/lib/intel_wa.h > > index 765a5948e..34cafecc4 100644 > > --- a/lib/intel_wa.h > > +++ b/lib/intel_wa.h > > @@ -6,6 +6,6 @@ > > #ifndef __INTEL_WA_H__ > > #define __INTEL_WA_H__ > > > > -int igt_has_intel_wa(int drm_fd, const char *check_wa); > > +bool igt_has_intel_wa(int drm_fd, const char *check_wa); > > > > #endif /* __INTEL_WA_H__ */ > > -- > > 2.53.0 > >