[PATCH 1/5] drm/xe: move memirq out of vf
Cavitt, Jonathan
jonathan.cavitt at intel.com
Thu Aug 22 14:39:16 UTC 2024
-----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
> -
> 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