[PATCH 2/5] drm/xe: memirq infra changes for msix
Ilia Levi
illevi at habana.ai
Mon Aug 26 12:22:44 UTC 2024
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.
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