[PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Alex Sierra]

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
 				amdgpu_hotplug_work_func);
 	}
 
+	if (adev->asic_type == CHIP_ARCTURUS)
+		adev->irq.ring_stride = 1;
+	else
+		adev->irq.ring_stride = 0;
 	INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 	INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 	struct irq_domain		*domain; /* GPU irq controller domain */
 	unsigned			virq[AMDGPU_MAX_IRQ_SRC_ID];
 	uint32_t                        srbm_soft_reset;
+	unsigned			ring_stride;
 };
 
 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Sierra]

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@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)
 
 static void vega10_ih_set_interrupt_funcs(struct amdgpu_device *adev);
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/amdgpu: replace rd/wr IH ring registers using mmIH_RING_REG macro

[Alex Sierra]

[Why]
Replace the way reads and writes are done to the IH ring registers at the vega10_ih.
This is due to different IH ring registers offset between Vega10 and Arcturus.

[How]
mmIH_RING_REG macro is used to calculate the register address first. Then RREG32 and WREG32 macros
are used to directly write/read into the register.

Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vega10_ih.c | 121 +++++++++++++------------
 1 file changed, 61 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
index 5bd9bc37fadf..8d41b4c27205 100644
--- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
@@ -51,7 +51,7 @@ static void vega10_ih_set_interrupt_funcs(struct amdgpu_device *adev);
  */
 static void vega10_ih_enable_interrupts(struct amdgpu_device *adev)
 {
-	u32 ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL);
+	u32 ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL, RING0));
 
 	ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL, RB_ENABLE, 1);
 	ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL, ENABLE_INTR, 1);
@@ -61,12 +61,12 @@ static void vega10_ih_enable_interrupts(struct amdgpu_device *adev)
 			return;
 		}
 	} else {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL, ih_rb_cntl);
+		WREG32(mmIH_RING_REG(RB_CNTL, RING0), ih_rb_cntl);
 	}
 	adev->irq.ih.enabled = true;
 
 	if (adev->irq.ih1.ring_size) {
-		ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1);
+		ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL_RING1, RING1));
 		ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL_RING1,
 					   RB_ENABLE, 1);
 		if (amdgpu_sriov_vf(adev)) {
@@ -76,13 +76,13 @@ static void vega10_ih_enable_interrupts(struct amdgpu_device *adev)
 				return;
 			}
 		} else {
-			WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1, ih_rb_cntl);
+			WREG32(mmIH_RING_REG(RB_CNTL_RING1, RING1), ih_rb_cntl);
 		}
 		adev->irq.ih1.enabled = true;
 	}
 
 	if (adev->irq.ih2.ring_size) {
-		ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2);
+		ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL_RING2, RING2));
 		ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL_RING2,
 					   RB_ENABLE, 1);
 		if (amdgpu_sriov_vf(adev)) {
@@ -92,7 +92,7 @@ static void vega10_ih_enable_interrupts(struct amdgpu_device *adev)
 				return;
 			}
 		} else {
-			WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2, ih_rb_cntl);
+			WREG32(mmIH_RING_REG(RB_CNTL_RING2, RING2), ih_rb_cntl);
 		}
 		adev->irq.ih2.enabled = true;
 	}
@@ -107,7 +107,7 @@ static void vega10_ih_enable_interrupts(struct amdgpu_device *adev)
  */
 static void vega10_ih_disable_interrupts(struct amdgpu_device *adev)
 {
-	u32 ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL);
+	u32 ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL, RING0));
 
 	ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL, RB_ENABLE, 0);
 	ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL, ENABLE_INTR, 0);
@@ -117,17 +117,17 @@ static void vega10_ih_disable_interrupts(struct amdgpu_device *adev)
 			return;
 		}
 	} else {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL, ih_rb_cntl);
