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

["Ma, Jun" <majun@amd.com>]

On 9/6/2023 3:23 PM, Christian König wrote:
> Am 06.09.23 um 08:55 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.
>>
>> v2:
>> - Remove local variable initializing code (Christian)
>> - Use PCI_IRQ_ALL_TYPES (Alex)
>>
>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 45 ++++++++++++++-----------
>>   1 file changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> index fa6d0adcec20..64c245015e17 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> @@ -270,29 +270,29 @@ static void amdgpu_restore_msix(struct amdgpu_device *adev)
>>    */
>>   int amdgpu_irq_init(struct amdgpu_device *adev)
>>   {
>> -	int r = 0;
>> -	unsigned int irq;
>> +	int r;
>> +	unsigned int irq, flags;
> 
> It's also good style to define variables like "r" and "i" last. Some 
> upstream maintainers even require reverse xmas tree style defines (e.g. 
> longest first, shortest last).
> 
> With that changed the patch is Acked-by: Christian König 
> <christian.koenig@amd.com>
> 

Thanks, I will update it when push.

Regards,
Ma Jun
> Regards,
> Christian.
> 
>>   
>>   	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_LEGACY;
>> +	else
>> +		flags = PCI_IRQ_ALL_TYPES;
>> +
>> +	/* 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)
>>   {
>