[announce] c64_tools (DSP loader and IPC)

[_wb_]

Yeah but you can run op_cpuspeed.sh with sudo and it won't ask for a password (even if it's not cached for 5 minutes by sudo) while for op_dspspeed.sh it currently will.

It's probably a good idea to add all those speed adjustment scripts to sudoers. It's not the most secure thing to do, but this is not a big web server, it's a handheld. The worst that will come from this is a PND that makes your handheld crash or run too slowly. It does not need those scripts to do that anyway.

Just make sure that the script clips the value to some reasonable interval (not too high, not too low).

It would be nice if those clock settings can be configured from e.g. System Info without it having to ask for root.

 

[notaz]

Okay, added default speeds to the startup.

Still need (or not?) some UI thing for user to control it, and maybe save his choice between reboots. op_dspspeed.sh can show zenity dialog, but there is no menu item currently.

It would be nice if those clock settings can be configured from e.g. System Info without it having to ask for root.

At least for RAM overclock, I don't think it' random PND's business to change it. We might end up with PNDs that set unsafe speeds in their launch scripts, ignoring the fact that it highly depends on how specific SoC+RAM can handle the overclock and there is no guarantee specific value will work for everyone. It's not obvious what it does, people should only change if they know what they're doing, and password dialog is a good sign that the PND is doing something it shouldn't be really doing.

So in short I'm strongly against op_sysspeed.sh not requiring a password, but still in doubt about op_dspspeed.sh

 

[EvilDragon]

If DSPSpeed will be configured similar to CPUSpeed so that users can set a max overclock limit, with default settings for 600MHz/1GHz setup, then we should be safe with a non-sudo dspspeed settings.

I want to try to use of gtkdialog to create a complete UI for CPUSpeed and DSPSpeed-Settings, should be a lot better than using multiple zenity dialogues.

 

[Linux-SWAT]

One request here, please use popups for "changes were made" confirmation instead of box needing to press "ok".

 

[EvilDragon]

Shouldn't even be needed when I use "Apply", "Save and Quit" and "Cancel"

 

[_wb_]

Okay, added default speeds to the startup.

Still need (or not?) some UI thing for user to control it, and maybe save his choice between reboots. op_dspspeed.sh can show zenity dialog, but there is no menu item currently.

It would be nice if those clock settings can be configured from e.g. System Info without it having to ask for root.

At least for RAM overclock, I don't think it' random PND's business to change it. We might end up with PNDs that set unsafe speeds in their launch scripts, ignoring the fact that it highly depends on how specific SoC+RAM can handle the overclock and there is no guarantee specific value will work for everyone. It's not obvious what it does, people should only change if they know what they're doing, and password dialog is a good sign that the PND is doing something it shouldn't be really doing.

So in short I'm strongly against op_sysspeed.sh not requiring a password, but still in doubt about op_dspspeed.sh

I think it would be good if you could set a global max RAM overclock that is respected by op_sysspeed.sh (and also a global minimum by the way, because underclocking it too much also leads to trouble). You would still need to enter the root password to change that global maximum. The initial value for the maximum could be as low as the default value for the SoC.

Why allow CPU overclock from a PND but not GPU overclock? For GPU-limited games, it makes sense to let the application try to overclock. The op_sysspeed.sh script should make sure that the values are clipped to a safe interval, and the user can experiment with modifying the safe interval - obviously at his own risk.

I don't see why the CPU clockspeed and the SYS/RAM/GPU clockspeed should be handled differently.

I would make all of them (CPU, SYS, DSP) changeable without password, but clipped to a safe interval (like we already do now with the CPU speed) that can only be modified with password.

 

[notaz]

I think it would be good if you could set a global max RAM overclock that is respected by op_sysspeed.sh (and also a global minimum by the way, because underclocking it too much also leads to trouble). You would still need to enter the root password to change that global maximum. The initial value for the maximum could be as low as the default value for the SoC.

I have no motivation to work on this, patches welcome.

Why allow CPU overclock from a PND but not GPU overclock? For GPU-limited games, it makes sense to let the application try to overclock. The op_sysspeed.sh script should make sure that the values are clipped to a safe interval, and the user can experiment with modifying the safe interval - obviously at his own risk.


