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 DACB7C3DA4A for ; Thu, 5 Sep 2024 16:36:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9D2D610E343; Thu, 5 Sep 2024 16:36:00 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="QhFUipbY"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8076810E343 for ; Thu, 5 Sep 2024 16:35:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1725554160; x=1757090160; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=+P91lf/5O20w5pOjIEOYychJNAwXaleaiz1nlrmAYfY=; b=QhFUipbYuo3nFmqu25D1dP/JlApmGZfleurkiUZ+sU/q7cEXbyWG+FUM UnyVoikqQbK+UgPXachUcEHbtlFojHDZ9j554VeAtxCa5WWIItE2q5tZ/ mzu25JryLpXWdZzlxgsMWkkSlJn9GYjrOwUFhxEFrAdXsTId4uzwORUyD MCRlKOn9EyVBc/FPVVQAJjqXPLNnNrgtTzpjkoqxaUBui92UNDd3h3VaD an5JJuB9/M9LZPYw28Ia8JvAIF/sFwg5rS1jIq8IEEWlwTqYC0HHA6mI6 ELanlmSPCYfLMSVux4suS0dyEJhb99ZBedI1CkEs4XLC+qqVCX9fs59kn Q==; X-CSE-ConnectionGUID: /dqoJZe1TeGWFG3+WAik3w== X-CSE-MsgGUID: gehemXTdR5aOEZqxohUkAQ== X-IronPort-AV: E=McAfee;i="6700,10204,11186"; a="24448330" X-IronPort-AV: E=Sophos;i="6.10,205,1719903600"; d="scan'208";a="24448330" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by fmvoesa109.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2024 09:35:59 -0700 X-CSE-ConnectionGUID: wCWMGxi8REKrMIV3BgsaLA== X-CSE-MsgGUID: LytSGdEmRieJLWJmQli+1g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.10,205,1719903600"; d="scan'208";a="96423157" Received: from irvmail002.ir.intel.com ([10.43.11.120]) by orviesa002.jf.intel.com with ESMTP; 05 Sep 2024 09:35:56 -0700 Received: from [10.246.1.253] (mwajdecz-MOBL.ger.corp.intel.com [10.246.1.253]) by irvmail002.ir.intel.com (Postfix) with ESMTP id 1F07F34EE8; Thu, 5 Sep 2024 17:35:55 +0100 (IST) Message-ID: <850e12fb-f51c-4a45-8781-ddd339d3f1d3@intel.com> Date: Thu, 5 Sep 2024 18:35:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/4] drm/xe: memirq infra changes for MSI-X To: Ilia Levi , intel-xe@lists.freedesktop.org Cc: jonathan.cavitt@intel.com, niranjana.vishwanathapura@intel.com, matthew.brost@intel.com, koby.elbaz@intel.com, yaron.avizrat@intel.com References: <20240902140826.2526259-1-ilia.levi@intel.com> <20240902140826.2526259-4-ilia.levi@intel.com> Content-Language: en-US From: Michal Wajdeczko In-Reply-To: <20240902140826.2526259-4-ilia.levi@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 02.09.2024 16:08, Ilia Levi wrote: > When using MSI-X, hw engines report interrupt status and source to engine > instance 0. For this scenario, in order to differentiate between the > engines, we need to pass different status/source pointers in the LRC. > > The requirements on those pointers are: > - Interrupt status should be 4KiB aligned > - Interrupt source should be 64 bytes aligned > > To accommodate this, we duplicate the current memirq page layout - > allocating a page for each engine instance and pass this page in the LRC. > Note that the same page can be reused for different engine types. > For example, an LRC executing on CCS #x will have pointers to page #x, > and an LRC executing on BCS #x will have the same pointers. Thus, to > locate the proper page, the pointer accessors were modified to receive > the hw engine. > > Signed-off-by: Ilia Levi > Reviewed-by: Jonathan Cavitt this was likely for a previous rev > --- > drivers/gpu/drm/xe/xe_lrc.c | 4 +- > drivers/gpu/drm/xe/xe_memirq.c | 59 +++++++++++++++++++++------- > drivers/gpu/drm/xe/xe_memirq.h | 5 ++- > drivers/gpu/drm/xe/xe_memirq_types.h | 4 +- > 4 files changed, 52 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c > index e40f48f4240f..f0976230012a 100644 > --- a/drivers/gpu/drm/xe/xe_lrc.c > +++ b/drivers/gpu/drm/xe/xe_lrc.c > @@ -613,9 +613,9 @@ static void set_memory_based_intr(u32 *regs, struct xe_hw_engine *hwe) > regs[CTX_LRI_INT_REPORT_PTR] = MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(2) | > MI_LRI_LRM_CS_MMIO | MI_LRI_FORCE_POSTED; > regs[CTX_INT_STATUS_REPORT_REG] = RING_INT_STATUS_RPT_PTR(0).addr; > - regs[CTX_INT_STATUS_REPORT_PTR] = xe_memirq_status_ptr(memirq); > + regs[CTX_INT_STATUS_REPORT_PTR] = xe_memirq_status_ptr(memirq, hwe); > regs[CTX_INT_SRC_REPORT_REG] = RING_INT_SRC_RPT_PTR(0).addr; > - regs[CTX_INT_SRC_REPORT_PTR] = xe_memirq_source_ptr(memirq); > + regs[CTX_INT_SRC_REPORT_PTR] = xe_memirq_source_ptr(memirq, hwe); > } > > static int lrc_ring_mi_mode(struct xe_hw_engine *hwe) > diff --git a/drivers/gpu/drm/xe/xe_memirq.c b/drivers/gpu/drm/xe/xe_memirq.c > index 0f12b8aa412b..5cbc366b2eea 100644 > --- a/drivers/gpu/drm/xe/xe_memirq.c > +++ b/drivers/gpu/drm/xe/xe_memirq.c > @@ -115,18 +115,32 @@ static void __release_xe_bo(struct drm_device *drm, void *arg) > xe_bo_unpin_map_no_vm(bo); > } > > +static inline bool hw_reports_to_instance_zero(struct xe_memirq *memirq) > +{ > + struct xe_device *xe = memirq_to_xe(memirq); > + > + /* > + * When the HW engines are configured to use MSI-X, > + * they report interrupt status and source to the offset of > + * engine instance 0. > + */ > + return xe_device_has_msix(xe); > +} > + > static int memirq_alloc_pages(struct xe_memirq *memirq) > { > struct xe_device *xe = memirq_to_xe(memirq); > struct xe_tile *tile = memirq_to_tile(memirq); > + size_t bo_size = hw_reports_to_instance_zero(memirq) ? > + XE_HW_ENGINE_MAX_INSTANCE * SZ_4K : SZ_4K; > struct xe_bo *bo; > int err; > > - BUILD_BUG_ON(!IS_ALIGNED(XE_MEMIRQ_SOURCE_OFFSET, SZ_64)); > - BUILD_BUG_ON(!IS_ALIGNED(XE_MEMIRQ_STATUS_OFFSET, SZ_4K)); > + BUILD_BUG_ON(!IS_ALIGNED(XE_MEMIRQ_SOURCE_OFFSET(0), SZ_64)); > + BUILD_BUG_ON(!IS_ALIGNED(XE_MEMIRQ_STATUS_OFFSET(0), SZ_4K)); > > /* XXX: convert to managed bo */ > - bo = xe_bo_create_pin_map(xe, tile, NULL, SZ_4K, > + bo = xe_bo_create_pin_map(xe, tile, NULL, bo_size, > ttm_bo_type_kernel, > XE_BO_FLAG_SYSTEM | > XE_BO_FLAG_GGTT | > @@ -141,19 +155,20 @@ static int memirq_alloc_pages(struct xe_memirq *memirq) > memirq_assert(memirq, !xe_bo_is_vram(bo)); > memirq_assert(memirq, !memirq->bo); > > - iosys_map_memset(&bo->vmap, 0, 0, SZ_4K); > + iosys_map_memset(&bo->vmap, 0, 0, bo_size); > > memirq->bo = bo; > - memirq->source = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_SOURCE_OFFSET); > - memirq->status = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_STATUS_OFFSET); > + memirq->source = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_SOURCE_OFFSET(0)); > + memirq->status = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_STATUS_OFFSET(0)); > memirq->mask = IOSYS_MAP_INIT_OFFSET(&bo->vmap, XE_MEMIRQ_ENABLE_OFFSET); > > memirq_assert(memirq, !memirq->source.is_iomem); > memirq_assert(memirq, !memirq->status.is_iomem); > memirq_assert(memirq, !memirq->mask.is_iomem); > > - memirq_debug(memirq, "page offsets: source %#x status %#x\n", > - xe_memirq_source_ptr(memirq), xe_memirq_status_ptr(memirq)); > + memirq_debug(memirq, "page offsets: bo %#x source %#x status %#x\n", > + xe_bo_ggtt_addr(bo), XE_MEMIRQ_SOURCE_OFFSET(0), > + XE_MEMIRQ_STATUS_OFFSET(0)); nit: since now the size could be != 4K, then maybe worth to print it? > > return drmm_add_action_or_reset(&xe->drm, __release_xe_bo, memirq->bo); > > @@ -204,35 +219,51 @@ int xe_memirq_init(struct xe_memirq *memirq) > /** > * xe_memirq_source_ptr - Get GGTT's offset of the `Interrupt Source Report Page`_. > * @memirq: the &xe_memirq to query > + * @hwe: the hw engine for which we want the report page > * > * Shall be called when `Memory Based Interrupts`_ are used > * and xe_memirq_init() didn't fail. > * > * Return: GGTT's offset of the `Interrupt Source Report Page`_. > */ > -u32 xe_memirq_source_ptr(struct xe_memirq *memirq) > +u32 xe_memirq_source_ptr(struct xe_memirq *memirq, struct xe_hw_engine *hwe) > { > + u16 instance = 0; > + > memirq_assert(memirq, xe_device_uses_memirq(memirq_to_xe(memirq))); > memirq_assert(memirq, memirq->bo); > > - return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_SOURCE_OFFSET; > + if (hw_reports_to_instance_zero(memirq)) { > + memirq_assert(memirq, hwe); nit: we usually don't add asserts to check null ptr, and even if you want to keep it, then place it next to other asserts above, as we expect hwe be != null regardless of hw_reports_to_instance_zero > + instance = hwe->instance; > + } > + > + return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_SOURCE_OFFSET(instance); > } > > /** > * xe_memirq_status_ptr - Get GGTT's offset of the `Interrupt Status Report Page`_. > * @memirq: the &xe_memirq to query > + * @hwe: the hw engine for which we want the report page > * > * Shall be called when `Memory Based Interrupts`_ are used > * and xe_memirq_init() didn't fail. > * > * Return: GGTT's offset of the `Interrupt Status Report Page`_. > */ > -u32 xe_memirq_status_ptr(struct xe_memirq *memirq) > +u32 xe_memirq_status_ptr(struct xe_memirq *memirq, struct xe_hw_engine *hwe) > { > + u16 instance = 0; > + > memirq_assert(memirq, xe_device_uses_memirq(memirq_to_xe(memirq))); > memirq_assert(memirq, memirq->bo); > > - return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_STATUS_OFFSET; > + if (hw_reports_to_instance_zero(memirq)) { > + memirq_assert(memirq, hwe); ditto > + instance = hwe->instance; > + } > + > + return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_STATUS_OFFSET(instance); > } > > /** > @@ -275,8 +306,8 @@ int xe_memirq_init_guc(struct xe_memirq *memirq, struct xe_guc *guc) > memirq_assert(memirq, xe_device_uses_memirq(memirq_to_xe(memirq))); > memirq_assert(memirq, memirq->bo); > > - source = xe_memirq_source_ptr(memirq) + offset; > - status = xe_memirq_status_ptr(memirq) + offset * SZ_16; > + source = xe_memirq_source_ptr(memirq, NULL) + offset; > + status = xe_memirq_status_ptr(memirq, NULL) + offset * SZ_16; > > err = xe_guc_self_cfg64(guc, GUC_KLV_SELF_CFG_MEMIRQ_SOURCE_ADDR_KEY, > source); > diff --git a/drivers/gpu/drm/xe/xe_memirq.h b/drivers/gpu/drm/xe/xe_memirq.h > index 2d40d03c3095..2934f03cfbb5 100644 > --- a/drivers/gpu/drm/xe/xe_memirq.h > +++ b/drivers/gpu/drm/xe/xe_memirq.h > @@ -10,11 +10,12 @@ > > struct xe_guc; > struct xe_memirq; > +struct xe_hw_engine; keep forward decls alphabetically ordered > > int xe_memirq_init(struct xe_memirq *memirq); > > -u32 xe_memirq_source_ptr(struct xe_memirq *memirq); > -u32 xe_memirq_status_ptr(struct xe_memirq *memirq); > +u32 xe_memirq_source_ptr(struct xe_memirq *memirq, struct xe_hw_engine *hwe); > +u32 xe_memirq_status_ptr(struct xe_memirq *memirq, struct xe_hw_engine *hwe); > u32 xe_memirq_enable_ptr(struct xe_memirq *memirq); > > void xe_memirq_reset(struct xe_memirq *memirq); > diff --git a/drivers/gpu/drm/xe/xe_memirq_types.h b/drivers/gpu/drm/xe/xe_memirq_types.h > index 625b6b8736cc..9d0f6c1cdb9d 100644 > --- a/drivers/gpu/drm/xe/xe_memirq_types.h > +++ b/drivers/gpu/drm/xe/xe_memirq_types.h > @@ -11,9 +11,9 @@ > struct xe_bo; > > /* ISR */ > -#define XE_MEMIRQ_STATUS_OFFSET 0x0 > +#define XE_MEMIRQ_STATUS_OFFSET(inst) ((inst) * SZ_4K + 0x0) > /* IIR */ > -#define XE_MEMIRQ_SOURCE_OFFSET 0x400 > +#define XE_MEMIRQ_SOURCE_OFFSET(inst) ((inst) * SZ_4K + 0x400) > /* IMR */ > #define XE_MEMIRQ_ENABLE_OFFSET 0x440 > otherwise LGTM