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 C3195C35FF3 for ; Sat, 15 Mar 2025 14:45:22 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7EFE310E0E5; Sat, 15 Mar 2025 14:45:22 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="DJqBW1bJ"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.9]) by gabe.freedesktop.org (Postfix) with ESMTPS id EE74C10E09B; Sat, 15 Mar 2025 14:45:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1742049921; x=1773585921; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=L79tqI/QXYQsA98OS8kP0Mn8L9fqlq/ZjoJbkKFIwh8=; b=DJqBW1bJ7T63fLQTW96sioAPllgydMVdAYC8S/j4EaNb/76gPO2GVD0J y6NltVOtUbf8e/BJ0JdY4eanPHvIdW5h2x2q2Qv/p7Nv1Ya/2uodH/LjR wUF7QzcBsEaruO2A4MqSngzS2p5oYoX+rsndx1g7DxRJ1R0otxXI+/fTk 9OvAq7Bfdlm5kOsUa9+dzTkwEozAsjU+NWflDSdgFI9VHvw1TpqeIbd6y 3Ab1iGL6nPILcewjcILGNkyZwazQ6p0cn+0B0viDKhOOqXjj+R4PthJWt FYCzgFHYx4DB6ZAbgX4OyKGb6fDlyBwUR9Gqpl96xWbbwPDeu668oT25E Q==; X-CSE-ConnectionGUID: 1Cp49K0ASMGgieRK4H0s3w== X-CSE-MsgGUID: h/JNogZmTVCYqTvqbDn+IQ== X-IronPort-AV: E=McAfee;i="6700,10204,11374"; a="65654311" X-IronPort-AV: E=Sophos;i="6.14,250,1736841600"; d="scan'208";a="65654311" Received: from fmviesa007.fm.intel.com ([10.60.135.147]) by orvoesa101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Mar 2025 07:45:19 -0700 X-CSE-ConnectionGUID: CJE6OzJ1S8mNv3z8RSTu9w== X-CSE-MsgGUID: Bb1Ez5tKT6GK1WgLO/pr0g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.14,250,1736841600"; d="scan'208";a="121573163" Received: from irvmail002.ir.intel.com ([10.43.11.120]) by fmviesa007.fm.intel.com with ESMTP; 15 Mar 2025 07:45:15 -0700 Received: from [10.246.5.201] (mwajdecz-MOBL.ger.corp.intel.com [10.246.5.201]) by irvmail002.ir.intel.com (Postfix) with ESMTP id 385FE34901; Sat, 15 Mar 2025 14:45:14 +0000 (GMT) Message-ID: <3c506ee2-7a1a-46ce-b4ff-4d774c87431f@intel.com> Date: Sat, 15 Mar 2025 15:45:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 2/6] drm/xe/xe_gt_pagefault: Move pagefault struct to header To: "Cavitt, Jonathan" , "intel-xe@lists.freedesktop.org" Cc: "Gupta, saurabhg" , "Zuo, Alex" , "joonas.lahtinen@linux.intel.com" , "Brost, Matthew" , "Zhang, Jianxun" , "Lin, Shuicheng" , "dri-devel@lists.freedesktop.org" , "michal.mzorek@intel.com" References: <20250313183415.133863-1-jonathan.cavitt@intel.com> <20250313183415.133863-3-jonathan.cavitt@intel.com> <9d85d2bb-1f64-4d6c-9982-a2d23cbbac32@intel.com> Content-Language: en-US From: Michal Wajdeczko In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On 14.03.2025 23:06, Cavitt, Jonathan wrote: > -----Original Message----- > From: Wajdeczko, Michal > Sent: Friday, March 14, 2025 10:02 AM > To: Cavitt, Jonathan ; intel-xe@lists.freedesktop.org > Cc: Gupta, saurabhg ; Zuo, Alex ; joonas.lahtinen@linux.intel.com; Brost, Matthew ; Zhang, Jianxun ; Lin, Shuicheng ; dri-devel@lists.freedesktop.org; michal.mzorek@intel.com > Subject: Re: [PATCH v8 2/6] drm/xe/xe_gt_pagefault: Move pagefault struct to header >> >> On 13.03.2025 19:34, Jonathan Cavitt wrote: >>> Move the pagefault struct from xe_gt_pagefault.c to the >>> xe_gt_pagefault_types.h header file, along with the associated enum values. >>> >>> v2: >>> - Normalize names for common header (Matt Brost) >>> >>> v3: >>> - s/Migrate/Move (Michal W) >>> - s/xe_pagefault/xe_gt_pagefault (Michal W) >>> - Create new header file, xe_gt_pagefault_types.h (Michal W) >>> - Add kernel docs (Michal W) >>> >>> Signed-off-by: Jonathan Cavitt >>> Cc: Michal Wajdeczko >>> --- >> >> ... >> >>> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.h b/drivers/gpu/drm/xe/xe_gt_pagefault.h >>> index 839c065a5e4c..69b700c4915a 100644 >>> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.h >>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.h >>> @@ -8,6 +8,8 @@ >>> >>> #include >>> >>> +#include "xe_gt_pagefault_types.h" >> >> it's not needed here, move it to .c >> >>> + >>> struct xe_gt; >>> struct xe_guc; >>> >>> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault_types.h b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h >>> new file mode 100644 >>> index 000000000000..90b7085d4b8e >>> --- /dev/null >>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h >>> @@ -0,0 +1,67 @@ >>> +/* SPDX-License-Identifier: MIT */ >>> +/* >>> + * Copyright (c) 2022-2025 Intel Corporation >>> + */ >>> + >>> +#ifndef _XE_GT_PAGEFAULT_TYPES_H_ >>> +#define _XE_GT_PAGEFAULT_TYPES_H_ >>> + >> >> don't forget to >> >> #include > > That explains why the kernel failed to compile on CI. It was compiling just > fine locally, so that's why I missed this. > >> >>> +/** >>> + * struct xe_gt_pagefault - Structure of pagefaults returned by the >>> + * pagefault handler >>> + */ >>> +struct xe_gt_pagefault { >>> + /** @page_addr: faulted address of this pagefault */ >>> + u64 page_addr; >>> + /** @asid: ASID of this pagefault */ >>> + u32 asid; >>> + /** @pdata: PDATA of this pagefault */ >>> + u16 pdata; >>> + /** @vfid: VFID of this pagefault */ >>> + u8 vfid; >> >> btw, IIRC the VFID from the descriptor will be zero'ed >> does it make sense to keep it here? > > Is the argument that every time pf->vfid is accessed, it's guaranteed to be it's FIELD_GET(PFD_VFID, desc->dw2) guaranteed to be zero > zero? I can't counter that claim, but wouldn't it be safer to keep reporting > the VFID in case we ever hit a case where it's no longer zero? it can't be non-zero, look at GuC ABI spec that says: ".. with the VF number being zero’d out in the descriptor." > > Also, did we know it would always be zero when we were making the > pagefault struct originally? If so, why did we include the vfid originally? dunno, ask authors, it was pushed part of the initial commit dd08ebf6c352 that was *not* going through normal review cycle > >> >>> + /** >>> + * @access_type: access type of this pagefault, as a value >>> + * from xe_gt_pagefault_access_type >>> + */ >>> + u8 access_type; >>> + /** >>> + * @fault_type: fault type of this pagefault, as a value >>> + * from xe_gt_pagefault_fault_type >>> + */ >>> + u8 fault_type; >>> + /** @fault_level: fault level of this pagefault */ >>> + u8 fault_level; >>> + /** @engine_class: engine class this pagefault was reported on */ >>> + u8 engine_class; >>> + /** @engine_instance: engine instance this pagefault was reported on */ >>> + u8 engine_instance; >>> + /** @fault_unsuccessful: flag for if the pagefault recovered or not */ >>> + u8 fault_unsuccessful; >>> + /** @prefetch: unused */ >>> + bool prefetch; >>> + /** @trva_fault: is set if this is a TRTT fault */ >>> + bool trva_fault; >>> +}; >>> + >>> +/** >>> + * enum xe_gt_pagefault_access_type - Access type reported to the xe_gt_pagefault >>> + * struct. Saved to xe_gt_pagefault@access_type >> >> this seems to be copied from G2H descriptor as-is. >> so shouldn't this be part of the GuC ABI? >> or based on HW ABI if GuC is just a proxy > > What information should I be including in the kernel docs for these enums? are you asking about xe enums or GuC ABI defs/enums? for the xe enums, we can say this is what HW/GuC provides for the HW defs, we can likely drop kernel-doc we can just point to the Bspec in the commit msg > -Jonathan Cavitt > >> >>> + */ >>> +enum xe_gt_pagefault_access_type { >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_READ = 0, >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_WRITE = 1, >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_ATOMIC = 2, >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_RESERVED = 3, >>> +}; >>> + >>> +/** >>> + * enum xe_gt_pagefault_fault_type - Fault type reported to the xe_gt_pagefault >>> + * struct. Saved to xe_gt_pagefault@fault_type >> >> ditto >> >>> + */ >>> +enum xe_gt_pagefault_fault_type { >>> + XE_GT_PAGEFAULT_TYPE_NOT_PRESENT = 0, >>> + XE_GT_PAGEFAULT_TYPE_WRITE_ACCESS_VIOLATION = 1, >>> + XE_GT_PAGEFAULT_TYPE_ATOMIC_ACCESS_VIOLATION = 2, >>> +}; >>> + >>> +#endif >> >>