[PATCH 2/4] drm/amdgpu: add macro to get proper ih ring register offset

[Sierra Guiza, Alejandro (Alex)]

[AMD Official Use Only - Internal Distribution Only]

Inline response

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling at amd.com> 
Sent: Friday, March 20, 2020 9:31 AM
To: Sierra Guiza, Alejandro (Alex) <Alex.Sierra at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/4] drm/amdgpu: add macro to get proper ih ring register offset

On 2020-03-19 20:22, Alex Sierra wrote:
>> This macro calculates the IH ring register offset based on the three 
>> ring numbers and asic type.
>> The parameters needed are the register's name without the prefix mmIH 
>> and the ring number taken from RING0, RING1 or RING2 macros.
>>
>> Signed-off-by: Alex Sierra <alex.sierra at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/vega10_ih.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c 
>> b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>> index 407c6093c2ec..5bd9bc37fadf 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>> @@ -34,6 +34,11 @@
>>   #include "vega10_ih.h"
>>   
>>   #define MAX_REARM_RETRY 10
>> +#define RING0 0
>> +#define RING1 (RING0 + 4)
>> +#define RING2 (RING1 + 4)
>> +
>> +#define mmIH_RING_REG(reg, ring) (SOC15_REG_OFFSET(OSSSYS, 0, 
>> +mmIH_##reg) + (ring) * adev->irq.ring_stride)
>
> I don't think you need the RINGx definitions. Just use numbers 0-2. The ring_stride should be the number of registers to skip from one ring to the next, which can be different for different ASICs. E.g. 
> (mmIH_RB_CNTL_RING1 - mmIH_RB_CNTL). It's 8 on Vega, 12 on Arcturus.
You're right. This was a different approach where I was using the actual registers and adding the proper offset based on the RING offset and ring_stride as enabler.
Ex.
Vega10
ring_strider = 0
RING1 = 1
mmIH_RB_RPTR_RING1 = 0x8b = mmIH_RB_RPTR_RING1 + (RING1 * ring_strider)
Arcturus
ring_strider = 1
RING1 = 4
mmIH_RB_RPTR_RING1 = 0x8f = mmIH_RB_RPTR_RING1 + (RING1 * ring_strider)
Your solution looks easier to read.
>
> I'd squash patches 1 and 2 to make this more obvious.
>
>Regards,
>  Felix
>
>>   
>>   static void vega10_ih_set_interrupt_funcs(struct amdgpu_device 
>> *adev);
>>