[announce] c64_tools (DSP loader and IPC)


I am open to suggestions.

My suggestions are:
 

  • non-application specific DSP images are placed in C64_IMAGE_SITE_PATH (currently set to "/lib/c64_tools/images/"). At the moment, this would be just the core.out image (rather small)
  • libc64.so goes to /usr/lib/
  • c64_load goes to /sbin/
  • go64.sh goes to ? (/usr/pandora/scripts/?). Applications/PNDs that want to use the DSP must call the script. The first app. using the DSP (after a reboot) will cause a dialog to popup and ask for the root password, then load the c64.ko kernel module and the core.out "DSP firmware". You can either change "op_dsp.sh" to use c64_tools instead of DspBridge, or provide two separate scripts, e.g. "op_dsp_bridgedriver.sh" and "op_dsp_c64.sh" since only one of these drivers can use the DSP (for obvious reasons). The dialog feature still needs to be implemented. Should I do that or will you do it ?
  • the example DSP images and applications should be bundled in a PND, no reason to install them in NAND (your choice, they do not take up much space but IMHO the NAND should only contain essential things). This would definitely by my task.
You said that there is no release date for the next firmware update and the only reason for an update will be the c64_tools.

If that is still true, I would suggest to wait for the next update (next weekend or the one after next).

I am currently working on some utility functions like e.g. integrating a memory manager so apps. can use dsp_shm_alloc() and dsp_shm_free().

This does not necessarily have to be a part of libc64 but since any real-world application will require this, it makes sense to put it there.

I also have not gotten around to implementing the eDMA3 DSP syscalls but this should be in the next update, too.
 

@EvilDragon: thanks for adding the new development section!
 
Last edited by a moderator:
@EvilDragon: thanks for adding the new development section!
Could you write a new thread with :- Instruction to use you libraries for DSP virgin devs

- Any usefull material you have at hands. Mods will sticky it, so we could have that at hand in no time
 
  • non-application specific DSP images are placed in C64_IMAGE_SITE_PATH (currently set to "/lib/c64_tools/images/"). At the moment, this would be just the core.out image (rather small)
I suggest /lib/dsp/c64_tools/images/ because we already have /lib/dsp/ for DSP.

  • libc64.so goes to /usr/lib/
ok. Are you ready too keep supporting that API? I.e. when it's included it should no longer be changed, only expanded, so it's a good chance to tune it for the last time now before it's included.

  • c64_load goes to /sbin/
I'd prefer /usr/sbin, /sbin is usually for limited set of core stuff.

  • go64.sh goes to ? (/usr/pandora/scripts/?). Applications/PNDs that want to use the DSP must call the script. The first app. using the DSP (after a reboot) will cause a dialog to popup and ask for the root password, then load the c64.ko kernel module and the core.out "DSP firmware". You can either change "op_dsp.sh" to use c64_tools instead of DspBridge, or provide two separate scripts, e.g. "op_dsp_bridgedriver.sh" and "op_dsp_c64.sh" since only one of these drivers can use the DSP (for obvious reasons). The dialog feature still needs to be implemented. Should I do that or will you do it ?
Ok /usr/pandora/scripts/op_dsp_c64.sh sounds good to me. I don't think we should be asking for password, as it will be part of firmware, it can at least pretend it knows what it's doing and load stuff silently. Otherwise it will just annoy the user everytime he wants to run something that supports the DSP.

  • the example DSP images and applications should be bundled in a PND, no reason to install them in NAND (your choice, they do not take up much space but IMHO the NAND should only contain essential things). This would definitely by my task.
Agreed.

You said that there is no release date for the next firmware update and the only reason for an update will be the c64_tools.


If that is still true, I would suggest to wait for the next update (next weekend or the one after next).
ok.
 
  • non-application specific DSP images are placed in C64_IMAGE_SITE_PATH (currently set to "/lib/c64_tools/images/"). At the moment, this would be just the core.out image (rather small)
I suggest /lib/dsp/c64_tools/images/ because we already have /lib/dsp/ for DSP.
that could be confusing. what about using the same dir. (/lib/dsp) for c64_tools images as well ? The dir. is almost empty right now and dspbridge uses a different image file suffix (.dof) than c64_tools (.out).

  • libc64.so goes to /usr/lib/
