[PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3)

[PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3)

[Hawking Zhang]

Add helper function in order to remove RREG32/WREG32
in current pcie_rreg/wreg function for soc15 and
onwards adapters.
PCIE_INDEX/DATA pairs are used to access regsiters
outside of mmio bar in the helper functions.
The new helper functions help remove the recursion
of amdgpu_mm_rreg/wreg from pcie_rreg/wreg and
provide the oppotunity to centralize direct and
indirect access in a single function.

v2: Fixed typo and refine the comments

v3: Remove unnecessary volatile local variable

Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Kevin Wang <kevin1.wang@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Guchun Chen <guchun.chen@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  13 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 129 +++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 13f92de..40ee44b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1031,6 +1031,19 @@ uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t offset);
 u32 amdgpu_io_rreg(struct amdgpu_device *adev, u32 reg);
 void amdgpu_io_wreg(struct amdgpu_device *adev, u32 reg, u32 v);
 
+u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
+				u32 pcie_index, u32 pcie_data,
+				u32 reg_addr);
+u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
+				  u32 pcie_index, u32 pcie_data,
+				  u32 reg_addr);
+void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
+				 u32 pcie_index, u32 pcie_data,
+				 u32 reg_addr, u32 reg_data);
+void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
+				   u32 pcie_index, u32 pcie_data,
+				   u32 reg_addr, u64 reg_data);
+
 bool amdgpu_device_asic_has_dc_support(enum amd_asic_type asic_type);
 bool amdgpu_device_has_dc_support(struct amdgpu_device *adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5c2eb46..77785b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -595,6 +595,135 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
 }
 
 /**
+ * amdgpu_device_indirect_rreg - read an indirect register
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ *
+ * Returns the value of indirect register @reg_addr
+ */
+u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
+				u32 pcie_index, u32 pcie_data,
+				u32 reg_addr)
+{
+	unsigned long flags;
+	u32 r;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	r = readl(pcie_data_offset);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+
+	return r;
+}
+
+/**
+ * amdgpu_device_indirect_rreg64 - read a 64bits indirect register
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ *
+ * Returns the value of indirect register @reg_addr
+ */
+u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
+				  u32 pcie_index, u32 pcie_data,
+				  u32 reg_addr)
+{
+	unsigned long flags;
+	u64 r;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	/* read low 32 bits */
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	r = readl(pcie_data_offset);
+	/* read high 32 bits */
+	writel(reg_addr + 4, pcie_index_offset);
+	readl(pcie_index_offset);
+	r |= ((u64)readl(pcie_data_offset) << 32);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+
+	return r;
+}
+
+/**
+ * amdgpu_device_indirect_wreg - write an indirect register address
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ * @reg_addr: indirect register offset
+ * @reg_data: indirect register data
+ *
+ */
+void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
+				 u32 pcie_index, u32 pcie_data,
+				 u32 reg_addr, u32 reg_data)
+{
+	unsigned long flags;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	writel(reg_data, pcie_data_offset);
+	readl(pcie_data_offset);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+}
+
+/**
+ * amdgpu_device_indirect_wreg64 - write a 64bits indirect register address
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ * @reg_addr: indirect register offset
+ * @reg_data: indirect register data
+ *
+ */
+void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
+				   u32 pcie_index, u32 pcie_data,
+				   u32 reg_addr, u64 reg_data)
+{
+	unsigned long flags;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	/* write low 32 bits */
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	writel((u32)(reg_data & 0xffffffffULL), pcie_data_offset);
+	readl(pcie_data_offset);
+	/* write high 32 bits */
+	writel(reg_addr + 4, pcie_index_offset);
+	readl(pcie_index_offset);
+	writel((u32)(reg_data >> 32), pcie_data_offset);
+	readl(pcie_data_offset);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+}
+
+/**
  * amdgpu_invalid_rreg - dummy reg read function
  *
  * @adev: amdgpu device pointer
-- 
2.7.4

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

[PATCH 2/3] drm/amdgpu: switch to indirect reg access helper

[Hawking Zhang]

Switch WREG32/RREG32_PCIE to use indirect reg access
helper for soc15 and onwards

Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Kevin Wang <kevin1.wang@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Guchun Chen <guchun.chen@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/nv.c    | 51 ++++++--------------------------------
 drivers/gpu/drm/amd/amdgpu/soc15.c | 51 ++++++--------------------------------
 2 files changed, 16 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index 0ec6603..91ca4ece 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -69,75 +69,40 @@ static const struct amd_ip_funcs nv_common_ip_funcs;
  */
 static u32 nv_pcie_rreg(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u32 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg(adev, address, data, reg);
 }
 
 static void nv_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, v);
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
 }
 
 static u64 nv_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u64 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* read low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-
-	/* read high 32 bit*/
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	r |= ((u64)RREG32(data) << 32);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
 }
 
 static void nv_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* write low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v & 0xffffffffULL));
-	(void)RREG32(data);
-
-	/* write high 32 bit */
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v >> 32));
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
 }
 
 static u32 nv_didt_rreg(struct amdgpu_device *adev, u32 reg)
diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
index ddd55e3..0de9df8 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -101,75 +101,40 @@
  */
 static u32 soc15_pcie_rreg(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u32 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg(adev, address, data, reg);
 }
 
 static void soc15_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, v);
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
 }
 
 static u64 soc15_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u64 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* read low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-
-	/* read high 32 bit*/
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	r |= ((u64)RREG32(data) << 32);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
 }
 
 static void soc15_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* write low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v & 0xffffffffULL));
-	(void)RREG32(data);
-
-	/* write high 32 bit */
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v >> 32));
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
 }
 
 static u32 soc15_uvd_ctx_rreg(struct amdgpu_device *adev, u32 reg)
-- 
2.7.4

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

[PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Hawking Zhang]

support both direct and indirect accessor in unified
helper functions.

v2: Retire indirect mmio access via mm_index/data

Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
 4 files changed, 51 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 40ee44b..50341f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
 
 void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
 			       uint32_t *buf, size_t size, bool write);
-uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
+uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
+			    uint32_t reg, uint32_t acc_flags);
+void amdgpu_device_wreg(struct amdgpu_device *adev,
+			uint32_t reg, uint32_t v,
 			uint32_t acc_flags);
-void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
-		    uint32_t acc_flags);
-void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
-		    uint32_t acc_flags);
+void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
+			     uint32_t reg, uint32_t v);
 void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value);
 uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t offset);
 
@@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
  */
 #define AMDGPU_REGS_NO_KIQ    (1<<1)
 
-#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_NO_KIQ)
-#define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
+#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg), AMDGPU_REGS_NO_KIQ)
+#define WREG32_NO_KIQ(reg, v) amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
 
 #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
 #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v))
@@ -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
 #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
 #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
 
-#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0)
-#define DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", amdgpu_mm_rreg(adev, (reg), 0))
-#define WREG32(reg, v) amdgpu_mm_wreg(adev, (reg), (v), 0)
+#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0)
+#define DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", amdgpu_device_rreg(adev, (reg), 0))
+#define WREG32(reg, v) amdgpu_device_wreg(adev, (reg), (v), 0)
 #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
 #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
 #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg))
@@ -1111,7 +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
 		WREG32_SMC(_Reg, tmp);                          \
 	} while (0)
 
-#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " : 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
+#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " : 0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
 #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
 #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index abe0c27..2d125b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct file *f,
 		} else {
 			r = get_user(value, (uint32_t *)buf);
 			if (!r)
-				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
+				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
 		}
 		if (r) {
 			result = r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 77785b2..beef764 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
 }
 
 /*
- * MMIO register access helper functions.
+ * register access helper functions.
  */
 /**
- * amdgpu_mm_rreg - read a memory mapped IO register
+ * amdgpu_device_rreg - read a memory mapped IO or indirect register
  *
  * @adev: amdgpu_device pointer
  * @reg: dword aligned register offset
@@ -314,33 +314,27 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
  *
  * Returns the 32 bit value from the offset specified.
  */
-uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
-			uint32_t acc_flags)
+uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
+			    uint32_t reg, uint32_t acc_flags)
 {
 	uint32_t ret;
 
 	if (adev->in_pci_err_recovery)
 		return 0;
 
-	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
-	    down_read_trylock(&adev->reset_sem)) {
-		ret = amdgpu_kiq_rreg(adev, reg);
-		up_read(&adev->reset_sem);
-		return ret;
-	}
-
-	if ((reg * 4) < adev->rmmio_size)
-		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
-	else {
-		unsigned long flags;
+	if ((reg * 4) < adev->rmmio_size) {
+		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
+		    amdgpu_sriov_runtime(adev) &&
+		    down_read_trylock(&adev->reset_sem)) {
+			ret = amdgpu_kiq_rreg(adev, reg);
+			up_read(&adev->reset_sem);
+		} else
+			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
+	} else
+		ret = adev->pcie_rreg(adev, reg * 4);
 
-		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
-		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
-		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
-		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
-	}
+	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
 
-	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
 	return ret;
 }
 
@@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value)
 		BUG();
 }
 
-static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
-				       uint32_t reg, uint32_t v,
-				       uint32_t acc_flags)
-{
-	if (adev->in_pci_err_recovery)
-		return;
-
-	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
-
-	if ((reg * 4) < adev->rmmio_size)
-		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
-	else {
-		unsigned long flags;
-
-		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
-		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
-		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
-		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
-	}
-}
-
 /**
- * amdgpu_mm_wreg - write to a memory mapped IO register
+ * amdgpu_device_wreg - write to a memory mapped IO or indirect register
  *
  * @adev: amdgpu_device pointer
  * @reg: dword aligned register offset
@@ -425,20 +398,25 @@ static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
  *
  * Writes the value specified to the offset specified.
  */
-void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
-		    uint32_t acc_flags)
+void amdgpu_device_wreg(struct amdgpu_device *adev,
+			uint32_t reg, uint32_t v,
+			uint32_t acc_flags)
 {
 	if (adev->in_pci_err_recovery)
 		return;
 
-	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
-	    down_read_trylock(&adev->reset_sem)) {
-		amdgpu_kiq_wreg(adev, reg, v);
-		up_read(&adev->reset_sem);
-		return;
-	}
+	if ((reg * 4) < adev->rmmio_size) {
+		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
+		    amdgpu_sriov_runtime(adev) &&
+		    down_read_trylock(&adev->reset_sem)) {
+			amdgpu_kiq_wreg(adev, reg, v);
+			up_read(&adev->reset_sem);
+		} else
+			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
+	} else
+		adev->pcie_wreg(adev, reg * 4, v);
 
-	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
+	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
 }
 
 /*
@@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
  *
  * this function is invoked only the debugfs register access
  * */
-void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
-		    uint32_t acc_flags)
+void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
+			     uint32_t reg, uint32_t v)
 {
 	if (adev->in_pci_err_recovery)
 		return;
 
 	if (amdgpu_sriov_fullaccess(adev) &&
-		adev->gfx.rlc.funcs &&
-		adev->gfx.rlc.funcs->is_rlcg_access_range) {
-
+	    adev->gfx.rlc.funcs &&
+	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
 		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
 			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
-	}
-
-	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
+	} else
+		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 63e734a..5da20fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -35,7 +35,7 @@
 #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
 	 job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished)
 
-TRACE_EVENT(amdgpu_mm_rreg,
+TRACE_EVENT(amdgpu_device_rreg,
 	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
 	    TP_ARGS(did, reg, value),
 	    TP_STRUCT__entry(
@@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
 		      (unsigned long)__entry->value)
 );
 
-TRACE_EVENT(amdgpu_mm_wreg,
+TRACE_EVENT(amdgpu_device_wreg,
 	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
 	    TP_ARGS(did, reg, value),
 	    TP_STRUCT__entry(
-- 
2.7.4

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

Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Christian König]

Am 18.09.20 um 11:51 schrieb Hawking Zhang:
> support both direct and indirect accessor in unified
> helper functions.
>
> v2: Retire indirect mmio access via mm_index/data
>
> Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
>   4 files changed, 51 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 40ee44b..50341f0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>   
>   void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>   			       uint32_t *buf, size_t size, bool write);
> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> +			    uint32_t reg, uint32_t acc_flags);
> +void amdgpu_device_wreg(struct amdgpu_device *adev,
> +			uint32_t reg, uint32_t v,
>   			uint32_t acc_flags);
> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags);
> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags);
> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> +			     uint32_t reg, uint32_t v);
>   void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value);
>   uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t offset);
>   
> @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>    */
>   #define AMDGPU_REGS_NO_KIQ    (1<<1)
>   
> -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_NO_KIQ)
> -#define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg), AMDGPU_REGS_NO_KIQ)
> +#define WREG32_NO_KIQ(reg, v) amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
>   
>   #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
>   #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v))
> @@ -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>   #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
>   #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
>   
> -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0)
> -#define DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", amdgpu_mm_rreg(adev, (reg), 0))
> -#define WREG32(reg, v) amdgpu_mm_wreg(adev, (reg), (v), 0)
> +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0)
> +#define DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", amdgpu_device_rreg(adev, (reg), 0))
> +#define WREG32(reg, v) amdgpu_device_wreg(adev, (reg), (v), 0)
>   #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>   #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>   #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg))
> @@ -1111,7 +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>   		WREG32_SMC(_Reg, tmp);                          \
>   	} while (0)
>   
> -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " : 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
> +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " : 0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
>   #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
>   #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index abe0c27..2d125b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct file *f,
>   		} else {
>   			r = get_user(value, (uint32_t *)buf);
>   			if (!r)
> -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
> +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
>   		}
>   		if (r) {
>   			result = r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 77785b2..beef764 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>   }
>   
>   /*
> - * MMIO register access helper functions.
> + * register access helper functions.
>    */
>   /**
> - * amdgpu_mm_rreg - read a memory mapped IO register
> + * amdgpu_device_rreg - read a memory mapped IO or indirect register
>    *
>    * @adev: amdgpu_device pointer
>    * @reg: dword aligned register offset
> @@ -314,33 +314,27 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>    *
>    * Returns the 32 bit value from the offset specified.
>    */
> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> -			uint32_t acc_flags)
> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> +			    uint32_t reg, uint32_t acc_flags)
>   {
>   	uint32_t ret;
>   
>   	if (adev->in_pci_err_recovery)
>   		return 0;
>   
> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> -	    down_read_trylock(&adev->reset_sem)) {
> -		ret = amdgpu_kiq_rreg(adev, reg);
> -		up_read(&adev->reset_sem);
> -		return ret;
> -	}
> -
> -	if ((reg * 4) < adev->rmmio_size)
> -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> -	else {
> -		unsigned long flags;
> +	if ((reg * 4) < adev->rmmio_size) {
> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> +		    amdgpu_sriov_runtime(adev) &&
> +		    down_read_trylock(&adev->reset_sem)) {
> +			ret = amdgpu_kiq_rreg(adev, reg);
> +			up_read(&adev->reset_sem);
> +		} else
> +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> +	} else

This should use "} else {". Apart from that looks good to me.

Christian.

> +		ret = adev->pcie_rreg(adev, reg * 4);
>   
> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> -	}
> +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
>   
> -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
>   	return ret;
>   }
>   
> @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value)
>   		BUG();
>   }
>   
> -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> -				       uint32_t reg, uint32_t v,
> -				       uint32_t acc_flags)
> -{
> -	if (adev->in_pci_err_recovery)
> -		return;
> -
> -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
> -
> -	if ((reg * 4) < adev->rmmio_size)
> -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> -	else {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> -	}
> -}
> -
>   /**
> - * amdgpu_mm_wreg - write to a memory mapped IO register
> + * amdgpu_device_wreg - write to a memory mapped IO or indirect register
>    *
>    * @adev: amdgpu_device pointer
>    * @reg: dword aligned register offset
> @@ -425,20 +398,25 @@ static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
>    *
>    * Writes the value specified to the offset specified.
>    */
> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags)
> +void amdgpu_device_wreg(struct amdgpu_device *adev,
> +			uint32_t reg, uint32_t v,
> +			uint32_t acc_flags)
>   {
>   	if (adev->in_pci_err_recovery)
>   		return;
>   
> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> -	    down_read_trylock(&adev->reset_sem)) {
> -		amdgpu_kiq_wreg(adev, reg, v);
> -		up_read(&adev->reset_sem);
> -		return;
> -	}
> +	if ((reg * 4) < adev->rmmio_size) {
> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> +		    amdgpu_sriov_runtime(adev) &&
> +		    down_read_trylock(&adev->reset_sem)) {
> +			amdgpu_kiq_wreg(adev, reg, v);
> +			up_read(&adev->reset_sem);
> +		} else
> +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> +	} else
> +		adev->pcie_wreg(adev, reg * 4, v);
>   
> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
>   }
>   
>   /*
> @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>    *
>    * this function is invoked only the debugfs register access
>    * */
> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags)
> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> +			     uint32_t reg, uint32_t v)
>   {
>   	if (adev->in_pci_err_recovery)
>   		return;
>   
>   	if (amdgpu_sriov_fullaccess(adev) &&
> -		adev->gfx.rlc.funcs &&
> -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
> -
> +	    adev->gfx.rlc.funcs &&
> +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
>   		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
>   			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
> -	}
> -
> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> +	} else
> +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index 63e734a..5da20fc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -35,7 +35,7 @@
>   #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
>   	 job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished)
>   
> -TRACE_EVENT(amdgpu_mm_rreg,
> +TRACE_EVENT(amdgpu_device_rreg,
>   	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>   	    TP_ARGS(did, reg, value),
>   	    TP_STRUCT__entry(
> @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
>   		      (unsigned long)__entry->value)
>   );
>   
> -TRACE_EVENT(amdgpu_mm_wreg,
> +TRACE_EVENT(amdgpu_device_wreg,
>   	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>   	    TP_ARGS(did, reg, value),
>   	    TP_STRUCT__entry(

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

RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Zhang, Hawking]

[AMD Public Use]

> +	} else

RE - This should use "} else {". Apart from that looks good to me.

Hi Chris,

Since there is only one line under "else", I dropped the "{}". I thought that is okay in current kernel coding style and checkpatch.pl also shows no complain.

So you mean you want the code to be the following?

} else {
                ret = adev->pcie_rreg(adev, reg * 4);
}

I can make the change anyway. Does that mean I can get your RB with coding style change for the series?

Regards,
Hawking


-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Friday, September 18, 2020 19:27
To: Zhang, Hawking <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

Am 18.09.20 um 11:51 schrieb Hawking Zhang:
> support both direct and indirect accessor in unified helper functions.
>
> v2: Retire indirect mmio access via mm_index/data
>
> Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
>   4 files changed, 51 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 40ee44b..50341f0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct 
> amdgpu_device *adev);
>   
>   void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>   			       uint32_t *buf, size_t size, bool write); -uint32_t 
> amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> +			    uint32_t reg, uint32_t acc_flags); void 
> +amdgpu_device_wreg(struct amdgpu_device *adev,
> +			uint32_t reg, uint32_t v,
>   			uint32_t acc_flags);
> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags);
> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags);
> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> +			     uint32_t reg, uint32_t v);
>   void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value);
>   uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t 
> offset);
>   
> @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>    */
>   #define AMDGPU_REGS_NO_KIQ    (1<<1)
>   
> -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg), 
> AMDGPU_REGS_NO_KIQ) -#define WREG32_NO_KIQ(reg, v) 
> amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg), 
> +AMDGPU_REGS_NO_KIQ) #define WREG32_NO_KIQ(reg, v) 
> +amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
>   
>   #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
>   #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v)) @@ 
> -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>   #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
>   #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
>   
> -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0) -#define 
> DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", 
> amdgpu_mm_rreg(adev, (reg), 0)) -#define WREG32(reg, v) 
> amdgpu_mm_wreg(adev, (reg), (v), 0)
> +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0) #define 
> +DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", 
> +amdgpu_device_rreg(adev, (reg), 0)) #define WREG32(reg, v) 
> +amdgpu_device_wreg(adev, (reg), (v), 0)
>   #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>   #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>   #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg)) @@ -1111,7 
> +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>   		WREG32_SMC(_Reg, tmp);                          \
>   	} while (0)
>   
> -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " : 
> 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
> +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " : 
> +0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
>   #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
>   #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index abe0c27..2d125b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct file *f,
>   		} else {
>   			r = get_user(value, (uint32_t *)buf);
>   			if (!r)
> -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
> +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
>   		}
>   		if (r) {
>   			result = r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 77785b2..beef764 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>   }
>   
>   /*
> - * MMIO register access helper functions.
> + * register access helper functions.
>    */
>   /**
> - * amdgpu_mm_rreg - read a memory mapped IO register
> + * amdgpu_device_rreg - read a memory mapped IO or indirect register
>    *
>    * @adev: amdgpu_device pointer
>    * @reg: dword aligned register offset @@ -314,33 +314,27 @@ void 
> amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>    *
>    * Returns the 32 bit value from the offset specified.
>    */
> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> -			uint32_t acc_flags)
> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> +			    uint32_t reg, uint32_t acc_flags)
>   {
>   	uint32_t ret;
>   
>   	if (adev->in_pci_err_recovery)
>   		return 0;
>   
> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> -	    down_read_trylock(&adev->reset_sem)) {
> -		ret = amdgpu_kiq_rreg(adev, reg);
> -		up_read(&adev->reset_sem);
> -		return ret;
> -	}
> -
> -	if ((reg * 4) < adev->rmmio_size)
> -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> -	else {
> -		unsigned long flags;
> +	if ((reg * 4) < adev->rmmio_size) {
> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> +		    amdgpu_sriov_runtime(adev) &&
> +		    down_read_trylock(&adev->reset_sem)) {
> +			ret = amdgpu_kiq_rreg(adev, reg);
> +			up_read(&adev->reset_sem);
> +		} else
> +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> +	} else

This should use "} else {". Apart from that looks good to me.

