[PATCH libdrm] amdgpu: add mmhub ras inject unit test

[PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Guchun Chen]

Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
Signed-off-by: Guchun Chen <guchun.chen@amd.com>
---
 data/amdgpu_ras.json | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json
index 26fd9465..484f12f2 100644
--- a/data/amdgpu_ras.json
+++ b/data/amdgpu_ras.json
@@ -121,6 +121,9 @@
                 "utc_atcl2_cache_4k_bank": 111
             }
         },
+        "mmhub": {
+            "index": 3
+        },
     },
     "type": {
         "parity": 1,
@@ -263,5 +266,12 @@
             "address": 0,
             "value": 0
         },
+        {
+            "name": "ras_mmhub.1.0",
+            "block": "mmhub",
+            "type": "single_correctable",
+            "address": 0,
+            "value": 0
+        },
     ]
 }
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Christian König]

Well this doesn't look like C to me. Did we added a configuration file 
for the ras unit tests or something like that?

Christian.

Am 16.08.19 um 13:04 schrieb Guchun Chen:
> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
> ---
>   data/amdgpu_ras.json | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json
> index 26fd9465..484f12f2 100644
> --- a/data/amdgpu_ras.json
> +++ b/data/amdgpu_ras.json
> @@ -121,6 +121,9 @@
>                   "utc_atcl2_cache_4k_bank": 111
>               }
>           },
> +        "mmhub": {
> +            "index": 3
> +        },
>       },
>       "type": {
>           "parity": 1,
> @@ -263,5 +266,12 @@
>               "address": 0,
>               "value": 0
>           },
> +        {
> +            "name": "ras_mmhub.1.0",
> +            "block": "mmhub",
> +            "type": "single_correctable",
> +            "address": 0,
> +            "value": 0
> +        },
>       ]
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Chen, Guchun]

Hi Christian,

Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.

Regards,
Guchun

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Friday, August 16, 2019 7:12 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
Cc: Li, Candice <Candice.Li@amd.com>
Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?

Christian.

Am 16.08.19 um 13:04 schrieb Guchun Chen:
> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
> ---
>   data/amdgpu_ras.json | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index 
> 26fd9465..484f12f2 100644
> --- a/data/amdgpu_ras.json
> +++ b/data/amdgpu_ras.json
> @@ -121,6 +121,9 @@
>                   "utc_atcl2_cache_4k_bank": 111
>               }
>           },
> +        "mmhub": {
> +            "index": 3
> +        },
>       },
>       "type": {
>           "parity": 1,
> @@ -263,5 +266,12 @@
>               "address": 0,
>               "value": 0
>           },
> +        {
> +            "name": "ras_mmhub.1.0",
> +            "block": "mmhub",
> +            "type": "single_correctable",
> +            "address": 0,
> +            "value": 0
> +        },
>       ]
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Koenig, Christian]

Hi Guchun,

in this case this is a bit awkward implemented.

See the files in the data directory are for installation together with 
the libdrm library and NOT for the unit tests. Please move the file to 
tests/amdgpu instead.

I would also re-consider this approach since we intentionally use the 
CUnit framework to avoid dependencies on external libraries like json 
and external files.

We should probably better configure the tests directly in the C code so 
that the unit test stays self containing.

Regards,
Christian.

Am 19.08.19 um 05:16 schrieb Chen, Guchun:
> Hi Christian,
>
> Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
> This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Friday, August 16, 2019 7:12 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
> Cc: Li, Candice <Candice.Li@amd.com>
> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>
> Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?
>
> Christian.
>
> Am 16.08.19 um 13:04 schrieb Guchun Chen:
>> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>> ---
>>    data/amdgpu_ras.json | 10 ++++++++++
>>    1 file changed, 10 insertions(+)
>>
>> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index
>> 26fd9465..484f12f2 100644
>> --- a/data/amdgpu_ras.json
>> +++ b/data/amdgpu_ras.json
>> @@ -121,6 +121,9 @@
>>                    "utc_atcl2_cache_4k_bank": 111
>>                }
>>            },
>> +        "mmhub": {
>> +            "index": 3
>> +        },
>>        },
>>        "type": {
>>            "parity": 1,
>> @@ -263,5 +266,12 @@
>>                "address": 0,
>>                "value": 0
>>            },
>> +        {
>> +            "name": "ras_mmhub.1.0",
>> +            "block": "mmhub",
>> +            "type": "single_correctable",
>> +            "address": 0,
>> +            "value": 0
>> +        },
>>        ]
>>    }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Chen, Guchun]

