|
Rockbox mail archiveSubject: Various bug fixes and improvements - where to put?Various bug fixes and improvements - where to put?
From: Jens Arnold <arnold-j_at_t-online.de>
Date: Mon, 01 Mar 2004 23:34:38 +0100 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 problematic box. 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 reads (0) - 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 running again. - 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. Regards, Jens _______________________________________________ http://cool.haxx.se/mailman/listinfo/rockbox Received on 2004-03-01 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |