[PATCH] drm/amdgpu: Update irq disable flow during unload

[PATCH] drm/amdgpu: Update irq disable flow during unload

[Asad Kamal]

In certain special cases, e.g device reset before module
unload, irq gets disabled as part of reset sequence and
won't get enabled back. Add special check to cover such scenarios

Signed-off-by: Asad Kamal <asad.kamal@amd.com>
Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 12 ++++++++++--
 drivers/gpu/drm/amd/amdgpu/soc15.c    | 13 +++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 372de9f1ce59..a4e1b9a58679 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -2361,6 +2361,7 @@ static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
 static int gmc_v9_0_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+	bool irq_release = true;
 
 	gmc_v9_0_gart_disable(adev);
 
@@ -2378,9 +2379,16 @@ static int gmc_v9_0_hw_fini(void *handle)
 	if (adev->mmhub.funcs->update_power_gating)
 		adev->mmhub.funcs->update_power_gating(adev, false);
 
-	amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
+	if (adev->shutdown)
+		irq_release = amdgpu_irq_enabled(adev, &adev->gmc.vm_fault, 0);
 
-	if (adev->gmc.ecc_irq.funcs &&
+	if (irq_release)
+		amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
+
+	if (adev->shutdown)
+		irq_release = amdgpu_irq_enabled(adev, &adev->gmc.ecc_irq, 0);
+
+	if (adev->gmc.ecc_irq.funcs && irq_release &&
 		amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__UMC))
 		amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
