[PATCH 1/5] drm/xe: move memirq out of vf
Ilia Levi
illevi at habana.ai
Mon Aug 26 11:59:57 UTC 2024
On 8/22/24 17:39, 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 1/5] drm/xe: move memirq out of vf
>> Up until now only VF used Memory Based Interrupts (memirq).
>> Moving it out of VF to cater for other usages.
>>
>> Signed-off-by: Ilia Levi <ilia.levi at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_device.c | 2 +-
>> drivers/gpu/drm/xe/xe_device_types.h | 6 ++--
>> drivers/gpu/drm/xe/xe_guc.c | 2 +-
>> drivers/gpu/drm/xe/xe_irq.c | 6 ++--
>> drivers/gpu/drm/xe/xe_lrc.c | 2 +-
>> drivers/gpu/drm/xe/xe_memirq.c | 46 ++++++++++------------------
>> 6 files changed, 26 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index b6db7e082d88..623e4a977ba3 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->sriov.vf.memirq);
>> + err = xe_memirq_init(&tile->memirq);
>> if (err)
>> return err;
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 5ed6f5434f42..59e2c8a0af10 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -200,14 +200,14 @@ struct xe_tile {
>> struct xe_lmtt lmtt;
>> } pf;
>> struct {
>> - /** @sriov.vf.memirq: Memory Based Interrupts. */
>> - struct xe_memirq memirq;
>> -
>> /** @sriov.vf.ggtt_balloon: GGTT regions excluded from use. */
>> struct drm_mm_node ggtt_balloon[2];
>> } vf;
>> } sriov;
>>
>> + /** @memirq: Memory Based Interrupts. */
>> + struct xe_memirq memirq;
>> +
>> /** @migrate: Migration helper for vram blits and clearing */
>> struct xe_migrate *migrate;
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index de0fe9e65746..513d61c48732 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -863,7 +863,7 @@ int xe_guc_enable_communication(struct xe_guc *guc)
>> struct xe_gt *gt = guc_to_gt(guc);
>> struct xe_tile *tile = gt_to_tile(gt);
>>
>> - err = xe_memirq_init_guc(&tile->sriov.vf.memirq, guc);
>> + err = xe_memirq_init_guc(&tile->memirq, guc);
>> if (err)
>> return err;
>> } else {
>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>> index 5f2c368c35ad..18a5272d9bb0 100644
>> --- a/drivers/gpu/drm/xe/xe_irq.c
>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>> @@ -566,7 +566,7 @@ static void vf_irq_reset(struct xe_device *xe)
>>
>> for_each_tile(tile, xe, id) {
>> if (xe_device_has_memirq(xe))
>> - xe_memirq_reset(&tile->sriov.vf.memirq);
>> + xe_memirq_reset(&tile->memirq);
>> else
>> gt_irq_reset(tile);
>> }
>> @@ -609,7 +609,7 @@ static void vf_irq_postinstall(struct xe_device *xe)
>>
>> for_each_tile(tile, xe, id)
>> if (xe_device_has_memirq(xe))
>> - xe_memirq_postinstall(&tile->sriov.vf.memirq);
>> + xe_memirq_postinstall(&tile->memirq);
>>
>> if (GRAPHICS_VERx100(xe) < 1210)
>> xelp_intr_enable(xe, true);
>> @@ -652,7 +652,7 @@ static irqreturn_t vf_mem_irq_handler(int irq, void *arg)
>> spin_unlock(&xe->irq.lock);
>>
>> for_each_tile(tile, xe, id)
>> - xe_memirq_handler(&tile->sriov.vf.memirq);
>> + xe_memirq_handler(&tile->memirq);
>>
>> return IRQ_HANDLED;
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
>> index 974a9cd8c379..e70857325429 100644
>> --- a/drivers/gpu/drm/xe/xe_lrc.c
>> +++ b/drivers/gpu/drm/xe/xe_lrc.c
>> @@ -599,7 +599,7 @@ static void set_context_control(u32 *regs, struct xe_hw_engine *hwe)
>>
>> static void set_memory_based_intr(u32 *regs, struct xe_hw_engine *hwe)
>> {
>> - struct xe_memirq *memirq = >_to_tile(hwe->gt)->sriov.vf.memirq;
>> + struct xe_memirq *memirq = >_to_tile(hwe->gt)->memirq;
>> struct xe_device *xe = gt_to_xe(hwe->gt);
>>
>> if (!IS_SRIOV_VF(xe) || !xe_device_has_memirq(xe))
>> diff --git a/drivers/gpu/drm/xe/xe_memirq.c b/drivers/gpu/drm/xe/xe_memirq.c
>> index 95b6e9d7b7db..0db0726b76f3 100644
>> --- a/drivers/gpu/drm/xe/xe_memirq.c
>> +++ b/drivers/gpu/drm/xe/xe_memirq.c
>> @@ -19,15 +19,13 @@
>> #include "xe_hw_engine.h"
>> #include "xe_map.h"
>> #include "xe_memirq.h"
>> -#include "xe_sriov.h"
>> -#include "xe_sriov_printk.h"
>>
>> #define memirq_assert(m, condition) xe_tile_assert(memirq_to_tile(m), condition)
>> -#define memirq_debug(m, msg...) xe_sriov_dbg_verbose(memirq_to_xe(m), "MEMIRQ: " msg)
>> +#define memirq_debug(m, msg...) drm_dbg(&memirq_to_xe(m)->drm, "MEMIRQ: " msg)
>>
>> static struct xe_tile *memirq_to_tile(struct xe_memirq *memirq)
>> {
>> - return container_of(memirq, struct xe_tile, sriov.vf.memirq);
>> + return container_of(memirq, struct xe_tile, memirq);
>> }
>>
>> static struct xe_device *memirq_to_xe(struct xe_memirq *memirq)
>> @@ -157,8 +155,8 @@ static int memirq_alloc_pages(struct xe_memirq *memirq)
>> return drmm_add_action_or_reset(&xe->drm, __release_xe_bo, memirq->bo);
>>
>> out:
>> - xe_sriov_err(memirq_to_xe(memirq),
>> - "Failed to allocate memirq page (%pe)\n", ERR_PTR(err));
>> + drm_err(&memirq_to_xe(memirq)->drm,
>> + "Failed to allocate memirq page (%pe)\n", ERR_PTR(err));
>> return err;
>> }
>>
>> @@ -178,9 +176,7 @@ static void memirq_set_enable(struct xe_memirq *memirq, bool enable)
>> *
>> * These allocations are managed and will be implicitly released on unload.
>> *
>> - * Note: This function shall be called only by the VF driver.
>> - *
>> - * If this function fails then VF driver won't be able to operate correctly.
>> + * If this function fails then the driver won't be able to operate correctly.
>> * If `Memory Based Interrupts`_ are not used this function will return 0.
>> *
>> * Return: 0 on success or a negative error code on failure.
>> @@ -190,8 +186,6 @@ int xe_memirq_init(struct xe_memirq *memirq)
>> struct xe_device *xe = memirq_to_xe(memirq);
>> int err;
>>
>> - memirq_assert(memirq, IS_SRIOV_VF(xe));
> I'm not entirely sure why we're removing so many SRIOV VF checks
> from the code as a part of this change. Though I guess most or all
> places we check this also check for the existence of the memirq
> itself, which isn't initialized when SRIOV VF is disabled. So perhaps
> the two checks (SRIOV VF and memirq existence) are logically
> equivalent?
>
> This is mostly a rhetorical question. I'm not blocking on this.
>
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> -Jonathan Cavitt
>
The goal here is to prepare the memirq infra to be used in a non-VF
scenario (specifically MSI-X, where IS_SRIOV_VF(xe) will return false).
So this patch moves the memirq structure location out of VF, removes VF
asserts to allow calling the infra from non-VF code and cleans up the
implementation to not rely on xe_sriov* functions.
Ilia
>> -
>> if (!xe_device_has_memirq(xe))
>> return 0;
>>
>> @@ -209,14 +203,13 @@ 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
>> *
>> - * Shall be called only on VF driver when `Memory Based Interrupts`_ are used
>> + * 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)
>> {
>> - memirq_assert(memirq, IS_SRIOV_VF(memirq_to_xe(memirq)));
>> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
>> memirq_assert(memirq, memirq->bo);
>>
>> @@ -227,14 +220,13 @@ u32 xe_memirq_source_ptr(struct xe_memirq *memirq)
>> * xe_memirq_status_ptr - Get GGTT's offset of the `Interrupt Status Report Page`_.
>> * @memirq: the &xe_memirq to query
>> *
>> - * Shall be called only on VF driver when `Memory Based Interrupts`_ are used
>> + * 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)
>> {
>> - memirq_assert(memirq, IS_SRIOV_VF(memirq_to_xe(memirq)));
>> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
>> memirq_assert(memirq, memirq->bo);
>>
>> @@ -245,14 +237,13 @@ u32 xe_memirq_status_ptr(struct xe_memirq *memirq)
>> * xe_memirq_enable_ptr - Get GGTT's offset of the Interrupt Enable Mask.
>> * @memirq: the &xe_memirq to query
>> *
>> - * Shall be called only on VF driver when `Memory Based Interrupts`_ are used
>> + * Shall be called when `Memory Based Interrupts`_ are used
>> * and xe_memirq_init() didn't fail.
>> *
>> * Return: GGTT's offset of the Interrupt Enable Mask.
>> */
>> u32 xe_memirq_enable_ptr(struct xe_memirq *memirq)
>> {
>> - memirq_assert(memirq, IS_SRIOV_VF(memirq_to_xe(memirq)));
>> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
>> memirq_assert(memirq, memirq->bo);
>>
>> @@ -267,7 +258,7 @@ u32 xe_memirq_enable_ptr(struct xe_memirq *memirq)
>> * Register `Interrupt Source Report Page`_ and `Interrupt Status Report Page`_
>> * to be used by the GuC when `Memory Based Interrupts`_ are required.
>> *
>> - * Shall be called only on VF driver when `Memory Based Interrupts`_ are used
>> + * Shall be called when `Memory Based Interrupts`_ are used
>> * and xe_memirq_init() didn't fail.
>> *
>> * Return: 0 on success or a negative error code on failure.
>> @@ -279,7 +270,6 @@ int xe_memirq_init_guc(struct xe_memirq *memirq, struct xe_guc *guc)
>> u32 source, status;
>> int err;
>>
>> - memirq_assert(memirq, IS_SRIOV_VF(memirq_to_xe(memirq)));
>> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
>> memirq_assert(memirq, memirq->bo);
>>
>> @@ -299,9 +289,9 @@ int xe_memirq_init_guc(struct xe_memirq *memirq, struct xe_guc *guc)
>> return 0;
>>
>> failed:
>> - xe_sriov_err(memirq_to_xe(memirq),
>> - "Failed to setup report pages in %s (%pe)\n",
>> - guc_name(guc), ERR_PTR(err));
>> + drm_err(&memirq_to_xe(memirq)->drm,
>> + "Failed to setup report pages in %s (%pe)\n",
>> + guc_name(guc), ERR_PTR(err));
>> return err;
>> }
>>
>> @@ -311,12 +301,11 @@ int xe_memirq_init_guc(struct xe_memirq *memirq, struct xe_guc *guc)
>> *
>> * This is part of the driver IRQ setup flow.
>> *
>> - * This function shall only be used by the VF driver on platforms that use
>> + * This function shall only be used on platforms that use
>> * `Memory Based Interrupts`_.
>> */
>> void xe_memirq_reset(struct xe_memirq *memirq)
>> {
>> - memirq_assert(memirq, IS_SRIOV_VF(memirq_to_xe(memirq)));
>> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
>>
>> if (memirq->bo)
>> @@ -329,12 +318,11 @@ void xe_memirq_reset(struct xe_memirq *memirq)
>> *
>> * This is part of the driver IRQ setup flow.
>> *
>> - * This function shall only be used by the VF driver on platforms that use
>> + * This function shall only be used on platforms that use
>> * `Memory Based Interrupts`_.
>> */
>> void xe_memirq_postinstall(struct xe_memirq *memirq)
>> {
>> - memirq_assert(memirq, IS_SRIOV_VF(memirq_to_xe(memirq)));
>> memirq_assert(memirq, xe_device_has_memirq(memirq_to_xe(memirq)));
>>
>> if (memirq->bo)
>> @@ -349,9 +337,9 @@ static bool memirq_received(struct xe_memirq *memirq, struct iosys_map *vector,
>> value = iosys_map_rd(vector, offset, u8);
>> if (value) {
>> if (value != 0xff)
>> - xe_sriov_err_ratelimited(memirq_to_xe(memirq),
>> - "Unexpected memirq value %#x from %s at %u\n",
>> - value, name, offset);
>> + drm_err_ratelimited(&memirq_to_xe(memirq)->drm,
>> + "Unexpected memirq value %#x from %s at %u\n",
>> + value, name, offset);
>> iosys_map_wr(vector, offset, u8, 0x00);
>> }
>>
>> --
>> 2.43.2
>>
>>
More information about the Intel-xe
mailing list