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 70AFBC3DA49 for ; Fri, 26 Jul 2024 15:01:37 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3283510E9BD; Fri, 26 Jul 2024 15:01:37 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="G/h3q5CK"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id C619910E9BD for ; Fri, 26 Jul 2024 15:01:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1722006095; x=1753542095; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=X1Wk7IMaLkDOt9h/dpS1FlOzvtZ3VGksjn24xyW84Gk=; b=G/h3q5CKHmmeAwtBZqbxVpeTolqRbuo0AxqRRzu2vtUBvX4IL5CYdSZQ nQXNXlfzP34d8LXjMJL3AIPJygPU2e0ielENRemgulVm0j/gySqnR/pqa 8uQf5Drh2G/4jYC3Zm9l7HDMRBekwEID8Zy48aNx8LLspZ7T13JbJyAE8 AieA6/OwACMfQvMd4XzjJkpTHx2EuTVrW4NFyTCSMPXQBJdAKq7RlgWVo y/Bs0S61g/cevflJglK9B9anrcXE8tYWSKNbGg7c7IsEiUXlKngA3cEto ECjkd/kWNPMnDAMYOIx1UUiSiUfrCaa+7ROyHRwzF1ehWmHsv6d9r0Hpp A==; X-CSE-ConnectionGUID: 69KacBAdQNGeyIRB1+W6RQ== X-CSE-MsgGUID: I9MzHlcFTIOS06RgYJNB1Q== X-IronPort-AV: E=McAfee;i="6700,10204,11145"; a="19958733" X-IronPort-AV: E=Sophos;i="6.09,238,1716274800"; d="scan'208";a="19958733" Received: from orviesa009.jf.intel.com ([10.64.159.149]) by fmvoesa109.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jul 2024 08:01:35 -0700 X-CSE-ConnectionGUID: +GK6mi9STyi5dnvfJrGwGA== X-CSE-MsgGUID: 82huWHGdTYe0pjXgaWj4Hg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.09,238,1716274800"; d="scan'208";a="53305487" Received: from trevorsx-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.125.177.104]) by orviesa009-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jul 2024 08:01:35 -0700 Date: Fri, 26 Jul 2024 07:43:40 -0700 Message-ID: <877cd8gqn7.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Peter Senna Tschudin Cc: "igt-dev@lists.freedesktop.org" , Kamil Konieczny Subject: Re: [PATCH i-g-t v2] lib/xe/xe_query: Do not require QUERY_OA_UNITS for all Xe tests In-Reply-To: <78c5f531-f482-49c7-8290-38710698e4a7@linux.intel.com> References: <78c5f531-f482-49c7-8290-38710698e4a7@linux.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/29.4 (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 Thu, 25 Jul 2024 07:03:19 -0700, Peter Senna Tschudin wrote: > Hi Peter, > Changes xe_query_oa_units_new() to return NULL instead of aborting > due to assertion failure when querying for DRM_XE_DEVICE_QUERY_OA_UNITS. > > It adds a skip to xe_oa.c to make sure it will not try to run the tests if the > query has failed before. > > The motivation is as follows: > - The only test that requires oa_units is tests/intel/xe_oa.c, however current > code will prevent all Xe tests from running if OA_UNITS is not supported > - Before this patch, trying to run IGT on the Xe module from Fedora 40 > (6.9.9-200.fc40.x86_64) fails > - After this patch, we can run some tests, such as xe_exec_basic > successfully As far as I understand the purpose of IGT is to test the latest drm-tip kernels. Also, I would think testing older kernels is of dubious value, since bugs you would find there would typically have been fixed in later kernels. I also consider this patch sort of temporary, I would imagine it can be reverted after distro kernels have moved to 6.11 where the uapi in this patch will be available. With that out of the way, I will approve this patch for merging but I have a couple of nit-picks below. > > Here is an example of the impact of this patch: > 1. Use Fedora 40 with the following kernel command line arguments: > "xe.force_probe=* i915.force_probe=!* modprobe.blacklist=i915,ast,snd_hda_intel,xe" > > 2. Running IGT without this patch will fail: > $ sudo ./build/tests/xe_exec_basic > $ echo $? > 98 > > 3. Running after fix applied will succeed: > sudo ./build/tests/xe_exec_basic > $ sudo ./build/tests/xe_exec_basic > $ echo $? > 0 > > V2: > - Cleanup headers from message body > - Add 'lib/xe/xe_query:' to subject line Also, note that, assuming you are new here, for IGT and kernel, the commit message should explain the "why" or the reason for the patch. So I would think the above commit message can be condensed, but that is up to you, it's ok with me. > > Signed-off-by: Peter Senna Tschudin > --- > lib/xe/xe_query.c | 6 +++++- > tests/intel/xe_oa.c | 9 ++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c > index 84eaaac96..f0643e1bc 100644 > --- a/lib/xe/xe_query.c > +++ b/lib/xe/xe_query.c > @@ -124,7 +124,11 @@ static struct drm_xe_query_oa_units *xe_query_oa_units_new(int fd) > .data = 0, > }; > > - igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0); > + /** I am seeing, in IGT, the "/**" comment pattern used only for test or subtest description. So we should just use this pattern for multi-line comments: /* * This is a multi-line comment */ > + * Only tests/intel/xe_oa.c needs OA_UNITS This comment is misleading and needs to be changed. Please change it to something like (no need for multi-line comment either): /* Support older kernels where this uapi is not yet available */ Since this is the real reason for this change. > + */ > + if (igt_ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &query) != 0) No need for "!= 0", just do: if (igt_ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &query)) > + return NULL; > > oa_units = malloc(query.size); > igt_assert(oa_units); > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c > index ff2218300..dd02f8608 100644 > --- a/tests/intel/xe_oa.c > +++ b/tests/intel/xe_oa.c > @@ -4503,6 +4503,7 @@ static const char *xe_engine_class_name(uint32_t engine_class) > igt_main > { > struct drm_xe_engine_class_instance *hwe = NULL; > + struct xe_device *xe_dev; > > igt_fixture { > struct stat sb; > @@ -4527,7 +4528,13 @@ igt_main > igt_assert_eq(drm_fd, -1); > > drm_fd = drm_open_driver(DRIVER_XE); > - xe_device_get(drm_fd); > + xe_dev = xe_device_get(drm_fd); > + > + /** > + * This test needs OA_UNITS to run, see xe_query_oa_units_new() > + * on lib/xe/xe_query.c Here just say: /* See xe_query_oa_units_new() */ Kernel style is to minimize comments and also minimize unnecessary verbosity in comments. > + */ > + igt_require(xe_dev->oa_units); > > devid = intel_get_drm_devid(drm_fd); > sysfs = igt_sysfs_open(drm_fd); > -- > 2.34.1 > Please fix these things up and resend. Then I will will R-b and merge the patch. Thanks. -- Ashutosh