index 15033efec2ba..7ee835049d57 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -1266,6 +1266,7 @@ static int soc15_common_hw_init(void *handle)
 static int soc15_common_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+	bool irq_release = true;
 
 	/* Disable the doorbell aperture and selfring doorbell aperture
 	 * separately in hw_fini because soc15_enable_doorbell_aperture
@@ -1280,10 +1281,18 @@ static int soc15_common_hw_fini(void *handle)
 
 	if (adev->nbio.ras_if &&
 	    amdgpu_ras_is_supported(adev, adev->nbio.ras_if->block)) {
-		if (adev->nbio.ras &&
+		if (adev->shutdown)
+			irq_release = amdgpu_irq_enabled(adev, &adev->nbio.ras_controller_irq, 0);
+
+		if (adev->nbio.ras && irq_release &&
 		    adev->nbio.ras->init_ras_controller_interrupt)
 			amdgpu_irq_put(adev, &adev->nbio.ras_controller_irq, 0);
-		if (adev->nbio.ras &&
+
+		if (adev->shutdown)
+			irq_release = amdgpu_irq_enabled(adev,
+					&adev->nbio.ras_err_event_athub_irq, 0);
+
+		if (adev->nbio.ras && irq_release &&
 		    adev->nbio.ras->init_ras_err_event_athub_interrupt)
 			amdgpu_irq_put(adev, &adev->nbio.ras_err_event_athub_irq, 0);
 	}
-- 
2.42.0

Re: [PATCH] drm/amdgpu: Update irq disable flow during unload

[Lazar, Lijo]

On 1/5/2024 8:51 PM, Asad Kamal wrote:
> In certain special cases, e.g device reset before module
> unload, irq gets disabled as part of reset sequence and
> won't get enabled back. Add special check to cover such scenarios
> 
> Signed-off-by: Asad Kamal <asad.kamal@amd.com>
> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>

Please also add the tag

Fixes: f5c7e7797060 ("drm/amdgpu: Adjust removal control flow for smu 
v13_0_2")

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 12 ++++++++++--
>   drivers/gpu/drm/amd/amdgpu/soc15.c    | 13 +++++++++++--
>   2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 372de9f1ce59..a4e1b9a58679 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -2361,6 +2361,7 @@ static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
>   static int gmc_v9_0_hw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +	bool irq_release = true;
>   
>   	gmc_v9_0_gart_disable(adev);
>   
> @@ -2378,9 +2379,16 @@ static int gmc_v9_0_hw_fini(void *handle)
>   	if (adev->mmhub.funcs->update_power_gating)
>   		adev->mmhub.funcs->update_power_gating(adev, false);
>   
> -	amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
> +	if (adev->shutdown)
> +		irq_release = amdgpu_irq_enabled(adev, &adev->gmc.vm_fault, 0);
>   
> -	if (adev->gmc.ecc_irq.funcs &&
> +	if (irq_release)
> +		amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
> +
> +	if (adev->shutdown)
> +		irq_release = amdgpu_irq_enabled(adev, &adev->gmc.ecc_irq, 0);
> +
> +	if (adev->gmc.ecc_irq.funcs && irq_release &&
>   		amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__UMC))
>   		amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index 15033efec2ba..7ee835049d57 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -1266,6 +1266,7 @@ static int soc15_common_hw_init(void *handle)
>   static int soc15_common_hw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +	bool irq_release = true;
>   
>   	/* Disable the doorbell aperture and selfring doorbell aperture
>   	 * separately in hw_fini because soc15_enable_doorbell_aperture
> @@ -1280,10 +1281,18 @@ static int soc15_common_hw_fini(void *handle)
>   
>   	if (adev->nbio.ras_if &&
>   	    amdgpu_ras_is_supported(adev, adev->nbio.ras_if->block)) {
> -		if (adev->nbio.ras &&
> +		if (adev->shutdown)
> +			irq_release = amdgpu_irq_enabled(adev, &adev->nbio.ras_controller_irq, 0);
> +
> +		if (adev->nbio.ras && irq_release &&
>   		    adev->nbio.ras->init_ras_controller_interrupt)
>   			amdgpu_irq_put(adev, &adev->nbio.ras_controller_irq, 0);
> -		if (adev->nbio.ras &&
> +
> +		if (adev->shutdown)
> +			irq_release = amdgpu_irq_enabled(adev,
> +					&adev->nbio.ras_err_event_athub_irq, 0);
> +
> +		if (adev->nbio.ras && irq_release &&
>   		    adev->nbio.ras->init_ras_err_event_athub_interrupt)
>   			amdgpu_irq_put(adev, &adev->nbio.ras_err_event_athub_irq, 0);
>   	}

RE: [PATCH] drm/amdgpu: Update irq disable flow during unload

[Kamal, Asad]

[AMD Official Use Only - General]

Hi Christian,

Thank you for the comment.

This is not normal reset, it is reset done during unload for smu v_13_0_2.

Thanks & Regards
Asad

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Monday, January 8, 2024 1:33 PM
To: Kamal, Asad <Asad.Kamal@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Update irq disable flow during unload

Am 05.01.24 um 16:21 schrieb Asad Kamal:
> In certain special cases, e.g device reset before module unload, irq
> gets disabled as part of reset sequence and won't get enabled back.
> Add special check to cover such scenarios

Well complete NAK to that. Resets shouldn't affect the IRQ state at all!

If this is an issue then something else is broken.

Regards,
Christian.

>
> Signed-off-by: Asad Kamal <asad.kamal@amd.com>
> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 12 ++++++++++--
>   drivers/gpu/drm/amd/amdgpu/soc15.c    | 13 +++++++++++--
>   2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 372de9f1ce59..a4e1b9a58679 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -2361,6 +2361,7 @@ static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
>   static int gmc_v9_0_hw_fini(void *handle)
>   {
>       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +     bool irq_release = true;
>
>       gmc_v9_0_gart_disable(adev);
>
> @@ -2378,9 +2379,16 @@ static int gmc_v9_0_hw_fini(void *handle)
>       if (adev->mmhub.funcs->update_power_gating)
>               adev->mmhub.funcs->update_power_gating(adev, false);
>
> -     amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
> +     if (adev->shutdown)
> +             irq_release = amdgpu_irq_enabled(adev, &adev->gmc.vm_fault, 0);
>
> -     if (adev->gmc.ecc_irq.funcs &&
> +     if (irq_release)
> +             amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
> +
> +     if (adev->shutdown)
> +             irq_release = amdgpu_irq_enabled(adev, &adev->gmc.ecc_irq, 0);
> +
> +     if (adev->gmc.ecc_irq.funcs && irq_release &&
>               amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__UMC))
>               amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
> b/drivers/gpu/drm/amd/amdgpu/soc15.c
> index 15033efec2ba..7ee835049d57 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -1266,6 +1266,7 @@ static int soc15_common_hw_init(void *handle)
>   static int soc15_common_hw_fini(void *handle)
>   {
>       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +     bool irq_release = true;
>
>       /* Disable the doorbell aperture and selfring doorbell aperture
>        * separately in hw_fini because soc15_enable_doorbell_aperture @@
> -1280,10 +1281,18 @@ static int soc15_common_hw_fini(void *handle)
>
>       if (adev->nbio.ras_if &&
>           amdgpu_ras_is_supported(adev, adev->nbio.ras_if->block)) {
> -             if (adev->nbio.ras &&
> +             if (adev->shutdown)
> +                     irq_release = amdgpu_irq_enabled(adev,
> +&adev->nbio.ras_controller_irq, 0);
> +
> +             if (adev->nbio.ras && irq_release &&
>                   adev->nbio.ras->init_ras_controller_interrupt)
>                       amdgpu_irq_put(adev, &adev->nbio.ras_controller_irq, 0);
> -             if (adev->nbio.ras &&
> +
> +             if (adev->shutdown)
> +                     irq_release = amdgpu_irq_enabled(adev,
> +                                     &adev->nbio.ras_err_event_athub_irq, 0);
> +
> +             if (adev->nbio.ras && irq_release &&
>                   adev->nbio.ras->init_ras_err_event_athub_interrupt)
>                       amdgpu_irq_put(adev, &adev->nbio.ras_err_event_athub_irq, 0);
>       }

Re: [PATCH] drm/amdgpu: Update irq disable flow during unload

[Christian König]

Am 08.01.24 um 09:13 schrieb Kamal, Asad:
> [AMD Official Use Only - General]
>
> Hi Christian,
>
> Thank you for the comment.
>
> This is not normal reset, it is reset done during unload for smu v_13_0_2.

Yeah, but this doesn't explain the rational for this.

IRQ enable/disable should be balanced in hw_init()/hw_fini(), 
independent of what else you do.

I'm not sure what you are trying to solve but this here is a complete no-go.

Regards,
Christian.

>
> Thanks & Regards
> Asad
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, January 8, 2024 1:33 PM
> To: Kamal, Asad <Asad.Kamal@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: Update irq disable flow during unload
>
> Am 05.01.24 um 16:21 schrieb Asad Kamal:
>> In certain special cases, e.g device reset before module unload, irq
>> gets disabled as part of reset sequence and won't get enabled back.
>> Add special check to cover such scenarios
> Well complete NAK to that. Resets shouldn't affect the IRQ state at all!
>
> If this is an issue then something else is broken.
>
> Regards,
> Christian.
>
>> Signed-off-by: Asad Kamal <asad.kamal@amd.com>
>> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 12 ++++++++++--
>>    drivers/gpu/drm/amd/amdgpu/soc15.c    | 13 +++++++++++--
>>    2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 372de9f1ce59..a4e1b9a58679 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -2361,6 +2361,7 @@ static void gmc_v9_0_gart_disable(struct amdgpu_device *adev)
>>    static int gmc_v9_0_hw_fini(void *handle)
>>    {
>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> +     bool irq_release = true;
>>
>>        gmc_v9_0_gart_disable(adev);
>>
>> @@ -2378,9 +2379,16 @@ static int gmc_v9_0_hw_fini(void *handle)
>>        if (adev->mmhub.funcs->update_power_gating)
>>                adev->mmhub.funcs->update_power_gating(adev, false);
>>
>> -     amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
>> +     if (adev->shutdown)
>> +             irq_release = amdgpu_irq_enabled(adev, &adev->gmc.vm_fault, 0);
>>
>> -     if (adev->gmc.ecc_irq.funcs &&
>> +     if (irq_release)
>> +             amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
>> +
>> +     if (adev->shutdown)
>> +             irq_release = amdgpu_irq_enabled(adev, &adev->gmc.ecc_irq, 0);
>> +
>> +     if (adev->gmc.ecc_irq.funcs && irq_release &&
>>                amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__UMC))
>>                amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> index 15033efec2ba..7ee835049d57 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>> @@ -1266,6 +1266,7 @@ static int soc15_common_hw_init(void *handle)
>>    static int soc15_common_hw_fini(void *handle)
>>    {
>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> +     bool irq_release = true;
>>
>>        /* Disable the doorbell aperture and selfring doorbell aperture
>>         * separately in hw_fini because soc15_enable_doorbell_aperture @@
>> -1280,10 +1281,18 @@ static int soc15_common_hw_fini(void *handle)
>>
>>        if (adev->nbio.ras_if &&
>>            amdgpu_ras_is_supported(adev, adev->nbio.ras_if->block)) {
>> -             if (adev->nbio.ras &&
>> +             if (adev->shutdown)
>> +                     irq_release = amdgpu_irq_enabled(adev,
>> +&adev->nbio.ras_controller_irq, 0);
>> +
>> +             if (adev->nbio.ras && irq_release &&
>>                    adev->nbio.ras->init_ras_controller_interrupt)
>>                        amdgpu_irq_put(adev, &adev->nbio.ras_controller_irq, 0);
>> -             if (adev->nbio.ras &&
>> +
>> +             if (adev->shutdown)
>> +                     irq_release = amdgpu_irq_enabled(adev,
>> +                                     &adev->nbio.ras_err_event_athub_irq, 0);
>> +
>> +             if (adev->nbio.ras && irq_release &&
>>                    adev->nbio.ras->init_ras_err_event_athub_interrupt)
>>                        amdgpu_irq_put(adev, &adev->nbio.ras_err_event_athub_irq, 0);
>>        }

Re: [PATCH] drm/amdgpu: Update irq disable flow during unload

[Lazar, Lijo]

On 1/8/2024 1:51 PM, Christian König wrote:
> Am 08.01.24 um 09:13 schrieb Kamal, Asad:
>> [AMD Official Use Only - General]
>>
>> Hi Christian,
>>
>> Thank you for the comment.
>>
>> This is not normal reset, it is reset done during unload for smu 
>> v_13_0_2.
> 
> Yeah, but this doesn't explain the rational for this.
> 
> IRQ enable/disable should be balanced in hw_init()/hw_fini(), 
> independent of what else you do.
> 
> I'm not sure what you are trying to solve but this here is a complete 
> no-go.
> 

This is a special reset done during module unload by this commit - 
f5c7e7797060 ("drm/amdgpu: Adjust removal control flow for smu
v13_0_2"). Without this commit, it seems driver reload doesnt' work.

In this particular case, a the reset is done during unload and only 
resume sequence of only select IPs are done (part of the workaround in 
the patch). For those IPs, irqs are enabled during 
late_init/ras_late_init, and not during hw_init(), that part gets skipped.

The module unload sequence causes a WARN trace during irq_put of those 
IPs during hw_fini(). Those are mix of generic irqs and ras irqs, so 
there is no clean way to untangle it.

One thing that could be done is to add an extra check for 13.0.2 version 
to make it clear that this workaround is done for only for those ASICs.

Thanks,
Lijo

> Regards,
> Christian.
> 
>>
>> Thanks & Regards
>> Asad
>>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Monday, January 8, 2024 1:33 PM
>> To: Kamal, Asad <Asad.Kamal@amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: Update irq disable flow during unload
>>
>> Am 05.01.24 um 16:21 schrieb Asad Kamal:
>>> In certain special cases, e.g device reset before module unload, irq
>>> gets disabled as part of reset sequence and won't get enabled back.
>>> Add special check to cover such scenarios
>> Well complete NAK to that. Resets shouldn't affect the IRQ state at all!
>>
>> If this is an issue then something else is broken.
>>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Asad Kamal <asad.kamal@amd.com>
>>> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 12 ++++++++++--
>>>    drivers/gpu/drm/amd/amdgpu/soc15.c    | 13 +++++++++++--
>>>    2 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> index 372de9f1ce59..a4e1b9a58679 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> @@ -2361,6 +2361,7 @@ static void gmc_v9_0_gart_disable(struct 
>>> amdgpu_device *adev)
>>>    static int gmc_v9_0_hw_fini(void *handle)
>>>    {
>>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>> +     bool irq_release = true;
>>>
>>>        gmc_v9_0_gart_disable(adev);
>>>
>>> @@ -2378,9 +2379,16 @@ static int gmc_v9_0_hw_fini(void *handle)
>>>        if (adev->mmhub.funcs->update_power_gating)
>>>                adev->mmhub.funcs->update_power_gating(adev, false);
>>>
>>> -     amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
>>> +     if (adev->shutdown)
>>> +             irq_release = amdgpu_irq_enabled(adev, 
>>> &adev->gmc.vm_fault, 0);
>>>
>>> -     if (adev->gmc.ecc_irq.funcs &&
>>> +     if (irq_release)
>>> +             amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
>>> +
>>> +     if (adev->shutdown)
>>> +             irq_release = amdgpu_irq_enabled(adev, 
>>> &adev->gmc.ecc_irq, 0);
>>> +
>>> +     if (adev->gmc.ecc_irq.funcs && irq_release &&
>>>                amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__UMC))
>>>                amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> index 15033efec2ba..7ee835049d57 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>> @@ -1266,6 +1266,7 @@ static int soc15_common_hw_init(void *handle)
>>>    static int soc15_common_hw_fini(void *handle)
>>>    {
>>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>> +     bool irq_release = true;
>>>
>>>        /* Disable the doorbell aperture and selfring doorbell aperture
>>>         * separately in hw_fini because 
>>> soc15_enable_doorbell_aperture @@
>>> -1280,10 +1281,18 @@ static int soc15_common_hw_fini(void *handle)
>>>
>>>        if (adev->nbio.ras_if &&
>>>            amdgpu_ras_is_supported(adev, adev->nbio.ras_if->block)) {
>>> -             if (adev->nbio.ras &&
>>> +             if (adev->shutdown)
>>> +                     irq_release = amdgpu_irq_enabled(adev,
>>> +&adev->nbio.ras_controller_irq, 0);
>>> +
>>> +             if (adev->nbio.ras && irq_release &&
>>>                    adev->nbio.ras->init_ras_controller_interrupt)
>>>                        amdgpu_irq_put(adev, 
>>> &adev->nbio.ras_controller_irq, 0);
>>> -             if (adev->nbio.ras &&
>>> +
>>> +             if (adev->shutdown)
>>> +                     irq_release = amdgpu_irq_enabled(adev,
>>> +                                     
>>> &adev->nbio.ras_err_event_athub_irq, 0);
>>> +
>>> +             if (adev->nbio.ras && irq_release &&
>>>                    adev->nbio.ras->init_ras_err_event_athub_interrupt)
>>>                        amdgpu_irq_put(adev, 
>>> &adev->nbio.ras_err_event_athub_irq, 0);
>>>        }
> 

Re: [PATCH] drm/amdgpu: Update irq disable flow during unload

[Christian König]

Am 08.01.24 um 09:32 schrieb Lazar, Lijo:
> On 1/8/2024 1:51 PM, Christian König wrote:
>> Am 08.01.24 um 09:13 schrieb Kamal, Asad:
>>> [AMD Official Use Only - General]
>>>
>>> Hi Christian,
>>>
>>> Thank you for the comment.
>>>
>>> This is not normal reset, it is reset done during unload for smu 
>>> v_13_0_2.
>>
>> Yeah, but this doesn't explain the rational for this.
>>
>> IRQ enable/disable should be balanced in hw_init()/hw_fini(), 
>> independent of what else you do.
>>
>> I'm not sure what you are trying to solve but this here is a complete 
>> no-go.
>>
>
> This is a special reset done during module unload by this commit - 
> f5c7e7797060 ("drm/amdgpu: Adjust removal control flow for smu
> v13_0_2"). Without this commit, it seems driver reload doesnt' work.
>
> In this particular case, a the reset is done during unload and only 
> resume sequence of only select IPs are done (part of the workaround in 
> the patch). For those IPs, irqs are enabled during 
> late_init/ras_late_init, and not during hw_init(), that part gets 
> skipped.

Please revert that immediately, this whole approach is completely broken 
as far as I can see.

>
> The module unload sequence causes a WARN trace during irq_put of those 
> IPs during hw_fini(). Those are mix of generic irqs and ras irqs, so 
> there is no clean way to untangle it.

This is WARN is just the tip of the iceberg here, the problem is that 
you are not supposed to call amdgpu_device_ip_resume_phase1() as you do 
in f5c7e7797060.

Please sync up with Alex how to do this cleanly.

Regards,
Christian.

>
> One thing that could be done is to add an extra check for 13.0.2 
> version to make it clear that this workaround is done for only for 
> those ASICs.
>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks & Regards
>>> Asad
>>>
>>> -----Original Message-----
>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>> Sent: Monday, January 8, 2024 1:33 PM
>>> To: Kamal, Asad <Asad.Kamal@amd.com>
>>> Subject: Re: [PATCH] drm/amdgpu: Update irq disable flow during unload
>>>
>>> Am 05.01.24 um 16:21 schrieb Asad Kamal:
>>>> In certain special cases, e.g device reset before module unload, irq
>>>> gets disabled as part of reset sequence and won't get enabled back.
>>>> Add special check to cover such scenarios
>>> Well complete NAK to that. Resets shouldn't affect the IRQ state at 
>>> all!
>>>
>>> If this is an issue then something else is broken.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Asad Kamal <asad.kamal@amd.com>
>>>> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 12 ++++++++++--
>>>>    drivers/gpu/drm/amd/amdgpu/soc15.c    | 13 +++++++++++--
>>>>    2 files changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> index 372de9f1ce59..a4e1b9a58679 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> @@ -2361,6 +2361,7 @@ static void gmc_v9_0_gart_disable(struct 
>>>> amdgpu_device *adev)
>>>>    static int gmc_v9_0_hw_fini(void *handle)
>>>>    {
>>>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>> +     bool irq_release = true;
>>>>
>>>>        gmc_v9_0_gart_disable(adev);
>>>>
>>>> @@ -2378,9 +2379,16 @@ static int gmc_v9_0_hw_fini(void *handle)
>>>>        if (adev->mmhub.funcs->update_power_gating)
>>>> adev->mmhub.funcs->update_power_gating(adev, false);
>>>>
>>>> -     amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
>>>> +     if (adev->shutdown)
>>>> +             irq_release = amdgpu_irq_enabled(adev, 
>>>> &adev->gmc.vm_fault, 0);
>>>>
>>>> -     if (adev->gmc.ecc_irq.funcs &&
>>>> +     if (irq_release)
>>>> +             amdgpu_irq_put(adev, &adev->gmc.vm_fault, 0);
>>>> +
>>>> +     if (adev->shutdown)
>>>> +             irq_release = amdgpu_irq_enabled(adev, 
>>>> &adev->gmc.ecc_irq, 0);
>>>> +
>>>> +     if (adev->gmc.ecc_irq.funcs && irq_release &&
>>>>                amdgpu_ras_is_supported(adev, AMDGPU_RAS_BLOCK__UMC))
>>>>                amdgpu_irq_put(adev, &adev->gmc.ecc_irq, 0);
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> index 15033efec2ba..7ee835049d57 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
>>>> @@ -1266,6 +1266,7 @@ static int soc15_common_hw_init(void *handle)
>>>>    static int soc15_common_hw_fini(void *handle)
>>>>    {
>>>>        struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>> +     bool irq_release = true;
>>>>
>>>>        /* Disable the doorbell aperture and selfring doorbell aperture
>>>>         * separately in hw_fini because 
>>>> soc15_enable_doorbell_aperture @@
>>>> -1280,10 +1281,18 @@ static int soc15_common_hw_fini(void *handle)
>>>>
>>>>        if (adev->nbio.ras_if &&
>>>>            amdgpu_ras_is_supported(adev, adev->nbio.ras_if->block)) {
>>>> -             if (adev->nbio.ras &&
>>>> +             if (adev->shutdown)
>>>> +                     irq_release = amdgpu_irq_enabled(adev,
>>>> +&adev->nbio.ras_controller_irq, 0);
>>>> +
>>>> +             if (adev->nbio.ras && irq_release &&
>>>> adev->nbio.ras->init_ras_controller_interrupt)
>>>>                        amdgpu_irq_put(adev, 
>>>> &adev->nbio.ras_controller_irq, 0);
>>>> -             if (adev->nbio.ras &&
>>>> +
>>>> +             if (adev->shutdown)
>>>> +                     irq_release = amdgpu_irq_enabled(adev,
>>>> + &adev->nbio.ras_err_event_athub_irq, 0);
>>>> +
>>>> +             if (adev->nbio.ras && irq_release &&
>>>> adev->nbio.ras->init_ras_err_event_athub_interrupt)
>>>>                        amdgpu_irq_put(adev, 
>>>> &adev->nbio.ras_err_event_athub_irq, 0);
>>>>        }
>>
>