[PATCH v2] drm/xe/irq: allocate all possible msix interrupts

[Dani Liberman]

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 at habana.ai>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Signed-off-by: Dani Liberman <dliberman at habana.ai>
>> ---
>> 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.

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
>>