+		WREG32(mmIH_RING_REG(RB_CNTL, RING0), ih_rb_cntl);
 	}
 
 	/* set rptr, wptr to 0 */
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR, 0);
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR, 0);
+	WREG32(mmIH_RING_REG(RB_RPTR, RING0), 0);
+	WREG32(mmIH_RING_REG(RB_WPTR, RING0), 0);
 	adev->irq.ih.enabled = false;
 	adev->irq.ih.rptr = 0;
 
 	if (adev->irq.ih1.ring_size) {
-		ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1);
+		ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL_RING1, RING1));
 		ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL_RING1,
 					   RB_ENABLE, 0);
 		if (amdgpu_sriov_vf(adev)) {
@@ -137,17 +137,17 @@ static void vega10_ih_disable_interrupts(struct amdgpu_device *adev)
 				return;
 			}
 		} else {
-			WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1, ih_rb_cntl);
+			WREG32(mmIH_RING_REG(RB_CNTL_RING1, RING1), ih_rb_cntl);
 		}
 		/* set rptr, wptr to 0 */
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR_RING1, 0);
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR_RING1, 0);
+		WREG32(mmIH_RING_REG(RB_RPTR_RING1, RING1), 0);
+		WREG32(mmIH_RING_REG(RB_WPTR_RING1, RING1), 0);
 		adev->irq.ih1.enabled = false;
 		adev->irq.ih1.rptr = 0;
 	}
 
 	if (adev->irq.ih2.ring_size) {
-		ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2);
+		ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL_RING2, RING2));
 		ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL_RING2,
 					   RB_ENABLE, 0);
 		if (amdgpu_sriov_vf(adev)) {
@@ -157,12 +157,12 @@ static void vega10_ih_disable_interrupts(struct amdgpu_device *adev)
 				return;
 			}
 		} else {
-			WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2, ih_rb_cntl);
+			WREG32(mmIH_RING_REG(RB_CNTL_RING2, RING2), ih_rb_cntl);
 		}
 
 		/* set rptr, wptr to 0 */
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR_RING2, 0);
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR_RING2, 0);
+		WREG32(mmIH_RING_REG(RB_RPTR_RING2, RING2), 0);
+		WREG32(mmIH_RING_REG(RB_WPTR_RING2, RING2), 0);
 		adev->irq.ih2.enabled = false;
 		adev->irq.ih2.rptr = 0;
 	}
@@ -235,10 +235,10 @@ static int vega10_ih_irq_init(struct amdgpu_device *adev)
 
 	ih = &adev->irq.ih;
 	/* Ring Buffer base. [39:8] of 40-bit address of the beginning of the ring buffer*/
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_BASE, ih->gpu_addr >> 8);
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_BASE_HI, (ih->gpu_addr >> 40) & 0xff);
+	WREG32(mmIH_RING_REG(RB_BASE, RING0), ih->gpu_addr >> 8);
+	WREG32(mmIH_RING_REG(RB_BASE_HI, RING0), (ih->gpu_addr >> 40) & 0xff);
 
-	ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL);
+	ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL, RING0));
 	ih_rb_cntl = vega10_ih_rb_cntl(ih, ih_rb_cntl);
 	ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL, RPTR_REARM,
 				   !!adev->irq.msi_enabled);
@@ -248,7 +248,7 @@ static int vega10_ih_irq_init(struct amdgpu_device *adev)
 			return -ETIMEDOUT;
 		}
 	} else {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL, ih_rb_cntl);
+		WREG32(mmIH_RING_REG(RB_CNTL, RING0), ih_rb_cntl);
 	}
 
 	if ((adev->asic_type == CHIP_ARCTURUS &&
@@ -266,25 +266,25 @@ static int vega10_ih_irq_init(struct amdgpu_device *adev)
 	}
 
 	/* set the writeback address whether it's enabled or not */
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR_ADDR_LO,
-		     lower_32_bits(ih->wptr_addr));
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR_ADDR_HI,
-		     upper_32_bits(ih->wptr_addr) & 0xFFFF);
+	WREG32(mmIH_RING_REG(RB_WPTR_ADDR_LO, RING0),
+			lower_32_bits(ih->wptr_addr));
+	WREG32(mmIH_RING_REG(RB_WPTR_ADDR_HI, RING0),
+			upper_32_bits(ih->wptr_addr) & 0xFFFF);
 
 	/* set rptr, wptr to 0 */
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR, 0);
-	WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR, 0);
+	WREG32(mmIH_RING_REG(RB_RPTR, RING0), 0);
+	WREG32(mmIH_RING_REG(RB_WPTR, RING0), 0);
 