Hi Christian,

Thanks for your suggestion.
Regarding the configuration file moving to test/amdgpu, I am fine with this.

But reg putting all the tests in C code, though it looks like the unit test can stay self containing, but without one independent configuration file, user will find it's inconvenient when performing the tests.
For example, if we move all RAS inject tests into C code, however, if the user only cares the RAS unit test in certain module/submodule test, he/she would not like to see inject tests in all modules run after launching amdgpu_test, so it's not friendly to user.
Anyway, your suggestion still makes sense, as amdgpu_test should get rid of other external packages except CUnit framework.

So we still the balance between above two cases:
1. Make amdgpu_test self containing without dependency on other external packages.
2. Allow user can configure the tests when running amdgpu_test.

Then the possible solutions I can illustrate are:
1. Still keep one configuration file, but not in json format. It’s a normal configuration file, we will add code in amdgpu_test to parse the configuration file, and remove all json APIs.
2. Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.

I am opened to above two solutions or other missed ones.

Regards,
Guchun

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com> 
Sent: Monday, August 19, 2019 3:14 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
Cc: Li, Candice <Candice.Li@amd.com>
Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

Hi Guchun,

in this case this is a bit awkward implemented.

See the files in the data directory are for installation together with the libdrm library and NOT for the unit tests. Please move the file to tests/amdgpu instead.

I would also re-consider this approach since we intentionally use the CUnit framework to avoid dependencies on external libraries like json and external files.

We should probably better configure the tests directly in the C code so that the unit test stays self containing.

Regards,
Christian.

Am 19.08.19 um 05:16 schrieb Chen, Guchun:
> Hi Christian,
>
> Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
> This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Friday, August 16, 2019 7:12 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; 
> Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis 
> <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao 
> <Tao.Zhou1@amd.com>
> Cc: Li, Candice <Candice.Li@amd.com>
> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>
> Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?
>
> Christian.
>
> Am 16.08.19 um 13:04 schrieb Guchun Chen:
>> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>> ---
>>    data/amdgpu_ras.json | 10 ++++++++++
>>    1 file changed, 10 insertions(+)
>>
>> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index
>> 26fd9465..484f12f2 100644
>> --- a/data/amdgpu_ras.json
>> +++ b/data/amdgpu_ras.json
>> @@ -121,6 +121,9 @@
>>                    "utc_atcl2_cache_4k_bank": 111
>>                }
>>            },
>> +        "mmhub": {
>> +            "index": 3
>> +        },
>>        },
>>        "type": {
>>            "parity": 1,
>> @@ -263,5 +266,12 @@
>>                "address": 0,
>>                "value": 0
>>            },
>> +        {
>> +            "name": "ras_mmhub.1.0",
>> +            "block": "mmhub",
>> +            "type": "single_correctable",
>> +            "address": 0,
>> +            "value": 0
>> +        },
>>        ]
>>    }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Koenig, Christian]

Hi Guchun,

> Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
Ah! Sounds like we have just a misunderstanding here. As far as I know 
we already have the functionality to select only specific tests to run 
from the command line of amdgpu_test.

That's actually the background reason why we use the CUnit framework.

I haven't used that functionality in a long time, could be that it is 
broken or something. But at least in theory it should be there.

Regards,
Christian.

Am 19.08.19 um 09:48 schrieb Chen, Guchun:
> Hi Christian,
>
> Thanks for your suggestion.
> Regarding the configuration file moving to test/amdgpu, I am fine with this.
>
> But reg putting all the tests in C code, though it looks like the unit test can stay self containing, but without one independent configuration file, user will find it's inconvenient when performing the tests.
> For example, if we move all RAS inject tests into C code, however, if the user only cares the RAS unit test in certain module/submodule test, he/she would not like to see inject tests in all modules run after launching amdgpu_test, so it's not friendly to user.
> Anyway, your suggestion still makes sense, as amdgpu_test should get rid of other external packages except CUnit framework.
>
> So we still the balance between above two cases:
> 1. Make amdgpu_test self containing without dependency on other external packages.
> 2. Allow user can configure the tests when running amdgpu_test.
>
> Then the possible solutions I can illustrate are:
> 1. Still keep one configuration file, but not in json format. It’s a normal configuration file, we will add code in amdgpu_test to parse the configuration file, and remove all json APIs.
> 2. Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
>
> I am opened to above two solutions or other missed ones.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, August 19, 2019 3:14 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
> Cc: Li, Candice <Candice.Li@amd.com>
> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>
> Hi Guchun,
>
> in this case this is a bit awkward implemented.
>
> See the files in the data directory are for installation together with the libdrm library and NOT for the unit tests. Please move the file to tests/amdgpu instead.
>
> I would also re-consider this approach since we intentionally use the CUnit framework to avoid dependencies on external libraries like json and external files.
>
> We should probably better configure the tests directly in the C code so that the unit test stays self containing.
>
> Regards,
> Christian.
>
> Am 19.08.19 um 05:16 schrieb Chen, Guchun:
>> Hi Christian,
>>
>> Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
>> This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.
>>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Friday, August 16, 2019 7:12 PM
>> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org;
>> Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis
>> <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao
>> <Tao.Zhou1@amd.com>
>> Cc: Li, Candice <Candice.Li@amd.com>
>> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>>
>> Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?
>>
>> Christian.
>>
>> Am 16.08.19 um 13:04 schrieb Guchun Chen:
>>> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
>>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>>> ---
>>>     data/amdgpu_ras.json | 10 ++++++++++
>>>     1 file changed, 10 insertions(+)
>>>
>>> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index
>>> 26fd9465..484f12f2 100644
>>> --- a/data/amdgpu_ras.json
>>> +++ b/data/amdgpu_ras.json
>>> @@ -121,6 +121,9 @@
>>>                     "utc_atcl2_cache_4k_bank": 111
>>>                 }
>>>             },
>>> +        "mmhub": {
>>> +            "index": 3
>>> +        },
>>>         },
>>>         "type": {
>>>             "parity": 1,
>>> @@ -263,5 +266,12 @@
>>>                 "address": 0,
>>>                 "value": 0
>>>             },
>>> +        {
>>> +            "name": "ras_mmhub.1.0",
>>> +            "block": "mmhub",
>>> +            "type": "single_correctable",
>>> +            "address": 0,
>>> +            "value": 0
>>> +        },
>>>         ]
>>>     }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Chen, Guchun]

Hi Christian,

Yes, we already have functionality to select one specific test from the command line, but that command line is limited.

It only allows user to select one general test, like ras test, but inside ras test, it has the enable/disable/error inject test respectively. Now what we are talking about is exactly the inject test. We support plenty of ras inject test in submodules like GFX, UMC, MMHUB. So without a command extension, when we run RAS error inject test, we will see all inject tests in different sub IP blocks are performed. My previous concern is the experience is not good if user wants to run one special ras inject test in sub block like UMC. However, I had one offline discussion with Hawking just now. We are now fine to maintain the test configuration in C code for upstream. This will keep amdgpu_test not depending on other external libraries.