I don't see why the CPU clockspeed and the SYS/RAM/GPU clockspeed should be handled differently.

Because CPU overclocing is used a lot and is well understood, stress test exists and default limits are defined. op_sysspeed.sh overclocks far more than just GPU and RAM, it's the whole L3 bus and most peripherals. Too large overclock may easily lead to SD card corruption, while to large CPU overclock is more likely to just crash. We don't have proper stress tests for sysspeed nor DSP and typically working ranges are not known.

Hell I'm starting to regret I've added op_sysspeed.sh thing at all.

 

[Linux-SWAT]

Hell I'm starting to regret I've added op_sysspeed.sh thing at all.

You've opened Pandora's box dude...

 

[bsp]

If DSPSpeed will be configured similar to CPUSpeed so that users can set a max overclock limit, with default settings for 600MHz/1GHz setup, then we should be safe with a non-sudo dspspeed settings.

I second that. If I am not mistaken, that's how notaz implemented it right now.

There's /etc/pandora/dsp.conf that lets you configure the min/max limits, then there's /usr/pandora/scripts/op_dspspeed.sh that lets change the current clock rate. /proc/pandora/dsp_mhz_max can be read to determine the current rate but it should not be written to directly since it ignores the min/max settings defined in dsp.conf (same thing with cpu_mhz_max and cpu.conf). You need to be root in order to write to the /proc files, though.


Just like op_cpuspeed.sh, op_dspspeed.sh should be added to the sudoers so no root passwd entry is required.


I do not see any reason why this would cause any serious problems.


(maybe a decreased unit life time but chances are that everything else will fall apart before the SoC surrenders yes this is completely off topic but ED, I beg you to use a better case in the DragonBox or whatever it will be called. My Ghz Pandora is just two months old, I treated it like a raw egg and yet the upper part of the case already has two cracks -- just from opening/closing the lid!)


Like the CPU max rate in cpu.conf, the DSP max rate has to depend on the Pandora edition -- you probably already set that during install/reflash (after checking the the SoC type ?)

I want to try to use of gtkdialog to create a complete UI for CPUSpeed and DSPSpeed-Settings, should be a lot better than using multiple zenity dialogue

It would be nice-to-have but is it really worth the effort ?


The current zenity dialogues are fine, IMHO, and ppl. will (most likely) install SysInfo anyway, so it would be great if _wb_ would add DSP clock info + controls to his tool (on the processor page). I would suggest to use the system bus speed (fn+<,>) keys for that, and move the sys.speed controls to a less comfortable key combo, e.g. shift+<,>

So in short I'm strongly against op_sysspeed.sh not requiring a password, but still in doubt about op_dspspeed.sh

I agreed with your op_sysspeed.sh concerns but see no issue with adding op_dspspeed.sh to sudoers (except for what I said at the end of this post).


Besides, you currently do not need to be root to change the DSP clock rate (if you think about it!) but my plea to all devs is not to try to be clever and abuse this loophole, but rather use the op_dspspeed.sh script instead, please!

I think it would be good if you could set a global max RAM overclock that is respected by op_sysspeed.sh (and also a global minimum by the way, because underclocking it too much also leads to trouble). You would still need to enter the root password to change that global maximum. The initial value for the maximum could be as low as the default value for the SoC.

I have no motivation to work on this, patches welcome.

A sys.conf would make sense, especially because of the reasons you mentioned (potentially dangerous resp. not so well understood like CPU/DSP under/overclocking), i.e. the user/UI-configurable values should be clamped to reasonable limits.

Then again, the comments in op_sysspeed.sh imply that this is quite a hack and changing the sysspeed, regardless of the value, could already be problematic..

Hell I'm starting to regret I've added op_sysspeed.sh thing at all.

well, it's up to the user to shoot her/himself in the foot. they've been warned. Although, assuming that gksudo is used when this is done via the UI (SysInfo), it would probably be a good idea to add a warning message (in SysInfo) that clearly states that this might cause e.g. SD card corruption ...



