Rockbox

When you say "strange HDD activity on iPod 5G", are you referring to  FS#8397 ? This bug was reported to appear before r16105 was committed.

No, I am talking of very long loading time for buffering on iPod Video 60/80GB (not the 30GB!). This patch was also tested by a volunteer in IRC and solves this issue.

Let me clarify. From rev 16105 onwards, the SVN had an issue on 60/80 Ipod videos, which caused the buffering of music to take a lot longer than before, which degraded teh battery life by 2+ hours. I stopwatched several SVNs buffering 21 files with a total size of 125MB or something. I measured the time from HD spin up to HD spindown, and also noted the times HD activity seemed to be done (immense disk reading).
svn pre 16105 came out 20 seconds intensive reading, total spin time of HD was 1:20 minutes.
svn post 16105 came out 1:07 minutes intesive reading, total spin time of HD was 2:30 mintes
i tested up to svn rev 16243 with the same issues as post 16105. Latest SVN (16254) with above patch returned the intense reading to 20 secs, and total spin time to 1:20

I hadn't taken the time to do controlled testing but I had definitely noticed this. Good job Sascha.

I'd suggest looking for a deeper reason for the problem and wouldn't consider this patch to really solve the problem but to be a reversion to hiding something important. How about finding _why_ it happens?

1) Why no issue on any other model? I did fix a stupid mistake of mine which did affect many models and sensibly so. I haven't perceived any negative effect on the many players I have whether flash based or HD based. Perhaps we simply have some other "oops" factor at play.

2) This should have no effect at all since a mutex provides no more a barrier to critical section entry and thread execution than the "spinlock". The difference may be a change in thread scheduling which could cause misbehavior due to race conditions.

3) Scheduler turnaround overheads are in the 15-25 microsecond range for even the worst-case of a queue_send to a thread on a timeout wait (PP502x at 80MHz). Hardly significant relative to HD speeds and the rather low 2000 or so read calls to fill the file buffer should at worst add up to a few millseconds. That's _if_ blocking and wakeup is involved.

4) Based on above, does 5g have any particularly time-consuming subsystem that placing a thread at the end of the run list may delay execution of the buffering thread? Changing to insert-next didn't show an effect but that was before fixing the (rather embarrassing) priority_yield mistake.

5) Does MPEGPlayer significantly vary in its buffering behavior with or without this patch? It should be equally if not more adversely affected. Observe the audio buffer fullness indication in "Show FPS".

Perhaps I'll post the change mentioned in 4) here so it may be tested with the other fix in place.

MikeS:

I do not want this patch as final solution. My aim is to kind of repair the svn for the 5G 60/80GB users. You will understand that it is quite hard to do battery runtime tests (which were and are needed for the device disables patch) when the svn degraded that much.

So, why not backrolling the changes in svn to have svn working for all and open an flyspray entry to re-add it and work on the 5G issue? There are several testers available who can (and I think will) support any testing.

Sure. I just really need to know what's going on here with one particular device. I'd even like to see a short video of the buffering screen under varying conditions with and without something else updating like the database or tagcache in the background. What's happening with only playback accessing the drive will have no codepath difference since the lock will be granted immediately with no scheduler involvement at all.

With this patch the thread using the disk will be more aggressively run if it was blocked and explicitly woken so it should actually have an advantage over spinlock.

MikeS: Only the 5G iPod with a large 60GB or 80GB disk has an issue (on my 5G 30GB there is no problem at all). For the 5G iPods there were added changes in ata.c and an additional #define (#define MAX_PHYS_SECTOR_SIZE 1024) in config-ipodvideo.h to get the large disks working in rockbox. Either if it's timing or an general issue -> I assume the root cause is somewhere within ata.c in combination with the larger 5G disks.

Michaels patch (applied on stock svn, new checkout) has slightly better values than unpatched SVN, but still quite a fair amount behind Andree's revert.
I now get 50 seconds of "buffering" and 2:05 minutes HD spin time. I have been asked several times, why my HD doesn't shut off right after buffering, so I think i should calrify this here.

in the first 20/50/70 seconds (depending on which patch applied) i can hear continous read activity on the HD (the usual "ticking" noise). After that, i can only hear the HD spining. Every 4 or 5 seconds read activity can be heard on the HD for a brief second. That's why the HD shuts down after 1:20/2:05/2:30 respectively

I hope these values are helpfull at all. All values were measured with no background activity (they were measured several times on each patch/build as well, all within +/- 2 seconds)

Ok, I retested several patches and here are the results (all based upon SVN 16263)

version: hd activity time, hd spintime

stock: 1:05min 2:10min
stock + ata_priority_realtime: 0:55min 2:05min
stock + ata_priority_realtime + thread_wakeup_insert_first: 0:45min 2:00min
stock + spinlock_rollback_v02 : 0:20min 1:30min

Sorry to bother, but do you happen to have a version of that patch that works with recent SVN? I do not have any coding skills apart from basic html, so I am not able to synch it myself. As the disk thrashing issue is still persistent, I would like to apply this patch to enhance my battery runtime.

thanks in advance

Horscht

Hi, I've tried to sync against svn 16313. Not sure whether it still works as there were many changes since the spinlock change.

Always hitting the wrong button :/

I think I'll just whip up a kernel object that does the yielding spinlock thing instead of rolling back for all devices and changing the one in current form and using that if MAX_PHYS_SECTOR_SIZE is defined.

Other work is in progress to deal properly with priorities in general and I could post a patch of that (perhaps under a different FS). It won't have the inheritance protocols yet (maybe) and will be a tiny bit out of tune but the results are quite good for priority in general anyway and I'm curious how it so far affects this problem. The said temp object can be easily and ultimately removed - no fuss, no muss.

BTW: I'm curious why spinlocks are rolled back outside the ata driver if the issue is the disk only.

This rolls back "spinlock" just in the ata driver for the affected targets only.

Patch works "fine", but I suppose this wont go into SVN as a fix to the issue?

I will if it's "fine" since it's a temp fix limited to the specific targets and not a global revert and so it's easily removed with a simple delete of a few lines. I hope you'll be willing to test more permanent threading developments now in the works anyway when they're ready.

I will be glad to help testing stuff. Please do consider, though, that I have close to no coding knowledge (by close i mean "very close"). I do know how to apply patches and compile using configure and make. Anything beyond that is out of my range (i.e. reading or writing code).