Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9775 - Premature and multiple storage_sleep() in buffering.c

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Saturday, 10 January 2009, 05:14 GMT+2
Task Type Bugs
Category Music playback
Status Unconfirmed
Assigned To No-one
Player Type SW-codec
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

While examining disk performance with  FS#9708 , I found one album where the disk stayed spinning with no activity for 5 s longer when using PIO. This is repeatable with that particular album. With the downloaded r19739-090109 build, this did not happen with the default settings but it started happening as soon as I enabled dircache. Prolonging the disk timeout prolongs the extra sleeping time.

Experimentation with shows that the storage_sleep() part of fill_buffer() (in apps/buffering.c) is executed early in the buffering and it is not executed at the end. With another album where spindown happens properly, ata_sleep() is called twice. Note that ata_sleep() just sets last_disk_activity, and sleep happens over half a second later if there is no disk activity during that period and last_user_activity has timed out.

I think this is a race condition based on how dircache and transfer rates change things. I wonder if it is related to  FS#9541 .

The data mentioned is located at http://spreadsheets.google.com/ccc?key=pkeRcfM0sg949P3a5EYXtew and the delayed spindown is seen in the EP7 PIO test. In that data, also note how sectors written are 5, 10 or 15, with differences between different tests with the same album.

I have a 30GB 5th generation iPod, but I expect this issue affects all SW-codec players.
This task depends upon

Comment by Boris Gjenero (dreamlayers) - Saturday, 10 January 2009, 07:04 GMT+2
The failure to storage_sleep() at the end happens because buffer filling is completed via buf_request_buffer_handle() and Q_SLEEP, without involving fill_buffer().

Sorry about the typo earlier. I meant increasing disk timeout prolongs extra spinning time, not sleeping time. I need to proofread more before I submit.
Comment by Boris Gjenero (dreamlayers) - Saturday, 10 January 2009, 07:09 GMT+2
*sigh* I mean: The failure to call storage_sleep() at the end happens because buffer filling is completed via buf_request_buffer_handle() and Q_START_FILL, without involving fill_buffer().
Comment by Boris Gjenero (dreamlayers) - Saturday, 10 January 2009, 08:18 GMT+2
It seems multiple calls to storage_sleep() routinely happen while loading albums with multiple tracks. They happen after various queue events in buffering_thread(). As mentioned earlier, in ata.c they don't actually cause sleep as long as other ATA activity happens soon. They do result in call_storage_idle_notifys(false) calls, but because of that false they shouldn't result in more than one execution of the idle notifys. I see two executions, I guess as a result of the call_storage_idle_notifys(true) after ACTION_WPS_PLAY.
Comment by Boris Gjenero (dreamlayers) - Sunday, 11 January 2009, 03:06 GMT+2
buffer_handle() only returns false when there isn't enough space left for one more BUFFERING_DEFAULT_FILECHUNK. It returns true in all other cases, including if a file has been buffered up to its end or if an event arrived on the buffering_queue and interrupted buffering. In fill_buffer(), if buffer_handle() returns true, the loop moves on to the next handle. Now, consider what happens when buffer_handle() for the last handle is interrupted by a buffering_queue event. The next pointer is NULL, so m becomes NULL, and the test after the loop causes storage_sleep() and returns false, which sets filling in buffering_thread() to zero.
Comment by Boris Gjenero (dreamlayers) - Sunday, 11 January 2009, 07:07 GMT+2
Here is a simple patch for the issue described in my previous comment. It resolves the delayed spindown problem mentioned earlier. It does not cause storage_sleep() to be called less while loading other albums however, and spindown might still be delayed sometimes when the entire playlist fits in the buffer (search for "filling = false" in buffering.c). I think the storage_sleep() calls happen whenever the buffering thread has loaded everything the playback thread has given it up to that point, and this can happen multiple times during a single buffer filling.
   fill_buffer-if_interrupted.patch (0.3 KiB) 
 apps/buffering.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comment by Andree Buschmann (Buschel) - Friday, 21 October 2011, 14:21 GMT+2
Is this still valid after the rework of the buffering?
Comment by Michael Sevakis (MikeS) - Friday, 21 October 2011, 15:25 GMT+2
Unlikely. Nothing really changed in the function in question and I thought about how to resolve it properly from within buffering and the point is that one cannot really do that since only playback knows when it has completed adding handles and only buffering really knows when it has completed all data for the last handle. Playback and buffering would have to better share info.
Comment by Boris Gjenero (dreamlayers) - Saturday, 05 November 2011, 00:14 GMT+2
When I first created this task, I don't think I understood the code very well. Now, I don't think this is really a bug because it doesn't cause problems. The mmc_sleep() and sd_sleep() functions do nothing. Calling ata_sleep() only causes a spindown after half a second if there is no new disk activity during that time. If this was fixed, that half second of disk spinning time per buffer fill could be eliminated. I expect that on most targets the additional power usage is negligible. Should this remain open due to that half second?
Comment by Michael Sevakis (MikeS) - Saturday, 05 November 2011, 01:09 GMT+2
I think opening file descriptors is particularly slow and it opens quite a few of them and that's what causes the delay between tracks. With dircache, there's no appreciable delay. If there's any concern, increasing the actual timeout to go to sleep could be increase but I've not had any player with the disk going to sleep and coming back online between fills.
Comment by Boris Gjenero (dreamlayers) - Friday, 11 November 2011, 21:48 GMT+2
When there's no dircache and file descriptors are being opened, there must be disk activity. The activity keeps resetting last_disk_activity in ata.c and prevents sleep.

I was just looking at fill_buffer-if_interrupted.patch. It is still relevant, but it doesn't improve anything because the extra storage_sleep() that can happen there shouldn't have any effect.

Loading...