Regards,
Guchun

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com> 
Sent: Monday, August 19, 2019 3:53 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
Cc: Li, Candice <Candice.Li@amd.com>
Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

Hi Guchun,

> Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
Ah! Sounds like we have just a misunderstanding here. As far as I know we already have the functionality to select only specific tests to run from the command line of amdgpu_test.

That's actually the background reason why we use the CUnit framework.

I haven't used that functionality in a long time, could be that it is broken or something. But at least in theory it should be there.

Regards,
Christian.

Am 19.08.19 um 09:48 schrieb Chen, Guchun:
> Hi Christian,
>
> Thanks for your suggestion.
> Regarding the configuration file moving to test/amdgpu, I am fine with this.
>
> But reg putting all the tests in C code, though it looks like the unit test can stay self containing, but without one independent configuration file, user will find it's inconvenient when performing the tests.
> For example, if we move all RAS inject tests into C code, however, if the user only cares the RAS unit test in certain module/submodule test, he/she would not like to see inject tests in all modules run after launching amdgpu_test, so it's not friendly to user.
> Anyway, your suggestion still makes sense, as amdgpu_test should get rid of other external packages except CUnit framework.
>
> So we still the balance between above two cases:
> 1. Make amdgpu_test self containing without dependency on other external packages.
> 2. Allow user can configure the tests when running amdgpu_test.
>
> Then the possible solutions I can illustrate are:
> 1. Still keep one configuration file, but not in json format. It’s a normal configuration file, we will add code in amdgpu_test to parse the configuration file, and remove all json APIs.
> 2. Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
>
> I am opened to above two solutions or other missed ones.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, August 19, 2019 3:14 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; 
> Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis 
> <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao 
> <Tao.Zhou1@amd.com>
> Cc: Li, Candice <Candice.Li@amd.com>
> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>
> Hi Guchun,
>
> in this case this is a bit awkward implemented.
>
> See the files in the data directory are for installation together with the libdrm library and NOT for the unit tests. Please move the file to tests/amdgpu instead.
>
> I would also re-consider this approach since we intentionally use the CUnit framework to avoid dependencies on external libraries like json and external files.
>
> We should probably better configure the tests directly in the C code so that the unit test stays self containing.
>
> Regards,
> Christian.
>
> Am 19.08.19 um 05:16 schrieb Chen, Guchun:
>> Hi Christian,
>>
>> Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
>> This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.
>>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Friday, August 16, 2019 7:12 PM
>> To: Chen, Guchun <Guchun.Chen@amd.com>; 
>> amd-gfx@lists.freedesktop.org; Zhang, Hawking 
>> <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora 
>> <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
>> Cc: Li, Candice <Candice.Li@amd.com>
>> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>>
>> Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?
>>
>> Christian.
>>
>> Am 16.08.19 um 13:04 schrieb Guchun Chen:
>>> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
>>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>>> ---
>>>     data/amdgpu_ras.json | 10 ++++++++++
>>>     1 file changed, 10 insertions(+)
>>>
>>> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index
>>> 26fd9465..484f12f2 100644
>>> --- a/data/amdgpu_ras.json
>>> +++ b/data/amdgpu_ras.json
>>> @@ -121,6 +121,9 @@
>>>                     "utc_atcl2_cache_4k_bank": 111
>>>                 }
>>>             },
>>> +        "mmhub": {
>>> +            "index": 3
>>> +        },
>>>         },
>>>         "type": {
>>>             "parity": 1,
>>> @@ -263,5 +266,12 @@
>>>                 "address": 0,
>>>                 "value": 0
>>>             },
>>> +        {
>>> +            "name": "ras_mmhub.1.0",
>>> +            "block": "mmhub",
>>> +            "type": "single_correctable",
>>> +            "address": 0,
>>> +            "value": 0
>>> +        },
>>>         ]
>>>     }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Koenig, Christian]

Hi Guchun,

yeah, I mean the requirement is perfectly valid and yes I agree that we 
should probably extend the command line parameters in this case.