Last but not least, the only reason I advocate password-less CPU/DSP clock rate changes are the system tools (CPU-Speed, DSP-Speed, SysInfo).


Normal applications should have no business changing these clock rates, IMHO.


AFAIK, unfortunately it's not possible to configure sudo so that it allows access only from certain parent processes (even if it were possible, you'd have to exclude e.g. bash).

 

[WizardStan]

well, it's up to the user to shoot her/himself in the foot

Until they blame the guy that gave them the gun for not making the safety feature idiot proof. Whether they're right or wrong it's still a headache for the maintainer.

 

[notaz]

Ok I'm convinced about op_dspspeed.sh , not adding op_sysspeed.sh though.

Besides, you currently do not need to be root to change the DSP clock rate (if you think about it!) but my plea to all devs is not to try to be clever and abuse this loophole, but rather use the op_dspspeed.sh script instead, please!

Hmm how's that, you mean programming the registers from DSP side or what?

 

[bsp]

yep.

we could just enable the MMU and forbid all access to the SCM but I'm also not very motivated to code such restrictions, at least not until there's a real need for it.

I would assume that a simple "please do not do this" request should be enough to keep this community's devs from doing such nasty things.

 

[_wb_]

The current zenity dialogues are fine, IMHO, and ppl. will (most likely) install SysInfo anyway, so it would be great if _wb_ would add DSP clock info + controls to his tool (on the processor page). I would suggest to use the system bus speed (fn+<,>) keys for that, and move the sys.speed controls to a less comfortable key combo, e.g. shift+<,>

Good suggestion. I'll do exactly that.

Where can I see the current DSP clock speed?

As for the sys overclock: it's exactly because this is potentially dangerous that it would make sense to clamp the value in the same way as the CPU clock speed is currently clamped. I now clamp the value manually in System Info (to 360 MHz for OMAP3 and to 440MHz for DM3730 units iirc) to avoid most of the trouble, but I think it would be better if there was a global user-configurable maximum (just like for the CPU speed) instead of relying on all applications to give sane values to the overclocking script. Regardless of whether the overclocking script requires a password or not.

 

[bsp]

Good suggestion. I'll do exactly that.

cool, looking forward to your new SysInfo!

Where can I see the current DSP clock speed?

I answered that already (in anticipation of this question) quote:

There's /etc/pandora/dsp.conf that lets you configure the min/max limits, then there's /usr/pandora/scripts/op_dspspeed.sh that lets change the current clock rate. /proc/pandora/dsp_mhz_max can be read to determine the current rate

(I know I know, me and my tl;dr posts.. )


Regarding the sysspeed thingie: I think we all agree that it would make sense to have that feature (sys.conf, min/max limit checking) but since no-one volunteers to actually implement it, it won't happen. I can fully understand notaz -- just because he is able to implement it does not mean that he has to be the one doing it. Like he said, if you really want that feature, write the script(s) and he'll be glad to include them in the FW.


..and like I already mentioned, if it weren't for the system tools (including your SysInfo), I would vote for removing all op_*speed.sh scripts from the sudoers list.


I have at least seen one application that constantly resets the cpuspeed to 600Mhz on a Ghz Pandora. It was quite annoying to manual change the speed back all the time. Although, IIRC, this particular app. has been fixed now but god knows how many other apps out there do sth. similar.


With regard to this (Pandora revision) problem, it would e.g. make sense to (alternatively) boil down the over/underclock settings to just a single /dev/pandora_opp device that allows reading/writing a single float value to set the OPP (e.g. 1..4). Applications would open the device, request their desired OPP by writing to it, and the driver would change (increase) the OPP if there is not any other app. that requested a higher OPP. If an app. closes the device (resp. exits), the driver would check if it can lower the OPP if no other app. needs a higher OPP.


This is just an idea, I won't implement it since I don't have the need for it. It would probably be a lot more interesting for Rebirth/CC Pandoras where the overclock headroom is much larger.

 

[notaz]

Out of curiosity I was looking at HAVE_BROKEN_MAILBOX code and I believe you're not using the mailbox correctly. According to the docs, you're supposed to read the FIFO first and only then clear IRQSTATUS by writing 1 to corresponding bit, meanwhile on DSP side you first write to IRQSTATUS and only then read the FIFO. I haven't tested if that actually helps as I failed to compile libc64_dsp.

There is also one thing you should know when programming OMAPs: L3 (or is it L4?) bus has it's own write buffer, which can be forcefully flushed by reading just written or another register. So after you write critical registers like acknowledging IRQs, you have to read them back, otherwise you'll get occasional spurious interrupts.

Edit: those BCACHE_wbInvAll() look pretty bad, sort of implies DSP at least has write buffer enabled on top of OMAP registers? In that case you should use BCACHE_wb every time after writing to MLB_* registers and before you clear interrupts on DSP internal regs.. It should be better to use writebacks on MLB register region only, not the whole cache.

I can guess DSP uses cache on all registers on L3 as it doesn't distinguish it from RAM, which I know is cached from testing. There seem to be XMC.MARi registers for that, but they only affect L2?
 

(I've started looking at the code when I saw erratic times that dsp_rpc_recv() seems to take, it seems to take twice as long on some tries compared to others with DSP side code that does nothing, which is suspiciuos.. Maybe the mailbox FIFO runs on some slow clock or something, which causes different delays depending on how you fit inside that clock periods..)

 

[bsp]

Out of curiosity I was looking at HAVE_BROKEN_MAILBOX code and I believe you're not using the mailbox correctly. According to the docs, you're supposed to read the FIFO first and only then clear IRQSTATUS by writing 1 to corresponding bit, meanwhile on DSP side you first write to IRQSTATUS and only then read the FIFO. I haven't tested if that actually helps as I failed to compile libc64_dsp.

You failed to compile libc64_dsp ? why ? It's quite easy to build once you have installed the prerequisites ?!

There is also one thing you should know when programming OMAPs: L3 (or is it L4?) bus has it's own write buffer, which can be forcefully flushed by reading just written or another register. So after you write critical registers like acknowledging IRQs, you have to read them back, otherwise you'll get occasional spurious interrupts.

Yes, I learned about that technique somewhen in the middle of this project, maybe we can get rid of the cache writeback calls after all.

There seem to be XMC.MARi registers for that, but they only affect L2?

Good question, I'm not sure. The MAR regs are currently setup to cache the 0x80000000.. (RAM) region, peripheral regs should be uncached but the c64_tools DSP/BIOS config relies on the defaults here. Could use an in-depth review, for sure.

I've started looking at the code when I saw erratic times that dsp_rpc_recv() seems to take, it seems to take twice as long on some tries compared to others with DSP side code that does nothing, which is suspiciuos..

This is not an RTOS so this could just be a "hickup" / scheduler related issue on the GPP side.

As for the mailbox issue: Yep, in the IRQ handler on the DSP-side I clear IRQSTATUS before checking the FIFO but there are not any other events mapped to that IRQ line.


I fixed the event mapping some weeks ago while improving the power management, so the only reason for that IRQ to be triggered on the DSP-side should be a mailbox event now.


I just tried some tests without the HAVE_BROKEN_MAILBOX define and things still work fine, so yeah, this can probably be removed in the next release.

I made some (minor) changes, though (in fly-by-night mode..) but I just double checked that clearing the IRQSTATUS before reading the FIFO was NOT the reason for the issues -- it makes no difference whether it's cleared before or after the FIFO read. Even clearing it unconditionally in the IRQ handler still works fine.


By the way, with the (now) correct clock setup, the IPC messaging performance on a Ghz Pandora is now

pandora2:/home/bsp/old# ./c64_tc 3
[...] selected testcase 3 ("TC_RPC_ADD_BENCHMARK")
[...] coff_load_overlay: text=0x000005a0  data=0x0000006c  bss=0x000010dc
[...] coff_load_overlay: text=0x00000620  data=0x0000006c  bss=0x00001114
[...] coff_load_overlay: text=0x00000600  data=0x00000074  bss=0x00001110
[...] starting benchmark
[...] benchmark finished. => 100000 iterations in 2848 millisecs.

==> 0.02848 millseconds per remote procedure call (roundtrip)
 ==> ~35112 calls per second

pandora2:/home/bsp/old# ./c64_tc 6
[...] selected testcase 6 ("TC_FASTCALL_RPC_ADD")
[...] coff_load_overlay: text=0x000005a0  data=0x0000006c  bss=0x000010dc
[...] coff_load_overlay: text=0x00000620  data=0x0000006c  bss=0x00001114
[...] coff_load_overlay: text=0x00000600  data=0x00000074  bss=0x00001110
[...] starting fastcall benchmark
[...] loc_tc_rpc_add_fastcall_benchmark: init rpc sent, r=0
[...] fastcall benchmark finished. => 1000000 iterations in 1614 millisecs.

==> 0.001614 milliseconds per remote procedure call (roundtrip)
 ==> ~619578 calls per second (10326 calls per 60Hz video frame)

pandora2:/home/bsp/old# cat /proc/pandora/dsp_mhz_max
800

 

[notaz]

You failed to compile libc64_dsp ? why ? It's quite easy to build once you have installed the prerequisites ?!

Yeah I probably miss some prerequisites, was trying to avoid those 600+MB downloads.. Missing includes like tsk.h and hwi.h , DSPBIOS package I guess?

There seem to be XMC.MARi registers for that, but they only affect L2?

 
Good question, I'm not sure. The MAR regs are currently setup to cache the 0x80000000.. (RAM) region, peripheral regs should be uncached but the c64_tools DSP/BIOS config relies on the defaults here. Could use an in-depth review, for sure.

The manual says there are 256 registers for address space in 0x01000000 steps, I could only find references for first 8 in some weird config files.

I've started looking at the code when I saw erratic times that dsp_rpc_recv() seems to take, it seems to take twice as long on some tries compared to others with DSP side code that does nothing, which is suspiciuos..

This is not an RTOS so this could just be a "hickup" / scheduler related issue on the GPP side.

Might be, but I don't think it should be that large on mostly idle system. From my experience the source of that latency is often ARM wakeup latency, like if interrupt arrives while ARM is processing something else, it's processed fast but if it is sleeping at the time, you get the wakeup delay. The wakeup problem can be avoided by running a low priority process in the background that does nothing but just spins a loop, but that doesn't help here.

I just tried some tests without the HAVE_BROKEN_MAILBOX define and things still work fine, so yeah, this can probably be removed in the next release.

Yeah it would be nice getting rid of those mailbox resets and unneeded interrupt disabling on each message, it should reduce amount of register programming needed, accesses of those registers are slow.

 

[bsp]

You failed to compile libc64_dsp ? why ? It's quite easy to build once you have installed the prerequisites ?!

Yeah I probably miss some prerequisites, was trying to avoid those 600+MB downloads.. Missing includes like tsk.h and hwi.h , DSPBIOS package I guess?

yes, those are from DSP/BIOS, which is a 100MB download btw (still big but not _that_ big).

There seem to be XMC.MARi registers for that, but they only affect L2?

Good question, I'm not sure. The MAR regs are currently setup to cache the 0x80000000.. (RAM) region, peripheral regs should be uncached but the c64_tools DSP/BIOS config relies on the defaults here. Could use an in-depth review, for sure.

The manual says there are 256 registers for address space in 0x01000000 steps, I could only find references for first 8 in some weird config files.

There are not 256 registers, there are 128 bits (i.e. 4  32bit registers). Each bit toggles caching for a 32mb block.


I took a look at the TRM and the info there (which mentions the 256 registers) does not make much sense since 4 registers are enough to configure the entire 32bit address space (plus why the heck they included those read-only regs in addition to the read-write regs beats me).

I just tried some tests without the HAVE_BROKEN_MAILBOX define and things still work fine, so yeah, this can probably be removed in the next release.

Yeah it would be nice getting rid of those mailbox resets and unneeded interrupt disabling on each message, it should reduce amount of register programming needed, accesses of those registers are slow.

I've posted the benchmark results (binaries compiled w/o the HAVE_BROKEN_MAILBOX define + some minor changes that remove the mailbox interrupt enable/disable code) earlier this morning.


As you can see, there's no measurable speed increase compared to the current binaries, so releasing a FW update purely for 'code aesthetics' is not worth the effort, IMHO.


Let's wait until there are more noteworthy changes (not that I have anything planned right now).

 

[notaz]

The manual says there are 256 registers for address space in 0x01000000 steps, I could only find references for first 8 in some weird config files.

There are not 256 registers, there are 128 bits (i.e. 4  32bit registers). Each bit toggles caching for a 32mb block.

What makes you think so? The manual clearly shows that there are 256 registers with bits 31-1 reserved and only bit0 effective. Maybe that was easier for them to do in hardware that way, or something. It could be mistake in the manual, but I don't see any signs to be so sure it is.


Here is someone struggling with DSP, I can guess he would have taken the opportunity to complain to TI if MARi was behaving wrong as he mentioned he programmed the registers:


http://e2e.ti.com/support/dsp/tms320c6000_high_performance_dsps/f/112/p/11829/46334.aspx

(doesn't really mean anything, I know..)

As you can see, there's no measurable speed increase compared to the current binaries, so releasing a FW update purely for 'code aesthetics' is not worth the effort, IMHO.


Let's wait until there are more noteworthy changes (not that I have anything planned right now).

Well I have a few kernel changes so I'll need to do update anyway, so it would be nice to get that in before final SZ update.

 

[bsp]

There are not 256 registers, there are 128 bits (i.e. 4  32bit registers). Each bit toggles caching for a 32mb block.

What makes you think so? The manual clearly shows that there are 256 registers with bits 31-1 reserved and only bit0 effective. Maybe that was easier for them to do in hardware that way, or something. It could be mistake in the manual, but I don't see any signs to be so sure it is.

Reread the docs, and yes, that's correct. For some reason they only used the LSB and therefore 128 regs are needed. Now it makes sense that the first 16 regs are read only (0x00000000..0x20000000 area).

I've programmed the MARs only via DSP/BIOS textual config which presents them as 4 32bit regs/values.


It does not really matter, anyway, since according to the docs, the MAR settings only apply to L2, which still does not work for some reason -- neither via DSP/BIOS configuration, nor via manual reg writes.

No idea why -- if someone from TI is peeking at this forums, any help/advise would greatly be appreciated!

However, c64_tools currently _does_ power up the L2SRAM and configures it as 'scratch' memory but I haven't done any tests with that, yet.

Here is someone struggling with DSP, I can guess he would have taken the opportunity to complain to TI if MARi was behaving wrong as he mentioned he programmed the registers:http://e2e.ti.com/su...1829/46334.aspx


(doesn't really mean anything, I know..)

From a quick glance at it I would say that this persons' mistake was that he did not consider that the DSP has a different view of the memory than the GPP. Like you said, not relevant here.

As you can see, there's no measurable speed increase compared to the current binaries, so releasing a FW update purely for 'code aesthetics' is not worth the effort, IMHO.

Let's wait until there are more noteworthy changes (not that I have anything planned right now).

Well I have a few kernel changes so I'll need to do update anyway, so it would be nice to get that in before final SZ update.

To be honest, I'm a bit reluctant to change such a critical part of c64_tools for the final update, given that it does not improve the performance in any meaningful way and just might potentially break things.


I've spent two months testing the code with the mailbox-reset compiled in, several users have run the code and confirmed that it works, and I really am a bit conservative in this regard, i.e. never change a running system for no good reason.


I would suggest that, for aforementioned reasons, we don't do this change for the SZ1.56 final but reserve it for an incremental (experimental) SZ1.56 update.

p.s.: If you should have doubts about this (performance improvements w/o mbox reset), I can send you the patched sources so you can see/try for yourself but please don't integrate those sources.

EDIT: attached the patch/updated files but pls remember this is a 2:30 a.m. hack, so just use it for testing, not for integration.

c64_tools_no_mbox_reset_patch-EXPERiMENTAL-06Nov2013.tar.gz