-	WREG32_SOC15(OSSSYS, 0, mmIH_DOORBELL_RPTR,
-		     vega10_ih_doorbell_rptr(ih));
+	WREG32(mmIH_RING_REG(DOORBELL_RPTR, RING0),
+			vega10_ih_doorbell_rptr(ih));
 
 	ih = &adev->irq.ih1;
 	if (ih->ring_size) {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_BASE_RING1, ih->gpu_addr >> 8);
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_BASE_HI_RING1,
-			     (ih->gpu_addr >> 40) & 0xff);
+		WREG32(mmIH_RING_REG(RB_BASE_RING1, RING1), ih->gpu_addr >> 8);
+		WREG32(mmIH_RING_REG(RB_BASE_HI_RING1, RING1),
+				(ih->gpu_addr >> 40) & 0xff);
 
-		ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1);
+		ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL_RING1, RING1));
 		ih_rb_cntl = vega10_ih_rb_cntl(ih, ih_rb_cntl);
 		ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL,
 					   WPTR_OVERFLOW_ENABLE, 0);
@@ -297,24 +297,24 @@ static int vega10_ih_irq_init(struct amdgpu_device *adev)
 				return -ETIMEDOUT;
 			}
 		} else {
-			WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1, ih_rb_cntl);
+			WREG32(mmIH_RING_REG(RB_CNTL_RING1, RING1), ih_rb_cntl);
 		}
 
 		/* set rptr, wptr to 0 */
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR_RING1, 0);
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR_RING1, 0);
+		WREG32(mmIH_RING_REG(RB_RPTR_RING1, RING1), 0);
+		WREG32(mmIH_RING_REG(RB_WPTR_RING1, RING1), 0);
 
-		WREG32_SOC15(OSSSYS, 0, mmIH_DOORBELL_RPTR_RING1,
-			     vega10_ih_doorbell_rptr(ih));
+		WREG32(mmIH_RING_REG(DOORBELL_RPTR_RING1, RING1),
+				vega10_ih_doorbell_rptr(ih));
 	}
 
 	ih = &adev->irq.ih2;
 	if (ih->ring_size) {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_BASE_RING2, ih->gpu_addr >> 8);
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_BASE_HI_RING2,
-			     (ih->gpu_addr >> 40) & 0xff);
+		WREG32(mmIH_RING_REG(RB_BASE_RING2, RING2), ih->gpu_addr >> 8);
+		WREG32(mmIH_RING_REG(RB_BASE_HI_RING2, RING2),
+				(ih->gpu_addr >> 40) & 0xff);
 
-		ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2);
+		ih_rb_cntl = RREG32(mmIH_RING_REG(RB_CNTL_RING2, RING2));
 		ih_rb_cntl = vega10_ih_rb_cntl(ih, ih_rb_cntl);
 
 		if (amdgpu_sriov_vf(adev)) {
@@ -324,15 +324,16 @@ static int vega10_ih_irq_init(struct amdgpu_device *adev)
 				return -ETIMEDOUT;
 			}
 		} else {
-			WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2, ih_rb_cntl);
+			WREG32(mmIH_RING_REG(RB_CNTL_RING2, RING2), ih_rb_cntl);
 		}
 
 		/* set rptr, wptr to 0 */
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_WPTR_RING2, 0);
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR_RING2, 0);
+		WREG32(mmIH_RING_REG(RB_RPTR_RING2, RING2), 0);
+		WREG32(mmIH_RING_REG(RB_WPTR_RING2, RING2), 0);
+
+		WREG32(mmIH_RING_REG(DOORBELL_RPTR_RING2, RING2),
+				vega10_ih_doorbell_rptr(ih));
 
-		WREG32_SOC15(OSSSYS, 0, mmIH_DOORBELL_RPTR_RING2,
-			     vega10_ih_doorbell_rptr(ih));
 	}
 
 	tmp = RREG32_SOC15(OSSSYS, 0, mmIH_STORM_CLIENT_LIST_CNTL);
@@ -390,11 +391,11 @@ static u32 vega10_ih_get_wptr(struct amdgpu_device *adev,
 	/* Double check that the overflow wasn't already cleared. */
 
 	if (ih == &adev->irq.ih)
-		reg = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_WPTR);
+		reg = mmIH_RING_REG(RB_WPTR, RING0);
 	else if (ih == &adev->irq.ih1)
-		reg = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_WPTR_RING1);
+		reg = mmIH_RING_REG(RB_WPTR_RING1, RING1);
 	else if (ih == &adev->irq.ih2)
-		reg = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_WPTR_RING2);
+		reg = mmIH_RING_REG(RB_WPTR_RING2, RING2);
 	else
 		BUG();
 
