[PATCH 2/5] drm/xe: memirq infra changes for msix
Cavitt, Jonathan
jonathan.cavitt at intel.com
Mon Aug 26 14:40:03 UTC 2024
-----Original Message-----
From: Ilia Levi <illevi at habana.ai>
Sent: Monday, August 26, 2024 5:23 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Levi, Ilia <ilia.levi at intel.com>; intel-xe at lists.freedesktop.org
Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>; Elbaz, Koby <koby.elbaz at intel.com>; Avizrat, Yaron <yaron.avizrat at intel.com>
Subject: Re: [PATCH 2/5] drm/xe: memirq infra changes for msix
>
> On 8/22/24 18:05, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Ilia Levi
> > Sent: Thursday, August 22, 2024 6:08 AM
> > To: intel-xe at lists.freedesktop.org
> > Cc: Levi, Ilia <ilia.levi at intel.com>; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>; Elbaz, Koby <koby.elbaz at intel.com>; Avizrat, Yaron <yaron.avizrat at intel.com>
> > Subject: [PATCH 2/5] drm/xe: memirq infra changes for msix
> >> When using MSI-X, hw engines report interrupt status and source to engine
> >> instance 0. We pass a flag to memirq initialization routine for this case.
> >>
> >> 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 <ilia.levi at intel.com>
> >> ---
> >> drivers/gpu/drm/xe/xe_device.c | 2 +-
> >> drivers/gpu/drm/xe/xe_lrc.c | 4 +--
> >> drivers/gpu/drm/xe/xe_memirq.c | 49 +++++++++++++++++++---------
> >> drivers/gpu/drm/xe/xe_memirq.h | 7 ++--
> >> drivers/gpu/drm/xe/xe_memirq_types.h | 6 ++--
> >> 5 files changed, 44 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> >> index 623e4a977ba3..c4f8bfd08b09 100644
> >> --- a/drivers/gpu/drm/xe/xe_device.c
> >> +++ b/drivers/gpu/drm/xe/xe_device.c
> >> @@ -661,7 +661,7 @@ int xe_device_probe(struct xe_device *xe)
> >> if (err)
> >> return err;
> >> if (IS_SRIOV_VF(xe)) {
> >> - err = xe_memirq_init(&tile->memirq);
> >> + err = xe_memirq_init(&tile->memirq, false);
> > We don't seem to set reports_to_e0 to True at any point in this patch
> > series. Are we expecting to add support for this override in some
> > future update?
> >
> Indeed, it will be used for MSI-X. I've uploaded it earlier together
> with its usage in
> https://patchwork.freedesktop.org/series/135422/, but decided to split
> to facilitate review.
>
> Ilia
>
> >> if (err)
> >> return err;
> >> }
> >> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> >> index e70857325429..30e65991d5bb 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 0db0726b76f3..a1d4bc3f9e42 100644
> >> --- a/drivers/gpu/drm/xe/xe_memirq.c
> >> +++ b/drivers/gpu/drm/xe/xe_memirq.c
> >> @@ -112,18 +112,18 @@ static void __release_xe_bo(struct drm_device *drm, void *arg)
> >> xe_bo_unpin_map_no_vm(bo);
> >> }
> >>
> >> -static int memirq_alloc_pages(struct xe_memirq *memirq)
> >> +static int memirq_alloc_pages(struct xe_memirq *memirq, size_t bo_size)
> >> {
> >> struct xe_device *xe = memirq_to_xe(memirq);
> >> struct xe_tile *tile = memirq_to_tile(memirq);
> >> 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 |
> >> @@ -138,11 +138,11 @@ 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);
> >> @@ -150,7 +150,7 @@ static int memirq_alloc_pages(struct xe_memirq *memirq)
> >> 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));
> >> + xe_memirq_source_ptr(memirq, NULL), xe_memirq_status_ptr(memirq, NULL));
> >>
> >> return drmm_add_action_or_reset(&xe->drm, __release_xe_bo, memirq->bo);
> >>
> >> @@ -170,6 +170,7 @@ static void memirq_set_enable(struct xe_memirq *memirq, bool enable)
> >> /**
> >> * xe_memirq_init - Initialize data used by `Memory Based Interrupts`_.
> >> * @memirq: the &xe_memirq to initialize
> >> + * @reports_to_e0: Does the HW report status and source to engine instance 0?
> > You may want to remark that this overrides other behavior. Something like:
> >
> > """
> > * @reports_to_e0: Override flag that forces HW to report status and source
> > * to engine instance 0 always.
> > """
>
> Hmm, but it's not overriding the hardware - it's a reflection of how the
> hardware behaves.
> Namely, if the hardware reports the source/status to the location of
> engine 0, then we initialize memirq differently from the previous behavior.
>
> Ilia
>
> >
> >> *
> >> * Allocate `Interrupt Source Report Page`_ and `Interrupt Status Report Page`_
> >> * used by `Memory Based Interrupts`_.
> >> @@ -181,15 +182,19 @@ static void memirq_set_enable(struct xe_memirq *memirq, bool enable)
> >> *
> >> * Return: 0 on success or a negative error code on failure.
> >> */
> >> -int xe_memirq_init(struct xe_memirq *memirq)
> >> +int xe_memirq_init(struct xe_memirq *memirq, bool reports_to_e0)
> >> {
> >> struct xe_device *xe = memirq_to_xe(memirq);
> >> + size_t bo_size;
> >> int err;
> >>
> >> if (!xe_device_has_memirq(xe))
> >> return 0;
> >>
> >> - err = memirq_alloc_pages(memirq);
> >> + memirq->reports_to_e0 = reports_to_e0;
> >> + bo_size = reports_to_e0 ? XE_HW_ENGINE_MAX_INSTANCE * SZ_4K : SZ_4K;
> >> +
> >> + err = memirq_alloc_pages(memirq, bo_size);
> >> if (unlikely(err))
> >> return err;
> >>
> >> @@ -202,35 +207,47 @@ 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;
> >> +
> >> + if (memirq->reports_to_e0 && hwe)
> >> + instance = hwe->instance;
> > Maybe I'm missing something, but shouldn't this check on
> > memirq be inverted? Because we're currently using the
> > provided hwe instance when memirq expects to report to
> > engine 0.
> >
> > More specifically, shouldn't the check be:
> >
> > """
> > if (!memirq->reports_to_e0 && hwe)
> > instance = hwe->instance;
> > """
> > ?
> No, notice the change in implementation of XE_MEMIRQ_SOURCE_OFFSET /
> XE_MEMIRQ_STATUS_OFFSET macros.
> When reports_to_e0 is false, using instance 0 means we use a single page
> with the exact same layout as before this change.
> Conversely, when reports_to_e0 is true, we use the page corresponding to
> the hwe instance.
Okay, I think I understand. reports_to_e0 is a flag that denotes that the memirq
is reporting to engine 0, and thus the hwe instance of the initial interrupt report
is lost. Ergo, to preserve the hwe instance, we need to use the page corresponding
to the hwe instance when getting the interrupt source and status pages.
I guess I thought that xe_memirq_source/status_pointer was the part that was
doing the reporting, and got confused as to why the reports_to_e0 flag was causing
memirq to "report" to "not engine 0". On one hand, that’s my bad, as that was mostly
explained in the commit message. On the other hand, perhaps changing the name
of the flag might be worth considering? Maybe "needs_hwe" or "uses_msxi"?
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt
>
> Ilia
>
> >> +
> >> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
> >> memirq_assert(memirq, memirq->bo);
> >>
> >> - return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_SOURCE_OFFSET;
> >> + 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;
> >> +
> >> + if (memirq->reports_to_e0 && hwe)
> >> + instance = hwe->instance;
> > See above note.
> >
> >> +
> >> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
> >> memirq_assert(memirq, memirq->bo);
> >>
> >> - return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_STATUS_OFFSET;
> >> + return xe_bo_ggtt_addr(memirq->bo) + XE_MEMIRQ_STATUS_OFFSET(instance);
> >> }
> >>
> >> /**
> >> @@ -273,8 +290,8 @@ int xe_memirq_init_guc(struct xe_memirq *memirq, struct xe_guc *guc)
> >> memirq_assert(memirq, xe_device_has_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..98e2b61ef973 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;
> >>
> >> -int xe_memirq_init(struct xe_memirq *memirq);
> >> +int xe_memirq_init(struct xe_memirq *memirq, bool reports_to_e0);
> >>
> >> -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..078a1500c918 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
> >>
> >> @@ -25,6 +25,7 @@ struct xe_bo;
> >> * @status: iosys pointer to `Interrupt Status Report Page`_.
> >> * @mask: iosys pointer to Interrupt Enable Mask.
> >> * @enabled: internal flag used to control processing of the interrupts.
> >> + * @reports_to_e0: configuration parameter (see xe_memirq_init)
> >> */
> >> struct xe_memirq {
> >> struct xe_bo *bo;
> >> @@ -32,6 +33,7 @@ struct xe_memirq {
> >> struct iosys_map status;
> >> struct iosys_map mask;
> >> bool enabled;
> >> + bool reports_to_e0;
> > It might be worthwhile in some future update to compress these
> > Boolean flags into a binary mask, but that probably shouldn't be
> > a part of this series.
> > -Jonathan Cavitt
> >
> >> };
> >>
> >> #endif
> >> --
> >> 2.43.2
> >>
> >>
>
>
>
More information about the Intel-xe
mailing list