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

["Christian König" <deathsimple@vodafone.de>]

Am 14.05.2016 um 16:19 schrieb Emil Velikov:
> Hi all,
>
> On 13 May 2016 at 17:48, Alex Deucher <alexdeucher@gmail.com> wrote:
>> From: Sonny Jiang <sonny.jiang@amd.com>
>>
>> Signed-off-by: Sonny Jiang <sonny.jiang@amd.com>
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Alex Deucher <alexander.deucher@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.

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 ?
>
>
>> +                       /*ctx size */
>> +                       ptr[0x2C] = 0x00;
>> +                       ptr[0x2D] = 0xAF;
>> +                       ptr[0x2E] = 0x50;
>> +                       ptr[0x2F] = 0x00;
>> +               }
> While this one does match ctx_size above, one should really set a
> macro for these magic values and use them throughout. Also considering
> that there's three almost identical places where this happens perhaps
> it's better to have a common helper ?
>
> Regards,
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel