[AMD Official Use Only - Internal Distribution Only] Inline response -----Original Message----- From: Kuehling, Felix <Felix.Kuehling@xxxxxxx> Sent: Friday, March 20, 2020 9:31 AM To: Sierra Guiza, Alejandro (Alex) <Alex.Sierra@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx 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@xxxxxxx> >> --- >> 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); >> _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