@@ -415,11 +416,11 @@ static u32 vega10_ih_get_wptr(struct amdgpu_device *adev,
 	ih->rptr = tmp;
 
 	if (ih == &adev->irq.ih)
-		reg = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_CNTL);
+		reg = mmIH_RING_REG(RB_CNTL, RING0);
 	else if (ih == &adev->irq.ih1)
-		reg = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_CNTL_RING1);
+		reg = mmIH_RING_REG(RB_CNTL_RING1, RING1);
 	else if (ih == &adev->irq.ih2)
-		reg = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_CNTL_RING2);
+		reg = mmIH_RING_REG(RB_CNTL_RING2, RING2);
 	else
 		BUG();
 
@@ -488,11 +489,11 @@ static void vega10_ih_irq_rearm(struct amdgpu_device *adev,
 	uint32_t i = 0;
 
 	if (ih == &adev->irq.ih)
-		reg_rptr = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_RPTR);
+		reg_rptr = mmIH_RING_REG(RB_RPTR, RING0);
 	else if (ih == &adev->irq.ih1)
-		reg_rptr = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_RPTR_RING1);
+		reg_rptr = mmIH_RING_REG(RB_RPTR_RING1, RING1);
 	else if (ih == &adev->irq.ih2)
-		reg_rptr = SOC15_REG_OFFSET(OSSSYS, 0, mmIH_RB_RPTR_RING2);
+		reg_rptr = mmIH_RING_REG(RB_RPTR_RING2, RING2);
 	else
 		return;
 
@@ -524,11 +525,11 @@ static void vega10_ih_set_rptr(struct amdgpu_device *adev,
 		if (amdgpu_sriov_vf(adev))
 			vega10_ih_irq_rearm(adev, ih);
 	} else if (ih == &adev->irq.ih) {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR, ih->rptr);
+		WREG32(mmIH_RING_REG(RB_RPTR, RING0), ih->rptr);
 	} else if (ih == &adev->irq.ih1) {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR_RING1, ih->rptr);
+		WREG32(mmIH_RING_REG(RB_RPTR_RING1, RING1), ih->rptr);
 	} else if (ih == &adev->irq.ih2) {
-		WREG32_SOC15(OSSSYS, 0, mmIH_RB_RPTR_RING2, ih->rptr);
+		WREG32(mmIH_RING_REG(RB_RPTR_RING2, RING2), ih->rptr);
 	}
 }
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/4] drm/amdgpu: reroute VMC and UMD to IH ring 1 for arcturus

[Alex Sierra]

[Why]
Due Page faults can easily overwhelm the interrupt handler.
So to make sure that we never lose valuable interrupts on the primary ring
we re-route page faults to IH ring 1.
It also facilitates the recovery page process, since it's already
running from a process context.

[How]
Setting IH_CLIENT_CFG_DATA for VMC and UMD IH clients.

Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vega10_ih.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
index 8d41b4c27205..95abbf67ead9 100644
--- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
@@ -210,6 +210,24 @@ static uint32_t vega10_ih_doorbell_rptr(struct amdgpu_ih_ring *ih)
 	return ih_doorbell_rtpr;
 }
 
+static void vega10_ih_reroute_ih(struct amdgpu_device *adev)
+{
+	uint32_t tmp;
+
+	/* Reroute to IH ring 1 for VMC */
+	WREG32_SOC15(OSSSYS, 0, mmIH_CLIENT_CFG_INDEX, 0x12);
+	tmp = RREG32_SOC15(OSSSYS, 0, mmIH_CLIENT_CFG_DATA);
+	tmp = REG_SET_FIELD(tmp, IH_CLIENT_CFG_DATA, CLIENT_TYPE, 1);
+	tmp = REG_SET_FIELD(tmp, IH_CLIENT_CFG_DATA, RING_ID, 1);
+	WREG32_SOC15(OSSSYS, 0, mmIH_CLIENT_CFG_DATA, tmp);
+
+	/* Reroute IH ring 1 for UMC */
+	WREG32_SOC15(OSSSYS, 0, mmIH_CLIENT_CFG_INDEX, 0x1B);
+	tmp = RREG32_SOC15(OSSSYS, 0, mmIH_CLIENT_CFG_DATA);
+	tmp = REG_SET_FIELD(tmp, IH_CLIENT_CFG_DATA, RING_ID, 1);
+	WREG32_SOC15(OSSSYS, 0, mmIH_CLIENT_CFG_DATA, tmp);
+}
+
 /**
  * vega10_ih_irq_init - init and enable the interrupt ring
  *
@@ -251,6 +269,10 @@ static int vega10_ih_irq_init(struct amdgpu_device *adev)
 		WREG32(mmIH_RING_REG(RB_CNTL, RING0), ih_rb_cntl);
 	}
 
+	if (adev->asic_type == CHIP_ARCTURUS) {
+		vega10_ih_reroute_ih(adev);
+	}
+
 	if ((adev->asic_type == CHIP_ARCTURUS &&
 	     adev->firmware.load_type == AMDGPU_FW_LOAD_DIRECT) ||
 	    adev->asic_type == CHIP_RENOIR) {
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Deucher, Alexander]

[-- Attachment #1.1: Type: text/plain, Size: 2947 bytes --]

[AMD Public Use]

This seems kind of complicated and error prone.  I didn't realize the extent to the changes required.  I think it would be better to either add arcturus specific versions of these functions or just go with your original approach and add a new arcturus_ih.c.  If you go with the second route however, no need to show all your intermediate steps, just add the new files in one commit.

Alex

________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Alex Sierra <alex.sierra@amd.com>
Sent: Thursday, March 19, 2020 8:22 PM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>
Subject: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra <alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
                                 amdgpu_hotplug_work_func);
         }

+       if (adev->asic_type == CHIP_ARCTURUS)
+               adev->irq.ring_stride = 1;
+       else
+               adev->irq.ring_stride = 0;
         INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
         INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
         struct irq_domain               *domain; /* GPU irq controller domain */
         unsigned                        virq[AMDGPU_MAX_IRQ_SRC_ID];
         uint32_t                        srbm_soft_reset;
+       unsigned                        ring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&amp;sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&amp;reserved=0

[-- Attachment #1.2: Type: text/html, Size: 5911 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Felix Kuehling]

[-- Attachment #1.1: Type: text/plain, Size: 4142 bytes --]

On 2020-03-20 10:06, Deucher, Alexander wrote:
>
> [AMD Public Use]
>
>
> This seems kind of complicated and error prone.  I didn't realize the 
> extent to the changes required.  I think it would be better to either 
> add arcturus specific versions of these functions or just go with your 
> original approach and add a new arcturus_ih.c.  If you go with the 
> second route however, no need to show all your intermediate steps, 
> just add the new files in one commit.

Hi Alex,


I suggested the approach in this patch series since to minimize code 
duplication and maintain readability of the code. I don't think it's 
very error prone. I believe this is more maintainable than a separate 
arcturus_ih.c. I'll have some more specific comments on Alejandro's patches.


Regards,
   Felix


>
> Alex
>
> ------------------------------------------------------------------------
> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
> Alex Sierra <alex.sierra@amd.com>
> *Sent:* Thursday, March 19, 2020 8:22 PM
> *To:* amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc:* Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>
> *Subject:* [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring 
> offsets
> Arcturus and vega10 share the same vega10_ih, however both
> have different register offsets at the ih ring section.
> This variable is used to help calculate ih ring register addresses
> from the osssys, that corresponds to the current asic type.
>
> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 5ed4227f304b..fa384ae9a9bc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
> amdgpu_hotplug_work_func);
>          }
>
> +       if (adev->asic_type == CHIP_ARCTURUS)
> +               adev->irq.ring_stride = 1;
> +       else
> +               adev->irq.ring_stride = 0;
>          INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
>          INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> index c718e94a55c9..1ec5b735cd9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> @@ -97,6 +97,7 @@ struct amdgpu_irq {
>          struct irq_domain               *domain; /* GPU irq 
> controller domain */
>          unsigned virq[AMDGPU_MAX_IRQ_SRC_ID];
>          uint32_t srbm_soft_reset;
> +       unsigned                        ring_stride;
>  };
>
>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
> -- 
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&amp;sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&amp;reserved=0 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&amp;sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&amp;reserved=0

[-- Attachment #1.2: Type: text/html, Size: 9855 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Felix Kuehling]

On 2020-03-19 20:22, Alex Sierra wrote:
> Arcturus and vega10 share the same vega10_ih, however both
> have different register offsets at the ih ring section.
> This variable is used to help calculate ih ring register addresses
> from the osssys, that corresponds to the current asic type.
>
> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
>   2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 5ed4227f304b..fa384ae9a9bc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>   				amdgpu_hotplug_work_func);
>   	}
>   
> +	if (adev->asic_type == CHIP_ARCTURUS)
> +		adev->irq.ring_stride = 1;
> +	else
> +		adev->irq.ring_stride = 0;

