+linux-pci and few people involved with
aff171641d18 ("PCI: Provide sensible IRQ vector alloc/free routines")
for possible feedback.
On Tue, Jan 23, 2024 at 11:34:17AM -0600, Dani Liberman wrote:
On 23/01/2024 18:37, Lucas De Marchi wrote:
On Sun, Jan 21, 2024 at 11:02:14AM +0200, Dani Liberman wrote:
If platform supports MSIX, driver needs to allocate all possible
interrupts.
v2:
- drop msix_cap and use the api return code instead.
- fix commit message.
Cc: Ohad Sharabi <osharabi@xxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Signed-off-by: Dani Liberman <dliberman@xxxxxxxxx>
---
drivers/gpu/drm/xe/xe_irq.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
index 907c8ff0fa21..7a23d25c1062 100644
--- a/drivers/gpu/drm/xe/xe_irq.c
+++ b/drivers/gpu/drm/xe/xe_irq.c
@@ -662,7 +662,7 @@ int xe_irq_install(struct xe_device *xe)
{
struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
irq_handler_t irq_handler;
- int err, irq;
+ int err, irq, nvec;
irq_handler = xe_irq_handler(xe);
if (!irq_handler) {
@@ -672,7 +672,18 @@ int xe_irq_install(struct xe_device *xe)
xe_irq_reset(xe);
- err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI |
PCI_IRQ_MSIX);
+ nvec = pci_msix_vec_count(pdev);
From docs:
Additionally there are APIs to provide the number of supported MSI
or MSI-X
vectors: pci_msi_vec_count() and pci_msix_vec_count(). In general
these
should be avoided in favor of letting pci_alloc_irq_vectors() cap the
number of vectors. If you have a legitimate special use case for
the count
of vectors we might have to revisit that decision and add a
pci_nr_irq_vectors() helper that handles MSI and MSI-X transparently.
+ if (nvec <= 0) {
+ if (nvec == -EINVAL) {
+ /* MSIX capability is not supported in the device, using
MSI */
+ nvec = 1;
+ } else {
+ drm_err(&xe->drm, "MSIX: Failed getting count\n");
+ return nvec;
+ }
+ }
+
+ err = pci_alloc_irq_vectors(pdev, nvec, nvec, PCI_IRQ_MSI |
PCI_IRQ_MSIX);
these are just possible flags, so what you did above was for nothing?
If platform doesn't support MSIX and we pass e.g. 16, we will still have
just 1 allocated. So, as I said in the other version, this is just a
maximum the **driver** wants to deal with. Still not clear why we need
this rather than just pass a value for max that covers all the
platforms.
Lucas De Marchi
We need it for the minimum msix vectors value.
The maximum msix vectors allowed is 2K, lets say we have a platform with
1K interrupts, we must ensure allocation of 1K vectors.
If we pass min_vec = 1, max_vec =2K and the api will allocate only 512,
it will still pass and we won't have any indication.
ok, so you need it for the **minimum** to be "all the platform
supports" and you don't accept less than that. From that pov it makes
sense. For people added in Cc, is that an acceptable use of
pci_msix_vec_count()?
thanks
Lucas De Marchi
About the docs, so yes, if we have a platform specific code, we know
exactly the number of interrupts and we will just use a define. But
since it's a common code that should support more than 1 msix platform,
we need to use this api.
Dani Liberman
if (err < 0) {
drm_err(&xe->drm, "MSI/MSIX: Failed to enable support %d\n",
err);
return err;
--
2.34.1
