<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 27/11/2024 10:24, Piotr Piórkowski wrote: </div> <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre">Ilia Levi <a class="moz-txt-link-rfc2396E" href="mailto:ilia.levi@intel.com"><ilia.levi@intel.com></a> wrote on nie [2024-lis-10 22:39:13 +0200]: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">A new flow is added for devices that support MSI-X: - MSI-X vector 0 is used for GuC-to-host interrupt - MSI-X vector 1 (aka default MSI-X) is used for HW engines - Configure the HW engines to work with MSI-X - Program the LRC to use memirq infra (similar to VF) - CS_INT_VEC field added to the LRC </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> IMO you should split this patch into two separate patches: the general configuration (init) of MSI-X and the addition of MSI-X support for HW engines</pre> </blockquote> Ack. For some reason patchwork created a new series: <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/141880/">https://patchwork.freedesktop.org/series/141880/</a> <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Bspec: 60342, 72547 Signed-off-by: Ilia Levi <a class="moz-txt-link-rfc2396E" href="mailto:ilia.levi@intel.com"><ilia.levi@intel.com></a> --- drivers/gpu/drm/xe/Makefile | 1 + drivers/gpu/drm/xe/regs/xe_engine_regs.h | 3 + drivers/gpu/drm/xe/regs/xe_lrc_layout.h | 3 + drivers/gpu/drm/xe/xe_device.c | 4 + drivers/gpu/drm/xe/xe_device.h | 3 +- drivers/gpu/drm/xe/xe_device_types.h | 10 ++ drivers/gpu/drm/xe/xe_exec_queue.c | 3 +- drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 + drivers/gpu/drm/xe/xe_execlist.c | 9 +- drivers/gpu/drm/xe/xe_hw_engine.c | 7 +- drivers/gpu/drm/xe/xe_irq.c | 116 ++++++++++++++------ drivers/gpu/drm/xe/xe_irq.h | 1 + drivers/gpu/drm/xe/xe_irq_msix.c | 134 +++++++++++++++++++++++ drivers/gpu/drm/xe/xe_irq_msix.h | 16 +++ drivers/gpu/drm/xe/xe_lrc.c | 24 +++- drivers/gpu/drm/xe/xe_lrc.h | 2 +- 16 files changed, 293 insertions(+), 45 deletions(-) create mode 100644 drivers/gpu/drm/xe/xe_irq_msix.c create mode 100644 drivers/gpu/drm/xe/xe_irq_msix.h diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index a93e6fcc0ad9..2f1e1f17b677 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -72,6 +72,7 @@ xe-y += xe_bb.o \ xe_hw_fence.o \ xe_huc.o \ xe_irq.o \ + xe_irq_msix.o \ </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> I have my doubts that we need a separate file for MSI-X. We already have xe_irq.c, and there is not much code for MSI-X. I think of it this way: if we need a separate file for MSI-X then why not have a separate one for MSI. And if xe_irq.c is for MSI then why don't we call it xe_irq_msi.c. But then what about the common functions which are used by MSI and MSI-X. And after such reasoning, I conclude that it would be just as well if the msi-x functions were in the xe_irq.c file. </pre> </blockquote> Makes sense, we can always separate later if it grows too cumbersome. <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> xe_lrc.o \ xe_migrate.o \ xe_mmio.o \ diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h index 7c78496e6213..d86219dedde2 100644 --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h @@ -83,6 +83,8 @@ #define RING_IMR(base) XE_REG((base) + 0xa8) #define RING_INT_STATUS_RPT_PTR(base) XE_REG((base) + 0xac) +#define CS_INT_VEC(base) XE_REG((base) + 0x1b8) + #define RING_EIR(base) XE_REG((base) + 0xb0) #define RING_EMR(base) XE_REG((base) + 0xb4) #define RING_ESR(base) XE_REG((base) + 0xb8) @@ -138,6 +140,7 @@ #define RING_MODE(base) XE_REG((base) + 0x29c) #define GFX_DISABLE_LEGACY_MODE REG_BIT(3) +#define GFX_MSIX_INTERRUPT_ENABLE REG_BIT(13) #define RING_TIMESTAMP(base) XE_REG((base) + 0x358) diff --git a/drivers/gpu/drm/xe/regs/xe_lrc_layout.h b/drivers/gpu/drm/xe/regs/xe_lrc_layout.h index 045dfd09db99..57944f90bbf6 100644 --- a/drivers/gpu/drm/xe/regs/xe_lrc_layout.h +++ b/drivers/gpu/drm/xe/regs/xe_lrc_layout.h @@ -25,6 +25,9 @@ #define CTX_INT_SRC_REPORT_REG (CTX_LRI_INT_REPORT_PTR + 3) #define CTX_INT_SRC_REPORT_PTR (CTX_LRI_INT_REPORT_PTR + 4) +#define CTX_CS_INT_VEC_REG 0x5a +#define CTX_CS_INT_VEC_DATA (CTX_CS_INT_VEC_REG + 1) + #define INDIRECT_CTX_RING_HEAD (0x02 + 1) #define INDIRECT_CTX_RING_TAIL (0x04 + 1) #define INDIRECT_CTX_RING_START (0x06 + 1) diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c index 0e2dd691bdae..eff7e51198d9 100644 --- a/drivers/gpu/drm/xe/xe_device.c +++ b/drivers/gpu/drm/xe/xe_device.c @@ -660,6 +660,10 @@ int xe_device_probe(struct xe_device *xe) xe_gt_mmio_init(gt); } + err = xe_irq_init(xe); + if (err) + return err; + for_each_tile(tile, xe, id) { if (IS_SRIOV_VF(xe)) { xe_guc_comm_init_early(&tile->primary_gt->uc.guc); diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h index f1fbfe916867..812f24a34e6b 100644 --- a/drivers/gpu/drm/xe/xe_device.h +++ b/drivers/gpu/drm/xe/xe_device.h @@ -157,8 +157,7 @@ static inline bool xe_device_has_sriov(struct xe_device *xe) </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> I have a problem with this name. In my opinion, the functions named xe_device_has_* let us know if the device has any specific capability. Mostly this is based on infromation of the gen number, or an attribute from intel_device_info. In the case of MSI-X, such information is provided to us from PCI subsystem. I think I understand why you used here a flag that you set yourself, but with a function named like this, the information about whether the device has msi-x or not should be available on early driver probe stage before we call xe_device_probe. If you were to call directly pci_msix_vec_count in this function, I would be ok with that. But if you want to call pci_msix_vec_count only once and cache this value where in xe_driver, then in my opinion you should name this function differently, for example xe_device_uses_msix. </pre> </blockquote> I have moved xe_irq_init to an earlier point - so now it should be aligned with the expectation you mentioned. Btw, I'm not sure it currently holds for other xe_device_has_* functions, for example xe_device_has_flat_ccs is set fairly late. <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> static inline bool xe_device_has_msix(struct xe_device *xe) { - /* TODO: change this when MSI-X support is fully integrated */ - return false; + return xe->irq.msix.enabled; </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> I think this flag is unnecessary and certainly misnamed. This is because it implies that msi-x is enabled, which is not true when you set it to true. You already have the num_of_interrupts variable (BTW, the name nvec would be ok for them too) Just do: static inline bool xe_device_uses_msix(struct xe_device *xe) { return xe->irq.msix.num_of_interrupts > 0; } </pre> </blockquote> Agreed, thanks! <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> } static inline bool xe_device_has_memirq(struct xe_device *xe) diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h index bccca63c8a48..159b0e25e2bb 100644 --- a/drivers/gpu/drm/xe/xe_device_types.h +++ b/drivers/gpu/drm/xe/xe_device_types.h @@ -346,6 +346,16 @@ struct xe_device { /** @irq.enabled: interrupts enabled on this device */ bool enabled; + + /** @irq.msix: irq info for platforms that support MSI-X */ + struct { + /** @irq.msix.enabled: MSI-X interrupts enabled on this device */ + bool enabled; </pre> </blockquote> <pre wrap="" class="moz-quote-pre">You don't need it </pre> </blockquote> Ack <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ /** @irq.msix.num_of_interrupts: number of MSI-X interrupts */ + u16 num_of_interrupts; </pre> </blockquote> <pre wrap="" class="moz-quote-pre">NIT: you can just name it nvec</pre> </blockquote> Done <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ /** @irq.msix.default_msix: Default MSI-X vector */ + u16 default_msix; </pre> </blockquote> <pre wrap="" class="moz-quote-pre">do we need it ? will the default MSI-X vector change ? maybe some define is enough</pre> </blockquote> Indeed, changed to a define. <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ } msix; } irq; /** @ttm: ttm device */ diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c index fd0f3b3c9101..fe3a10825245 100644 --- a/drivers/gpu/drm/xe/xe_exec_queue.c +++ b/drivers/gpu/drm/xe/xe_exec_queue.c @@ -68,6 +68,7 @@ static struct xe_exec_queue *__xe_exec_queue_alloc(struct xe_device *xe, q->gt = gt; q->class = hwe->class; q->width = width; + q->msix = xe->irq.msix.default_msix; q->logical_mask = logical_mask; q->fence_irq = &gt->fence_irq[hwe->class]; q->ring_ops = gt->ring_ops[hwe->class]; @@ -117,7 +118,7 @@ static int __xe_exec_queue_init(struct xe_exec_queue *q) } for (i = 0; i < q->width; ++i) { - q->lrc[i] = xe_lrc_create(q->hwe, q->vm, SZ_16K); + q->lrc[i] = xe_lrc_create(q->hwe, q->vm, SZ_16K, q->msix); if (IS_ERR(q->lrc[i])) { err = PTR_ERR(q->lrc[i]); goto err_unlock; diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h index 1158b6062a6c..19fd66d59dd7 100644 --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h @@ -63,6 +63,8 @@ struct xe_exec_queue { char name[MAX_FENCE_NAME_LEN]; /** @width: width (number BB submitted per exec) of this exec queue */ u16 width; + /** @msix: MSI-X vector (for platforms that support it) */ + u16 msix; </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> NIT: Maybe msix_vec, msix seems ambiguous to me </pre> </blockquote> Ok <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> /** @fence_irq: fence IRQ used to signal job completion */ struct xe_hw_fence_irq *fence_irq; diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c index a8c416a48812..dcbc87a31ba3 100644 --- a/drivers/gpu/drm/xe/xe_execlist.c +++ b/drivers/gpu/drm/xe/xe_execlist.c @@ -47,6 +47,7 @@ static void __start_lrc(struct xe_hw_engine *hwe, struct xe_lrc *lrc, struct xe_mmio *mmio = &gt->mmio; struct xe_device *xe = gt_to_xe(gt); u64 lrc_desc; + u32 ring_mode = _MASKED_BIT_ENABLE(GFX_DISABLE_LEGACY_MODE); lrc_desc = xe_lrc_descriptor(lrc); @@ -80,8 +81,10 @@ static void __start_lrc(struct xe_hw_engine *hwe, struct xe_lrc *lrc, xe_mmio_write32(mmio, RING_HWS_PGA(hwe->mmio_base), xe_bo_ggtt_addr(hwe->hwsp)); xe_mmio_read32(mmio, RING_HWS_PGA(hwe->mmio_base)); - xe_mmio_write32(mmio, RING_MODE(hwe->mmio_base), - _MASKED_BIT_ENABLE(GFX_DISABLE_LEGACY_MODE)); + + if (xe_device_has_msix(gt_to_xe(hwe->gt))) + ring_mode |= _MASKED_BIT_ENABLE(GFX_MSIX_INTERRUPT_ENABLE); + xe_mmio_write32(mmio, RING_MODE(hwe->mmio_base), ring_mode); xe_mmio_write32(mmio, RING_EXECLIST_SQ_CONTENTS_LO(hwe->mmio_base), lower_32_bits(lrc_desc)); @@ -265,7 +268,7 @@ struct xe_execlist_port *xe_execlist_port_create(struct xe_device *xe, port->hwe = hwe; - port->lrc = xe_lrc_create(hwe, NULL, SZ_16K); + port->lrc = xe_lrc_create(hwe, NULL, SZ_16K, xe->irq.msix.default_msix); if (IS_ERR(port->lrc)) { err = PTR_ERR(port->lrc); goto err; diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c index 1557acee3523..080506b2c4ad 100644 --- a/drivers/gpu/drm/xe/xe_hw_engine.c +++ b/drivers/gpu/drm/xe/xe_hw_engine.c @@ -324,6 +324,7 @@ void xe_hw_engine_enable_ring(struct xe_hw_engine *hwe) { u32 ccs_mask = xe_hw_engine_mask_per_class(hwe->gt, XE_ENGINE_CLASS_COMPUTE); + u32 ring_mode = _MASKED_BIT_ENABLE(GFX_DISABLE_LEGACY_MODE); if (hwe->class == XE_ENGINE_CLASS_COMPUTE && ccs_mask) xe_mmio_write32(&hwe->gt->mmio, RCU_MODE, @@ -332,8 +333,10 @@ void xe_hw_engine_enable_ring(struct xe_hw_engine *hwe) xe_hw_engine_mmio_write32(hwe, RING_HWSTAM(0), ~0x0); xe_hw_engine_mmio_write32(hwe, RING_HWS_PGA(0), xe_bo_ggtt_addr(hwe->hwsp)); - xe_hw_engine_mmio_write32(hwe, RING_MODE(0), - _MASKED_BIT_ENABLE(GFX_DISABLE_LEGACY_MODE)); + + if (xe_device_has_msix(gt_to_xe(hwe->gt))) + ring_mode |= _MASKED_BIT_ENABLE(GFX_MSIX_INTERRUPT_ENABLE); + xe_hw_engine_mmio_write32(hwe, RING_MODE(0), ring_mode); xe_hw_engine_mmio_write32(hwe, RING_MI_MODE(0), _MASKED_BIT_DISABLE(STOP_RING)); xe_hw_engine_mmio_read32(hwe, RING_MI_MODE(0)); diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c index b7995ebd54ab..bad446739b96 100644 --- a/drivers/gpu/drm/xe/xe_irq.c +++ b/drivers/gpu/drm/xe/xe_irq.c @@ -4,6 +4,7 @@ */ #include "xe_irq.h" +#include "xe_irq_msix.h" #include <linux/sched/clock.h> @@ -577,8 +578,12 @@ static void xe_irq_reset(struct xe_device *xe) struct xe_tile *tile; u8 id; - if (IS_SRIOV_VF(xe)) + if (IS_SRIOV_VF(xe)) { return vf_irq_reset(xe); + } else if (xe_device_uses_memirq(xe)) { </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> do we need an else if? In my opinion, it is enough: if (IS_SRIOV_VF(xe)) return vf_irq_reset(xe); if (xe_device_uses_memirq(xe)) for_each_tile(tile, xe, id) xe_memirq_reset(&tile->memirq);</pre> </blockquote> Fixed, thanks! <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ for_each_tile(tile, xe, id) + xe_memirq_reset(&tile->memirq); + } for_each_tile(tile, xe, id) { if (GRAPHICS_VERx100(xe) >= 1210) @@ -619,8 +624,15 @@ static void vf_irq_postinstall(struct xe_device *xe) static void xe_irq_postinstall(struct xe_device *xe) { - if (IS_SRIOV_VF(xe)) + if (IS_SRIOV_VF(xe)) { return vf_irq_postinstall(xe); + } else if (xe_device_uses_memirq(xe)) { </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> similar to above: we have return for VF case, else if is not needed </pre> </blockquote> Ack <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ struct xe_tile *tile; + unsigned int id; + + for_each_tile(tile, xe, id) + xe_memirq_postinstall(&tile->memirq); + } xe_display_irq_postinstall(xe, xe_root_mmio_gt(xe)); @@ -668,61 +680,94 @@ static irq_handler_t xe_irq_handler(struct xe_device *xe) return xelp_irq_handler; } -static void irq_uninstall(void *arg) +static int xe_irq_msi_request(struct xe_device *xe) +{ + struct pci_dev *pdev = to_pci_dev(xe->drm.dev); + irq_handler_t irq_handler; + int irq, err; + + irq_handler = xe_irq_handler(xe); + if (!irq_handler) { + drm_err(&xe->drm, "No supported interrupt handler"); + return -EINVAL; + } + + irq = pci_irq_vector(pdev, 0); + err = request_irq(irq, irq_handler, IRQF_SHARED, DRIVER_NAME, xe); + if (err < 0) { + drm_err(&xe->drm, "Failed to request MSI IRQ %d\n", err); + return err; + } + + return 0; +} + +static void xe_irq_msi_free(struct xe_device *xe) { - struct xe_device *xe = arg; struct pci_dev *pdev = to_pci_dev(xe->drm.dev); int irq; + irq = pci_irq_vector(pdev, 0); + free_irq(irq, xe); +} + +static void irq_uninstall(void *arg) +{ + struct xe_device *xe = arg; + if (!xe->irq.enabled) return; xe->irq.enabled = false; xe_irq_reset(xe); - irq = pci_irq_vector(pdev, 0); - free_irq(irq, xe); + if (xe_device_has_msix(xe)) + xe_irq_msix_free(xe); + else + xe_irq_msi_free(xe); } -int xe_irq_install(struct xe_device *xe) +int xe_irq_init(struct xe_device *xe) { </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Another function I have a problem with. The xe_irq_init by name is a common function for MSI/MSI-X, but it looks like it is dedicated only for MSI-X, even though you have a separate xe_irq_msix_init. IMO, you should move pci_msix_vec_count to the inside of xe_irq_msix_init. int xe_irq_msix_init(struct xe_device *xe) { struct pci_dev *pdev = to_pci_dev(xe->drm.dev); int nvec = pci_msix_vec_count(pdev); if (nvec == -EINVAL) { return 0; } else if (nvec < 0) { drm_err(&xe->drm, "Failed getting MSI-X vectors count: %pe\n", ERR_PTR(nvec)); return nvec; } xe->irq.msix.num_of_interrupts = nvec; xe->irq.msix.default_msix = DEFAULT_MSIX; return nvec; } int xe_irq_init(struct xe_device *xe) { return xe_irq_msix_init(xe); } </pre> </blockquote> Hmmm, pci_msix_vec_count is an unfortunate way to distinguish between MSI and MSI-X, so I felt it would be better placed in "common" code. But I agree with your suggestion. Also xe_irq_init now has a spin_lock_init call. <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> struct pci_dev *pdev = to_pci_dev(xe->drm.dev); - unsigned int irq_flags = PCI_IRQ_MSIX; - irq_handler_t irq_handler; - int err, irq, nvec; + int nvec = pci_msix_vec_count(pdev); - irq_handler = xe_irq_handler(xe); - if (!irq_handler) { - drm_err(&xe->drm, "No supported interrupt handler"); - return -EINVAL; + if (nvec > 0) { + xe_irq_msix_init(xe, nvec); + return 0; } + if (nvec == -EINVAL) + return 0; /* MSI */ + + drm_err(&xe->drm, "Failed getting MSI-X vectors count: %d\n", nvec); + return nvec; +} + +int xe_irq_install(struct xe_device *xe) +{ + struct pci_dev *pdev = to_pci_dev(xe->drm.dev); + unsigned int irq_flags = PCI_IRQ_MSI; + int nvec = 1; + int err; + xe_irq_reset(xe); - nvec = pci_msix_vec_count(pdev); - if (nvec <= 0) { - if (nvec == -EINVAL) { - /* MSIX capability is not supported in the device, using MSI */ - irq_flags = PCI_IRQ_MSI; - nvec = 1; - } else { - drm_err(&xe->drm, "MSIX: Failed getting count\n"); - return nvec; - } + if (xe_device_has_msix(xe)) { + nvec = xe->irq.msix.num_of_interrupts; + irq_flags = PCI_IRQ_MSIX; } err = pci_alloc_irq_vectors(pdev, nvec, nvec, irq_flags); if (err < 0) { - drm_err(&xe->drm, "MSI/MSIX: Failed to enable support %d\n", err); + drm_err(&xe->drm, "Failed to allocate IRQ vectors: %d\n", err); return err; } - irq = pci_irq_vector(pdev, 0); - err = request_irq(irq, irq_handler, IRQF_SHARED, DRIVER_NAME, xe); - if (err < 0) { - drm_err(&xe->drm, "Failed to request MSI/MSIX IRQ %d\n", err); + err = xe_device_has_msix(xe) ? xe_irq_msix_request(xe) : + xe_irq_msi_request(xe); + if (err) return err; - } xe->irq.enabled = true; @@ -735,7 +780,10 @@ int xe_irq_install(struct xe_device *xe) return 0; free_irq_handler: - free_irq(irq, xe); + if (xe_device_has_msix(xe)) + xe_irq_msix_free(xe); + else + xe_irq_msi_free(xe); return err; } @@ -748,7 +796,11 @@ void xe_irq_suspend(struct xe_device *xe) xe->irq.enabled = false; /* no new irqs */ spin_unlock_irq(&xe->irq.lock); - synchronize_irq(irq); /* flush irqs */ + /* flush irqs */ + if (xe_device_has_msix(xe)) + xe_irq_msix_synchronize_irq(xe); + else + synchronize_irq(irq); </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> If you created xe_irq_msix_synchronize_irq make the corresponding one for MSI: xe_irq_msi_synchronize_irq. Actully it looks bad, especially since for msi-x you also use synchronize_irq inside. </pre> </blockquote> Done <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> xe_irq_reset(xe); /* turn irqs off */ } diff --git a/drivers/gpu/drm/xe/xe_irq.h b/drivers/gpu/drm/xe/xe_irq.h index 067514e13675..abc13412260a 100644 --- a/drivers/gpu/drm/xe/xe_irq.h +++ b/drivers/gpu/drm/xe/xe_irq.h @@ -10,6 +10,7 @@ struct xe_device; struct xe_tile; struct xe_gt; +int xe_irq_init(struct xe_device *xe); int xe_irq_install(struct xe_device *xe); void xe_irq_suspend(struct xe_device *xe); void xe_irq_resume(struct xe_device *xe); diff --git a/drivers/gpu/drm/xe/xe_irq_msix.c b/drivers/gpu/drm/xe/xe_irq_msix.c new file mode 100644 index 000000000000..dac339ba5ed2 --- /dev/null +++ b/drivers/gpu/drm/xe/xe_irq_msix.c @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include "xe_irq_msix.h" + +#include "regs/xe_guc_regs.h" + +#include "xe_device.h" +#include "xe_drv.h" +#include "xe_guc.h" +#include "xe_memirq.h" + +enum xe_irq_msix_static { + GUC2HOST_MSIX = 0, + DEFAULT_MSIX, + /* Must be last */ + NUM_OF_STATIC_MSIX, +}; + +void xe_irq_msix_init(struct xe_device *xe, int nvec) +{ + xe->irq.msix.enabled = true; + xe->irq.msix.num_of_interrupts = nvec; + xe->irq.msix.default_msix = DEFAULT_MSIX; +} + +static irqreturn_t guc2host_irq_handler(int irq, void *arg) +{ + struct xe_device *xe = arg; + struct xe_tile *tile; + u8 id; + + for_each_tile(tile, xe, id) + xe_guc_irq_handler(&tile->primary_gt->uc.guc, + GUC_INTR_GUC2HOST); + + return IRQ_HANDLED; +} + +static irqreturn_t xe_irq_msix_default_hwe_handler(int irq, void *arg) +{ + unsigned int tile_id, gt_id; + struct xe_device *xe = arg; + struct xe_memirq *memirq; + struct xe_hw_engine *hwe; + enum xe_hw_engine_id id; + struct xe_tile *tile; + struct xe_gt *gt; + + for_each_tile(tile, xe, tile_id) { + memirq = &tile->memirq; + if (!memirq->bo) + continue; + + for_each_gt(gt, xe, gt_id) { + if (gt->tile != tile) + continue; + + for_each_hw_engine(hwe, gt, id) + xe_memirq_hwe_handler(memirq, hwe); + } + } + + return IRQ_HANDLED; +} + +static int xe_irq_msix_request_irq(struct xe_device *xe, irq_handler_t handler, + const char *name, u16 msix) +{ + struct pci_dev *pdev = to_pci_dev(xe->drm.dev); + int ret, irq; + + irq = pci_irq_vector(pdev, msix); + if (irq < 0) + return irq; + + ret = request_irq(irq, handler, IRQF_SHARED, name, xe); + if (ret < 0) + return ret; + + return 0; +} + +static void xe_irq_msix_free_irq(struct xe_device *xe, u16 msix) +{ + struct pci_dev *pdev = to_pci_dev(xe->drm.dev); + int irq; + + irq = pci_irq_vector(pdev, msix); + if (irq < 0) { + drm_err(&xe->drm, "MSI-X %u can't be released, there is no matching IRQ\n", msix); + return; + } + + free_irq(irq, xe); +} + +int xe_irq_msix_request(struct xe_device *xe) </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> maybe xe_irq_msix_request_irqs ? </pre> </blockquote> Done <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+{ + int err; + + err = xe_irq_msix_request_irq(xe, guc2host_irq_handler, + DRIVER_NAME "-guc2host", GUC2HOST_MSIX); + if (err) { + drm_err(&xe->drm, "Failed to request MSI-X IRQ %d: %d\n", GUC2HOST_MSIX, err); + return err; + } + + err = xe_irq_msix_request_irq(xe, xe_irq_msix_default_hwe_handler, + DRIVER_NAME "-default-msix", DEFAULT_MSIX); + if (err) { + drm_err(&xe->drm, "Failed to request MSI-X IRQ %d: %d\n", DEFAULT_MSIX, err); + xe_irq_msix_free_irq(xe, GUC2HOST_MSIX); + return err; + } + + return 0; +} + +void xe_irq_msix_free(struct xe_device *xe) +{ + xe_irq_msix_free_irq(xe, GUC2HOST_MSIX); + xe_irq_msix_free_irq(xe, DEFAULT_MSIX); +} + +void xe_irq_msix_synchronize_irq(struct xe_device *xe) +{ + struct pci_dev *pdev = to_pci_dev(xe->drm.dev); + + synchronize_irq(pci_irq_vector(pdev, GUC2HOST_MSIX)); + synchronize_irq(pci_irq_vector(pdev, DEFAULT_MSIX)); +} </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> For these two functions above, use for loop. You have defined NUM_OF_STATIC_MSIX. Use it az limit in for loop. </pre> </blockquote> These statements are actually changed into a loop with a subsequent patch in these series introducing MSI-X allocator. In this one I felt that explicit calls are better (e. g. when searching for references). <blockquote type="cite" cite="mid:20241127082408.cf3lqksj4j6jusgp@intel.com"> <pre wrap="" class="moz-quote-pre"> Thanks, Piotr </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">diff --git a/drivers/gpu/drm/xe/xe_irq_msix.h b/drivers/gpu/drm/xe/xe_irq_msix.h new file mode 100644 index 000000000000..9c36d11f05fc --- /dev/null +++ b/drivers/gpu/drm/xe/xe_irq_msix.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2024 Intel Corporation + */ + +#ifndef _XE_IRQ_MSIX_H_ +#define _XE_IRQ_MSIX_H_ + +struct xe_device; + +void xe_irq_msix_init(struct xe_device *xe, int nvec); +void xe_irq_msix_free(struct xe_device *xe); +int xe_irq_msix_request(struct xe_device *xe); +void xe_irq_msix_synchronize_irq(struct xe_device *xe); + +#endif diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c index 4b65da77c6e0..ca27c3be3b8a 100644 --- a/drivers/gpu/drm/xe/xe_lrc.c +++ b/drivers/gpu/drm/xe/xe_lrc.c @@ -584,6 +584,7 @@ static void set_memory_based_intr(u32 *regs, struct xe_hw_engine *hwe) { struct xe_memirq *memirq = &gt_to_tile(hwe->gt)->memirq; struct xe_device *xe = gt_to_xe(hwe->gt); + u8 num_regs; if (!xe_device_uses_memirq(xe)) return; @@ -593,12 +594,18 @@ static void set_memory_based_intr(u32 *regs, struct xe_hw_engine *hwe) regs[CTX_INT_MASK_ENABLE_REG] = RING_IMR(0).addr; regs[CTX_INT_MASK_ENABLE_PTR] = xe_memirq_enable_ptr(memirq); - regs[CTX_LRI_INT_REPORT_PTR] = MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(2) | + num_regs = xe_device_has_msix(xe) ? 3 : 2; + regs[CTX_LRI_INT_REPORT_PTR] = MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(num_regs) | 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, 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, hwe); + + if (xe_device_has_msix(xe)) { + regs[CTX_CS_INT_VEC_REG] = CS_INT_VEC(0).addr; + /* CTX_CS_INT_VEC_DATA will be set in xe_lrc_init */ + } } static int lrc_ring_mi_mode(struct xe_hw_engine *hwe) @@ -876,7 +883,7 @@ static void xe_lrc_finish(struct xe_lrc *lrc) #define PVC_CTX_ACC_CTR_THOLD (0x2a + 1) static int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, - struct xe_vm *vm, u32 ring_size) + struct xe_vm *vm, u32 ring_size, u16 msix) { struct xe_gt *gt = hwe->gt; struct xe_tile *tile = gt_to_tile(gt); @@ -945,6 +952,14 @@ static int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, xe_drm_client_add_bo(vm->xef->client, lrc->bo); } + if (xe_device_has_msix(xe)) { + xe_lrc_write_ctx_reg(lrc, CTX_INT_STATUS_REPORT_PTR, + xe_memirq_status_ptr(&tile->memirq, hwe)); + xe_lrc_write_ctx_reg(lrc, CTX_INT_SRC_REPORT_PTR, + xe_memirq_source_ptr(&tile->memirq, hwe)); + xe_lrc_write_ctx_reg(lrc, CTX_CS_INT_VEC_DATA, msix << 16 | msix); + } + if (xe_gt_has_indirect_ring_state(gt)) { xe_lrc_write_ctx_reg(lrc, CTX_INDIRECT_RING_STATE, __xe_lrc_indirect_ring_ggtt_addr(lrc)); @@ -1005,6 +1020,7 @@ static int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, * @hwe: Hardware Engine * @vm: The VM (address space) * @ring_size: LRC ring size + * @msix: MSI-X interrupt vector (for platforms that support it) * * Allocate and initialize the Logical Ring Context (LRC). * @@ -1012,7 +1028,7 @@ static int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, * upon failure. */ struct xe_lrc *xe_lrc_create(struct xe_hw_engine *hwe, struct xe_vm *vm, - u32 ring_size) + u32 ring_size, u16 msix) { struct xe_lrc *lrc; int err; @@ -1021,7 +1037,7 @@ struct xe_lrc *xe_lrc_create(struct xe_hw_engine *hwe, struct xe_vm *vm, if (!lrc) return ERR_PTR(-ENOMEM); - err = xe_lrc_init(lrc, hwe, vm, ring_size); + err = xe_lrc_init(lrc, hwe, vm, ring_size, msix); if (err) { kfree(lrc); return ERR_PTR(err); diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h index 40d8f6906d3e..6d1c2f6c0559 100644 --- a/drivers/gpu/drm/xe/xe_lrc.h +++ b/drivers/gpu/drm/xe/xe_lrc.h @@ -40,7 +40,7 @@ struct xe_lrc_snapshot { #define LRC_PPHWSP_SCRATCH_ADDR (0x34 * 4) struct xe_lrc *xe_lrc_create(struct xe_hw_engine *hwe, struct xe_vm *vm, - u32 ring_size); + u32 ring_size, u16 msix); void xe_lrc_destroy(struct kref *ref); /** -- 2.43.2 </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> </pre> </blockquote> </body> </html>