Rockbox mail archive
Subject: Various bug fixes and improvements - where to put?
From: Jens Arnold (arnold-j_at_t-online.de)
Hello core developers,
as I have done various patches and improvements on different
low-level routines in Rockbox I would like to know where I can
put these in order to be revised and eventually integrated into
the main cvs.
(Yes I know that there is a patch tracker, but it seems to me
that the patches stay there a rather long time.)
In detail I did the following:
- corrected memory waste in "bitswap.s" and "descramble.s"
(simple .align issue)
- 20% faster bitswap
- new turbo-charged copy_read_sectors() in "ata.c", with speed
relations as follows:
| word-aligned | unaligned
C original | 2.02 MB/s (100 %) | 1.71 MB/s (100 %)
[IDC]Dragon | 3.42 MB/s (169 %) | 2.22 MB/s (130 %)
new version | 4.76 MB/s (236 %) | 3.33 MB/s (195 %)
(These values were calculated from the machine clock cycles
not taking memory or i/o wait states into account, but the
relations should be correct)
I know that there is an issue even with Jörg's version
depending on the disk type, but I could not test this since my
recorder did not have problems with Jörg's version either.
Perhaps somebody could test my new routine on such a
I did also modify set_features() to explicitly set PIO3 resp.
PIO4 with IORDY support.
- the take-out of cli() / sti() and replacement of these by
set_irq_level() made disk loading in the video player extremely
slow. I found the cause of this being that set_irq_level
is called from within lcd_write_data twice for every byte,
which means that it is called almost 120,000 times per second
while watching video. But, set_irq_level is not in IRAM!
I worked around the problem by replacing set_irq_level by
inline assembler (only in lcd_write_data).
I don't understand what the intention in replacing cli() / sti()
by set_irq_level() really is. The comment says that cli() / sti()
are not safe.
As far as I understand it, cli() / sti() _would_ be safe as long
as it is _mandatory_ that a thread that calls cli() _must not_ call
yield() until it calls sti() again, as otherwise another thread
could then call sti() before it was intended by the first thread.
With the new set_irq_level(), you store the old irq level when
disabling interrupts and re-set this old level afterwards. As
long as the same preconditions hold here, this is exactly the same
security level as with cli() / sti(), only with more overhead,
but _if_ you allow yield() from within an interrupt-disabled area,
this is _also unsafe_:
(For clarity there are only 2 threads in this example)
- Thread 1 disables interrupts and stores old level (15)
- Thread 1 then yield()s, thread 2 is now running
- Thread 2 also disables interrupts storing the old level it
- Thread 2 then yield()s, thread 1 is again running
- Thread 1 re-sets interrupt level to its old level(15).
Problem 1: This is certainly not what thread 2 expects when
- Thread 1 yield()s again, thread 2 runs
- Thread 2 re-sets interrupt level to its old level(0).
Problem 2: This is (a) not what thread 1 expects and (b)
interrupts are disabled now in spite of both threads wanting
to re-enable them!
If you want to allow yield() from within "interupt disabled"
blocks, we would need cli() and sti() functions that preserve a
"nesting count", i.e. cli() disables interrupts (always) and
increments this count, sti() decrements this count and enables
interrupts _only_ if the nesting count reaches zero.
Page was last modified "Jan 10 2012" The Rockbox Crew