Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11268 - Optimization for tremor on targets that don't use iram.

Attached to Project: Rockbox
Opened by Nils Wallménius (nls) - Thursday, 13 May 2010, 14:45 GMT+2
Last edited by Nils Wallménius (nls) - Monday, 06 June 2011, 15:28 GMT+2
Task Type Patches
Category Codecs
Status Closed
Assigned To No-one
Player Type SW-codec
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Next release
Due Date Undecided
Percent Complete 100%
Private No

Details

Some of our targets don't use iram sections as most do, in tremor we assume that all targets use iram so these targets do the same copying etc. that targets with a little iram do (due to that being the default case in the preprocessor logic.

This patch inserts #ifdef USE_IRAM around the iram specific parts especially avoiding the vector copy in block.c. This speeds up decoding on the gigabeast about 2MHz across the test set.

I'm not entirely confident that i understand the logic in the memry handling so i would apreciate reviewing of the changes.

Other targets that should see a speedup are gigabeat f/x and the ams targets with little ram that have the codec buffer entirely in iram (clip at least).
   no_iram.diff (5.4 KiB) 
 apps/codecs/libtremor/oggmalloc.c |    8 +++-
 apps/codecs/libtremor/block.c     |   62 ++++++++++++++++++++++----------------
 2 files changed, 41 insertions(+), 29 deletions(-)

This task depends upon

Closed by  Nils Wallménius (nls)
Monday, 06 June 2011, 15:28 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  committed
Comment by Nils Wallménius (nls) - Friday, 14 May 2010, 10:27 GMT+2
Hmm, this seems to *sometimes* cause a sim to output garbled sound...
Usually the first track is fine but if i skip back to the beginning the sound sometimes becomes messed up.
any ideas about that?
Comment by Nils Wallménius (nls) - Monday, 17 May 2010, 15:08 GMT+2
This fixes the garbled sound but still causes crashes on trackchange sometimes, probably something in the cleanup that goes wrong.
Of course just setting IRAM_IBSS_SIZE to be large enough to hold the double buffer in "iram" works but it doesn't give as large speedup.
Anyway, this patch turns tremor into a bit more of an #ifdef mess so i would like suggestions how to make it cleaner.
Also if anyone knows why, in vorbis_synthesis_blockin prevCenter is different if the iram double buffer is used.
   no_iram_v2.diff (6.6 KiB) 
 apps/codecs/libtremor/oggmalloc.c    |   26 +++++++------
 apps/codecs/libtremor/ivorbiscodec.h |    2 +
 apps/codecs/libtremor/block.c        |   68 +++++++++++++++++++++--------------
 3 files changed, 58 insertions(+), 38 deletions(-)

Comment by Nils Wallménius (nls) - Monday, 17 May 2010, 22:04 GMT+2
This patch seems to fix the crash in the sim, (was freeing the wrong pointer) but it seems to still have the
bug where the sound becomes garbled after skipping back to the beginning of a song. Sometimes i need
to do it 10 times to trigger the bug, sometimes less :/
   no_iram_v3.diff (7.1 KiB) 
 apps/codecs/libtremor/config-tremor.h |    6 ++
 apps/codecs/libtremor/oggmalloc.c     |    3 +
 apps/codecs/libtremor/block.c         |   77 ++++++++++++++++++++--------------
 3 files changed, 54 insertions(+), 32 deletions(-)

Comment by Dave Hooper (stripwax) - Wednesday, 19 May 2010, 15:15 GMT+2
I know this code pretty well, as most of it came from one of my tremor patches, so I could take a look. I know exactly why prevCenter is different if the iram double buffer is used.
What is the main intention here - use more of the Gigabeast's ram to avoid having to do the memcpy in the blockin code? (does Gigabeast have larger plugin buffer than other targets?). The fact its garbled suggests the pointers are not being reset properly on reinit. The code is admittedly a bit of a spaghetti bowl.
Comment by Nils Wallménius (nls) - Monday, 24 May 2010, 10:39 GMT+2
Yes, tremor currently pretends to have iram buffers with the same size as on targets with 96k on targets that don't use iram at all.
As you say this leads to copying between the "iram" buffer and the dram one which is completely useless when the "iram" buffer is not in iram.
Benchmarking on the beast shows that this saves about 2MHz or about 5-7%
AFAIU it shouldn't use any more ram than currently as the static buffer is gone and the malloc()'s that are now used as fallbacks when
the iram buffer is full should be used always, it works but there is that sound garbling bug i've gotten on the sim...
I found one bug with the fallback malloc, v->first_pcm is never freed (though this should not happen currently since all targets use the static "iram" buffer)
Another approach that would probably work better but is slightly hacky is to just pretend to have more iram so the static buffer is
large enough to avoid the copy, should be a less intrusive change too.

Not only the gigabeast is affected, the gigabeat f/x don't use iram either and the clip has the entire codec buffer in iram so it doesn't use
it in the traditional way either.

I would be happy if you took a look and commented on the patch.
Comment by Nils Wallménius (nls) - Monday, 06 June 2011, 00:34 GMT+2
Found and fixed the bug so this should now be good for commit. I'll run some tests tomorrow.
   no_iram_v8.diff (7.4 KiB) 
 apps/codecs/libtremor/config-tremor.h |    4 +
 apps/codecs/libtremor/oggmalloc.c     |    2 
 apps/codecs/libtremor/ivorbiscodec.h  |    2 
 apps/codecs/libtremor/block.c         |   94 ++++++++++++++++++++--------------
 4 files changed, 63 insertions(+), 39 deletions(-)

Loading...