ok. Are you ready too keep supporting that API? I.e. when it's included it should no longer be changed, only expanded, so it's a good chance to tune it for the last time now before it's included.
I am not the kind of developer who frequently changes his APIs (in fact, I like fixed APIs). I encourage everyone who reads this to review the API (in dsp.h) and voice any concerns about it.

If there are any bugs, I'll fix them. Except for aforementioned features I am not planning on adding many new features. I will probably create an "emulation" version of the API so development is more comfortable / possible on other machines. I am also thinking about writing some kind of commandlist parser generator since this is a repeating pattern in DSP development. None of these things will break the current API, though.

  • c64_load goes to /sbin/
I'd prefer /usr/sbin, /sbin is usually for limited set of core stuff.

Hmm, that was the idea. It does not get much more low level / core than this ?!

My understanding of the filesystem structure is that anything close to HW goes to /sbin, and the more high level system-level software goes to /usr/sbin.

Ok /usr/pandora/scripts/op_dsp_c64.sh sounds good to me. I don't think we should be asking for password, as it will be part of firmware, it can at least pretend it knows what it's doing and load stuff silently. Otherwise it will just annoy the user everytime he wants to run something that supports the DSP.

ok, agreed then (regarding script naming). I'll write the script then. I'd still suggest to use the dialog but I'll make that configurable. I assume you are suggesting to set the "setuid" flag for the script instead of the dialog ?
 
  • c64_load goes to /sbin/
  •  
I'd prefer /usr/sbin, /sbin is usually for limited set of core stuff.

Hmm, that was the idea. It does not get much more low level / core than this ?!

My understanding of the filesystem structure is that anything close to HW goes to /sbin, and the more high level system-level software goes to /usr/sbin.
http://www.pathname.com/fhs/pub/fhs-2.3.html#SBINSYSTEMBINARIES

Utilities used for system administration (and other root-only commands) are stored in /sbin, /usr/sbin, and /usr/local/sbin. /sbin contains binaries essential for booting, restoring, recovering, and/or repairing the system in addition to the binaries in /bin. [18] Programs executed after /usr is known to be mounted (when there are no problems) are generally placed into /usr/sbin. Locally-installed system administration programs should be placed into /usr/local/sbin. [19]
 
Last edited by a moderator:
well, c64_load boots the DSP but in this case this is optional so we might as well put it into /usr/sbin *shrug*

Could you write a new thread with :
- Instruction to use you libraries for DSP virgin devs
- Any usefull material you have at hands. Mods will sticky it, so we could have that at hand in no time
I'll do that (originally I wanted to write a Wiki article but this could start as a thread, why not). The doclinks are in the "Reference material" thread.
 
Last edited by a moderator:
that could be confusing. what about using the same dir. (/lib/dsp) for c64_tools images as well ? The dir. is almost empty right now and dspbridge uses a different image file suffix (.dof) than c64_tools (.out).
Ok let it be /lib/dsp/ then. 

Ok /usr/pandora/scripts/op_dsp_c64.sh sounds good to me. I don't think we should be asking for password, as it will be part of firmware, it can at least pretend it knows what it's doing and load stuff silently. Otherwise it will just annoy the user everytime he wants to run something that supports the DSP.
ok, agreed then (regarding script naming). I'll write the script then. I'd still suggest to use the dialog but I'll make that configurable. I assume you are suggesting to set the "setuid" flag for the script instead of the dialog ?
One problem with the dialog is that fullscreen framebuffer apps can't use it. Your dialog will be started but the user won't see it because app is controlling the whole screen. That will look like the fb app hung while it's actually waiting for your script to finish, and your script is in turn waiting for invisible uncontrollable dialog. You'd also have dependency on X server which isn't needed for the job of booting the DSP.


Currently we just put scripts like this into sudoers file, which allows user (or programs) to run it without password with sudo. Things like overclocking, DSS gamma/filters are already done this way.
 
