Re: [PATCH] drm/radeon: Inline r100_mm_rreg, v2

["Christian König" <deathsimple@vodafone.de>]

Am 19.04.2014 19:33, schrieb Lauri Kasanen:
> On Sat, 19 Apr 2014 11:15:53 -0400
> Alex Deucher <alexdeucher@gmail.com> wrote:
>
>> On Sat, Apr 19, 2014 at 6:06 AM, Christian König
>>>> 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.
> 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.

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

> - Lauri