On Tue, Jun 2, 2020 at 10:35 AM Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Tue, Jun 2, 2020 at 5:21 PM Alex Deucher alexdeucher@gmail.com wrote:
On Tue, Jun 2, 2020 at 10:00 AM Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Tue, Jun 2, 2020 at 4:38 PM Ruhl, Michael J michael.j.ruhl@intel.com wrote:
From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Piotr Stankiewicz
int nvec = pci_msix_vec_count(adev->pdev); unsigned int flags;
if (nvec <= 0) {
if (nvec > 0)flags = PCI_IRQ_MSI_TYPES;else flags = PCI_IRQ_MSI;
} else {flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;}Minor nit:
Is it really necessary to set do this check? Can flags just be set?
I.e.: flags = PCI_IRQ_MSI_TYPES;
pci_alloc_irq_vector() tries stuff in order. If MSIX is not available, it will try MSI.
That's also what I proposed earlier. But I suggested as well to wait for AMD people to confirm that neither pci_msix_vec_count() nor flags is needed and we can directly supply MSI_TYPES to the below call.
I think it was leftover from debugging and just to be careful. We had some issues when we originally enabled MSI-X on certain boards. The fix was to just allocate a single vector (since that is all we use anyway) and we were using the wrong irq (pdev->irq vs pci_irq_vector(pdev, 0)).
Do you agree that simple
nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, PCI_IRQ_MSI_TYPES);
will work and we can remove that leftover?
Yes, I believe so. Tom, can you give this a quick spin on raven just in case if you get a chance? Something like this:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c index 0cc4c67f95f7..c59111b57cc2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c @@ -248,16 +248,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev) adev->irq.msi_enabled = false;
if (amdgpu_msi_ok(adev)) { - int nvec = pci_msix_vec_count(adev->pdev); - unsigned int flags; + int nvec;
- 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); + nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, PCI_IRQ_MSI | PCI_IRQ_MSIX); if (nvec > 0) { adev->irq.msi_enabled = true; dev_dbg(adev->dev, "using MSI/MSI-X.\n");
Thanks,
Alex