I've looked at the code a bit, some review/suggestions:

  • would be nice not to have to include include/inc_libc64.h, just include/dsp.h instead and that dsp.h would take care of it's dependencies. inc_libc64.h pulls in stuff that not everyone needs, like log, osal or c64_dev.h with ioctl numbers that apps shouldn't really use (the whole point of libc64). It also increases chances of name collisions if DSP is integrated in some existing program.
  • inc_libc64.h includes c64_dev.h and c64_dev.h is marked GPL only, making impossible to use libc64 from non-GPL program.
  • maybe move c64_load stuff out of dsp.h (dsp_private.h?) to reduce confusion?
  • maybe error out on multiple dsp_open()s in a row?
  • DSP_WRITE_REGS in dsp.h - some leftover?

Feature requests:

(I'm not saying these should be implemented now/for first fw release, but I think the API could be expanded now to allow it. I'm willing to work on this in future, if needed).

  • Write-through cached (on GPP side) shared memory. Such memory is cached but all writes always go to RAM, meaning that no cache cleaning is needed to pass data to DSP, improving performance (Cortex-A8 cache operations are slow). Perhaps this could be implemented as memory attribute argument to dsp_shm_init/dsp_shm_get, which would also allow uncached memory (might be useful in some cases).
  • With the above, it might make sense to allow 2 shared memory regions with different memory attributes, one for streaming data to DSP and one for streaming back.
  • Allow using user-supplied buffer as shared memory. This would be limited to either single 4kB page, or 2MB huge page (both are contiguous in physical RAM). I'm already using huge pages in PCSX ReARMed which allows to reduce TLB cache misses, noticeably improving performance. I'd like to be able to access those buffers from DSP.
 
GCC on the build server doesn't like your asm:


dsp.c: In function ‘dsp_fastcall_rpc’:
dsp.c:1472: error: expected string literal before ‘)’ token
dsp.c:1507: error: expected string literal before ‘)’ token
dsp.c: In function ‘dsp_fastcall_end’:
dsp.c:1601: error: expected string literal before ‘)’ token

 
Removing extra colons seems to help:


diff -ur c64_tools_/libc64/dsp.c c64_tools/libc64/dsp.c
--- c64_tools_/libc64/dsp.c     2013-10-05 16:17:02.000000000 +0300
+++ c64_tools/libc64/dsp.c      2013-10-15 02:11:28.260001505 +0300
@@ -1469,7 +1469,7 @@
           "\n\t nop"
           "\n\t nop"
           "\n\t nop"
-          ::;
+          );
 
       ////log_printf("timeout=0x%08x\n", timeout);
    }
@@ -1504,7 +1504,7 @@
              "\n\t nop"
              "\n\t nop"
              "\n\t nop"
-             ::;
+             );
       }
       while((fcCtl != DSP_FSHM_FASTCALL_CTL_ACK) &&
             (timeout < FASTCALL_TIMEOUT)
@@ -1598,7 +1598,7 @@
           "\n\t nop"
           "\n\t nop"
           "\n\t nop"
-          ::;
+          );
    }
 
    if(FASTCALL_TIMEOUT == timeout)

(if you had github it would be easier to send you patches, but forum diffs can also do it I guess)..

Edit: the build also system wants libc64.so to have a version, so you need to use -Wl,-soname,libc64.so.1 or similar.

