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

[Lucas De Marchi]

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

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