The main problem is not the additional external library requirement, but 
rather that we need an external file additional to amdgpu_test.

See the test binary should be self containing in the sense that you only 
need the binary and nothing else so that we can give it QA or other testers.

Regards,
Christian.

Am 19.08.19 um 10:09 schrieb Chen, Guchun:
> Hi Christian,
>
> Yes, we already have functionality to select one specific test from the command line, but that command line is limited.
>
> It only allows user to select one general test, like ras test, but inside ras test, it has the enable/disable/error inject test respectively. Now what we are talking about is exactly the inject test. We support plenty of ras inject test in submodules like GFX, UMC, MMHUB. So without a command extension, when we run RAS error inject test, we will see all inject tests in different sub IP blocks are performed. My previous concern is the experience is not good if user wants to run one special ras inject test in sub block like UMC. However, I had one offline discussion with Hawking just now. We are now fine to maintain the test configuration in C code for upstream. This will keep amdgpu_test not depending on other external libraries.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, August 19, 2019 3:53 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
> Cc: Li, Candice <Candice.Li@amd.com>
> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>
> Hi Guchun,
>
>> Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
> Ah! Sounds like we have just a misunderstanding here. As far as I know we already have the functionality to select only specific tests to run from the command line of amdgpu_test.
>
> That's actually the background reason why we use the CUnit framework.
>
> I haven't used that functionality in a long time, could be that it is broken or something. But at least in theory it should be there.
>
> Regards,
> Christian.
>
> Am 19.08.19 um 09:48 schrieb Chen, Guchun:
>> Hi Christian,
>>
>> Thanks for your suggestion.
>> Regarding the configuration file moving to test/amdgpu, I am fine with this.
>>
>> But reg putting all the tests in C code, though it looks like the unit test can stay self containing, but without one independent configuration file, user will find it's inconvenient when performing the tests.
>> For example, if we move all RAS inject tests into C code, however, if the user only cares the RAS unit test in certain module/submodule test, he/she would not like to see inject tests in all modules run after launching amdgpu_test, so it's not friendly to user.
>> Anyway, your suggestion still makes sense, as amdgpu_test should get rid of other external packages except CUnit framework.
>>
>> So we still the balance between above two cases:
>> 1. Make amdgpu_test self containing without dependency on other external packages.
>> 2. Allow user can configure the tests when running amdgpu_test.
>>
>> Then the possible solutions I can illustrate are:
>> 1. Still keep one configuration file, but not in json format. It’s a normal configuration file, we will add code in amdgpu_test to parse the configuration file, and remove all json APIs.
>> 2. Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
>>
>> I am opened to above two solutions or other missed ones.
>>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Monday, August 19, 2019 3:14 PM
>> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org;
>> Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis
>> <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao
>> <Tao.Zhou1@amd.com>
>> Cc: Li, Candice <Candice.Li@amd.com>
>> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>>
>> Hi Guchun,
>>
>> in this case this is a bit awkward implemented.
>>
>> See the files in the data directory are for installation together with the libdrm library and NOT for the unit tests. Please move the file to tests/amdgpu instead.
>>
>> I would also re-consider this approach since we intentionally use the CUnit framework to avoid dependencies on external libraries like json and external files.
>>
>> We should probably better configure the tests directly in the C code so that the unit test stays self containing.
>>
>> Regards,
>> Christian.
>>
>> Am 19.08.19 um 05:16 schrieb Chen, Guchun:
>>> Hi Christian,
>>>
>>> Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
>>> This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.
>>>
>>> Regards,
>>> Guchun
>>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Friday, August 16, 2019 7:12 PM
>>> To: Chen, Guchun <Guchun.Chen@amd.com>;
>>> amd-gfx@lists.freedesktop.org; Zhang, Hawking
>>> <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora
>>> <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
>>> Cc: Li, Candice <Candice.Li@amd.com>
>>> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>>>
>>> Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?
>>>
>>> Christian.
>>>
>>> Am 16.08.19 um 13:04 schrieb Guchun Chen:
>>>> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
>>>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>>>> ---
>>>>      data/amdgpu_ras.json | 10 ++++++++++
>>>>      1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index
>>>> 26fd9465..484f12f2 100644
>>>> --- a/data/amdgpu_ras.json
>>>> +++ b/data/amdgpu_ras.json
>>>> @@ -121,6 +121,9 @@
>>>>                      "utc_atcl2_cache_4k_bank": 111
>>>>                  }
>>>>              },
>>>> +        "mmhub": {
>>>> +            "index": 3
>>>> +        },
>>>>          },
>>>>          "type": {
>>>>              "parity": 1,
>>>> @@ -263,5 +266,12 @@
>>>>                  "address": 0,
>>>>                  "value": 0
>>>>              },
>>>> +        {
>>>> +            "name": "ras_mmhub.1.0",
>>>> +            "block": "mmhub",
>>>> +            "type": "single_correctable",
>>>> +            "address": 0,
>>>> +            "value": 0
>>>> +        },
>>>>          ]
>>>>      }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

