[PATCH] drm/amd: Fix the flag setting code for interrupt request

[Ma, Jun]

On 9/5/2023 10:46 PM, Alex Deucher wrote:
> On Mon, Sep 4, 2023 at 2:30 AM Ma Jun <Jun.Ma2 at amd.com> wrote:
>>
>> [1] Remove the irq flags setting code since pci_alloc_irq_vectors()
>> handles these flags.
>> [2] Free the msi vectors in case of error.
>>
>> Signed-off-by: Ma Jun <Jun.Ma2 at amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 43 ++++++++++++++-----------
>>  1 file changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> index fa6d0adcec20..17043a1e37a5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> @@ -271,28 +271,28 @@ static void amdgpu_restore_msix(struct amdgpu_device *adev)
>>  int amdgpu_irq_init(struct amdgpu_device *adev)
>>  {
>>         int r = 0;
>> -       unsigned int irq;
>> +       unsigned int irq, flags;
>>
>>         spin_lock_init(&adev->irq.lock);
>>
>>         /* Enable MSI if not disabled by module parameter */
>>         adev->irq.msi_enabled = false;
>>
>> +       if (amdgpu_msi_ok(adev))
>> +               flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
>> +       else
>> +               flags = PCI_IRQ_LEGACY;
> 
> I think this logic could be something like:
> 
> if (!amdgpu_msi_ok(adev))
>               flags = PCI_IRQ_LEGACY;
> else
>               flags = PCI_IRQ_ALL_TYPES
> 
> Other than that, looks fine to me.

Thanks, will update later.

Regards,
Ma Jun
> 
> Alex
> 
> 
>> +
>> +       /* we only need one vector */
>> +       r = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
>> +       if (r < 0) {
>> +               dev_err(adev->dev, "Failed to alloc msi vectors\n");
>> +               return r;
>> +       }
>> +
>>         if (amdgpu_msi_ok(adev)) {
>> -               int nvec = pci_msix_vec_count(adev->pdev);
>> -               unsigned int flags;
>> -
>> -               if (nvec <= 0)
>> -                       flags = PCI_IRQ_MSI;
>> -               else
>> -                       flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
>> -
>> -               /* we only need one vector */
>> -               nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
>> -               if (nvec > 0) {
>> -                       adev->irq.msi_enabled = true;
>> -                       dev_dbg(adev->dev, "using MSI/MSI-X.\n");
>> -               }
>> +               adev->irq.msi_enabled = true;
>> +               dev_dbg(adev->dev, "using MSI/MSI-X.\n");
>>         }
>>
>>         INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
>> @@ -302,22 +302,29 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>>         /* Use vector 0 for MSI-X. */
>>         r = pci_irq_vector(adev->pdev, 0);
>>         if (r < 0)
>> -               return r;
>> +               goto free_vectors;
>>         irq = r;
>>
>>         /* PCI devices require shared interrupts. */
>>         r = request_irq(irq, amdgpu_irq_handler, IRQF_SHARED, adev_to_drm(adev)->driver->name,
>>                         adev_to_drm(adev));
>>         if (r)
>> -               return r;
>> +               goto free_vectors;
>> +
>>         adev->irq.installed = true;
>>         adev->irq.irq = irq;
>>         adev_to_drm(adev)->max_vblank_count = 0x00ffffff;
>>
>>         DRM_DEBUG("amdgpu: irq initialized.\n");
>>         return 0;
>> -}
>>
>> +free_vectors:
>> +       if (adev->irq.msi_enabled)
>> +               pci_free_irq_vectors(adev->pdev);
>> +
>> +       adev->irq.msi_enabled = false;
>> +       return r;
>> +}
>>
>>  void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>>  {
>> --
>> 2.34.1
>>