We should probably also create a -dev package, so the includes should go to /usr/include/ somewhere, I suggest /usr/include/c64_tools/ (that's optional, but would be nice as we have some people compiling on pandoras themselves).
 
Last edited by a moderator:
We should probably also create a -dev package, so the includes should go to /usr/include/ somewhere, I suggest /usr/include/c64_tools/ (that's optional, but would be nice as we have some people compiling on pandoras themselves).
+1 to that; I write and compile all my Pandora stuff on the Pandora itself.
 
+1 to that; I write and compile all my Pandora stuff on the Pandora itself.
But get ready to use a compiler that will create shitty DSP code thuss probably slower than the ARM core could do...
I'm not planning to write DSP code, but maybe I will call some DSP routines from some standard library. For that I would still need the libdsp include files.
 
One problem with the dialog is that fullscreen framebuffer apps can't use it. Your dialog will be started but the user won't see it because app is controlling the whole screen.
good point. Now that you mention this I've seen this happening with the WiFi dialog as well (e.g. when toggling WiFi from the MiniMenu). Maybe this could (somehow) be addressed in the firmware update as well ?

would be nice not to have to include include/inc_libc64.h, just include/dsp.h instead and that dsp.h would take care of it's dependencies. inc_libc64.h pulls in stuff that not everyone needs, like log, osal or c64_dev.h with ioctl numbers that apps shouldn't really use (the whole point of libc64). It also increases chances of name collisions if DSP is integrated in some existing program.
You may have noticed that I used inc_libc64.h only in the examples. I dislike header files that include other header files, I like to be able to quickly see what headers are effectively included in a source file. The inc_libc64.h is a compromise for devs who are not too familiar with the libc64 header file dependencies and rather have a few unused header files included than spend time figuring out the dependencies.

If I were to move the content of inc_libc64.h to dsp.h, it would only make the situation worse because dsp.h would still include possibly unused headers and there would be no way to only include what's really needed (e.g. skip log.h if you have your own log routines).

  • inc_libc64.h includes c64_dev.h and c64_dev.h is marked GPL only, making impossible to use libc64 from non-GPL program.
  • maybe move c64_load stuff out of dsp.h (dsp_private.h?) to reduce confusion?
  • maybe error out on multiple dsp_open()s in a row?
  • DSP_WRITE_REGS in dsp.h - some leftover?
fixed that just now. c64_dev.h is now LGPL and also not included by inc_libc64.h anymore (was not needed anyway). libc64 internal dsp_* functions were moved to dsp_priv.h. dsp_open() can now be called multiple times (the 2nd (redudant) call will print a warning). DSP_WRITE_REGS was indeed a leftover and was used nowhere, I removed it.

Write-through cached (on GPP side) shared memory. Such memory is cached but all writes always go to RAM, meaning that no cache cleaning is needed to pass data to DSP, improving performance (Cortex-A8 cache operations are slow). Perhaps this could be implemented as memory attribute argument to dsp_shm_init/dsp_shm_get, which would also allow uncached memory (might be useful in some cases).

[..]

With the above, it might make sense to allow 2 shared memory regions with different memory attributes, one for streaming data to DSP and one for streaming back.
Regarding support for different memory attributes: I was thinking about adding this, too (cached + uncached types) (CMEM has this feature, too). Having a (third) write-through memory type would indeed be very useful.

A side note: as you may have noticed, dsp_shm_init() may only called once (for each mem type). The reasoning behind this is that I wanted to avoid that dma_alloc*() gets called for small memory blocks (i.e. avoid waste (allocations are aligned to 4k page size) and any potential instabilities, and keep the kernel module as simple as possible). Instead, applications should estimate the max. shared mem usage, then use a userland allocator to manage the shared memory region (I am currently adding Doug Lea's allocator). If this strategy turns out to be problematic we can add support for multiple mem. regions later w/o changing the API.

TBH, I have no idea how to implement this [write-through mem. type], though. The DMA kernel API does not support this, yet, as far as I can see (looked at arch/arm/include/asm/dma-mapping.h and include/linux/dma-attrs.h). I presume this would require a kernel modification ?

If that's true it might be a good idea to add this before releasing a new firmware..?

It would also be very good if someone could investigate why the CACHE_AC_INV and CACHE_AC_WBINV ioctls resp. dmac_map_area() kernel calls cause sporadic system freezes. Currently flush_cache_all() is called instead. CACHE_AC_WB (also uses dmac_map_area()) works fine, though.

I would say that this should have higher priority than the write-through feature since the performance impact (in general) will be bigger.

This depends on the exact use case, of course - if an app. uses a relatively small memory region for "to-dsp" data that is frequently read by the GPP as well, write-through is more practical / performant (I assume that write-through write speed is the same as writing to uncached memory).

For memory regions mostly written to by the GPP, and mostly/only read by the DSP, fast / working cache functions are more critical.

Allow using user-supplied buffer as shared memory. This would be limited to either single 4kB page, or 2MB huge page (both are contiguous in physical RAM). I'm already using huge pages in PCSX ReARMed which allows to reduce TLB cache misses, noticeably improving performance. I'd like to be able to access those buffers from DSP.
Sounds interesting but I am totally unfamiliar with the huge page feature. I would suggest that I'll upload the c64_tools sources to github once the API is final, then you can add this feature if you know how to do that and find the time.

           "\n\t nop"-          :: :) ;

+          );
I am using the code sourcery arm-2011.09 toolchain here. I removed the colons and the code still compiles fine so consider this fixed.

the build also system wants libc64.so to have a version, so you need to use -Wl,-soname,libc64.so.1 or similar.
interestingly, the cmdline above triggers sporadic link errors (wth?!).

Is it ok to use "-soname,libc64.so", then mv libc64.so to libc64.so.1 and ln -s libc64.so.1 libc64.so ?

(just added that to the makefile)

We should probably also create a -dev package
ok, that can be done later (has nothing to do with firmware update, right?)

For now, just don't try to build things within the dsp/ directory. That should work fine on the Pandora itself.
 
Last edited by a moderator:
One problem with the dialog is that fullscreen framebuffer apps can't use it. Your dialog will be started but the user won't see it because app is controlling the whole screen.
 good point. Now that you mention this I've seen this happening with the WiFi dialog as well (e.g. when toggling WiFi from the MiniMenu). Maybe this could (somehow) be addressed in the firmware update as well ?
Maybe, but I don't have many ideas about that.
You may have noticed that I used inc_libc64.h only in the examples. I dislike header files that include other header files, I like to be able to quickly see what headers are effectively included in a source file.
Then you should add#ifndef SOMEFILE_INCLUDED

#error please include <somefile.h>

#endif

or something like that to headers with dependencies, otherwise you force people dig through your include dependency tree, adding unneeded roadblocks for development. It would be fine if for example dsp.h had no deps on other includes, but now it does.

The inc_libc64.h is a compromise for devs who are not too familiar with the libc64 header file dependencies and rather have a few unused header files included than spend time figuring out the dependencies.
But it pulls in quite some potentially unwanted extras like logging and the osal thing. At least something like inc_libc64_minimal.h would be nice if you really want to have it that way.
If I were to move the content of inc_libc64.h to dsp.h, it would only make the situation worse because dsp.h would still include possibly unused headers and there would be no way to only include what's really needed (e.g. skip log.h if you have your own log routines).
A proper header file should only include it's dependencies and nothing else. Imagine if including <stdio.h> would spit errors at you until you found and included some 10+ dependencies that it needs..
fixed that just now. c64_dev.h is now LGPL and also not included by inc_libc64.h anymore (was not needed anyway). libc64 internal dsp_* functions were moved to dsp_priv.h. dsp_open() can now be called multiple times (the 2nd (redudant) call will print a warning). DSP_WRITE_REGS was indeed a leftover and was used nowhere, I removed it.
Thanks.
TBH, I have no idea how to implement this [write-through mem. type], though. The DMA kernel API does not support this, yet, as far as I can see (looked at arch/arm/include/asm/dma-mapping.h and include/linux/dma-attrs.h). I presume this would require a kernel modification ?

If that's true it might be a good idea to add this before releasing a new firmware..?
I'm not sure too but it can be done one way or another. I don't think it's worth delaying the release though, I was just bringing this up for API considerations.
It would also be very good if someone could investigate why the CACHE_AC_INV and CACHE_AC_WBINV ioctls resp. dmac_map_area() kernel calls cause sporadic system freezes. Currently flush_cache_all() is called instead. CACHE_AC_WB (also uses dmac_map_area()) works fine, though.

I would say that this should have higher priority than the write-through feature since the performance impact (in general) will be bigger.
For large buffers (above L2 cache size) flush_cache_all() is actually better because otherwise the code has to walk the whole address range in cacheline steps issuing slow cache control instructions. But I agree this should be looked at eventually for small buffers at least.
Allow using user-supplied buffer as shared memory. This would be limited to either single 4kB page, or 2MB huge page (both are contiguous in physical RAM). I'm already using huge pages in PCSX ReARMed which allows to reduce TLB cache misses, noticeably improving performance. I'd like to be able to access those buffers from DSP.
 Sounds interesting but I am totally unfamiliar with the huge page feature. I would suggest that I'll upload the c64_tools sources to github once the API is final, then you can add this feature if you know how to do that and find the time.
Sounds good to me.
the build also system wants libc64.so to have a version, so you need to use -Wl,-soname,libc64.so.1 or similar.
 interestingly, the cmdline above triggers sporadic link errors (wth?!).

Is it ok to use "-soname,libc64.so", then mv libc64.so to libc64.so.1 and ln -s libc64.so.1 libc64.so ?

(just added that to the makefile)
That won't work because it puts the soname into ELF headers and the OE build system is checking that. Doesn't doing "-soname,libc64.so.1" and "ln -s libc64.so.1 libc64.so" work?
We should probably also create a -dev package
ok, that can be done later (has nothing to do with firmware update, right?)For now, just don't try to build things within the dsp/ directory. That should work fine on the Pandora itself.
The -dev package is automatically built and published, if we don't fix it now it will just make a broken -dev package. You just need to tell me include location (I suggest /usr/include/c64_tools/) and I'll take care of the rest.
 
Last edited by a moderator:
At least something like inc_libc64_minimal.h would be nice if you really want to have it that way.
okay, I'll add that.

TBH, I have no idea how to implement this [write-through mem. type], though. The DMA kernel API does not support this, yet, as far as I can see (looked at arch/arm/include/asm/dma-mapping.h and include/linux/dma-attrs.h). I presume this would require a kernel modification ?


If that's true it might be a good idea to add this before releasing a new firmware..?
I'm not sure too but it can be done one way or another. I don't think it's worth delaying the release though, I was just bringing this up for API considerations.
I am currently busy adding the three different mem types (cached, uncached, write-through -- almost done) and noticed that the writethrough code _is already there_ ! currently dsp_shm_init() allocates write-through / read cached memory :D

should be simple to change the __pgprot() params to support the uncached and read/write-cached variants.

For large buffers (above L2 cache size) flush_cache_all() is actually better because otherwise the code has to walk the whole address range in cacheline steps issuing slow cache control instructions. But I agree this should be looked at eventually for small buffers at least.
hmm I just discovered some assembly functions in the cmemk.c source: v7_dma_inv_range, v7_dma_clean_range, v7_dma_flush_range. maybe these can be borrowed (haven't tested them, yet).

That won't work because it puts the soname into ELF headers and the OE build system is checking that. Doesn't doing "-soname,libc64.so.1" and "ln -s libc64.so.1 libc64.so" work?
not reliably, sometimes the build fails and claims that the "pthread_yield" symbol suddenly cannot be resolved.

strange. I have to try if there is a newer version of the toolchain.

Maybe I can also add a workaround that checks whether the CS 2011.09 tools are used and use the workaround to build an .so with the wrong version info (only for dev. on my system).

The -dev package is automatically built and published, if we don't fix it now it will just make a broken -dev package. You just need to tell me include location (I suggest /usr/include/c64_tools/) and I'll take care of the rest.
ok then. /usr/include/c64_tools/ sounds fine to me. This folder should include libc64/ and include/ subfolders (yea, not ideal, but this way the paths are exactly the same like when the c64_tools.tar.gz is used)
 
Last edited by a moderator:
That won't work because it puts the soname into ELF headers and the OE build system is checking that. Doesn't doing "-soname,libc64.so.1" and "ln -s libc64.so.1 libc64.so" work?
 
not reliably, sometimes the build fails and claims that the "pthread_yield" symbol suddenly cannot be resolved.


strange. I have to try if there is a newer version of the toolchain.


Maybe I can also add a workaround that checks whether the CS 2011.09 tools are used and use the workaround to build an .so with the wrong version info (only for dev. on my system).
Weird, it doesn't do that for me.


I've noticed running make rebuilds libc64.so even if nothing changed. You should really just use $(TARGET_SHARED) and $(TARGET_STATIC) as targets instead of bin_shared and bin_static that you currently have.

The -dev package is automatically built and published, if we don't fix it now it will just make a broken -dev package. You just need to tell me include location (I suggest /usr/include/c64_tools/) and I'll take care of the rest.
 
ok then. /usr/include/c64_tools/ sounds fine to me. This folder should include libc64/ and include/ subfolders (yea, not ideal, but this way the paths are exactly the same like when the c64_tools.tar.gz is used)
libc64/ ? What for? I don't see any headers there. libc64.a will go to /usr/lib/ in the -dev package.

There is also this in inc_libc64.h:

#include "../dsp/libc64_dsp/include/com.h"

could it maybe be moved to include/ and renamed to dsp_com.h or something?
 
Back
Top