[Chen, Guchun]

Hi Christian,

Appreciate your concern and suggestion!
I am fine to remove the external file and move all test configurations into C code.
I will prepare patches to address this later.

Regards,
Guchun

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com> 
Sent: Monday, August 19, 2019 4:42 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
Cc: Li, Candice <Candice.Li@amd.com>
Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test

Hi Guchun,

yeah, I mean the requirement is perfectly valid and yes I agree that we should probably extend the command line parameters in this case.

The main problem is not the additional external library requirement, but rather that we need an external file additional to amdgpu_test.

See the test binary should be self containing in the sense that you only need the binary and nothing else so that we can give it QA or other testers.

Regards,
Christian.

Am 19.08.19 um 10:09 schrieb Chen, Guchun:
> Hi Christian,
>
> Yes, we already have functionality to select one specific test from the command line, but that command line is limited.
>
> It only allows user to select one general test, like ras test, but inside ras test, it has the enable/disable/error inject test respectively. Now what we are talking about is exactly the inject test. We support plenty of ras inject test in submodules like GFX, UMC, MMHUB. So without a command extension, when we run RAS error inject test, we will see all inject tests in different sub IP blocks are performed. My previous concern is the experience is not good if user wants to run one special ras inject test in sub block like UMC. However, I had one offline discussion with Hawking just now. We are now fine to maintain the test configuration in C code for upstream. This will keep amdgpu_test not depending on other external libraries.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, August 19, 2019 3:53 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org; 
> Zhang, Hawking <Hawking.Zhang@amd.com>; Li, Dennis 
> <Dennis.Li@amd.com>; Cui, Flora <Flora.Cui@amd.com>; Zhou1, Tao 
> <Tao.Zhou1@amd.com>
> Cc: Li, Candice <Candice.Li@amd.com>
> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>
> Hi Guchun,
>
>> Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
> Ah! Sounds like we have just a misunderstanding here. As far as I know we already have the functionality to select only specific tests to run from the command line of amdgpu_test.
>
> That's actually the background reason why we use the CUnit framework.
>
> I haven't used that functionality in a long time, could be that it is broken or something. But at least in theory it should be there.
>
> Regards,
> Christian.
>
> Am 19.08.19 um 09:48 schrieb Chen, Guchun:
>> Hi Christian,
>>
>> Thanks for your suggestion.
>> Regarding the configuration file moving to test/amdgpu, I am fine with this.
>>
>> But reg putting all the tests in C code, though it looks like the unit test can stay self containing, but without one independent configuration file, user will find it's inconvenient when performing the tests.
>> For example, if we move all RAS inject tests into C code, however, if the user only cares the RAS unit test in certain module/submodule test, he/she would not like to see inject tests in all modules run after launching amdgpu_test, so it's not friendly to user.
>> Anyway, your suggestion still makes sense, as amdgpu_test should get rid of other external packages except CUnit framework.
>>
>> So we still the balance between above two cases:
>> 1. Make amdgpu_test self containing without dependency on other external packages.
>> 2. Allow user can configure the tests when running amdgpu_test.
>>
>> Then the possible solutions I can illustrate are:
>> 1. Still keep one configuration file, but not in json format. It’s a normal configuration file, we will add code in amdgpu_test to parse the configuration file, and remove all json APIs.
>> 2. Put all tests in c code, and extend the args when running amdgpu_test, which will allow user can run special RAS test.
>>
>> I am opened to above two solutions or other missed ones.
>>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Monday, August 19, 2019 3:14 PM
>> To: Chen, Guchun <Guchun.Chen@amd.com>; 
>> amd-gfx@lists.freedesktop.org; Zhang, Hawking 
>> <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora 
>> <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
>> Cc: Li, Candice <Candice.Li@amd.com>
>> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>>
>> Hi Guchun,
>>
>> in this case this is a bit awkward implemented.
>>
>> See the files in the data directory are for installation together with the libdrm library and NOT for the unit tests. Please move the file to tests/amdgpu instead.
>>
>> I would also re-consider this approach since we intentionally use the CUnit framework to avoid dependencies on external libraries like json and external files.
>>
>> We should probably better configure the tests directly in the C code so that the unit test stays self containing.
>>
>> Regards,
>> Christian.
>>
>> Am 19.08.19 um 05:16 schrieb Chen, Guchun:
>>> Hi Christian,
>>>
>>> Yes, we added one configuration file named "amdgpu_ras.json" for RAS inject unit test to drm master branch.
>>> This unit test will be maintained to illustrate all the RAS tests we absolutely support in IP modules/submodules.
>>>
>>> Regards,
>>> Guchun
>>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Friday, August 16, 2019 7:12 PM
>>> To: Chen, Guchun <Guchun.Chen@amd.com>; 
>>> amd-gfx@lists.freedesktop.org; Zhang, Hawking 
>>> <Hawking.Zhang@amd.com>; Li, Dennis <Dennis.Li@amd.com>; Cui, Flora 
>>> <Flora.Cui@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>
>>> Cc: Li, Candice <Candice.Li@amd.com>
>>> Subject: Re: [PATCH libdrm] amdgpu: add mmhub ras inject unit test
>>>
>>> Well this doesn't look like C to me. Did we added a configuration file for the ras unit tests or something like that?
>>>
>>> Christian.
>>>
>>> Am 16.08.19 um 13:04 schrieb Guchun Chen:
>>>> Change-Id: Ia76b95162f5f6f419f70b53ef443bceaf2e092e0
>>>> Signed-off-by: Guchun Chen <guchun.chen@amd.com>
>>>> ---
>>>>      data/amdgpu_ras.json | 10 ++++++++++
>>>>      1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/data/amdgpu_ras.json b/data/amdgpu_ras.json index
>>>> 26fd9465..484f12f2 100644
>>>> --- a/data/amdgpu_ras.json
>>>> +++ b/data/amdgpu_ras.json
>>>> @@ -121,6 +121,9 @@
>>>>                      "utc_atcl2_cache_4k_bank": 111
>>>>                  }
>>>>              },
>>>> +        "mmhub": {
>>>> +            "index": 3
>>>> +        },
>>>>          },
>>>>          "type": {
>>>>              "parity": 1,
>>>> @@ -263,5 +266,12 @@
>>>>                  "address": 0,
>>>>                  "value": 0
>>>>              },
>>>> +        {
>>>> +            "name": "ras_mmhub.1.0",
>>>> +            "block": "mmhub",
>>>> +            "type": "single_correctable",
>>>> +            "address": 0,
>>>> +            "value": 0
>>>> +        },
>>>>          ]
>>>>      }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx