On Mon, Sep 4, 2023 at 2:30 AM Ma Jun <Jun.Ma2@xxxxxxx> 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@xxxxxxx>
> ---
> 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.
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
>