This can't be right. ring_stride==0 would result in all mmIH_RING(...) 
register access to map to the same physical registers. So effectively 
everything would go to ring0.

Regards,
   Felix


>   	INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
>   	INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> index c718e94a55c9..1ec5b735cd9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> @@ -97,6 +97,7 @@ struct amdgpu_irq {
>   	struct irq_domain		*domain; /* GPU irq controller domain */
>   	unsigned			virq[AMDGPU_MAX_IRQ_SRC_ID];
>   	uint32_t                        srbm_soft_reset;
> +	unsigned			ring_stride;
>   };
>   
>   void amdgpu_irq_disable_all(struct amdgpu_device *adev);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Felix Kuehling]

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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Deucher, Alexander]

[-- Attachment #1.1: Type: text/plain, Size: 5120 bytes --]

[AMD Public Use]

I'm worried we'll miss a register by accident.  We went with per IP sub drivers to avoid handling complexities around IP differences if possible.  Also the scheme seems like kind of a one off compared to what we do for other IPs.  Can we structure it more like how we handle SDMA instancing since it seems to mainly affect IH RB instances?

Alex

________________________________
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Friday, March 20, 2020 10:20 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

On 2020-03-20 10:06, Deucher, Alexander wrote:

[AMD Public Use]

This seems kind of complicated and error prone.  I didn't realize the extent to the changes required.  I think it would be better to either add arcturus specific versions of these functions or just go with your original approach and add a new arcturus_ih.c.  If you go with the second route however, no need to show all your intermediate steps, just add the new files in one commit.

Hi Alex,


I suggested the approach in this patch series since to minimize code duplication and maintain readability of the code. I don't think it's very error prone. I believe this is more maintainable than a separate arcturus_ih.c. I'll have some more specific comments on Alejandro's patches.


Regards,
  Felix


Alex

________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org><mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Alex Sierra <alex.sierra@amd.com><mailto:alex.sierra@amd.com>
Sent: Thursday, March 19, 2020 8:22 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Cc: Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com><mailto:Alex.Sierra@amd.com>
Subject: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra <alex.sierra@amd.com><mailto:alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
                                 amdgpu_hotplug_work_func);
         }

+       if (adev->asic_type == CHIP_ARCTURUS)
+               adev->irq.ring_stride = 1;
+       else
+               adev->irq.ring_stride = 0;
         INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
         INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
         struct irq_domain               *domain; /* GPU irq controller domain */
         unsigned                        virq[AMDGPU_MAX_IRQ_SRC_ID];
         uint32_t                        srbm_soft_reset;
+       unsigned                        ring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&amp;sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&amp;reserved=0<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>



_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&amp;sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&amp;reserved=0


[-- Attachment #1.2: Type: text/html, Size: 10011 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Felix Kuehling]

[-- Attachment #1.1: Type: text/plain, Size: 5872 bytes --]


On 2020-03-20 10:39, Deucher, Alexander wrote:
>
> [AMD Public Use]
>
>
> I'm worried we'll miss a register by accident.  We went with per IP 
> sub drivers to avoid handling complexities around IP differences if 
> possible.  Also the scheme seems like kind of a one off compared to 
> what we do for other IPs.  Can we structure it more like how we handle 
> SDMA instancing since it seems to mainly affect IH RB instances?

That's more or less what I had in mind, but haven't looked at the SDMA 
implementation in detail. So do you mean defining macros 
WREG32_IH_RING(ring, offset, value) and RREG32_IH_RING(ring, offset) 
analogous to WREG32_SDMA and RREG32_SDMA? It would only apply to IH 
ring-specific registers. Not to other general IH registers.


Regards,
   Felix


>
> Alex
>
> ------------------------------------------------------------------------
> *From:* Kuehling, Felix <Felix.Kuehling@amd.com>
> *Sent:* Friday, March 20, 2020 10:20 AM
> *To:* Deucher, Alexander <Alexander.Deucher@amd.com>; Sierra Guiza, 
> Alejandro (Alex) <Alex.Sierra@amd.com>; amd-gfx@lists.freedesktop.org 
> <amd-gfx@lists.freedesktop.org>
> *Subject:* Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss 
> ring offsets
> On 2020-03-20 10:06, Deucher, Alexander wrote:
>>
>> [AMD Public Use]
>>
>>
>> This seems kind of complicated and error prone.  I didn't realize the 
>> extent to the changes required.  I think it would be better to either 
>> add arcturus specific versions of these functions or just go with 
>> your original approach and add a new arcturus_ih.c.  If you go with 
>> the second route however, no need to show all your intermediate 
>> steps, just add the new files in one commit.
>
> Hi Alex,
>
>
> I suggested the approach in this patch series since to minimize code 
> duplication and maintain readability of the code. I don't think it's 
> very error prone. I believe this is more maintainable than a separate 
> arcturus_ih.c. I'll have some more specific comments on Alejandro's 
> patches.
>
>
> Regards,
>   Felix
>
>
>>
>> Alex
>>
>> ------------------------------------------------------------------------
>> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 
>> <mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Alex 
>> Sierra <alex.sierra@amd.com> <mailto:alex.sierra@amd.com>
>> *Sent:* Thursday, March 19, 2020 8:22 PM
>> *To:* amd-gfx@lists.freedesktop.org 
>> <mailto:amd-gfx@lists.freedesktop.org> 
>> <amd-gfx@lists.freedesktop.org> <mailto:amd-gfx@lists.freedesktop.org>
>> *Cc:* Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com> 
>> <mailto:Alex.Sierra@amd.com>
>> *Subject:* [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring 
>> offsets
>> Arcturus and vega10 share the same vega10_ih, however both
>> have different register offsets at the ih ring section.
>> This variable is used to help calculate ih ring register addresses
>> from the osssys, that corresponds to the current asic type.
>>
>> Signed-off-by: Alex Sierra <alex.sierra@amd.com> 
>> <mailto:alex.sierra@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> index 5ed4227f304b..fa384ae9a9bc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> @@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>> amdgpu_hotplug_work_func);
>>          }
>>
>> +       if (adev->asic_type == CHIP_ARCTURUS)
>> +               adev->irq.ring_stride = 1;
>> +       else
>> +               adev->irq.ring_stride = 0;
>>          INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
>>          INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>> index c718e94a55c9..1ec5b735cd9e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>> @@ -97,6 +97,7 @@ struct amdgpu_irq {
>>          struct irq_domain               *domain; /* GPU irq 
>> controller domain */
>>          unsigned virq[AMDGPU_MAX_IRQ_SRC_ID];
>>          uint32_t srbm_soft_reset;
>> +       unsigned ring_stride;
>>  };
>>
>>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&amp;sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&amp;reserved=0 
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org  <mailto:amd-gfx@lists.freedesktop.org>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&amp;sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&amp;reserved=0

[-- Attachment #1.2: Type: text/html, Size: 14301 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Deucher, Alexander]

[-- Attachment #1.1: Type: text/plain, Size: 6122 bytes --]

[AMD Public Use]

Yes, something like that.

Alex
________________________________
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Friday, March 20, 2020 10:47 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets



On 2020-03-20 10:39, Deucher, Alexander wrote:

[AMD Public Use]

I'm worried we'll miss a register by accident.  We went with per IP sub drivers to avoid handling complexities around IP differences if possible.  Also the scheme seems like kind of a one off compared to what we do for other IPs.  Can we structure it more like how we handle SDMA instancing since it seems to mainly affect IH RB instances?

That's more or less what I had in mind, but haven't looked at the SDMA implementation in detail. So do you mean defining macros WREG32_IH_RING(ring, offset, value) and RREG32_IH_RING(ring, offset) analogous to WREG32_SDMA and RREG32_SDMA? It would only apply to IH ring-specific registers. Not to other general IH registers.


Regards,
  Felix


Alex

________________________________
From: Kuehling, Felix <Felix.Kuehling@amd.com><mailto:Felix.Kuehling@amd.com>
Sent: Friday, March 20, 2020 10:20 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com><mailto:Alexander.Deucher@amd.com>; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com><mailto:Alex.Sierra@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

On 2020-03-20 10:06, Deucher, Alexander wrote:

[AMD Public Use]

This seems kind of complicated and error prone.  I didn't realize the extent to the changes required.  I think it would be better to either add arcturus specific versions of these functions or just go with your original approach and add a new arcturus_ih.c.  If you go with the second route however, no need to show all your intermediate steps, just add the new files in one commit.

Hi Alex,


I suggested the approach in this patch series since to minimize code duplication and maintain readability of the code. I don't think it's very error prone. I believe this is more maintainable than a separate arcturus_ih.c. I'll have some more specific comments on Alejandro's patches.


Regards,
  Felix


Alex

________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org><mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Alex Sierra <alex.sierra@amd.com><mailto:alex.sierra@amd.com>
Sent: Thursday, March 19, 2020 8:22 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Cc: Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com><mailto:Alex.Sierra@amd.com>
Subject: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra <alex.sierra@amd.com><mailto:alex.sierra@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
                                 amdgpu_hotplug_work_func);
         }

+       if (adev->asic_type == CHIP_ARCTURUS)
+               adev->irq.ring_stride = 1;
+       else
+               adev->irq.ring_stride = 0;
         INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
         INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
         struct irq_domain               *domain; /* GPU irq controller domain */
         unsigned                        virq[AMDGPU_MAX_IRQ_SRC_ID];
         uint32_t                        srbm_soft_reset;
+       unsigned                        ring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&amp;sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&amp;reserved=0<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>



_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&amp;sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&amp;reserved=0


[-- Attachment #1.2: Type: text/html, Size: 12367 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 6008 bytes --]

Am 20.03.20 um 15:20 schrieb Felix Kuehling:
> On 2020-03-20 10:06, Deucher, Alexander wrote:
>>
>> [AMD Public Use]
>>
>>
>> This seems kind of complicated and error prone.  I didn't realize the 
>> extent to the changes required.  I think it would be better to either 
>> add arcturus specific versions of these functions or just go with 
>> your original approach and add a new arcturus_ih.c.  If you go with 
>> the second route however, no need to show all your intermediate 
>> steps, just add the new files in one commit.
>
> Hi Alex,
>
>
> I suggested the approach in this patch series since to minimize code 
> duplication and maintain readability of the code. I don't think it's 
> very error prone. I believe this is more maintainable than a separate 
> arcturus_ih.c. I'll have some more specific comments on Alejandro's 
> patches.
>

Question is rather if Arcturus has really the same OSS block than Vega10 
or if the registers are just the same and at a different offset?

If the later (which I suspect) than that should really be the same file.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> Alex
>>
>> ------------------------------------------------------------------------
>> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
>> Alex Sierra <alex.sierra@amd.com>
>> *Sent:* Thursday, March 19, 2020 8:22 PM
>> *To:* amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
>> *Cc:* Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>
>> *Subject:* [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring 
>> offsets
>> Arcturus and vega10 share the same vega10_ih, however both
>> have different register offsets at the ih ring section.
>> This variable is used to help calculate ih ring register addresses
>> from the osssys, that corresponds to the current asic type.
>>
>> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 ++++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> index 5ed4227f304b..fa384ae9a9bc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>> @@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>> amdgpu_hotplug_work_func);
>>          }
>>
>> +       if (adev->asic_type == CHIP_ARCTURUS)
>> +               adev->irq.ring_stride = 1;
>> +       else
>> +               adev->irq.ring_stride = 0;
>>          INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
>>          INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>> index c718e94a55c9..1ec5b735cd9e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>> @@ -97,6 +97,7 @@ struct amdgpu_irq {
>>          struct irq_domain               *domain; /* GPU irq 
>> controller domain */
>>          unsigned virq[AMDGPU_MAX_IRQ_SRC_ID];
>>          uint32_t srbm_soft_reset;
>> +       unsigned                        ring_stride;
>>  };
>>
>>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&amp;sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&amp;reserved=0< 
>> /a>
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>
>>
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0> 
>>
>> _______________________________________________ amd-gfx mailing list   <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&amp;sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&amp;reserved=0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 13186 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [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@amd.com> 
Sent: Friday, March 20, 2020 9:31 AM
To: Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>; amd-gfx@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@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);
>>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx