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

Mon Aug 19 08:50:59 UTC 2019

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 at amd.com> 
Sent: Monday, August 19, 2019 4:42 PM
To: Chen, Guchun <Guchun.Chen at amd.com>; amd-gfx at lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang at amd.com>; Li, Dennis <Dennis.Li at amd.com>; Cui, Flora <Flora.Cui at amd.com>; Zhou1, Tao <Tao.Zhou1 at amd.com>
Cc: Li, Candice <Candice.Li at 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 at amd.com>
> Sent: Monday, August 19, 2019 3:53 PM
> To: Chen, Guchun <Guchun.Chen at amd.com>; amd-gfx at lists.freedesktop.org; 
> Zhang, Hawking <Hawking.Zhang at amd.com>; Li, Dennis 
> <Dennis.Li at amd.com>; Cui, Flora <Flora.Cui at amd.com>; Zhou1, Tao 
> <Tao.Zhou1 at amd.com>
> Cc: Li, Candice <Candice.Li at 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 at amd.com>
>> Sent: Monday, August 19, 2019 3:14 PM
>> To: Chen, Guchun <Guchun.Chen at amd.com>; 
>> amd-gfx at lists.freedesktop.org; Zhang, Hawking 
>> <Hawking.Zhang at amd.com>; Li, Dennis <Dennis.Li at amd.com>; Cui, Flora 
>> <Flora.Cui at amd.com>; Zhou1, Tao <Tao.Zhou1 at amd.com>
>> Cc: Li, Candice <Candice.Li at 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 at gmail.com>
>>> Sent: Friday, August 16, 2019 7:12 PM
>>> To: Chen, Guchun <Guchun.Chen at amd.com>; 
>>> amd-gfx at lists.freedesktop.org; Zhang, Hawking 
>>> <Hawking.Zhang at amd.com>; Li, Dennis <Dennis.Li at amd.com>; Cui, Flora 
>>> <Flora.Cui at amd.com>; Zhou1, Tao <Tao.Zhou1 at amd.com>
>>> Cc: Li, Candice <Candice.Li at 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 at 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
>>>> +        },
>>>>          ]
>>>>      }