Christian.

> +		ret = adev->pcie_rreg(adev, reg * 4);
>   
> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> -	}
> +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
>   
> -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
>   	return ret;
>   }
>   
> @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value)
>   		BUG();
>   }
>   
> -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> -				       uint32_t reg, uint32_t v,
> -				       uint32_t acc_flags)
> -{
> -	if (adev->in_pci_err_recovery)
> -		return;
> -
> -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
> -
> -	if ((reg * 4) < adev->rmmio_size)
> -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> -	else {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> -	}
> -}
> -
>   /**
> - * amdgpu_mm_wreg - write to a memory mapped IO register
> + * amdgpu_device_wreg - write to a memory mapped IO or indirect 
> + register
>    *
>    * @adev: amdgpu_device pointer
>    * @reg: dword aligned register offset @@ -425,20 +398,25 @@ static 
> inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
>    *
>    * Writes the value specified to the offset specified.
>    */
> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags)
> +void amdgpu_device_wreg(struct amdgpu_device *adev,
> +			uint32_t reg, uint32_t v,
> +			uint32_t acc_flags)
>   {
>   	if (adev->in_pci_err_recovery)
>   		return;
>   
> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> -	    down_read_trylock(&adev->reset_sem)) {
> -		amdgpu_kiq_wreg(adev, reg, v);
> -		up_read(&adev->reset_sem);
> -		return;
> -	}
> +	if ((reg * 4) < adev->rmmio_size) {
> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> +		    amdgpu_sriov_runtime(adev) &&
> +		    down_read_trylock(&adev->reset_sem)) {
> +			amdgpu_kiq_wreg(adev, reg, v);
> +			up_read(&adev->reset_sem);
> +		} else
> +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> +	} else
> +		adev->pcie_wreg(adev, reg * 4, v);
>   
> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
>   }
>   
>   /*
> @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>    *
>    * this function is invoked only the debugfs register access
>    * */
> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> -		    uint32_t acc_flags)
> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> +			     uint32_t reg, uint32_t v)
>   {
>   	if (adev->in_pci_err_recovery)
>   		return;
>   
>   	if (amdgpu_sriov_fullaccess(adev) &&
> -		adev->gfx.rlc.funcs &&
> -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
> -
> +	    adev->gfx.rlc.funcs &&
> +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
>   		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
>   			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
> -	}
> -
> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> +	} else
> +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index 63e734a..5da20fc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -35,7 +35,7 @@
>   #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
>   	 
> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence-
> >finished)
>   
> -TRACE_EVENT(amdgpu_mm_rreg,
> +TRACE_EVENT(amdgpu_device_rreg,
>   	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>   	    TP_ARGS(did, reg, value),
>   	    TP_STRUCT__entry(
> @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
>   		      (unsigned long)__entry->value)
>   );
>   
> -TRACE_EVENT(amdgpu_mm_wreg,
> +TRACE_EVENT(amdgpu_device_wreg,
>   	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>   	    TP_ARGS(did, reg, value),
>   	    TP_STRUCT__entry(
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Russell, Kent]

[AMD Public Use]

It's preferred to have braces if the else has a single line:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
Right above "3.1) Spaces"

 Kent

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhang, Hawking
> Sent: Friday, September 18, 2020 8:07 AM
> To: Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org;
> Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
> <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
> Subject: RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)
> 
> [AMD Public Use]
> 
> > +	} else
> 
> RE - This should use "} else {". Apart from that looks good to me.
> 
> Hi Chris,
> 
> Since there is only one line under "else", I dropped the "{}". I thought that is okay in current
> kernel coding style and checkpatch.pl also shows no complain.
> 
> So you mean you want the code to be the following?
> 
> } else {
>                 ret = adev->pcie_rreg(adev, reg * 4);
> }
> 
> I can make the change anyway. Does that mean I can get your RB with coding style change
> for the series?
> 
> Regards,
> Hawking
> 
> 
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Friday, September 18, 2020 19:27
> To: Zhang, Hawking <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org; Deucher,
> Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
> <Kevin1.Wang@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Chen, Guchun
> <Guchun.Chen@amd.com>
> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)
> 
> Am 18.09.20 um 11:51 schrieb Hawking Zhang:
> > support both direct and indirect accessor in unified helper functions.
> >
> > v2: Retire indirect mmio access via mm_index/data
> >
> > Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
> >   4 files changed, 51 insertions(+), 74 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 40ee44b..50341f0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct
> > amdgpu_device *adev);
> >
> >   void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> >   			       uint32_t *buf, size_t size, bool write); -uint32_t
> > amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> > +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> > +			    uint32_t reg, uint32_t acc_flags); void
> > +amdgpu_device_wreg(struct amdgpu_device *adev,
> > +			uint32_t reg, uint32_t v,
> >   			uint32_t acc_flags);
> > -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> > -		    uint32_t acc_flags);
> > -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> > -		    uint32_t acc_flags);
> > +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> > +			     uint32_t reg, uint32_t v);
> >   void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value);
> >   uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t
> > offset);
> >
> > @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >    */
> >   #define AMDGPU_REGS_NO_KIQ    (1<<1)
> >
> > -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg),
> > AMDGPU_REGS_NO_KIQ) -#define WREG32_NO_KIQ(reg, v)
> > amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> > +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg),
> > +AMDGPU_REGS_NO_KIQ) #define WREG32_NO_KIQ(reg, v)
> > +amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> >
> >   #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
> >   #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v)) @@
> > -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >   #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
> >   #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
> >
> > -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0) -#define
> > DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
> > amdgpu_mm_rreg(adev, (reg), 0)) -#define WREG32(reg, v)
> > amdgpu_mm_wreg(adev, (reg), (v), 0)
> > +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0) #define
> > +DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
> > +amdgpu_device_rreg(adev, (reg), 0)) #define WREG32(reg, v)
> > +amdgpu_device_wreg(adev, (reg), (v), 0)
> >   #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
> >   #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
> >   #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg)) @@ -1111,7
> > +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >   		WREG32_SMC(_Reg, tmp);                          \
> >   	} while (0)
> >
> > -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
> > 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
> > +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
> > +0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
> >   #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
> >   #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > index abe0c27..2d125b8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct file
> *f,
> >   		} else {
> >   			r = get_user(value, (uint32_t *)buf);
> >   			if (!r)
> > -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
> > +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
> >   		}
> >   		if (r) {
> >   			result = r;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 77785b2..beef764 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device
> *adev, loff_t pos,
> >   }
> >
> >   /*
> > - * MMIO register access helper functions.
> > + * register access helper functions.
> >    */
> >   /**
> > - * amdgpu_mm_rreg - read a memory mapped IO register
> > + * amdgpu_device_rreg - read a memory mapped IO or indirect register
> >    *
> >    * @adev: amdgpu_device pointer
> >    * @reg: dword aligned register offset @@ -314,33 +314,27 @@ void
> > amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> >    *
> >    * Returns the 32 bit value from the offset specified.
> >    */
> > -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> > -			uint32_t acc_flags)
> > +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> > +			    uint32_t reg, uint32_t acc_flags)
> >   {
> >   	uint32_t ret;
> >
> >   	if (adev->in_pci_err_recovery)
> >   		return 0;
> >
> > -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> > -	    down_read_trylock(&adev->reset_sem)) {
> > -		ret = amdgpu_kiq_rreg(adev, reg);
> > -		up_read(&adev->reset_sem);
> > -		return ret;
> > -	}
> > -
> > -	if ((reg * 4) < adev->rmmio_size)
> > -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> > -	else {
> > -		unsigned long flags;
> > +	if ((reg * 4) < adev->rmmio_size) {
> > +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> > +		    amdgpu_sriov_runtime(adev) &&
> > +		    down_read_trylock(&adev->reset_sem)) {
> > +			ret = amdgpu_kiq_rreg(adev, reg);
> > +			up_read(&adev->reset_sem);
> > +		} else
> > +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> > +	} else
> 
> This should use "} else {". Apart from that looks good to me.
> 
> Christian.
> 
> > +		ret = adev->pcie_rreg(adev, reg * 4);
> >
> > -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> > -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> > -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> > -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> > -	}
> > +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
> >
> > -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
> >   	return ret;
> >   }
> >
> > @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t
> offset, uint8_t value)
> >   		BUG();
> >   }
> >
> > -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> > -				       uint32_t reg, uint32_t v,
> > -				       uint32_t acc_flags)
> > -{
> > -	if (adev->in_pci_err_recovery)
> > -		return;
> > -
> > -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
> > -
> > -	if ((reg * 4) < adev->rmmio_size)
> > -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> > -	else {
> > -		unsigned long flags;
> > -
> > -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> > -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> > -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> > -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> > -	}
> > -}
> > -
> >   /**
> > - * amdgpu_mm_wreg - write to a memory mapped IO register
> > + * amdgpu_device_wreg - write to a memory mapped IO or indirect
> > + register
> >    *
> >    * @adev: amdgpu_device pointer
> >    * @reg: dword aligned register offset @@ -425,20 +398,25 @@ static
> > inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> >    *
> >    * Writes the value specified to the offset specified.
> >    */
> > -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> > -		    uint32_t acc_flags)
> > +void amdgpu_device_wreg(struct amdgpu_device *adev,
> > +			uint32_t reg, uint32_t v,
> > +			uint32_t acc_flags)
> >   {
> >   	if (adev->in_pci_err_recovery)
> >   		return;
> >
> > -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> > -	    down_read_trylock(&adev->reset_sem)) {
> > -		amdgpu_kiq_wreg(adev, reg, v);
> > -		up_read(&adev->reset_sem);
> > -		return;
> > -	}
> > +	if ((reg * 4) < adev->rmmio_size) {
> > +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> > +		    amdgpu_sriov_runtime(adev) &&
> > +		    down_read_trylock(&adev->reset_sem)) {
> > +			amdgpu_kiq_wreg(adev, reg, v);
> > +			up_read(&adev->reset_sem);
> > +		} else
> > +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> > +	} else
> > +		adev->pcie_wreg(adev, reg * 4, v);
> >
> > -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> > +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
> >   }
> >
> >   /*
> > @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t
> reg, uint32_t v,
> >    *
> >    * this function is invoked only the debugfs register access
> >    * */
> > -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> > -		    uint32_t acc_flags)
> > +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> > +			     uint32_t reg, uint32_t v)
> >   {
> >   	if (adev->in_pci_err_recovery)
> >   		return;
> >
> >   	if (amdgpu_sriov_fullaccess(adev) &&
> > -		adev->gfx.rlc.funcs &&
> > -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
> > -
> > +	    adev->gfx.rlc.funcs &&
> > +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
> >   		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
> >   			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
> > -	}
> > -
> > -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> > +	} else
> > +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >   }
> >
> >   /**
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> > index 63e734a..5da20fc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> > @@ -35,7 +35,7 @@
> >   #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
> >
> > job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence-
> > >finished)
> >
> > -TRACE_EVENT(amdgpu_mm_rreg,
> > +TRACE_EVENT(amdgpu_device_rreg,
> >   	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
> >   	    TP_ARGS(did, reg, value),
> >   	    TP_STRUCT__entry(
> > @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
> >   		      (unsigned long)__entry->value)
> >   );
> >
> > -TRACE_EVENT(amdgpu_mm_wreg,
> > +TRACE_EVENT(amdgpu_device_wreg,
> >   	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
> >   	    TP_ARGS(did, reg, value),
> >   	    TP_STRUCT__entry(
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.or
> g%2Fmailman%2Flistinfo%2Famd-
> gfx&amp;data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb
> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241&amp;
> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Christian König]

To be more precise you can either use "else" or "} else {", but don't 
use "} else" or "else {".

Interesting that checkpatch.pl doesn't complain, I thought that this 
would be checked.

Regards,
Christian.

Am 18.09.20 um 14:12 schrieb Russell, Kent:
> [AMD Public Use]
>
> It's preferred to have braces if the else has a single line:
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
> Right above "3.1) Spaces"
>
>   Kent
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhang, Hawking
>> Sent: Friday, September 18, 2020 8:07 AM
>> To: Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org;
>> Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
>> <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
>> Subject: RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)
>>
>> [AMD Public Use]
>>
>>> +	} else
>> RE - This should use "} else {". Apart from that looks good to me.
>>
>> Hi Chris,
>>
>> Since there is only one line under "else", I dropped the "{}". I thought that is okay in current
>> kernel coding style and checkpatch.pl also shows no complain.
>>
>> So you mean you want the code to be the following?
>>
>> } else {
>>                  ret = adev->pcie_rreg(adev, reg * 4);
>> }
>>
>> I can make the change anyway. Does that mean I can get your RB with coding style change
>> for the series?
>>
>> Regards,
>> Hawking
>>
>>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Friday, September 18, 2020 19:27
>> To: Zhang, Hawking <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org; Deucher,
>> Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
>> <Kevin1.Wang@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Chen, Guchun
>> <Guchun.Chen@amd.com>
>> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)
>>
>> Am 18.09.20 um 11:51 schrieb Hawking Zhang:
>>> support both direct and indirect accessor in unified helper functions.
>>>
>>> v2: Retire indirect mmio access via mm_index/data
>>>
>>> Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
>>>    4 files changed, 51 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 40ee44b..50341f0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct
>>> amdgpu_device *adev);
>>>
>>>    void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>>>    			       uint32_t *buf, size_t size, bool write); -uint32_t
>>> amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
>>> +			    uint32_t reg, uint32_t acc_flags); void
>>> +amdgpu_device_wreg(struct amdgpu_device *adev,
>>> +			uint32_t reg, uint32_t v,
>>>    			uint32_t acc_flags);
>>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>> -		    uint32_t acc_flags);
>>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>> -		    uint32_t acc_flags);
>>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
>>> +			     uint32_t reg, uint32_t v);
>>>    void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t value);
>>>    uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t
>>> offset);
>>>
>>> @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>     */
>>>    #define AMDGPU_REGS_NO_KIQ    (1<<1)
>>>
>>> -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg),
>>> AMDGPU_REGS_NO_KIQ) -#define WREG32_NO_KIQ(reg, v)
>>> amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
>>> +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg),
>>> +AMDGPU_REGS_NO_KIQ) #define WREG32_NO_KIQ(reg, v)
>>> +amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
>>>
>>>    #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
>>>    #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v)) @@
>>> -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>    #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
>>>    #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
>>>
>>> -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0) -#define
>>> DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
>>> amdgpu_mm_rreg(adev, (reg), 0)) -#define WREG32(reg, v)
>>> amdgpu_mm_wreg(adev, (reg), (v), 0)
>>> +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0) #define
>>> +DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
>>> +amdgpu_device_rreg(adev, (reg), 0)) #define WREG32(reg, v)
>>> +amdgpu_device_wreg(adev, (reg), (v), 0)
>>>    #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>>>    #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>>>    #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg)) @@ -1111,7
>>> +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>    		WREG32_SMC(_Reg, tmp);                          \
>>>    	} while (0)
>>>
>>> -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
>>> 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
>>> +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
>>> +0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
>>>    #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
>>>    #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index abe0c27..2d125b8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct file
>> *f,
>>>    		} else {
>>>    			r = get_user(value, (uint32_t *)buf);
>>>    			if (!r)
>>> -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
>>> +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
>>>    		}
>>>    		if (r) {
>>>    			result = r;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 77785b2..beef764 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device
>> *adev, loff_t pos,
>>>    }
>>>
>>>    /*
>>> - * MMIO register access helper functions.
>>> + * register access helper functions.
>>>     */
>>>    /**
>>> - * amdgpu_mm_rreg - read a memory mapped IO register
>>> + * amdgpu_device_rreg - read a memory mapped IO or indirect register
>>>     *
>>>     * @adev: amdgpu_device pointer
>>>     * @reg: dword aligned register offset @@ -314,33 +314,27 @@ void
>>> amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>>>     *
>>>     * Returns the 32 bit value from the offset specified.
>>>     */
>>> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>>> -			uint32_t acc_flags)
>>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
>>> +			    uint32_t reg, uint32_t acc_flags)
>>>    {
>>>    	uint32_t ret;
>>>
>>>    	if (adev->in_pci_err_recovery)
>>>    		return 0;
>>>
>>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
>>> -	    down_read_trylock(&adev->reset_sem)) {
>>> -		ret = amdgpu_kiq_rreg(adev, reg);
>>> -		up_read(&adev->reset_sem);
>>> -		return ret;
>>> -	}
>>> -
>>> -	if ((reg * 4) < adev->rmmio_size)
>>> -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
>>> -	else {
>>> -		unsigned long flags;
>>> +	if ((reg * 4) < adev->rmmio_size) {
>>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
>>> +		    amdgpu_sriov_runtime(adev) &&
>>> +		    down_read_trylock(&adev->reset_sem)) {
>>> +			ret = amdgpu_kiq_rreg(adev, reg);
>>> +			up_read(&adev->reset_sem);
>>> +		} else
>>> +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
>>> +	} else
>> This should use "} else {". Apart from that looks good to me.
>>
>> Christian.
>>
>>> +		ret = adev->pcie_rreg(adev, reg * 4);
>>>
>>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
>>> -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
>>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>> -	}
>>> +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
>>>
>>> -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
>>>    	return ret;
>>>    }
>>>
>>> @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t
>> offset, uint8_t value)
>>>    		BUG();
>>>    }
>>>
>>> -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
>>> -				       uint32_t reg, uint32_t v,
>>> -				       uint32_t acc_flags)
>>> -{
>>> -	if (adev->in_pci_err_recovery)
>>> -		return;
>>> -
>>> -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
>>> -
>>> -	if ((reg * 4) < adev->rmmio_size)
>>> -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>>> -	else {
>>> -		unsigned long flags;
>>> -
>>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
>>> -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
>>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>> -	}
>>> -}
>>> -
>>>    /**
>>> - * amdgpu_mm_wreg - write to a memory mapped IO register
>>> + * amdgpu_device_wreg - write to a memory mapped IO or indirect
>>> + register
>>>     *
>>>     * @adev: amdgpu_device pointer
>>>     * @reg: dword aligned register offset @@ -425,20 +398,25 @@ static
>>> inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
>>>     *
>>>     * Writes the value specified to the offset specified.
>>>     */
>>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>> -		    uint32_t acc_flags)
>>> +void amdgpu_device_wreg(struct amdgpu_device *adev,
>>> +			uint32_t reg, uint32_t v,
>>> +			uint32_t acc_flags)
>>>    {
>>>    	if (adev->in_pci_err_recovery)
>>>    		return;
>>>
>>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
>>> -	    down_read_trylock(&adev->reset_sem)) {
>>> -		amdgpu_kiq_wreg(adev, reg, v);
>>> -		up_read(&adev->reset_sem);
>>> -		return;
>>> -	}
>>> +	if ((reg * 4) < adev->rmmio_size) {
>>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
>>> +		    amdgpu_sriov_runtime(adev) &&
>>> +		    down_read_trylock(&adev->reset_sem)) {
>>> +			amdgpu_kiq_wreg(adev, reg, v);
>>> +			up_read(&adev->reset_sem);
>>> +		} else
>>> +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>>> +	} else
>>> +		adev->pcie_wreg(adev, reg * 4, v);
>>>
>>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
>>> +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
>>>    }
>>>
>>>    /*
>>> @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t
>> reg, uint32_t v,
>>>     *
>>>     * this function is invoked only the debugfs register access
>>>     * */
>>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>> -		    uint32_t acc_flags)
>>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
>>> +			     uint32_t reg, uint32_t v)
>>>    {
>>>    	if (adev->in_pci_err_recovery)
>>>    		return;
>>>
>>>    	if (amdgpu_sriov_fullaccess(adev) &&
>>> -		adev->gfx.rlc.funcs &&
>>> -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
>>> -
>>> +	    adev->gfx.rlc.funcs &&
>>> +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
>>>    		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
>>>    			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
>>> -	}
>>> -
>>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
>>> +	} else
>>> +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>>>    }
>>>
>>>    /**
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> index 63e734a..5da20fc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> @@ -35,7 +35,7 @@
>>>    #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
>>>
>>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence-
>>>> finished)
>>> -TRACE_EVENT(amdgpu_mm_rreg,
>>> +TRACE_EVENT(amdgpu_device_rreg,
>>>    	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>>>    	    TP_ARGS(did, reg, value),
>>>    	    TP_STRUCT__entry(
>>> @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
>>>    		      (unsigned long)__entry->value)
>>>    );
>>>
>>> -TRACE_EVENT(amdgpu_mm_wreg,
>>> +TRACE_EVENT(amdgpu_device_wreg,
>>>    	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>>>    	    TP_ARGS(did, reg, value),
>>>    	    TP_STRUCT__entry(
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.or
>> g%2Fmailman%2Flistinfo%2Famd-
>> gfx&amp;data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb
>> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241&amp;
>> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&amp;reserved=0

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

RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Russell, Kent]

[AMD Public Use]

Thanks for clarifying my ambiguity, Christian. And for sanity, I just ran checkpatch on a local file with an "} else" and it came up as warning/error-free. Peculiar.

 Kent 

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, September 18, 2020 8:17 AM
> To: Russell, Kent <Kent.Russell@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
> amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>;
> Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)
> 
> To be more precise you can either use "else" or "} else {", but don't
> use "} else" or "else {".
> 
> Interesting that checkpatch.pl doesn't complain, I thought that this
> would be checked.
> 
> Regards,
> Christian.
> 
> Am 18.09.20 um 14:12 schrieb Russell, Kent:
> > [AMD Public Use]
> >
> > It's preferred to have braces if the else has a single line:
> > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-
> spaces
> > Right above "3.1) Spaces"
> >
> >   Kent
> >
> >> -----Original Message-----
> >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhang, Hawking
> >> Sent: Friday, September 18, 2020 8:07 AM
> >> To: Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org;
> >> Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
> >> <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
> >> Subject: RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar
> (v2)
> >>
> >> [AMD Public Use]
> >>
> >>> +	} else
> >> RE - This should use "} else {". Apart from that looks good to me.
> >>
> >> Hi Chris,
> >>
> >> Since there is only one line under "else", I dropped the "{}". I thought that is okay in
> current
> >> kernel coding style and checkpatch.pl also shows no complain.
> >>
> >> So you mean you want the code to be the following?
> >>
> >> } else {
> >>                  ret = adev->pcie_rreg(adev, reg * 4);
> >> }
> >>
> >> I can make the change anyway. Does that mean I can get your RB with coding style
> change
> >> for the series?
> >>
> >> Regards,
> >> Hawking
> >>
> >>
> >> -----Original Message-----
> >> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >> Sent: Friday, September 18, 2020 19:27
> >> To: Zhang, Hawking <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org;
> Deucher,
> >> Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
> >> <Kevin1.Wang@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Chen,
> Guchun
> >> <Guchun.Chen@amd.com>
> >> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar
> (v2)
> >>
> >> Am 18.09.20 um 11:51 schrieb Hawking Zhang:
> >>> support both direct and indirect accessor in unified helper functions.
> >>>
> >>> v2: Retire indirect mmio access via mm_index/data
> >>>
> >>> Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
> >>> ---
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
> >>>    4 files changed, 51 insertions(+), 74 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index 40ee44b..50341f0 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct
> >>> amdgpu_device *adev);
> >>>
> >>>    void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> >>>    			       uint32_t *buf, size_t size, bool write); -uint32_t
> >>> amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> >>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> >>> +			    uint32_t reg, uint32_t acc_flags); void
> >>> +amdgpu_device_wreg(struct amdgpu_device *adev,
> >>> +			uint32_t reg, uint32_t v,
> >>>    			uint32_t acc_flags);
> >>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> >>> -		    uint32_t acc_flags);
> >>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg,
> uint32_t v,
> >>> -		    uint32_t acc_flags);
> >>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> >>> +			     uint32_t reg, uint32_t v);
> >>>    void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t
> value);
> >>>    uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t
> >>> offset);
> >>>
> >>> @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >>>     */
> >>>    #define AMDGPU_REGS_NO_KIQ    (1<<1)
> >>>
> >>> -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg),
> >>> AMDGPU_REGS_NO_KIQ) -#define WREG32_NO_KIQ(reg, v)
> >>> amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> >>> +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg),
> >>> +AMDGPU_REGS_NO_KIQ) #define WREG32_NO_KIQ(reg, v)
> >>> +amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> >>>
> >>>    #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
> >>>    #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v)) @@
> >>> -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >>>    #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
> >>>    #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
> >>>
> >>> -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0) -#define
> >>> DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
> >>> amdgpu_mm_rreg(adev, (reg), 0)) -#define WREG32(reg, v)
> >>> amdgpu_mm_wreg(adev, (reg), (v), 0)
> >>> +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0) #define
> >>> +DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
> >>> +amdgpu_device_rreg(adev, (reg), 0)) #define WREG32(reg, v)
> >>> +amdgpu_device_wreg(adev, (reg), (v), 0)
> >>>    #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
> >>>    #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
> >>>    #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg)) @@ -1111,7
> >>> +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >>>    		WREG32_SMC(_Reg, tmp);                          \
> >>>    	} while (0)
> >>>
> >>> -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
> >>> 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
> >>> +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
> >>> +0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
> >>>    #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
> >>>    #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> index abe0c27..2d125b8 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct
> file
> >> *f,
> >>>    		} else {
> >>>    			r = get_user(value, (uint32_t *)buf);
> >>>    			if (!r)
> >>> -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
> >>> +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
> >>>    		}
> >>>    		if (r) {
> >>>    			result = r;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> index 77785b2..beef764 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device
> >> *adev, loff_t pos,
> >>>    }
> >>>
> >>>    /*
> >>> - * MMIO register access helper functions.
> >>> + * register access helper functions.
> >>>     */
> >>>    /**
> >>> - * amdgpu_mm_rreg - read a memory mapped IO register
> >>> + * amdgpu_device_rreg - read a memory mapped IO or indirect register
> >>>     *
> >>>     * @adev: amdgpu_device pointer
> >>>     * @reg: dword aligned register offset @@ -314,33 +314,27 @@ void
> >>> amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> >>>     *
> >>>     * Returns the 32 bit value from the offset specified.
> >>>     */
> >>> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> >>> -			uint32_t acc_flags)
> >>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> >>> +			    uint32_t reg, uint32_t acc_flags)
> >>>    {
> >>>    	uint32_t ret;
> >>>
> >>>    	if (adev->in_pci_err_recovery)
> >>>    		return 0;
> >>>
> >>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> >>> -	    down_read_trylock(&adev->reset_sem)) {
> >>> -		ret = amdgpu_kiq_rreg(adev, reg);
> >>> -		up_read(&adev->reset_sem);
> >>> -		return ret;
> >>> -	}
> >>> -
> >>> -	if ((reg * 4) < adev->rmmio_size)
> >>> -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> >>> -	else {
> >>> -		unsigned long flags;
> >>> +	if ((reg * 4) < adev->rmmio_size) {
> >>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> >>> +		    amdgpu_sriov_runtime(adev) &&
> >>> +		    down_read_trylock(&adev->reset_sem)) {
> >>> +			ret = amdgpu_kiq_rreg(adev, reg);
> >>> +			up_read(&adev->reset_sem);
> >>> +		} else
> >>> +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> >>> +	} else
> >> This should use "} else {". Apart from that looks good to me.
> >>
> >> Christian.
> >>
> >>> +		ret = adev->pcie_rreg(adev, reg * 4);
> >>>
> >>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> >>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> >>> -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> >>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> >>> -	}
> >>> +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
> >>>
> >>> -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
> >>>    	return ret;
> >>>    }
> >>>
> >>> @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev,
> uint32_t
> >> offset, uint8_t value)
> >>>    		BUG();
> >>>    }
> >>>
> >>> -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> >>> -				       uint32_t reg, uint32_t v,
> >>> -				       uint32_t acc_flags)
> >>> -{
> >>> -	if (adev->in_pci_err_recovery)
> >>> -		return;
> >>> -
> >>> -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
> >>> -
> >>> -	if ((reg * 4) < adev->rmmio_size)
> >>> -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >>> -	else {
> >>> -		unsigned long flags;
> >>> -
> >>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> >>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> >>> -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> >>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> >>> -	}
> >>> -}
> >>> -
> >>>    /**
> >>> - * amdgpu_mm_wreg - write to a memory mapped IO register
> >>> + * amdgpu_device_wreg - write to a memory mapped IO or indirect
> >>> + register
> >>>     *
> >>>     * @adev: amdgpu_device pointer
> >>>     * @reg: dword aligned register offset @@ -425,20 +398,25 @@ static
> >>> inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> >>>     *
> >>>     * Writes the value specified to the offset specified.
> >>>     */
> >>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> >>> -		    uint32_t acc_flags)
> >>> +void amdgpu_device_wreg(struct amdgpu_device *adev,
> >>> +			uint32_t reg, uint32_t v,
> >>> +			uint32_t acc_flags)
> >>>    {
> >>>    	if (adev->in_pci_err_recovery)
> >>>    		return;
> >>>
> >>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> >>> -	    down_read_trylock(&adev->reset_sem)) {
> >>> -		amdgpu_kiq_wreg(adev, reg, v);
> >>> -		up_read(&adev->reset_sem);
> >>> -		return;
> >>> -	}
> >>> +	if ((reg * 4) < adev->rmmio_size) {
> >>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> >>> +		    amdgpu_sriov_runtime(adev) &&
> >>> +		    down_read_trylock(&adev->reset_sem)) {
> >>> +			amdgpu_kiq_wreg(adev, reg, v);
> >>> +			up_read(&adev->reset_sem);
> >>> +		} else
> >>> +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >>> +	} else
> >>> +		adev->pcie_wreg(adev, reg * 4, v);
> >>>
> >>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> >>> +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
> >>>    }
> >>>
> >>>    /*
> >>> @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev,
> uint32_t
> >> reg, uint32_t v,
> >>>     *
> >>>     * this function is invoked only the debugfs register access
> >>>     * */
> >>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg,
> uint32_t v,
> >>> -		    uint32_t acc_flags)
> >>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> >>> +			     uint32_t reg, uint32_t v)
> >>>    {
> >>>    	if (adev->in_pci_err_recovery)
> >>>    		return;
> >>>
> >>>    	if (amdgpu_sriov_fullaccess(adev) &&
> >>> -		adev->gfx.rlc.funcs &&
> >>> -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
> >>> -
> >>> +	    adev->gfx.rlc.funcs &&
> >>> +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
> >>>    		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
> >>>    			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
> >>> -	}
> >>> -
> >>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> >>> +	} else
> >>> +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >>>    }
> >>>
> >>>    /**
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> index 63e734a..5da20fc 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> @@ -35,7 +35,7 @@
> >>>    #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
> >>>
> >>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence-
> >>>> finished)
> >>> -TRACE_EVENT(amdgpu_mm_rreg,
> >>> +TRACE_EVENT(amdgpu_device_rreg,
> >>>    	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
> >>>    	    TP_ARGS(did, reg, value),
> >>>    	    TP_STRUCT__entry(
> >>> @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
> >>>    		      (unsigned long)__entry->value)
> >>>    );
> >>>
> >>> -TRACE_EVENT(amdgpu_mm_wreg,
> >>> +TRACE_EVENT(amdgpu_device_wreg,
> >>>    	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
> >>>    	    TP_ARGS(did, reg, value),
> >>>    	    TP_STRUCT__entry(
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.or
> >> g%2Fmailman%2Flistinfo%2Famd-
> >>
> gfx&amp;data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb
> >>
> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241&amp;
> >> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Christian König]

You comment was clear enough, I just wanted to explain it inline instead 
of referencing the documentation :)

Christian.


Am 18.09.20 um 15:32 schrieb Russell, Kent:
> [AMD Public Use]
>
> Thanks for clarifying my ambiguity, Christian. And for sanity, I just ran checkpatch on a local file with an "} else" and it came up as warning/error-free. Peculiar.
>
>   Kent
>
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, September 18, 2020 8:17 AM
>> To: Russell, Kent <Kent.Russell@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>;
>> Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
>> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)
>>
>> To be more precise you can either use "else" or "} else {", but don't
>> use "} else" or "else {".
>>
>> Interesting that checkpatch.pl doesn't complain, I thought that this
>> would be checked.
>>
>> Regards,
>> Christian.
>>
>> Am 18.09.20 um 14:12 schrieb Russell, Kent:
>>> [AMD Public Use]
>>>
>>> It's preferred to have braces if the else has a single line:
>>> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-
>> spaces
>>> Right above "3.1) Spaces"
>>>
>>>    Kent
>>>
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhang, Hawking
>>>> Sent: Friday, September 18, 2020 8:07 AM
>>>> To: Koenig, Christian <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org;
>>>> Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
>>>> <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
>>>> Subject: RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar
>> (v2)
>>>> [AMD Public Use]
>>>>
>>>>> +	} else
>>>> RE - This should use "} else {". Apart from that looks good to me.
>>>>
>>>> Hi Chris,
>>>>
>>>> Since there is only one line under "else", I dropped the "{}". I thought that is okay in
>> current
>>>> kernel coding style and checkpatch.pl also shows no complain.
>>>>
>>>> So you mean you want the code to be the following?
>>>>
>>>> } else {
>>>>                   ret = adev->pcie_rreg(adev, reg * 4);
>>>> }
>>>>
>>>> I can make the change anyway. Does that mean I can get your RB with coding style
>> change
>>>> for the series?
>>>>
>>>> Regards,
>>>> Hawking
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>> Sent: Friday, September 18, 2020 19:27
>>>> To: Zhang, Hawking <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org;
>> Deucher,
>>>> Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang)
>>>> <Kevin1.Wang@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Chen,
>> Guchun
>>>> <Guchun.Chen@amd.com>
>>>> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar
>> (v2)
>>>> Am 18.09.20 um 11:51 schrieb Hawking Zhang:
>>>>> support both direct and indirect accessor in unified helper functions.
>>>>>
>>>>> v2: Retire indirect mmio access via mm_index/data
>>>>>
>>>>> Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
>>>>>     4 files changed, 51 insertions(+), 74 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> index 40ee44b..50341f0 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct
>>>>> amdgpu_device *adev);
>>>>>
>>>>>     void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>>>>>     			       uint32_t *buf, size_t size, bool write); -uint32_t
>>>>> amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>>>>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
>>>>> +			    uint32_t reg, uint32_t acc_flags); void
>>>>> +amdgpu_device_wreg(struct amdgpu_device *adev,
>>>>> +			uint32_t reg, uint32_t v,
>>>>>     			uint32_t acc_flags);
>>>>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>>>> -		    uint32_t acc_flags);
>>>>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg,
>> uint32_t v,
>>>>> -		    uint32_t acc_flags);
>>>>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
>>>>> +			     uint32_t reg, uint32_t v);
>>>>>     void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t offset, uint8_t
>> value);
>>>>>     uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t
>>>>> offset);
>>>>>
>>>>> @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>>>      */
>>>>>     #define AMDGPU_REGS_NO_KIQ    (1<<1)
>>>>>
>>>>> -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg),
>>>>> AMDGPU_REGS_NO_KIQ) -#define WREG32_NO_KIQ(reg, v)
>>>>> amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
>>>>> +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg),
>>>>> +AMDGPU_REGS_NO_KIQ) #define WREG32_NO_KIQ(reg, v)
>>>>> +amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
>>>>>
>>>>>     #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
>>>>>     #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v)) @@
>>>>> -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>>>     #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
>>>>>     #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
>>>>>
>>>>> -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0) -#define
>>>>> DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
>>>>> amdgpu_mm_rreg(adev, (reg), 0)) -#define WREG32(reg, v)
>>>>> amdgpu_mm_wreg(adev, (reg), (v), 0)
>>>>> +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0) #define
>>>>> +DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n",
>>>>> +amdgpu_device_rreg(adev, (reg), 0)) #define WREG32(reg, v)
>>>>> +amdgpu_device_wreg(adev, (reg), (v), 0)
>>>>>     #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>>>>>     #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
>>>>>     #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg)) @@ -1111,7
>>>>> +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>>>>>     		WREG32_SMC(_Reg, tmp);                          \
>>>>>     	} while (0)
>>>>>
>>>>> -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
>>>>> 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
>>>>> +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
>>>>> +0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
>>>>>     #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
>>>>>     #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> index abe0c27..2d125b8 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool read, struct
>> file
>>>> *f,
>>>>>     		} else {
>>>>>     			r = get_user(value, (uint32_t *)buf);
>>>>>     			if (!r)
>>>>> -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
>>>>> +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
>>>>>     		}
>>>>>     		if (r) {
>>>>>     			result = r;
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index 77785b2..beef764 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct amdgpu_device
>>>> *adev, loff_t pos,
>>>>>     }
>>>>>
>>>>>     /*
>>>>> - * MMIO register access helper functions.
>>>>> + * register access helper functions.
>>>>>      */
>>>>>     /**
>>>>> - * amdgpu_mm_rreg - read a memory mapped IO register
>>>>> + * amdgpu_device_rreg - read a memory mapped IO or indirect register
>>>>>      *
>>>>>      * @adev: amdgpu_device pointer
>>>>>      * @reg: dword aligned register offset @@ -314,33 +314,27 @@ void
>>>>> amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>>>>>      *
>>>>>      * Returns the 32 bit value from the offset specified.
>>>>>      */
>>>>> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>>>>> -			uint32_t acc_flags)
>>>>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
>>>>> +			    uint32_t reg, uint32_t acc_flags)
>>>>>     {
>>>>>     	uint32_t ret;
>>>>>
>>>>>     	if (adev->in_pci_err_recovery)
>>>>>     		return 0;
>>>>>
>>>>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
>>>>> -	    down_read_trylock(&adev->reset_sem)) {
>>>>> -		ret = amdgpu_kiq_rreg(adev, reg);
>>>>> -		up_read(&adev->reset_sem);
>>>>> -		return ret;
>>>>> -	}
>>>>> -
>>>>> -	if ((reg * 4) < adev->rmmio_size)
>>>>> -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
>>>>> -	else {
>>>>> -		unsigned long flags;
>>>>> +	if ((reg * 4) < adev->rmmio_size) {
>>>>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
>>>>> +		    amdgpu_sriov_runtime(adev) &&
>>>>> +		    down_read_trylock(&adev->reset_sem)) {
>>>>> +			ret = amdgpu_kiq_rreg(adev, reg);
>>>>> +			up_read(&adev->reset_sem);
>>>>> +		} else
>>>>> +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
>>>>> +	} else
>>>> This should use "} else {". Apart from that looks good to me.
>>>>
>>>> Christian.
>>>>
>>>>> +		ret = adev->pcie_rreg(adev, reg * 4);
>>>>>
>>>>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>>>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
>>>>> -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
>>>>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>>>> -	}
>>>>> +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
>>>>>
>>>>> -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
>>>>>     	return ret;
>>>>>     }
>>>>>
>>>>> @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device *adev,
>> uint32_t
>>>> offset, uint8_t value)
>>>>>     		BUG();
>>>>>     }
>>>>>
>>>>> -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
>>>>> -				       uint32_t reg, uint32_t v,
>>>>> -				       uint32_t acc_flags)
>>>>> -{
>>>>> -	if (adev->in_pci_err_recovery)
>>>>> -		return;
>>>>> -
>>>>> -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
>>>>> -
>>>>> -	if ((reg * 4) < adev->rmmio_size)
>>>>> -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>>>>> -	else {
>>>>> -		unsigned long flags;
>>>>> -
>>>>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>>>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
>>>>> -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
>>>>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>>>> -	}
>>>>> -}
>>>>> -
>>>>>     /**
>>>>> - * amdgpu_mm_wreg - write to a memory mapped IO register
>>>>> + * amdgpu_device_wreg - write to a memory mapped IO or indirect
>>>>> + register
>>>>>      *
>>>>>      * @adev: amdgpu_device pointer
>>>>>      * @reg: dword aligned register offset @@ -425,20 +398,25 @@ static
>>>>> inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
>>>>>      *
>>>>>      * Writes the value specified to the offset specified.
>>>>>      */
>>>>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>>>>> -		    uint32_t acc_flags)
>>>>> +void amdgpu_device_wreg(struct amdgpu_device *adev,
>>>>> +			uint32_t reg, uint32_t v,
>>>>> +			uint32_t acc_flags)
>>>>>     {
>>>>>     	if (adev->in_pci_err_recovery)
>>>>>     		return;
>>>>>
>>>>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
>>>>> -	    down_read_trylock(&adev->reset_sem)) {
>>>>> -		amdgpu_kiq_wreg(adev, reg, v);
>>>>> -		up_read(&adev->reset_sem);
>>>>> -		return;
>>>>> -	}
>>>>> +	if ((reg * 4) < adev->rmmio_size) {
>>>>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
>>>>> +		    amdgpu_sriov_runtime(adev) &&
>>>>> +		    down_read_trylock(&adev->reset_sem)) {
>>>>> +			amdgpu_kiq_wreg(adev, reg, v);
>>>>> +			up_read(&adev->reset_sem);
>>>>> +		} else
>>>>> +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>>>>> +	} else
>>>>> +		adev->pcie_wreg(adev, reg * 4, v);
>>>>>
>>>>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
>>>>> +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
>>>>>     }
>>>>>
>>>>>     /*
>>>>> @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev,
>> uint32_t
>>>> reg, uint32_t v,
>>>>>      *
>>>>>      * this function is invoked only the debugfs register access
>>>>>      * */
>>>>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t reg,
>> uint32_t v,
>>>>> -		    uint32_t acc_flags)
>>>>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
>>>>> +			     uint32_t reg, uint32_t v)
>>>>>     {
>>>>>     	if (adev->in_pci_err_recovery)
>>>>>     		return;
>>>>>
>>>>>     	if (amdgpu_sriov_fullaccess(adev) &&
>>>>> -		adev->gfx.rlc.funcs &&
>>>>> -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
>>>>> -
>>>>> +	    adev->gfx.rlc.funcs &&
>>>>> +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
>>>>>     		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
>>>>>     			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
>>>>> -	}
>>>>> -
>>>>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
>>>>> +	} else
>>>>> +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
>>>>>     }
>>>>>
>>>>>     /**
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>>>> index 63e734a..5da20fc 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>>>> @@ -35,7 +35,7 @@
>>>>>     #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
>>>>>
>>>>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence-
>>>>>> finished)
>>>>> -TRACE_EVENT(amdgpu_mm_rreg,
>>>>> +TRACE_EVENT(amdgpu_device_rreg,
>>>>>     	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>>>>>     	    TP_ARGS(did, reg, value),
>>>>>     	    TP_STRUCT__entry(
>>>>> @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
>>>>>     		      (unsigned long)__entry->value)
>>>>>     );
>>>>>
>>>>> -TRACE_EVENT(amdgpu_mm_wreg,
>>>>> +TRACE_EVENT(amdgpu_device_wreg,
>>>>>     	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
>>>>>     	    TP_ARGS(did, reg, value),
>>>>>     	    TP_STRUCT__entry(
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.or
>>>> g%2Fmailman%2Flistinfo%2Famd-
>>>>
>> gfx&amp;data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb
>> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241&amp;
>>>> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&amp;reserved=0

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

RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[Zhang, Hawking]

[AMD Public Use]

Re - it came up as warning/error-free. Peculiar

This is not something new. simple grep else in current kernel tree, there will be bunches of "} else" there. 

Regards,
Hawking

e.g. 

if (!IS_ERR(dacl) && !IS_ERR(pacl)) {
		set_cached_acl(inode, ACL_TYPE_DEFAULT, dacl);
		set_cached_acl(inode, ACL_TYPE_ACCESS, pacl);
	} else
		retval = -EIO;

-----Original Message-----
From: Russell, Kent <Kent.Russell@amd.com> 
Sent: Friday, September 18, 2020 21:33
To: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
Subject: RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2)

[AMD Public Use]

Thanks for clarifying my ambiguity, Christian. And for sanity, I just ran checkpatch on a local file with an "} else" and it came up as warning/error-free. Peculiar.

 Kent 

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, September 18, 2020 8:17 AM
> To: Russell, Kent <Kent.Russell@amd.com>; Zhang, Hawking 
> <Hawking.Zhang@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, 
> Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang) 
> <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg 
> outside of mmio bar (v2)
> 
> To be more precise you can either use "else" or "} else {", but don't 
> use "} else" or "else {".
> 
> Interesting that checkpatch.pl doesn't complain, I thought that this 
> would be checked.
> 
> Regards,
> Christian.
> 
> Am 18.09.20 um 14:12 schrieb Russell, Kent:
> > [AMD Public Use]
> >
> > It's preferred to have braces if the else has a single line:
> > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#plac
> > ing-braces-and-
> spaces
> > Right above "3.1) Spaces"
> >
> >   Kent
> >
> >> -----Original Message-----
> >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of 
> >> Zhang, Hawking
> >> Sent: Friday, September 18, 2020 8:07 AM
> >> To: Koenig, Christian <Christian.Koenig@amd.com>; 
> >> amd-gfx@lists.freedesktop.org; Deucher, Alexander 
> >> <Alexander.Deucher@amd.com>; Wang, Kevin(Yang) 
> >> <Kevin1.Wang@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>
> >> Subject: RE: [PATCH 3/3] drm/amdgpu: support indirect access reg 
> >> outside of mmio bar
> (v2)
> >>
> >> [AMD Public Use]
> >>
> >>> +	} else
> >> RE - This should use "} else {". Apart from that looks good to me.
> >>
> >> Hi Chris,
> >>
> >> Since there is only one line under "else", I dropped the "{}". I 
> >> thought that is okay in
> current
> >> kernel coding style and checkpatch.pl also shows no complain.
> >>
> >> So you mean you want the code to be the following?
> >>
> >> } else {
> >>                  ret = adev->pcie_rreg(adev, reg * 4); }
> >>
> >> I can make the change anyway. Does that mean I can get your RB with 
> >> coding style
> change
> >> for the series?
> >>
> >> Regards,
> >> Hawking
> >>
> >>
> >> -----Original Message-----
> >> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >> Sent: Friday, September 18, 2020 19:27
> >> To: Zhang, Hawking <Hawking.Zhang@amd.com>; 
> >> amd-gfx@lists.freedesktop.org;
> Deucher,
> >> Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang) 
> >> <Kevin1.Wang@amd.com>; Koenig, Christian 
> >> <Christian.Koenig@amd.com>; Chen,
> Guchun
> >> <Guchun.Chen@amd.com>
> >> Subject: Re: [PATCH 3/3] drm/amdgpu: support indirect access reg 
> >> outside of mmio bar
> (v2)
> >>
> >> Am 18.09.20 um 11:51 schrieb Hawking Zhang:
> >>> support both direct and indirect accessor in unified helper functions.
> >>>
> >>> v2: Retire indirect mmio access via mm_index/data
> >>>
> >>> Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
> >>> ---
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 23 +++----
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  2 +-
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  | 96 +++++++++++------------------
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  4 +-
> >>>    4 files changed, 51 insertions(+), 74 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index 40ee44b..50341f0 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -1019,12 +1019,13 @@ int amdgpu_gpu_wait_for_idle(struct 
> >>> amdgpu_device *adev);
> >>>
> >>>    void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> >>>    			       uint32_t *buf, size_t size, bool write); -uint32_t 
> >>> amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> >>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> >>> +			    uint32_t reg, uint32_t acc_flags); void 
> >>> +amdgpu_device_wreg(struct amdgpu_device *adev,
> >>> +			uint32_t reg, uint32_t v,
> >>>    			uint32_t acc_flags);
> >>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> >>> -		    uint32_t acc_flags);
> >>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t 
> >>> reg,
> uint32_t v,
> >>> -		    uint32_t acc_flags);
> >>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> >>> +			     uint32_t reg, uint32_t v);
> >>>    void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t 
> >>> offset, uint8_t
> value);
> >>>    uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t 
> >>> offset);
> >>>
> >>> @@ -1054,8 +1055,8 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >>>     */
> >>>    #define AMDGPU_REGS_NO_KIQ    (1<<1)
> >>>
> >>> -#define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg),
> >>> AMDGPU_REGS_NO_KIQ) -#define WREG32_NO_KIQ(reg, v) 
> >>> amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> >>> +#define RREG32_NO_KIQ(reg) amdgpu_device_rreg(adev, (reg),
> >>> +AMDGPU_REGS_NO_KIQ) #define WREG32_NO_KIQ(reg, v) 
> >>> +amdgpu_device_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
> >>>
> >>>    #define RREG32_KIQ(reg) amdgpu_kiq_rreg(adev, (reg))
> >>>    #define WREG32_KIQ(reg, v) amdgpu_kiq_wreg(adev, (reg), (v)) @@
> >>> -1063,9 +1064,9 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >>>    #define RREG8(reg) amdgpu_mm_rreg8(adev, (reg))
> >>>    #define WREG8(reg, v) amdgpu_mm_wreg8(adev, (reg), (v))
> >>>
> >>> -#define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0) -#define
> >>> DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", 
> >>> amdgpu_mm_rreg(adev, (reg), 0)) -#define WREG32(reg, v) 
> >>> amdgpu_mm_wreg(adev, (reg), (v), 0)
> >>> +#define RREG32(reg) amdgpu_device_rreg(adev, (reg), 0) #define
> >>> +DREG32(reg) printk(KERN_INFO "REGISTER: " #reg " : 0x%08X\n", 
> >>> +amdgpu_device_rreg(adev, (reg), 0)) #define WREG32(reg, v) 
> >>> +amdgpu_device_wreg(adev, (reg), (v), 0)
> >>>    #define REG_SET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
> >>>    #define REG_GET(FIELD, v) (((v) << FIELD##_SHIFT) & FIELD##_MASK)
> >>>    #define RREG32_PCIE(reg) adev->pcie_rreg(adev, (reg)) @@ 
> >>> -1111,7
> >>> +1112,7 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
> >>>    		WREG32_SMC(_Reg, tmp);                          \
> >>>    	} while (0)
> >>>
> >>> -#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
> >>> 0x%08X\n", amdgpu_mm_rreg((adev), (reg), false))
> >>> +#define DREG32_SYS(sqf, adev, reg) seq_printf((sqf), #reg " :
> >>> +0x%08X\n", amdgpu_device_rreg((adev), (reg), false))
> >>>    #define RREG32_IO(reg) amdgpu_io_rreg(adev, (reg))
> >>>    #define WREG32_IO(reg, v) amdgpu_io_wreg(adev, (reg), (v))
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> index abe0c27..2d125b8 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>> @@ -267,7 +267,7 @@ static int  amdgpu_debugfs_process_reg_op(bool 
> >>> read, struct
> file
> >> *f,
> >>>    		} else {
> >>>    			r = get_user(value, (uint32_t *)buf);
> >>>    			if (!r)
> >>> -				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
> >>> +				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value);
> >>>    		}
> >>>    		if (r) {
> >>>    			result = r;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> index 77785b2..beef764 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>> @@ -303,10 +303,10 @@ void amdgpu_device_vram_access(struct 
> >>> amdgpu_device
> >> *adev, loff_t pos,
> >>>    }
> >>>
> >>>    /*
> >>> - * MMIO register access helper functions.
> >>> + * register access helper functions.
> >>>     */
> >>>    /**
> >>> - * amdgpu_mm_rreg - read a memory mapped IO register
> >>> + * amdgpu_device_rreg - read a memory mapped IO or indirect 
> >>> + register
> >>>     *
> >>>     * @adev: amdgpu_device pointer
> >>>     * @reg: dword aligned register offset @@ -314,33 +314,27 @@ 
> >>> void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> >>>     *
> >>>     * Returns the 32 bit value from the offset specified.
> >>>     */
> >>> -uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> >>> -			uint32_t acc_flags)
> >>> +uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
> >>> +			    uint32_t reg, uint32_t acc_flags)
> >>>    {
> >>>    	uint32_t ret;
> >>>
> >>>    	if (adev->in_pci_err_recovery)
> >>>    		return 0;
> >>>
> >>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> >>> -	    down_read_trylock(&adev->reset_sem)) {
> >>> -		ret = amdgpu_kiq_rreg(adev, reg);
> >>> -		up_read(&adev->reset_sem);
> >>> -		return ret;
> >>> -	}
> >>> -
> >>> -	if ((reg * 4) < adev->rmmio_size)
> >>> -		ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> >>> -	else {
> >>> -		unsigned long flags;
> >>> +	if ((reg * 4) < adev->rmmio_size) {
> >>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> >>> +		    amdgpu_sriov_runtime(adev) &&
> >>> +		    down_read_trylock(&adev->reset_sem)) {
> >>> +			ret = amdgpu_kiq_rreg(adev, reg);
> >>> +			up_read(&adev->reset_sem);
> >>> +		} else
> >>> +			ret = readl(((void __iomem *)adev->rmmio) + (reg * 4));
> >>> +	} else
> >> This should use "} else {". Apart from that looks good to me.
> >>
> >> Christian.
> >>
> >>> +		ret = adev->pcie_rreg(adev, reg * 4);
> >>>
> >>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> >>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> >>> -		ret = readl(((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> >>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> >>> -	}
> >>> +	trace_amdgpu_device_rreg(adev->pdev->device, reg, ret);
> >>>
> >>> -	trace_amdgpu_mm_rreg(adev->pdev->device, reg, ret);
> >>>    	return ret;
> >>>    }
> >>>
> >>> @@ -394,29 +388,8 @@ void amdgpu_mm_wreg8(struct amdgpu_device 
> >>> *adev,
> uint32_t
> >> offset, uint8_t value)
> >>>    		BUG();
> >>>    }
> >>>
> >>> -static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> >>> -				       uint32_t reg, uint32_t v,
> >>> -				       uint32_t acc_flags)
> >>> -{
> >>> -	if (adev->in_pci_err_recovery)
> >>> -		return;
> >>> -
> >>> -	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
> >>> -
> >>> -	if ((reg * 4) < adev->rmmio_size)
> >>> -		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >>> -	else {
> >>> -		unsigned long flags;
> >>> -
> >>> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> >>> -		writel((reg * 4), ((void __iomem *)adev->rmmio) + (mmMM_INDEX * 4));
> >>> -		writel(v, ((void __iomem *)adev->rmmio) + (mmMM_DATA * 4));
> >>> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> >>> -	}
> >>> -}
> >>> -
> >>>    /**
> >>> - * amdgpu_mm_wreg - write to a memory mapped IO register
> >>> + * amdgpu_device_wreg - write to a memory mapped IO or indirect 
> >>> + register
> >>>     *
> >>>     * @adev: amdgpu_device pointer
> >>>     * @reg: dword aligned register offset @@ -425,20 +398,25 @@ 
> >>> static inline void amdgpu_mm_wreg_mmio(struct amdgpu_device *adev,
> >>>     *
> >>>     * Writes the value specified to the offset specified.
> >>>     */
> >>> -void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> >>> -		    uint32_t acc_flags)
> >>> +void amdgpu_device_wreg(struct amdgpu_device *adev,
> >>> +			uint32_t reg, uint32_t v,
> >>> +			uint32_t acc_flags)
> >>>    {
> >>>    	if (adev->in_pci_err_recovery)
> >>>    		return;
> >>>
> >>> -	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) &&
> >>> -	    down_read_trylock(&adev->reset_sem)) {
> >>> -		amdgpu_kiq_wreg(adev, reg, v);
> >>> -		up_read(&adev->reset_sem);
> >>> -		return;
> >>> -	}
> >>> +	if ((reg * 4) < adev->rmmio_size) {
> >>> +		if (!(acc_flags & AMDGPU_REGS_NO_KIQ) &&
> >>> +		    amdgpu_sriov_runtime(adev) &&
> >>> +		    down_read_trylock(&adev->reset_sem)) {
> >>> +			amdgpu_kiq_wreg(adev, reg, v);
> >>> +			up_read(&adev->reset_sem);
> >>> +		} else
> >>> +			writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >>> +	} else
> >>> +		adev->pcie_wreg(adev, reg * 4, v);
> >>>
> >>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> >>> +	trace_amdgpu_device_wreg(adev->pdev->device, reg, v);
> >>>    }
> >>>
> >>>    /*
> >>> @@ -446,21 +424,19 @@ void amdgpu_mm_wreg(struct amdgpu_device 
> >>> *adev,
> uint32_t
> >> reg, uint32_t v,
> >>>     *
> >>>     * this function is invoked only the debugfs register access
> >>>     * */
> >>> -void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev, uint32_t 
> >>> reg,
> uint32_t v,
> >>> -		    uint32_t acc_flags)
> >>> +void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> >>> +			     uint32_t reg, uint32_t v)
> >>>    {
> >>>    	if (adev->in_pci_err_recovery)
> >>>    		return;
> >>>
> >>>    	if (amdgpu_sriov_fullaccess(adev) &&
> >>> -		adev->gfx.rlc.funcs &&
> >>> -		adev->gfx.rlc.funcs->is_rlcg_access_range) {
> >>> -
> >>> +	    adev->gfx.rlc.funcs &&
> >>> +	    adev->gfx.rlc.funcs->is_rlcg_access_range) {
> >>>    		if (adev->gfx.rlc.funcs->is_rlcg_access_range(adev, reg))
> >>>    			return adev->gfx.rlc.funcs->rlcg_wreg(adev, reg, v);
> >>> -	}
> >>> -
> >>> -	amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags);
> >>> +	} else
> >>> +		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> >>>    }
> >>>
> >>>    /**
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> index 63e734a..5da20fc 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> >>> @@ -35,7 +35,7 @@
> >>>    #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \
> >>>
> >>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fe
> >>> job->nce-
> >>>> finished)
> >>> -TRACE_EVENT(amdgpu_mm_rreg,
> >>> +TRACE_EVENT(amdgpu_device_rreg,
> >>>    	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
> >>>    	    TP_ARGS(did, reg, value),
> >>>    	    TP_STRUCT__entry(
> >>> @@ -54,7 +54,7 @@ TRACE_EVENT(amdgpu_mm_rreg,
> >>>    		      (unsigned long)__entry->value)
> >>>    );
> >>>
> >>> -TRACE_EVENT(amdgpu_mm_wreg,
> >>> +TRACE_EVENT(amdgpu_device_wreg,
> >>>    	    TP_PROTO(unsigned did, uint32_t reg, uint32_t value),
> >>>    	    TP_ARGS(did, reg, value),
> >>>    	    TP_STRUCT__entry(
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.or
> >> g%2Fmailman%2Flistinfo%2Famd-
> >>
> gfx&amp;data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08
> d85bcb
> >>
> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241&a
> mp;
> >> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&amp;reserved
> >> =0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3)

[Hawking Zhang]

Add helper function in order to remove RREG32/WREG32
in current pcie_rreg/wreg function for soc15 and
onwards adapters.
PCIE_INDEX/DATA pairs are used to access regsiters
outside of mmio bar in the helper functions.
The new helper functions help remove the recursion
of amdgpu_mm_rreg/wreg from pcie_rreg/wreg and
provide the oppotunity to centralize direct and
indirect access in a single function.

v2: Fixed typo and refine the comments

v3: Remove unnecessary volatile local variable

Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Kevin Wang <kevin1.wang@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Guchun Chen <guchun.chen@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  13 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 129 +++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 13f92de..40ee44b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1031,6 +1031,19 @@ uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t offset);
 u32 amdgpu_io_rreg(struct amdgpu_device *adev, u32 reg);
 void amdgpu_io_wreg(struct amdgpu_device *adev, u32 reg, u32 v);
 
+u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
+				u32 pcie_index, u32 pcie_data,
+				u32 reg_addr);
+u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
+				  u32 pcie_index, u32 pcie_data,
+				  u32 reg_addr);
+void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
+				 u32 pcie_index, u32 pcie_data,
+				 u32 reg_addr, u32 reg_data);
+void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
+				   u32 pcie_index, u32 pcie_data,
+				   u32 reg_addr, u64 reg_data);
+
 bool amdgpu_device_asic_has_dc_support(enum amd_asic_type asic_type);
 bool amdgpu_device_has_dc_support(struct amdgpu_device *adev);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5c2eb46..77785b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -595,6 +595,135 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
 }
 
 /**
+ * amdgpu_device_indirect_rreg - read an indirect register
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ *
+ * Returns the value of indirect register @reg_addr
+ */
+u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
+				u32 pcie_index, u32 pcie_data,
+				u32 reg_addr)
+{
+	unsigned long flags;
+	u32 r;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	r = readl(pcie_data_offset);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+
+	return r;
+}
+
+/**
+ * amdgpu_device_indirect_rreg64 - read a 64bits indirect register
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ *
+ * Returns the value of indirect register @reg_addr
+ */
+u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
+				  u32 pcie_index, u32 pcie_data,
+				  u32 reg_addr)
+{
+	unsigned long flags;
+	u64 r;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	/* read low 32 bits */
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	r = readl(pcie_data_offset);
+	/* read high 32 bits */
+	writel(reg_addr + 4, pcie_index_offset);
+	readl(pcie_index_offset);
+	r |= ((u64)readl(pcie_data_offset) << 32);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+
+	return r;
+}
+
+/**
+ * amdgpu_device_indirect_wreg - write an indirect register address
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ * @reg_addr: indirect register offset
+ * @reg_data: indirect register data
+ *
+ */
+void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
+				 u32 pcie_index, u32 pcie_data,
+				 u32 reg_addr, u32 reg_data)
+{
+	unsigned long flags;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	writel(reg_data, pcie_data_offset);
+	readl(pcie_data_offset);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+}
+
+/**
+ * amdgpu_device_indirect_wreg64 - write a 64bits indirect register address
+ *
+ * @adev: amdgpu_device pointer
+ * @pcie_index: mmio register offset
+ * @pcie_data: mmio register offset
+ * @reg_addr: indirect register offset
+ * @reg_data: indirect register data
+ *
+ */
+void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
+				   u32 pcie_index, u32 pcie_data,
+				   u32 reg_addr, u64 reg_data)
+{
+	unsigned long flags;
+	void __iomem *pcie_index_offset;
+	void __iomem *pcie_data_offset;
+
+	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
+	pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
+	pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
+
+	/* write low 32 bits */
+	writel(reg_addr, pcie_index_offset);
+	readl(pcie_index_offset);
+	writel((u32)(reg_data & 0xffffffffULL), pcie_data_offset);
+	readl(pcie_data_offset);
+	/* write high 32 bits */
+	writel(reg_addr + 4, pcie_index_offset);
+	readl(pcie_index_offset);
+	writel((u32)(reg_data >> 32), pcie_data_offset);
+	readl(pcie_data_offset);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+}
+
+/**
  * amdgpu_invalid_rreg - dummy reg read function
  *
  * @adev: amdgpu device pointer
-- 
2.7.4

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

[PATCH 2/3] drm/amdgpu: switch to indirect reg access helper

[Hawking Zhang]

Switch WREG32/RREG32_PCIE to use indirect reg access
helper for soc15 and onwards

Signed-off-by: Hawking Zhang <Hawking.Zhang@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Kevin Wang <kevin1.wang@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Guchun Chen <guchun.chen@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/nv.c    | 51 ++++++--------------------------------
 drivers/gpu/drm/amd/amdgpu/soc15.c | 51 ++++++--------------------------------
 2 files changed, 16 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index 0ec6603..91ca4ece 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -69,75 +69,40 @@ static const struct amd_ip_funcs nv_common_ip_funcs;
  */
 static u32 nv_pcie_rreg(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u32 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg(adev, address, data, reg);
 }
 
 static void nv_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, v);
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
 }
 
 static u64 nv_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u64 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* read low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-
-	/* read high 32 bit*/
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	r |= ((u64)RREG32(data) << 32);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
 }
 
 static void nv_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* write low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v & 0xffffffffULL));
-	(void)RREG32(data);
-
-	/* write high 32 bit */
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v >> 32));
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
 }
 
 static u32 nv_didt_rreg(struct amdgpu_device *adev, u32 reg)
diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c
index ddd55e3..0de9df8 100644
--- a/drivers/gpu/drm/amd/amdgpu/soc15.c
+++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
@@ -101,75 +101,40 @@
  */
 static u32 soc15_pcie_rreg(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u32 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg(adev, address, data, reg);
 }
 
 static void soc15_pcie_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, v);
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg(adev, address, data, reg, v);
 }
 
 static u64 soc15_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
 {
-	unsigned long flags, address, data;
-	u64 r;
+	unsigned long address, data;
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* read low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	r = RREG32(data);
-
-	/* read high 32 bit*/
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	r |= ((u64)RREG32(data) << 32);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-	return r;
+	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
 }
 
 static void soc15_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
 {
-	unsigned long flags, address, data;
+	unsigned long address, data;
 
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
-	/* write low 32 bit */
-	WREG32(address, reg);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v & 0xffffffffULL));
-	(void)RREG32(data);
-
-	/* write high 32 bit */
-	WREG32(address, reg + 4);
-	(void)RREG32(address);
-	WREG32(data, (u32)(v >> 32));
-	(void)RREG32(data);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
+	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
 }
 
 static u32 soc15_uvd_ctx_rreg(struct amdgpu_device *adev, u32 reg)
-- 
2.7.4

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