* [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) @ 2020-09-18 9:51 Hawking Zhang 2020-09-18 9:51 ` [PATCH 2/3] drm/amdgpu: switch to indirect reg access helper Hawking Zhang 2020-09-18 9:51 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang 0 siblings, 2 replies; 12+ messages in thread From: Hawking Zhang @ 2020-09-18 9:51 UTC (permalink / raw) To: amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen Cc: 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm/amdgpu: switch to indirect reg access helper 2020-09-18 9:51 [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) Hawking Zhang @ 2020-09-18 9:51 ` Hawking Zhang 2020-09-18 9:51 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang 1 sibling, 0 replies; 12+ messages in thread From: Hawking Zhang @ 2020-09-18 9:51 UTC (permalink / raw) To: amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen Cc: 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 9:51 [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) Hawking Zhang 2020-09-18 9:51 ` [PATCH 2/3] drm/amdgpu: switch to indirect reg access helper Hawking Zhang @ 2020-09-18 9:51 ` Hawking Zhang 2020-09-18 11:27 ` Christian König 1 sibling, 1 reply; 12+ messages in thread From: Hawking Zhang @ 2020-09-18 9:51 UTC (permalink / raw) To: amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen Cc: 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 9:51 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang @ 2020-09-18 11:27 ` Christian König 2020-09-18 12:06 ` Zhang, Hawking 0 siblings, 1 reply; 12+ messages in thread From: Christian König @ 2020-09-18 11:27 UTC (permalink / raw) To: Hawking Zhang, amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 11:27 ` Christian König @ 2020-09-18 12:06 ` Zhang, Hawking 2020-09-18 12:12 ` Russell, Kent 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Hawking @ 2020-09-18 12:06 UTC (permalink / raw) To: Koenig, Christian, amd-gfx, Deucher, Alexander, Wang, Kevin(Yang), Chen, Guchun [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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 12:06 ` Zhang, Hawking @ 2020-09-18 12:12 ` Russell, Kent 2020-09-18 12:17 ` Christian König 0 siblings, 1 reply; 12+ messages in thread From: Russell, Kent @ 2020-09-18 12:12 UTC (permalink / raw) To: Zhang, Hawking, Koenig, Christian, amd-gfx, Deucher, Alexander, Wang, Kevin(Yang), Chen, Guchun [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&data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb > 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241& > sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&reserved=0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 12:12 ` Russell, Kent @ 2020-09-18 12:17 ` Christian König 2020-09-18 13:32 ` Russell, Kent 0 siblings, 1 reply; 12+ messages in thread From: Christian König @ 2020-09-18 12:17 UTC (permalink / raw) To: Russell, Kent, Zhang, Hawking, amd-gfx, Deucher, Alexander, Wang, Kevin(Yang), Chen, Guchun 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&data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb >> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241& >> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&reserved=0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 12:17 ` Christian König @ 2020-09-18 13:32 ` Russell, Kent 2020-09-18 13:51 ` Christian König 2020-09-18 14:02 ` Zhang, Hawking 0 siblings, 2 replies; 12+ messages in thread From: Russell, Kent @ 2020-09-18 13:32 UTC (permalink / raw) To: Koenig, Christian, Zhang, Hawking, amd-gfx, Deucher, Alexander, Wang, Kevin(Yang), Chen, Guchun [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&data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb > >> > 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241& > >> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&reserved=0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 13:32 ` Russell, Kent @ 2020-09-18 13:51 ` Christian König 2020-09-18 14:02 ` Zhang, Hawking 1 sibling, 0 replies; 12+ messages in thread From: Christian König @ 2020-09-18 13:51 UTC (permalink / raw) To: Russell, Kent, Zhang, Hawking, amd-gfx, Deucher, Alexander, Wang, Kevin(Yang), Chen, Guchun 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&data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08d85bcb >> 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241& >>>> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&reserved=0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 13:32 ` Russell, Kent 2020-09-18 13:51 ` Christian König @ 2020-09-18 14:02 ` Zhang, Hawking 1 sibling, 0 replies; 12+ messages in thread From: Zhang, Hawking @ 2020-09-18 14:02 UTC (permalink / raw) To: Russell, Kent, Koenig, Christian, amd-gfx, Deucher, Alexander, Wang, Kevin(Yang), Chen, Guchun [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&data=02%7C01%7Ckent.russell%40amd.com%7Cb04cb251dd2a49fc322c08 > d85bcb > >> > 5c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360276278468241&a > mp; > >> sdata=dxFCasxUIrvjPfnft7EQuM1BFbNwzgNSWdRX0l%2BV0l8%3D&reserved > >> =0 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) @ 2020-09-18 12:37 Hawking Zhang 2020-09-18 12:37 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang 0 siblings, 1 reply; 12+ messages in thread From: Hawking Zhang @ 2020-09-18 12:37 UTC (permalink / raw) To: amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen Cc: 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 12:37 [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) Hawking Zhang @ 2020-09-18 12:37 ` Hawking Zhang 2020-09-18 12:51 ` Christian König 0 siblings, 1 reply; 12+ messages in thread From: Hawking Zhang @ 2020-09-18 12:37 UTC (permalink / raw) To: amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen Cc: 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 | 95 ++++++++++++----------------- drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 4 +- 4 files changed, 53 insertions(+), 71 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..365ced6 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,29 @@ 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) { + 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); } - if ((reg * 4) < adev->rmmio_size) - ret = readl(((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)); - 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 +390,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 +400,27 @@ 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 +428,20 @@ 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); + } else { + writel(v, ((void __iomem *)adev->rmmio) + (reg * 4)); } - - amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags); } /** 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) 2020-09-18 12:37 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang @ 2020-09-18 12:51 ` Christian König 0 siblings, 0 replies; 12+ messages in thread From: Christian König @ 2020-09-18 12:51 UTC (permalink / raw) To: Hawking Zhang, amd-gfx, Alex Deucher, Kevin Wang, Christian König, Guchun Chen Am 18.09.20 um 14:37 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> Reviewed-by: Christian König <christian.koenig@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 | 95 ++++++++++++----------------- > drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 4 +- > 4 files changed, 53 insertions(+), 71 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..365ced6 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,29 @@ 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) { > + 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); > } > > - if ((reg * 4) < adev->rmmio_size) > - ret = readl(((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)); > - 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 +390,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 +400,27 @@ 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 +428,20 @@ 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); > + } else { > + writel(v, ((void __iomem *)adev->rmmio) + (reg * 4)); > } > - > - amdgpu_mm_wreg_mmio(adev, reg, v, acc_flags); > } > > /** > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-18 14:02 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-18 9:51 [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) Hawking Zhang 2020-09-18 9:51 ` [PATCH 2/3] drm/amdgpu: switch to indirect reg access helper Hawking Zhang 2020-09-18 9:51 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang 2020-09-18 11:27 ` Christian König 2020-09-18 12:06 ` Zhang, Hawking 2020-09-18 12:12 ` Russell, Kent 2020-09-18 12:17 ` Christian König 2020-09-18 13:32 ` Russell, Kent 2020-09-18 13:51 ` Christian König 2020-09-18 14:02 ` Zhang, Hawking 2020-09-18 12:37 [PATCH 1/3] drm/amdgpu: add helper function for indirect reg access (v3) Hawking Zhang 2020-09-18 12:37 ` [PATCH 3/3] drm/amdgpu: support indirect access reg outside of mmio bar (v2) Hawking Zhang 2020-09-18 12:51 ` Christian König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).