drm/radeon: Inline r100_mm_rreg, v2
Subsystems (2)
PATCH 1/1 drm/radeon: Inline r100_mm_rreg, v2
This was originally un-inlined by Andi Kleen in 2011 citing size concerns. Indeed, a first attempt at inlining it grew radeon.ko by 7%.
However, 2% of cpu is spent in this function. Simply inlining it gave 1% more fps in Urban Terror.
v2: We know the minimum MMIO size. Adding it to the if allows the compiler to optimize the branch out, improving both performance and size.
The v2 patch decreases radeon.ko size by 2%. I didn't re-benchmark, but common sense says perf is now more than 1% better.
--- drivers/gpu/drm/radeon/r100.c | 18 ------------------ drivers/gpu/drm/radeon/radeon.h | 21 +++++++++++++++++++-- 2 files changed, 19 insertions(+), 20 deletions(-)
The _wreg function could be given similar treatment, but as it's nowhere as hot (0.009% vs 2%), didn't bother.
drivers/gpu/drm/radeon/r100.c +0 -18
@@ -4086,24 +4086,6 @@ int r100_init(struct radeon_device *rdev)return 0 ; } -uint32_t r100_mm_rreg (struct radeon_device *rdev,uint32_t reg, -bool always_indirect) -{ -if (reg < rdev->rmmio_size && !always_indirect) -return readl (((void __iomem *)rdev->rmmio) + reg); -else { -unsigned long flags; -uint32_t ret; - -spin_lock_irqsave (&rdev->mmio_idx_lock, flags); -writel (reg, ((void __iomem *)rdev->rmmio) +RADEON_MM_INDEX ); - ret =readl (((void __iomem *)rdev->rmmio) +RADEON_MM_DATA ); -spin_unlock_irqrestore (&rdev->mmio_idx_lock, flags); - -return ret; - } -} -void r100_mm_wreg (struct radeon_device *rdev,uint32_t reg,uint32_t v,bool always_indirect) {
drivers/gpu/drm/radeon/radeon.h +19 -2
@@ -2328,8 +2328,25 @@ int radeon_device_init(struct radeon_device *rdev,void radeon_device_fini (struct radeon_device *rdev);int radeon_gpu_wait_for_idle (struct radeon_device *rdev); -uint32_t r100_mm_rreg (struct radeon_device *rdev,uint32_t reg, -bool always_indirect); +static inline uint32_t r100_mm_rreg (struct radeon_device *rdev,uint32_t reg, +bool always_indirect) +{ +/* The mmio size is 64kb at minimum. Allows the if to be optimized out. */ +if ((reg < rdev->rmmio_size || reg <0x10000 ) && !always_indirect) +return readl (((void __iomem *)rdev->rmmio) + reg); +else { +unsigned long flags; +uint32_t ret; + +spin_lock_irqsave (&rdev->mmio_idx_lock, flags); +writel (reg, ((void __iomem *)rdev->rmmio) +RADEON_MM_INDEX ); + ret =readl (((void __iomem *)rdev->rmmio) +RADEON_MM_DATA ); +spin_unlock_irqrestore (&rdev->mmio_idx_lock, flags); + +return ret; + } +} +void r100_mm_wreg (struct radeon_device *rdev,uint32_t reg,uint32_t v,bool always_indirect);u32 r100_io_rreg (struct radeon_device *rdev,u32 reg);
Signature · 1 line
1.8.3.1
Am 18.04.2014 23:11, schrieb Lauri Kasanen:
Quoted 11 lines
This was originally un-inlined by Andi Kleen in 2011 citing size concerns. Indeed, a first attempt at inlining it grew radeon.ko by 7%.
However, 2% of cpu is spent in this function. Simply inlining it gave 1% more fps in Urban Terror.
v2: We know the minimum MMIO size. Adding it to the if allows the compiler to optimize the branch out, improving both performance and size.
The v2 patch decreases radeon.ko size by 2%. I didn't re-benchmark, but common sense says perf is now more than 1% better.
Nice!
But are you sure that the register PCI bar is always at least 64K in size? Keep in mind that this code is used over all generations since R100. Additional to that we probably should have a define for that and also apply the optimizations to r100_mm_wreg as well.
Christian.
Quoted 71 lines
Signed-off-by Lauri Kasanen <[email protected]>--- drivers/gpu/drm/radeon/r100.c | 18 ------------------ drivers/gpu/drm/radeon/radeon.h | 21 +++++++++++++++++++-- 2 files changed, 19 insertions(+), 20 deletions(-)The
_wregfunction could be given similar treatment, but as it's nowhere as hot (0.009% vs 2%), didn't bother.drivers/gpu/drm/radeon/r100.c +0
-18@@ -4086,24 +4086,6 @@ int r100_init(struct radeon_device *rdev)return 0 ; } -uint32_t r100_mm_rreg (struct radeon_device *rdev,uint32_t reg, -bool always_indirect) -{ -if (reg < rdev->rmmio_size && !always_indirect) -return readl (((void __iomem *)rdev->rmmio) + reg); -else { -unsigned long flags; -uint32_t ret; - -spin_lock_irqsave (&rdev->mmio_idx_lock, flags); -writel (reg, ((void __iomem *)rdev->rmmio) +RADEON_MM_INDEX ); - ret =readl (((void __iomem *)rdev->rmmio) +RADEON_MM_DATA ); -spin_unlock_irqrestore (&rdev->mmio_idx_lock, flags); - -return ret; - } -} -void r100_mm_wreg (struct radeon_device *rdev,uint32_t reg,uint32_t v,bool always_indirect) {drivers/gpu/drm/radeon/radeon.h +19
-2@@ -2328,8 +2328,25 @@ int radeon_device_init(struct radeon_device *rdev,void radeon_device_fini (struct radeon_device *rdev);int radeon_gpu_wait_for_idle (struct radeon_device *rdev); -uint32_t r100_mm_rreg (struct radeon_device *rdev,uint32_t reg, -bool always_indirect); +static inline uint32_t r100_mm_rreg (struct radeon_device *rdev,uint32_t reg, +bool always_indirect) +{ +/* The mmio size is 64kb at minimum. Allows the if to be optimized out. */ +if ((reg < rdev->rmmio_size || reg <0x10000 ) && !always_indirect) +return readl (((void __iomem *)rdev->rmmio) + reg); +else { +unsigned long flags; +uint32_t ret; + +spin_lock_irqsave (&rdev->mmio_idx_lock, flags); +writel (reg, ((void __iomem *)rdev->rmmio) +RADEON_MM_INDEX ); + ret =readl (((void __iomem *)rdev->rmmio) +RADEON_MM_DATA ); +spin_unlock_irqrestore (&rdev->mmio_idx_lock, flags); + +return ret; + } +} +void r100_mm_wreg (struct radeon_device *rdev,uint32_t reg,uint32_t v,bool always_indirect);u32 r100_io_rreg (struct radeon_device *rdev,u32 reg);
Quoted 18 lines · Christian König
Am 18.04.2014 23:11, schrieb Lauri Kasanen:
Quoted 7 lines
This was originally un-inlined by Andi Kleen in 2011 citing size concerns. Indeed, a first attempt at inlining it grew radeon.ko by 7%.
However, 2% of cpu is spent in this function. Simply inlining it gave 1% more fps
in Urban Terror.Quoted 7 lines
v2: We know the minimum MMIO size. Adding it to the if allows the compiler to
optimize the branch out, improving both performance and size.The v2 patch decreases radeon.ko size by 2%. I didn't re-benchmark, but common sense
says perf is now more than 1% better.
Quoted 7 lines
Nice!
But are you sure that the register PCI bar is always at least 64K in size? Keep in mind that this code is used over all generations since R100. Additional to that we probably should have a define for that and also apply the optimizations to
r100_mm_wregas well.
If most of the register accesses are for the interrupt setup, I wonder if it would be better to just clean up the irq_set functions to reduce the register accesses. E.g., only touch the registers for the specific irq masks have changed.
Alex
Quoted 78 lines
Christian.
Quoted 75 lines
Signed-off-by Lauri Kasanen <[email protected]>--- drivers/gpu/drm/radeon/r100.c | 18 ------------------ drivers/gpu/drm/radeon/radeon.h | 21 +++++++++++++++++++-- 2 files changed, 19 insertions(+), 20 deletions(-)The
_wregfunction could be given similar treatment, but as it's nowhere as hot
(0.009% vs 2%), didn't bother.drivers/gpu/drm/radeon/r100.c +0
-17@@ -4086,24 +4086,6 @@ int r100_init(struct radeon_device *rdev)return 0 ; } -uint32_t r100_mm_rreg(struct radeon_device *rdev, uint32_t reg, - bool always_indirect) -{ - if (reg < rdev->rmmio_size && !always_indirect) - return readl(((void __iomem *)rdev->rmmio) + reg); - else { - unsigned long flags; - uint32_t ret; - - spin_lock_irqsave(&rdev->mmio_idx_lock, flags); - writel(reg, ((void __iomem *)rdev->rmmio) + RADEON_MM_INDEX); - ret = readl(((void __iomem *)rdev->rmmio) + RADEON_MM_DATA); - spin_unlock_irqrestore(&rdev->mmio_idx_lock, flags); - - return ret; - } -} -void r100_mm_wreg (struct radeon_device *rdev,uint32_t reg,uint32_t v,bool always_indirect) {drivers/gpu/drm/radeon/radeon.h +4
-1@@ -2328,8 +2328,25 @@ int radeon_device_init(struct radeon_device *rdev,void radeon_device_fini (struct radeon_device *rdev);int radeon_gpu_wait_for_idle (struct radeon_device *rdev); -uint32_t r100_mm_rreg(struct radeon_device *rdev, uint32_t reg, - bool always_indirect); +static inline uint32_t r100_mm_rreg(struct radeon_device *rdev, uint32_t reg, + bool always_indirect) +{ +/* The mmio size is 64kb at minimum. Allows the if to be optimized out. */
drivers/gpu/drm/radeon/radeon.h +8
-0+if ((reg < rdev->rmmio_size || reg <0x10000 ) && !always_indirect) +return readl (((void __iomem *)rdev->rmmio) + reg); +else { +unsigned long flags; +uint32_t ret; + +spin_lock_irqsave (&rdev->mmio_idx_lock, flags); + writel(reg, ((void __iomem *)rdev->rmmio) +
RADEON_MM_INDEX);
+ ret = readl(((void__iomem*)rdev->rmmio) +RADEON_MM_DATA);drivers/gpu/drm/radeon/radeon.h +6
-0+spin_unlock_irqrestore (&rdev->mmio_idx_lock, flags); + +return ret; + } +} + void r100_mm_wreg(struct radeon_device *rdev, uint32_t reg, uint32_t v,
Quoted 8 lines
Quoted 2 lines
bool always_indirect);u32
r100_io_rreg(struct radeon_device *rdev, u32 reg);_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 22 lines · On Sat, 19 Apr 2014 11:15:53 -0400 Alex Deucher <[email protected]> wrote:
On Sat, Apr 19, 2014 at 6:06 AM, Christian König
Quoted 21 lines
Quoted 7 lines
This was originally un-inlined by Andi Kleen in 2011 citing size concerns. Indeed, a first attempt at inlining it grew radeon.ko by 7%.
However, 2% of cpu is spent in this function. Simply inlining it gave 1% more fps
in Urban Terror.Quoted 7 lines
v2: We know the minimum MMIO size. Adding it to the if allows the compiler to
optimize the branch out, improving both performance and size.The v2 patch decreases radeon.ko size by 2%. I didn't re-benchmark, but common sense
says perf is now more than 1% better.Nice!
But are you sure that the register PCI bar is always at least 64K in size? Keep in mind that this code is used over all generations since R100. Additional to that we probably should have a define for that and also apply the optimizations to
r100_mm_wregas well.
Yes, I checked the earlier code. It had 64kb hard-coded, and when it was changed in 2010 to use the dynamic value, the commit said later asics are larger. (07bec2df01)
A quick google also didn't find any dmesg with smaller values, R100 cards had 64kb.
Quoted 4 lines
If most of the register accesses are for the interrupt setup, I wonder if it would be better to just clean up the
irq_setfunctions to reduce the register accesses. E.g., only touch the registers for the specific irq masks have changed.
Yes, that should also be done. But as this function is used elsewhere as well, having it fast (not to mention the size decrease) would be good.
I think this patch is safe enough for 3.15, but perhaps it's too late now.
- Lauri
Am 19.04.2014 19:33, schrieb Lauri Kasanen:
Quoted 24 lines
Quoted 21 lines · On Sat, 19 Apr 2014 11:15:53 -0400 Alex Deucher <[email protected]> wrote:
On Sat, Apr 19, 2014 at 6:06 AM, Christian König
Quoted 20 lines
Quoted 7 lines
This was originally un-inlined by Andi Kleen in 2011 citing size concerns. Indeed, a first attempt at inlining it grew radeon.ko by 7%.
However, 2% of cpu is spent in this function. Simply inlining it gave 1% more fps
in Urban Terror.Quoted 7 lines
v2: We know the minimum MMIO size. Adding it to the if allows the compiler to
optimize the branch out, improving both performance and size.The v2 patch decreases radeon.ko size by 2%. I didn't re-benchmark, but common sense
says perf is now more than 1% better.Nice!
But are you sure that the register PCI bar is always at least 64K in size? Keep in mind that this code is used over all generations since R100. Additional to that we probably should have a define for that and also apply the optimizations to
r100_mm_wregas well.
Quoted 6 lines
Yes, I checked the earlier code. It had 64kb hard-coded, and when it was changed in 2010 to use the dynamic value, the commit said later asics are larger. (
07bec2df01)A quick google also didn't find any dmesg with smaller values, R100 cards had 64kb.
Thanks for digging that up, this indeed sounds valid and like a very nice optimization.
Just as suggested before add a define for this in radeon.h, something like RADEON_MIN_PCI_BAR_SIZE and do the same for r100_mm_wreg as well.
Quoted 10 lines
Quoted 4 lines
If most of the register accesses are for the interrupt setup, I wonder if it would be better to just clean up the
irq_setfunctions to reduce the register accesses. E.g., only touch the registers for the specific irq masks have changed.Yes, that should also be done. But as this function is used elsewhere as well, having it fast (not to mention the size decrease) would be good.
I think this patch is safe enough for 3.15, but perhaps it's too late now.
Yeah, probably. But 3.16 should work as well.
Christian.
Quoted 1 line
- Lauri