- Status Closed
- Percent Complete
- Task Type Patches
- Category Drivers
- Assigned To No-one
- Operating System All players
- Severity Low
- Priority Very Low
- Reported Version Version 3.1
- Due in Version Undecided
-
Due Date
Undecided
- Votes
- Private
FS#10015 - Use chained transfer descriptor to speed up USB transfers on PP and iMX31
Use chained transfer descriptors to allow larger blocks of data in a single transfer. This speeds up USB transfers significantly.
Closed by fg
2009-03-29 19:59
Reason for closing: Accepted
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
2009-03-29 19:59
Reason for closing: Accepted
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
Committed as r20570
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
This crashes on gigabeat S for some reason. Taking out some error handling (that I’ve never seen triggered) helps. The problem is that this also takes size of transfered data, which doesn’t matter for storage, but it does for e.g. serial. This means that this patch, while good for testing, isn’t ready for commit.
It seems like each transfer descriptor can handle 20K. Why are they being limited to 16K?
They can handle 20K, if the data is 4K-aligned. If not, you can only get 16K (or actually, you can get whatever fits. Just assuming 16K is safe and simple)
I have tested the patch on H10-20 with 60GB HDD using UDMA1 mode.
USB wite (PC→H10) speed is 9.4MB/s (about 1.5MB/s faster then using OF) without the patch and about 11.2MB/s with the patch, and USB read speed is 8MB/s (OF attains 14-18MB/s) no matter the patch is applied or not.
Tested on my e250v1.
3.1mb/s write, 6.1mb/s read without patch.
3.3mb/s write, 6.1mb/s read with patch.
I tried chained-tds-2.diff on my PC that had disconnects with Rockbox’s USB mode and I saw a pretty big improvement. With this patch I didn’t get a disconnect until over 5GB had been transfered. Without it I typically got one within a couple hundred MB, with 500MB being relatively rare.
Tested chained-tds-2.diff with r20323 and
FS#9708v0.6 (UDMA1 and UDMA4) on my 5.5G.Keeping the CPU at 80MHz I get:
PIO4: 8100 KB/s write, 4700 KB/s read
UDMA1: 7100 KB/s write, 9400 KB/s read
UDMA4: 7100 KB/s write, 11300 KB/s read
When setting the CPU to 100MHz I get:
UDMA1: 8100 KB/s write, 9400 KB/s read
UDMA4: 8100 KB/s write, 11300 KB/s read
Edit: Oops, corrected read/write.
This one adds proper length handling, but it gets data aborts on gigabeat S at 0x59a18 (see the attached elf file for context)
Edit: I forgot the buffer size change to usb_storage in this one. It needs that too of course to get proper speed benefits
The data abort seems to happen while accessing td→reserved in:
while(td!=QH_NEXT_TERMINATE)
{
length += 1);
}
By the way, do ‘transfer_descriptor’ and ‘queue_head’ need to be byte packed? If the related struct members are accessed often, there might be a performance loss.
((td→size_ioc_sts & DTD_PACKET_SIZE) » DTD_LENGTH_BIT_POS
I don’t really see how this can happen, as td→reserved should be nicely aligned. I’ll check if it never jumps to an invalid location.
transfer_descriptor and queue_head are used directly by the hardware, so they need to be precisely packed and aligned. They only consist of 4-byte int fields though, so I don’t think it should make any difference
It was a bad pointer. Fixed now (and committed)