Rockbox.org home
Downloads
release
dev builds
extras
themes
Documentation
manual
wiki
device status
Support
forums
mailing lists
IRC
Development
bugs
patches
dev guide
Search



Donate

Rockbox mail archive

Subject: Various bug fixes and improvements - where to put?
From: Jens Arnold (arnold-j_at_t-online.de)
Date: 2004-03-01

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

Page was last modified "Jan 10 2012" The Rockbox Crew
aaa