Rockbox

Tasklist

FS#12403 - INIT_ATTR for SH Archos HWCODEC targets

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Wednesday, 23 November 2011, 20:06 GMT
Task Type Patches
Category Operating System/Drivers
Status New
Assigned To No-one
Operating System HW-codec
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

I saw that INIT_ATTR isn't implemented HWCODEC targets so I decided to implement it. This patch consists of several parts, which could be split if desired.
- Rewriting of SH crt0.S to copy and fill sections using a list of values in memory instead of repeated code. This adds an insignificant bit of overhead between operations, but the actual loops themselves are as fast as before. (Using copy and paste to create .init copying code felt wrong, and SH assembler was intriguing.)
- Enabling INIT_ATTR by editing config.h, adding the section to linker scripts, and copying .init in crt0.S.
- Changes to allow use of the plugin buffer for INIT_ATTR. Normally, the codec buffer would be used, but HWCODEC targets don't have a codec buffer. In skin_data_load, buffer space is obtained from plugin_get_buffer. This buffer would otherwise overwrite INIT_ATTR code. The code only uses a small fraction of the buffer, and plenty remains for other uses.
- Adding INIT_ATTR mp3_init and functions only called from it.

This is currently a bit detrimental for RomBox. Initialization code in flash does not use RAM, and code added to implement this uses a bit of additional flash space. I didn't even test RomBox because I don't know how to fit current versions of Rockbox in ROM on my Recorder v2. However, I have in idea on how this could help RomBox in the future: .data and .init could be UCL packed to save flash space.

BTW While reviewing Archos target specific code for INIT_ATTR addition, I saw that the following functions could probably use INIT_ATTR:
serial_setup();
button_init_device(), but it's declared in many header files
usb_init_device(), but it's declared in many header files
system_init() for CONFIG_CPU != SH7034 (due to rolo)
i2c_init() for CONFIG_CPU != SH7034 (due to rolo)
ata_device_init() STORAGE_INIT_ATTR
ata_is_coldstart() STORAGE_INIT_ATTR
This task depends upon

Comment by Thomas Martitz (kugel.) - Thursday, 24 November 2011, 07:55 GMT
Don't really like the plugin buffer related changes (can't comment on the SH specifics).

I suggest putting it into the audio buffer. Buflib has one function (buflib_buffer_out()) which removes buffer from the start of the pool. You can use this to remove the .init memory of the pool before the first alloc is made. You can add it back to buflib once .init isn't needed anymore with buflib_buffer_in(). (buflub_buffer_out() may not even be needed if buflib_init() is called accordingly). This can be done for all targets.
Comment by Boris Gjenero (dreamlayers) - Thursday, 24 November 2011, 16:27 GMT
buflib_buffer_out() and buflib_buffer_in() move data but don't call the move callbacks. I shouldn't use them because some allocations made during init() may depend on move callbacks.

Can you suggest another solution? Should they (or at least buflib_buffer_in()) be changed to call move callbacks? Using core_alloc() is another possibility, but I don't like that because the linker script would be assuming things about internals of core_alloc().

Edit: buflib.c could reserve space for the header it puts at the beginning of the allocation by putting some data in a specific section which the linker script puts right before .init. For example, this ought to reserve enough for an allocation with name ".init":
__attribute__((section(".init.header"))) __attribute__((aligned(sizeof(union buflib_data)))) union buflib_data buflib_init_header[5];
Then, that can be put at the right place by putting "*(.init.header)" before *(.init)" in the linker script.
Comment by Thomas Martitz (kugel.) - Friday, 25 November 2011, 08:53 GMT
Well, of course buflib_buffer_out() is safe if no allocation has been made. I meant to make this call as part of core_allocator_init(). However, here that might not even be needed, as buflib_init() can be initialized with the buffer that has .init excluded.

buflib_buffer_in() is safe as it doesn't make the buffer smaller and overwrites no memory. It doesnt call the move_callback but that only means the buffer is not compact (not a problem) after the call.
Comment by Boris Gjenero (dreamlayers) - Friday, 25 November 2011, 16:13 GMT
Look at these two snippets from firmware/buflib.c. Clearly, buflib_buffer_in() moves data.

void
buflib_buffer_in(struct buflib_context *ctx, int size)
{
size /= sizeof(union buflib_data);
buflib_buffer_shift(ctx, -size);
}

static void
buflib_buffer_shift(struct buflib_context *ctx, int shift)
{
memmove(ctx->buf_start + shift, ctx->buf_start,
(ctx->alloc_end - ctx->buf_start) * sizeof(union buflib_data));

(buflib_buffer_out() does the same thing, but yeah, that doesn't matter.) Should buflib_buffer_in() be changed to create an empty block at the start instead of moving data? It seems free blocks don't need a handle. The only new limitation is that the added space at the start must be large enough to hold the header of the free block, and that isn't a problem.
Comment by Thomas Martitz (kugel.) - Friday, 25 November 2011, 16:52 GMT
I'm sorry. My memory was wrong. I must have mixed it up with something.

buflib_buffer_in() could be changed. However, I IIRC pictureflow uses it so need to be careful with changing the behavior. Perhaps add a new function or parameter instead, or make it call the move callbacks (that would be equal to no memmove and forcing a compaction run). All three should be trivial. The header of a free block is one union buflib_data, where val is negative. Only allocated blocks have more metadata.
Comment by Boris Gjenero (dreamlayers) - Sunday, 27 November 2011, 02:15 GMT
Pictureflow doesn't assume things about where allocations are located. It uses buflib_get_data properly, and it only stores handles. Changing buflib_buffer_in() to put a free block at the start should work. I'm attaching a patch for this. However, it doesn't work, because of bugs in buflib.

In r31066 I fixed one of the bugs, which caused handles to be updated incorrectly. The bug didn't cause problems because only pictureflow uses it, and pictureflow doesn't attempt to use the buffer while it's in a moved state.

I also ran into another bug: buflib_compact() lost the last allocation, which ends at ctx->alloc_end after buflib_buffer_out(). I don't have a plan yet for how to fix buflib_compact(). I posted my observations at  FS#12409 .

Finally, I wonder how to reserve the .init space at the start. Something should be done in core_alloc code, probably core_alloc_init(), because core_alloc code can access core_ctx. However, core_allocator_init() currently doesn't use addresses from the linker, and it instead gets buffer size from buffer_get_buffer(). A solution could involve both firmware/core_alloc.c and firmware/buffer.c, but buffer.c seems obsolete, and it would be cleaner to just get rid of buffer.c and put the needed functionality in core_alloc.c.
Comment by Boris Gjenero (dreamlayers) - Friday, 09 December 2011, 20:15 GMT
Here's an updated patch which puts .init in the buffer used by core_alloc. It requires and includes buflib_buffer_in_no_moving.patch.

in r31189 on the Recorder v2, It saves 3228 bytes of RAM but increases binary size by 84 bytes. It's not exciting but I think it's worth it. I suspect INIT_ATTR isn't used to its full potential, and this patch will allow HWCODEC targets to benefit as more INIT_ATTR is added.

It's possible to put .init at audiobuf for all targets. However, I don't like the idea because that requires:
- increasing binary size a bit and additional moves of allocated data.
- editing many linker scripts

The firmware/buflib.c change could be made only for (CONFIG_CODEC != SWCODEC) && defined(HAVE_INIT_ATTR) if desired. I think such different buflib behaviour on different targets is not a good idea.
Comment by Boris Gjenero (dreamlayers) - Monday, 16 January 2012, 16:32 GMT
My main obstacle to finishing this task: I don't like the idea of committing a change that has a negative effect on popular SWCODEC targets, just for the sake of adding INIT_ATTR to unpopular HWCODEC targets.

Moving .init to the main buffer has no benefits for SWCODEC. It just means added code and probably also more buflib data moves at startup. The codec buffer is a fine place for .init, and I don't expect any change that will make it unsuitable.

Even just the different buflib_buffer_in() may have a negative effect. It's needless added code, and it may slow things down because some moving will be done via buflib_compact() instead of via a simple memcpy() of the whole area. I thought about adding preprocessor conditionals there, but I guess two different buflib_buffer_in() implementations may be even more unpopular than two different .init locations.

Maybe I'm too much of a perfectionist? I don't know.

Loading...