On 9/5/2023 1:24 PM, Christian König wrote:
> Am 04.09.23 um 08:05 schrieb Ma Jun:
>> [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....@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;
>
> While at it please remove the assignment here.
>
> Unless really necessary initializing local variables is rather frowned upon.
>
Thanks for review. Will fix it in the next version.
Regards,
Ma Jun
> Apart from that Alex needs to take a look at this, I'm not that familiar
> with this code.
>
> Christian.
>
>> - 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;
>> +
>> + /* 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)
>> {
>
