* Re: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
2022-03-21 9:38 [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2) Tao Zhou
@ 2022-03-21 10:47 ` Paul Menzel
2022-03-22 2:57 ` Zhou1, Tao
2022-03-21 10:50 ` Zhang, Hawking
2022-03-21 11:21 ` Lazar, Lijo
2 siblings, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2022-03-21 10:47 UTC (permalink / raw)
To: Tao Zhou
Cc: Felix.Kuehling, yipeng.chai, stanley.yang, amd-gfx, hawking.zhang
Dear Tao,
Thank you for the patch.
Am 21.03.22 um 10:38 schrieb Tao Zhou:
> Print the status out when it passes, and also tell user gpu reset
> is triggered when we fallback to legacy way.
>
> v2: make the message more explicitly.
>
> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> index 56902b5bb7b6..32c451f21db7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> @@ -105,8 +105,6 @@ static void event_interrupt_poison_consumption(struct kfd_dev *dev,
> if (old_poison)
> return;
>
> - pr_warn("RAS poison consumption handling: client id %d\n", client_id);
> -
> switch (client_id) {
> case SOC15_IH_CLIENTID_SE0SH:
> case SOC15_IH_CLIENTID_SE1SH:
> @@ -130,10 +128,15 @@ static void event_interrupt_poison_consumption(struct kfd_dev *dev,
> /* resetting queue passes, do page retirement without gpu reset
> * resetting queue fails, fallback to gpu reset solution
> */
> - if (!ret)
> + if (!ret) {
> + pr_warn("RAS poison consumption, unmap queue flow succeeds: client id %d\n",
> + client_id);
succeeded? As it’s a success message, should it be an informational message?
> amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev, false);
> - else
> + } else {
> + pr_warn("RAS poison consumption, fallback to gpu reset flow: client id %d\n",
Fall back.
> + client_id);
> amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev, true);
Could the log be moved somehow to the handler?
> + }
> }
>
> static bool event_interrupt_isr_v9(struct kfd_dev *dev,
Unrelated to the patch, at least I as user, would wish these warnings to
be more elaborate, telling me, what the problem is, what effects it has,
and what to do to fix it.
Kind regards,
Paul
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
2022-03-21 10:47 ` Paul Menzel
@ 2022-03-22 2:57 ` Zhou1, Tao
0 siblings, 0 replies; 7+ messages in thread
From: Zhou1, Tao @ 2022-03-22 2:57 UTC (permalink / raw)
To: Paul Menzel
Cc: Kuehling, Felix, Chai, Thomas, Yang, Stanley, amd-gfx, Zhang, Hawking
[AMD Official Use Only]
> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Monday, March 21, 2022 6:47 PM
> To: Zhou1, Tao <Tao.Zhou1@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> <Hawking.Zhang@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Yang,
> Stanley <Stanley.Yang@amd.com>; Chai, Thomas <YiPeng.Chai@amd.com>
> Subject: Re: [PATCH] drm/amdkfd: print unmap queue status for RAS poison
> consumption (v2)
>
> Dear Tao,
>
>
> Thank you for the patch.
>
>
> Am 21.03.22 um 10:38 schrieb Tao Zhou:
> > Print the status out when it passes, and also tell user gpu reset is
> > triggered when we fallback to legacy way.
> >
> > v2: make the message more explicitly.
> >
> > Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > index 56902b5bb7b6..32c451f21db7 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > @@ -105,8 +105,6 @@ static void
> event_interrupt_poison_consumption(struct kfd_dev *dev,
> > if (old_poison)
> > return;
> >
> > - pr_warn("RAS poison consumption handling: client id %d\n", client_id);
> > -
> > switch (client_id) {
> > case SOC15_IH_CLIENTID_SE0SH:
> > case SOC15_IH_CLIENTID_SE1SH:
> > @@ -130,10 +128,15 @@ static void
> event_interrupt_poison_consumption(struct kfd_dev *dev,
> > /* resetting queue passes, do page retirement without gpu reset
> > * resetting queue fails, fallback to gpu reset solution
> > */
> > - if (!ret)
> > + if (!ret) {
> > + pr_warn("RAS poison consumption, unmap queue flow succeeds:
> client id %d\n",
> > + client_id);
>
> succeeded? As it’s a success message, should it be an informational message?
[Tao] thanks, will change to use succeeded before push. Although it reports success, poison consumption is not a usual event.
>
> > amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev,
> false);
> > - else
> > + } else {
> > + pr_warn("RAS poison consumption, fallback to gpu reset flow:
> client
> > +id %d\n",
>
> Fall back.
>
> > + client_id);
> > amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev,
> true);
>
> Could the log be moved somehow to the handler?
[Tao] Could not. Unmap queue isn’t called in the handler and client_id isn't transferred to the handler.
>
> > + }
> > }
> >
> > static bool event_interrupt_isr_v9(struct kfd_dev *dev,
>
> Unrelated to the patch, at least I as user, would wish these warnings to be more
> elaborate, telling me, what the problem is, what effects it has, and what to do
> to fix it.
[Tao] It's difficult. You need a document instead of dmesg log to tell you all the details.
>
>
> Kind regards,
>
> Paul
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
2022-03-21 9:38 [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2) Tao Zhou
2022-03-21 10:47 ` Paul Menzel
@ 2022-03-21 10:50 ` Zhang, Hawking
2022-03-21 11:21 ` Lazar, Lijo
2 siblings, 0 replies; 7+ messages in thread
From: Zhang, Hawking @ 2022-03-21 10:50 UTC (permalink / raw)
To: Zhou1, Tao, amd-gfx, Kuehling, Felix, Yang, Stanley, Chai, Thomas
[AMD Official Use Only]
Reviewed-by: Hawking Zhang <Hawking.Zhang@amd.com>
Regards,
Hawking
-----Original Message-----
From: Zhou1, Tao <Tao.Zhou1@amd.com>
Sent: Monday, March 21, 2022 17:38
To: amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Yang, Stanley <Stanley.Yang@amd.com>; Chai, Thomas <YiPeng.Chai@amd.com>
Cc: Zhou1, Tao <Tao.Zhou1@amd.com>
Subject: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
Print the status out when it passes, and also tell user gpu reset is triggered when we fallback to legacy way.
v2: make the message more explicitly.
Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
index 56902b5bb7b6..32c451f21db7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
@@ -105,8 +105,6 @@ static void event_interrupt_poison_consumption(struct kfd_dev *dev,
if (old_poison)
return;
- pr_warn("RAS poison consumption handling: client id %d\n", client_id);
-
switch (client_id) {
case SOC15_IH_CLIENTID_SE0SH:
case SOC15_IH_CLIENTID_SE1SH:
@@ -130,10 +128,15 @@ static void event_interrupt_poison_consumption(struct kfd_dev *dev,
/* resetting queue passes, do page retirement without gpu reset
* resetting queue fails, fallback to gpu reset solution
*/
- if (!ret)
+ if (!ret) {
+ pr_warn("RAS poison consumption, unmap queue flow succeeds: client id %d\n",
+ client_id);
amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev, false);
- else
+ } else {
+ pr_warn("RAS poison consumption, fallback to gpu reset flow: client id %d\n",
+ client_id);
amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev, true);
+ }
}
static bool event_interrupt_isr_v9(struct kfd_dev *dev,
--
2.35.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
2022-03-21 9:38 [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2) Tao Zhou
2022-03-21 10:47 ` Paul Menzel
2022-03-21 10:50 ` Zhang, Hawking
@ 2022-03-21 11:21 ` Lazar, Lijo
2022-03-22 3:17 ` Zhou1, Tao
2 siblings, 1 reply; 7+ messages in thread
From: Lazar, Lijo @ 2022-03-21 11:21 UTC (permalink / raw)
To: Tao Zhou, amd-gfx, hawking.zhang, Felix.Kuehling, stanley.yang,
yipeng.chai
On 3/21/2022 3:08 PM, Tao Zhou wrote:
> Print the status out when it passes, and also tell user gpu reset
> is triggered when we fallback to legacy way.
>
> v2: make the message more explicitly.
>
> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> index 56902b5bb7b6..32c451f21db7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> @@ -105,8 +105,6 @@ static void event_interrupt_poison_consumption(struct kfd_dev *dev,
> if (old_poison)
> return;
>
> - pr_warn("RAS poison consumption handling: client id %d\n", client_id);
> -
> switch (client_id) {
> case SOC15_IH_CLIENTID_SE0SH:
> case SOC15_IH_CLIENTID_SE1SH:
> @@ -130,10 +128,15 @@ static void event_interrupt_poison_consumption(struct kfd_dev *dev,
> /* resetting queue passes, do page retirement without gpu reset
> * resetting queue fails, fallback to gpu reset solution
> */
> - if (!ret)
> + if (!ret) {
> + pr_warn("RAS poison consumption, unmap queue flow succeeds: client id %d\n",
> + client_id);
As discussed in another patch, I understand that pr_* is the legacy
usage in the file. But it won't be helpful for this case with multiple
devices. Would suggest to change to dev_info() - the message here and
below seems informational about the handling of this situation rather
than warning of something bad.
Thanks,
Lijo
> amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev, false);
> - else
> + } else {
> + pr_warn("RAS poison consumption, fallback to gpu reset flow: client id %d\n",
> + client_id);
> amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev, true);
> + }
> }
>
> static bool event_interrupt_isr_v9(struct kfd_dev *dev,
>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
2022-03-21 11:21 ` Lazar, Lijo
@ 2022-03-22 3:17 ` Zhou1, Tao
2022-03-22 14:05 ` Felix Kuehling
0 siblings, 1 reply; 7+ messages in thread
From: Zhou1, Tao @ 2022-03-22 3:17 UTC (permalink / raw)
To: Lazar, Lijo, amd-gfx, Zhang, Hawking, Kuehling, Felix, Yang,
Stanley, Chai, Thomas
[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Monday, March 21, 2022 7:21 PM
> To: Zhou1, Tao <Tao.Zhou1@amd.com>; amd-gfx@lists.freedesktop.org; Zhang,
> Hawking <Hawking.Zhang@amd.com>; Kuehling, Felix
> <Felix.Kuehling@amd.com>; Yang, Stanley <Stanley.Yang@amd.com>; Chai,
> Thomas <YiPeng.Chai@amd.com>
> Subject: Re: [PATCH] drm/amdkfd: print unmap queue status for RAS poison
> consumption (v2)
>
>
>
> On 3/21/2022 3:08 PM, Tao Zhou wrote:
> > Print the status out when it passes, and also tell user gpu reset is
> > triggered when we fallback to legacy way.
> >
> > v2: make the message more explicitly.
> >
> > Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > index 56902b5bb7b6..32c451f21db7 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
> > @@ -105,8 +105,6 @@ static void
> event_interrupt_poison_consumption(struct kfd_dev *dev,
> > if (old_poison)
> > return;
> >
> > - pr_warn("RAS poison consumption handling: client id %d\n", client_id);
> > -
> > switch (client_id) {
> > case SOC15_IH_CLIENTID_SE0SH:
> > case SOC15_IH_CLIENTID_SE1SH:
> > @@ -130,10 +128,15 @@ static void
> event_interrupt_poison_consumption(struct kfd_dev *dev,
> > /* resetting queue passes, do page retirement without gpu reset
> > * resetting queue fails, fallback to gpu reset solution
> > */
> > - if (!ret)
> > + if (!ret) {
> > + pr_warn("RAS poison consumption, unmap queue flow succeeds:
> client id %d\n",
> > + client_id);
>
> As discussed in another patch, I understand that pr_* is the legacy usage in the
> file. But it won't be helpful for this case with multiple devices. Would suggest to
> change to dev_info() - the message here and below seems informational about
> the handling of this situation rather than warning of something bad.
>
> Thanks,
> Lijo
[Tao] I'll replace pr_warn with dev_info. I think we need a dedicated cleanup to retire all pr format message in amdgpu.
RAS poison consumption is a special event should be paid attention to, I think a waning is also reasonable.
>
> > amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev,
> false);
> > - else
> > + } else {
> > + pr_warn("RAS poison consumption, fallback to gpu reset flow:
> client id %d\n",
> > + client_id);
> > amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev,
> true);
> > + }
> > }
> >
> > static bool event_interrupt_isr_v9(struct kfd_dev *dev,
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: print unmap queue status for RAS poison consumption (v2)
2022-03-22 3:17 ` Zhou1, Tao
@ 2022-03-22 14:05 ` Felix Kuehling
0 siblings, 0 replies; 7+ messages in thread
From: Felix Kuehling @ 2022-03-22 14:05 UTC (permalink / raw)
To: Zhou1, Tao, Lazar, Lijo, amd-gfx, Zhang, Hawking, Yang, Stanley,
Chai, Thomas
Am 2022-03-21 um 23:17 schrieb Zhou1, Tao:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Monday, March 21, 2022 7:21 PM
>> To: Zhou1, Tao <Tao.Zhou1@amd.com>; amd-gfx@lists.freedesktop.org; Zhang,
>> Hawking <Hawking.Zhang@amd.com>; Kuehling, Felix
>> <Felix.Kuehling@amd.com>; Yang, Stanley <Stanley.Yang@amd.com>; Chai,
>> Thomas <YiPeng.Chai@amd.com>
>> Subject: Re: [PATCH] drm/amdkfd: print unmap queue status for RAS poison
>> consumption (v2)
>>
>>
>>
>> On 3/21/2022 3:08 PM, Tao Zhou wrote:
>>> Print the status out when it passes, and also tell user gpu reset is
>>> triggered when we fallback to legacy way.
>>>
>>> v2: make the message more explicitly.
>>>
>>> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 11 +++++++----
>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
>>> index 56902b5bb7b6..32c451f21db7 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
>>> @@ -105,8 +105,6 @@ static void
>> event_interrupt_poison_consumption(struct kfd_dev *dev,
>>> if (old_poison)
>>> return;
>>>
>>> - pr_warn("RAS poison consumption handling: client id %d\n", client_id);
>>> -
>>> switch (client_id) {
>>> case SOC15_IH_CLIENTID_SE0SH:
>>> case SOC15_IH_CLIENTID_SE1SH:
>>> @@ -130,10 +128,15 @@ static void
>> event_interrupt_poison_consumption(struct kfd_dev *dev,
>>> /* resetting queue passes, do page retirement without gpu reset
>>> * resetting queue fails, fallback to gpu reset solution
>>> */
>>> - if (!ret)
>>> + if (!ret) {
>>> + pr_warn("RAS poison consumption, unmap queue flow succeeds:
>> client id %d\n",
>>> + client_id);
>> As discussed in another patch, I understand that pr_* is the legacy usage in the
>> file. But it won't be helpful for this case with multiple devices. Would suggest to
>> change to dev_info() - the message here and below seems informational about
>> the handling of this situation rather than warning of something bad.
>>
>> Thanks,
>> Lijo
> [Tao] I'll replace pr_warn with dev_info. I think we need a dedicated cleanup to retire all pr format message in amdgpu.
> RAS poison consumption is a special event should be paid attention to, I think a waning is also reasonable.
Or you could make the "unmap success" case a dev_info and the "gpu
reset" case a dev_warn.
Either way, v3 of your patch looks good to me and is
Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
Regards,
Felix
>
>>> amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev,
>> false);
>>> - else
>>> + } else {
>>> + pr_warn("RAS poison consumption, fallback to gpu reset flow:
>> client id %d\n",
>>> + client_id);
>>> amdgpu_amdkfd_ras_poison_consumption_handler(dev->adev,
>> true);
>>> + }
>>> }
>>>
>>> static bool event_interrupt_isr_v9(struct kfd_dev *dev,
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread