[PATCH 4/4] tests/amdgpu: adapt to new polaris10/11 uvd fw

[Emil Velikov]

On 14 May 2016 at 19:29, Christian König <deathsimple at vodafone.de> wrote:
> Am 14.05.2016 um 16:19 schrieb Emil Velikov:
>>
>> Hi all,
>>
>> On 13 May 2016 at 17:48, Alex Deucher <alexdeucher at gmail.com> wrote:
>>>
>>> From: Sonny Jiang <sonny.jiang at amd.com>
>>>
>>> Signed-off-by: Sonny Jiang <sonny.jiang at amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>>   tests/amdgpu/cs_tests.c | 48
>>> ++++++++++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 42 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tests/amdgpu/cs_tests.c b/tests/amdgpu/cs_tests.c
>>> index c6930c0..a01ee48 100644
>>> --- a/tests/amdgpu/cs_tests.c
>>> +++ b/tests/amdgpu/cs_tests.c
>>> @@ -43,6 +43,8 @@ static amdgpu_device_handle device_handle;
>>>   static uint32_t major_version;
>>>   static uint32_t minor_version;
>>>   static uint32_t family_id;
>>> +static uint32_t chip_rev;
>>> +static uint32_t chip_id;
>>>
>>>   static amdgpu_context_handle context_handle;
>>>   static amdgpu_bo_handle ib_handle;
>>> @@ -78,6 +80,9 @@ int suite_cs_tests_init(void)
>>>                  return CUE_SINIT_FAILED;
>>>
>>>          family_id = device_handle->info.family_id;
>>> +       /* VI asic POLARIS10/11 have specific external_rev_id */
>>> +       chip_rev = device_handle->info.chip_rev;
>>> +       chip_id = device_handle->info.chip_external_rev;
>>>
>>>          r = amdgpu_cs_ctx_create(device_handle, &context_handle);
>>>          if (r)
>>> @@ -200,8 +205,17 @@ static void amdgpu_cs_uvd_create(void)
>>>          CU_ASSERT_EQUAL(r, 0);
>>>
>>>          memcpy(msg, uvd_create_msg, sizeof(uvd_create_msg));
>>> -       if (family_id >= AMDGPU_FAMILY_VI)
>>> +       if (family_id >= AMDGPU_FAMILY_VI) {
>>>                  ((uint8_t*)msg)[0x10] = 7;
>>> +               /* chip polaris 10/11 */
>>> +               if (chip_id == chip_rev+0x50 || chip_id == chip_rev+0x5A)
>>> {
>>> +                       /* dpb size */
>>> +                       ((uint8_t*)msg)[0x28] = 0x00;
>>> +                       ((uint8_t*)msg)[0x29] = 0x94;
>>> +                       ((uint8_t*)msg)[0x2A] = 0x6B;
>>> +                       ((uint8_t*)msg)[0x2B] = 0x00;
>>
>> I realise that many of the UVD stuff is 'top secret', although one
>> should really try and give symbolic names for magic numbers. With them
>> it's be easier and less error prone when/if the above value changes.
>
>
> Actually we have exposed mostly everything in the UVD headers in mesa and
> those binary values here are just captured example streams.
>
> Saying so I would also prefer that we don't patch the messages on the fly as
> necessary, but rather have a full copy for each chipset family they differ.
> On the other hand it's just the unit tests.
>
Just saw copy copy/paste job so I'd suggested unification. At the end
it's up-to you guys to decide for/against and (if so) do it.

Do you have any information on the dpb_size comment below ?

> Regards,
> Christian.
>
>>
>>> +               }
>>> +       }
>>>
>>>          r = amdgpu_bo_cpu_unmap(buf_handle);
>>>          CU_ASSERT_EQUAL(r, 0);
>>> @@ -230,8 +244,8 @@ static void amdgpu_cs_uvd_create(void)
>>>
>>>   static void amdgpu_cs_uvd_decode(void)
>>>   {
>>> -       const unsigned dpb_size = 15923584, dt_size = 737280;
>>> -       uint64_t msg_addr, fb_addr, bs_addr, dpb_addr, dt_addr, it_addr;
>>> +       const unsigned dpb_size = 15923584, ctx_size = 5287680, dt_size =
>>> 737280;
>>> +       uint64_t msg_addr, fb_addr, bs_addr, dpb_addr, ctx_addr, dt_addr,
>>> it_addr;
>>>          struct amdgpu_bo_alloc_request req = {0};
>>>          amdgpu_bo_handle buf_handle;
>>>          amdgpu_va_handle va_handle;
>>> @@ -269,8 +283,21 @@ static void amdgpu_cs_uvd_decode(void)
>>>          memcpy(ptr, uvd_decode_msg, sizeof(uvd_create_msg));
>>>          if (family_id >= AMDGPU_FAMILY_VI) {
>>>                  ptr[0x10] = 7;
>>> -               ptr[0x98] = 0xb0;
>>> -               ptr[0x99] = 0x1;
>>> +               ptr[0x98] = 0x00;
>>> +               ptr[0x99] = 0x02;
>>> +               /* chip polaris10/11 */
>>> +               if (chip_id == chip_rev+0x50 || chip_id == chip_rev+0x5A)
>>> {
>>> +                       /*dpb size */
>>> +                       ptr[0x24] = 0x00;
>>> +                       ptr[0x25] = 0x94;
>>> +                       ptr[0x26] = 0x6B;
>>> +                       ptr[0x27] = 0x00;
>>
>> Based on the const dpb_size a few lines above... this value is
>> incorrect. So either the comment is off, or one/both of the values ?
>>
Namely this one ?

Thanks
Emil