Rockbox

Tasklist

FS#10371 - Recording for Sansa AMS

Attached to Project: Rockbox
Opened by Rafaël Carré (funman) - Monday, 22 June 2009, 21:15 GMT
Last edited by Rafaël Carré (funman) - Tuesday, 24 November 2009, 17:59 GMT
Task Type Patches
Category Recording
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

currently deadlocks when recording start.

deadlocks when entering the recording screen when a 8pixels font is selected?
This task depends upon

Closed by  Rafaël Carré (funman)
Tuesday, 24 November 2009, 17:59 GMT
Reason for closing:  Accepted
Comment by Rafaël Carré (funman) - Monday, 22 June 2009, 21:27 GMT
Note: I tested on Fuze, c200v2 buffer would be too small with recording enabled I think.

Also the diff in ata_sd-as3525.c are not relevent to recording, but I had to add random modifications when rockbox wouldn't boot.
Comment by Rafaël Carré (funman) - Tuesday, 23 June 2009, 07:55 GMT
The lock happens because of pcm_rec_dma_stop() : it calls dma_release() assuming a previous call to dma_retain()

But pcm_rec_dma_stop() is called before pcm_rec_dma_start() (which calls dma_retain() )

I commented out dma_release(), but still get no sound incoming :/
Comment by Rafaël Carré (funman) - Tuesday, 23 June 2009, 13:35 GMT
Note that the speakers are muted (bit 7 set in HPH_OUT_L) after entering the record screen.

I couldn't tell where in as3514.c this happens
Comment by Rafaël Carré (funman) - Tuesday, 23 June 2009, 21:50 GMT
the dma callback isn't entered, and fifo pop half full interrupt doesn't happen.

I tried to use mic2 instead of mic1 as a source in as3514.c without luck.

Would it be possible that we have to use spdif source ?
Comment by Rafaël Carré (funman) - Wednesday, 24 June 2009, 10:36 GMT
A little more experiment : here i explicitely set the recording frequency, enable all status interrupts (except fifo empty/almost empty), disable DMA, and the FIFO still isn't filled up ..
Comment by Michael Chicoine (mc2739) - Tuesday, 14 July 2009, 00:18 GMT
Here is a patch for config-e200v2.h

I noticed two things while testing.
1) It takes a considerable amount of time to get to the main menu when booting. I saw the same thing while testing the FM Radio with longer init delays.
2) The FM Radio no longer outputs sound with this patch applied, although it appears to be working properly otherwise.
Comment by Rafaël Carré (funman) - Monday, 05 October 2009, 02:37 GMT
added e200v2 diff

Not much change on the code but some new findings (tested on fuze only):

If I start by playing some music, then I'll have sound in FM, else I won't have any sound
When I enter recording menu (FM or MIC), I won't have sound anymore.

If I comment out DMA code and instead enable i2sin interrupts (mask = 0x4f), I'll have an interrupt with i2sin status set to 0xE (pop fifo full)

Perhaps audio/as3514 code needs some modifications
Comment by Rafaël Carré (funman) - Monday, 05 October 2009, 15:31 GMT
(Tested on Fuze)

Don't use audiohw_preinit() in rec code => now headphones aren't muted
Add a define to use DMA / noDMA modes

DMA => deadlock as soon as recording screen is entered (for MIC or FM)

no DMA => no deadlock. Sometimes I can see an interrupt happened (with status = 0xe = fifo full, fifo half full, fifo almost full).
I still don't know which conditions exactly make the interrupt happen, and it's always one only (no consecutive interrupts).

You can count them in debug menu > io ports.

I tried on the Clip as well and it deadlocks as soon as I enter recording screen. (Normal FM screen deadlocks as well, even in current build, so I'll focus on Fuze for now)
Comment by Rafaël Carré (funman) - Tuesday, 13 October 2009, 16:48 GMT
r23156 doesn't help :/

I got a mail from Fred Bauer which I'll just quote because I think it can be useful:

After applying the rec-v5 patch I added some lines to clear out the
input FIFO before activating the interrupt.

This is in pcm-as3525.c in pcm_rec_dma_start before setting I2SIN_MASK.
while ( (I2SIN_RAW_STATUS & (1<<5)) == 0 ) { /* while not FIFO_EMPTY */
tmp=*I2SIN_DATA;
fifos_read++;
}

This resulted in 32 fifos_read. There were no interrupts generated if
I activated it after clearing the FIFO. I mean I think there were
already 32 entries in FIFO and that was triggering the single
interrupt.
Comment by Fred Bauer (freddyb) - Thursday, 15 October 2009, 16:28 GMT
I tried to trace what happens to set up I2SOUT to get insight into how to set up I2SIN.

pcm_play_dma_init()
CGU_PERI > 0xF93018F - CGU_PERI |= CGU_I2SOUT_APB_CLOCK_ENABLE;
CGU_AUDIO >0x7FD - max divider & source plla_fout
I2SOCTL- 0x48 > dma, stereo, 16bit
audiohw_preinit();
pcm_dma_apply_set
CGU_AUDIO - AC0AD - divider = 43d & sources = plla_fout
play_start_pcm()
CGU_PERI - 0FD3C18F CGU_PERI |= CGU_I2SOUT_APB_CLOCK_ENABLE;
CGU_AUDIO - AC8AD CGU_AUDIO |= I2SO_MCLK_EN;

The following is the I2SIN setup:
pcm_apply_settings()
CGU_AUDIO > AC0AD - dividers = 43 & sources = plla_fout
pcm_rec_dma_start()
CGU_PERI > 0xf9b018f - CGU_PERI |= CGU_I2SIN_APB_CLOCK_ENABLE;
CGU_AUDIO > 0x8AC0AD turn on clock enable
I2SIN_CONTROL > 0x20 (set sample size)
CGU_AUDIO |= (1<<23); // I2SIN_MCLK_EN

I don't see any equivalent of audiohw_preinit() on the recording side to shut off DAC, activate ADC, or mess with the MICs.
(I tried adding audiohw_enable_recording(true) inside pcm_rec_dma_start() without effect.)

I'm unsure if I2SOUT functions have to be deactivated or unclocked and what order I2SIN has to be brought up.

I can now get additional interrupts but they are single events, triggered by starting playback and then starting record.

I posted a diff against r23155 but it's a mess. I'll post a cleaner diff if I can get something more promising.
Comment by Rafaël Carré (funman) - Thursday, 15 October 2009, 16:50 GMT
I think the rockbox code prevents i2sin and i2sout to be used a the same time, this is why i use the dma channel 1 for i2sin (same than i2sout)

DAC/ADC and input source are set by audiohw_*() functions inside audio-as3525.c which call functions in firmware/drivers/audio/as3514.c

Somehow we must miss one bit to send continuous data to i2sin
Comment by Fred Bauer (freddyb) - Saturday, 17 October 2009, 00:25 GMT
I haven't had any luck. I tried setting I2SIN_CONTROL:sclk_edge+, I2SIN_CONTROL:mclk_invert, and both together.

I even tried running I2SIN on PLLB with the clock settings from the spec sheet. (p. 113 - 2nd row of 44100Hz).

I did find where audiohw_enable_recording() gets called. So unless there's something wrong with it the ADC should be on.

Is IRQ_ENRD_1:I2S_changed (p. 159) or IRQ_ENRD_2:ADC_endcon (p. 161) anything I should look into?
Comment by Fred Bauer (freddyb) - Monday, 19 October 2009, 03:55 GMT
I managed to get I2SIN interrupts going.

The patch is against r23155. It's not nearly ready for inclusion but it allows you to go into the recording screen and generate a bunch of i2sin irq's.
Leaving the screen doesn't stop the irqs from coming but playing does. They still come after CGU_AUDIO:I2SI_MCLK_EN is off?
If I knew the right spot to kill them I'd put in a I2SIN_MASK = 0x0;
This doesn't actually record anything and the input frequency divider is just copied from the output f divider.

Hope this helps!

Comment by Rafaël Carré (funman) - Saturday, 24 October 2009, 12:23 GMT
Sync to SVN

I removed unrelated diffs (keymap) and your debug variables (impossible to read if you're not fred!)

Not much new stuff: I added the number of transferred words in the debug menu and it's always 2048 ?

I need to check more exactly what I have changed in pcm.c

We're getting closer! Thanks much for your work !
Comment by Fred Bauer (freddyb) - Saturday, 24 October 2009, 19:02 GMT
Funman, I put an unnecessary "sleep(HZ/4)" in pcm_rec_dma_start that might cause some headache down the road.

After I applied rec-fred7-funman.diff I get a FIFO full panic the 2nd time I try to record, I wonder if I'm clearing the I2SIN FIFO in the wrong place (pcm_dma_record_start)?

All that funny debugging code was to try to understand the I2SIN function call sequence without chasing all over the source tree. It's not what I expected.
Record function calls from power on to record screen (from rec-fred7.diff):
pcm_rec_dma_stop,
pcm_rec_dma_close,
pcm_rec_dma_init,
pcm_rec_dma_stop,
pcm_rec_dma_close,
pcm_rec_dma_init,
rec_dma_start,
pcm_rec_dma_start,
pcm_rec_dma_stop,
pcm_rec_dma_stop,
pcm_rec_dma_close,
INT_I2SIN <- This seems bad

About the 2048 words transferred, is dma_rec_size treated consistently? There is a comment on the declaration:
static size_t dma_rec_size; /* in 4*32 bits */
in the ISR it seems to be in bytes (after reading a 32bit word):
dma_rec_size -= 4;
Comment by Rafaël Carré (funman) - Saturday, 24 October 2009, 19:08 GMT
perhaps the INT_I2SIN is enabled by a pcm_rec_unlock() call ?

I think I wrote this comment when writing DMA code : I think DMA requests transfer 4 * 32 bits so that would be the number of transfers.

Since we don't use DMA and don't scale the size, I think we should just remove or explicit the comment (ideally the final code would use DMA)

For reference you can compare to other drivers: I looked at drivers for ARM CPUs: pcm-pp.c and imx31/gigabeat-s/pcm-imx31.c
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 14:22 GMT
Attaching my current diff (not much changed from v7)

I could record my voice, it's somehow understandable although the sound is really ugly and distorted, and playback can't be used after recording.

I just didn't want to lose a patch where recording somehow worked ;)
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 16:42 GMT
Cleaned patch, removed DMA code

What works:
- playback after recording
- recording (records data at least ..)
- FM & MIC
- loopback while recording

What doesn't work:
- only tested on Fuze
- list of frequencies only shows 22.05kHz while I defined all frequencies in config-*.h
- recording speed is too fast (twice? more?)
- recorded data is unrecognizable, although there is some real data in there
- DMA

I could swear at some point I had a normal recording but I can't find what's missing

For the quality, we could play with 24bits mode (needs some tweaking in the fifo copy routine to shift the samples)
For the speed, we need to see what's wrong with the list of frequencies I think
Comment by Fred Bauer (freddyb) - Sunday, 01 November 2009, 17:40 GMT
That's great.

I had to set I2SIN_CONTROL:sclk_edge inside pcm_rec_dma_start to get a good recording and the sample rate was wrong. The data is for 44.1KHz but the wav file is set to 22. It sounds pretty good with fixed rate. The audio was on the left channel.
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 18:52 GMT
I added your fix and small changes (Fuze manual, cosmetics)

I can see all the frequencies after a make clean (not sure if it's that or if i removed the _default SAMPR in config-*.h)

Recording is now correct (tested 44.1kHz with mic/fm)

btw you can change mono mode in options.

I sometimes see a panic with "unhandled interrupt source" .. weird

Also note I disabled panic in case of errors in the isr (perhaps we're not fast enough, we should try DMA again..)

Clip deadlocks when I enter recording screen (source == mic), I can't test FM because it doesn't work anymore on the Clip

Fred which model do you have ? Perhaps we'll need some additional magic for recording to work on targets with low memory ...
Comment by Fred Bauer (freddyb) - Sunday, 01 November 2009, 19:07 GMT
I have a fuze. I don't know anything about the Clip. I got the unhandled interrupt, too. My first patch turned on CGU_PERI:CGU_I2C_AUDIO_MASTER_CLOCK_ENABLE and everything seems to work ok without it. Maybe that should get backed out. I don't think we would have a problem with speed if we start with an empty FIFO.
Comment by Michael Chicoine (mc2739) - Sunday, 01 November 2009, 19:12 GMT
Great work funman and freddyb. FM Radio and mic recording works on e200v2. The recorded volume seems to be a bit lower than the source volume. Are you seeing the same on the Fuze?
Comment by Jack Halpin (FlynDice) - Sunday, 01 November 2009, 19:20 GMT
Same here with e200v2. I do get intermittant unhandled unknown interrupts and the wheelclicks seem to be a bit off. Voice appears to be only on left channel but sounds very good. FM seems to be on both left and right channels but sounds lower volume and maybe a bit of a high freq buzz with it? Not sure how quite to describe that.
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 19:24 GMT
@freddyb : I wonder why you enabled this clock, we don't use I2C in the pcm code
@mc2739 : Yes I see the same, it's visible when recording the FM because when you enter recording the audio output is set to i2sin loopback
@FlynDice : Did you try with different frequencies?

I don't know how other models behave with respect to recording volume (like e200v1), perhaps we should set it to the same volume than output when recording FM (it can be modified from the recording screen)
Comment by Dominik Wenger (Domonoky) - Sunday, 01 November 2009, 19:55 GMT
I just tried this on my m200v4, and it also deadlocks when i enter the recording screen.. Maybe that has todo with the low amount of ram ?
Comment by Jack Halpin (FlynDice) - Sunday, 01 November 2009, 20:03 GMT
High freq buzz is gone with higher frequency setting. Still getting uhhandled INT's that I can't seem to find any consistent pattern for...
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 20:13 GMT
@domonoky : did you try to start recording from the FM screen ? If it deadlocks as well we could blame the low memory
Comment by Dominik Wenger (Domonoky) - Sunday, 01 November 2009, 20:15 GMT
I just tried it from the FM screen, but it also Deadlocks. So lets blame Memory size :-)
Comment by Fred Bauer (freddyb) - Sunday, 01 November 2009, 20:32 GMT
Funman: I'm pretty new to this. I tried a LOT of things that wouldn't make any sense to someone more seasoned.

I have something weird going on with mine. I added these two lines to the hwinfo screen:

lcd_putsf(0, line++, "I2SIN_CTL :%8x", (unsigned int)(I2SIN_CONTROL));
lcd_putsf(0, line++, "I2SIN_RAW :%8x", (unsigned int)(I2SIN_RAW_STATUS));

and they always have the same last byte: FFF5FDC6 & 000000C6.
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 21:24 GMT
Well that paid because now we can record :p

What I changed from the previous patches is make I2SIN_CONTROL a long (32 bits) instead of a char (8 bits) because it was more than 8 bits
You could try changing the definition to short (16 bits) in as3525.h
Comment by Rafaël Carré (funman) - Sunday, 01 November 2009, 22:52 GMT
I committed the last patch with additional #ifdef HAVE_RECORDING

Recording is still disabled on every models for the moment, having the code in SVN will let us post smaller diffs here ;)
Comment by Rafaël Carré (funman) - Monday, 02 November 2009, 00:54 GMT
I looked at the code generated for INT_I2SIN since it's an isr and it should be fast

It's a bit ugly (I think we only use -O and not -O2 gcc option) and I optimized it a little: now I'm under the impression that spurious interrupts happen more often with this patch.. do I dream ?
Comment by Fred Bauer (freddyb) - Monday, 02 November 2009, 01:01 GMT
I checked the unhandled interrupt and it was I2SIN which wasn't too surprising, but then I zeroed out I2SIN_MASK wherever I thought it might cause a problem and now I get an unhandled interrupt without a number? I changed the UINT sub to panic showing VIC_RAW_INTR and it's zero. I haven't looked at the VIC controller specs yet. Does this make any sense? Can there be an interrupt without a source?
Comment by Rafaël Carré (funman) - Monday, 02 November 2009, 01:18 GMT
freddy there is no explanation for this :/

We have already seen this happen in the past. I don't remember the exact causes, but it was the fault of buggy code every time ;)

We could check what happens if we remove this panic and just return, but I don't feel very safe.

I think it might be better to convert the current code to use DMA instead, so there wouldn't be I2SIN interrupts anymore.

I'll try to do that tomorrow
Comment by Fred Bauer (freddyb) - Monday, 02 November 2009, 01:30 GMT
-O shows in the top makefile. (Not -O2 or -Os?) I made my last post without seeing your ASM patch.

Just for kicks I put a 1000 loop delay in the ISR to see how bad it would break things but it didn't, even when encoding MP3. I still get the unhandled irq regardless.
Comment by Rafaël Carré (funman) - Monday, 02 November 2009, 13:09 GMT
DMA code

It works because we have loopback and we can see the peakmeter.

I could make one successful recording to disk, however most of the time I have a disk is full error, which means:
- audio_status () & AUDIO_STATUS_ERROR, which means:
- pcm_rec_status() & AUDIO_STATUS_ERROR ... to investigate.

By the way we should check how much times the _init()/_close() functions are called so we could move clock enable/disable and dma retain/release into functions which are called exactly once at start and once at stop.
Comment by Rafaël Carré (funman) - Monday, 02 November 2009, 13:50 GMT
I see 2 errors in logf : first PCMREC_E_FNQ_DESYNC and then this error ORed with PCMREC_E_IO

To investigate further ..
Comment by Fred Bauer (freddyb) - Monday, 02 November 2009, 14:55 GMT
You're doing something right. I reverted and applied your rec-dma.diff and now I've got 20 minutes of recording and it's still going. (48kHz,mono,mp3,128kbps)
Comment by Fred Bauer (freddyb) - Monday, 02 November 2009, 15:15 GMT
Make that 40 minutes.
Comment by Rafaël Carré (funman) - Monday, 02 November 2009, 16:09 GMT
Good!

But for me the errors happen when leaving the recording screen ... :/

BTW I forgot to enable I2SIN interrupt, so we don't check for errors ...

Also we need to check if recording quality is still there
Comment by Fred Bauer (freddyb) - Monday, 02 November 2009, 16:29 GMT
I just tried the 2nd patch. They both have a repeating spike in the audio every 2048th sample.
Comment by Fred Bauer (freddyb) - Tuesday, 03 November 2009, 05:31 GMT
I think I have interrupt driven recording pretty solid now. I've recorded for 13 minutes now at 96k without FIFO errors or unhandled interrupts and the quality is good.

I changed the pcm_rec_lock so that it shuts off ALL interrupts and pcm_rec_unlock restores them. I believe that there was an occasional interrupt collision causing the problems. Should locks be done the same using DMA?

Set I2SIN clock source to PLLa.
Comment by Rafaël Carré (funman) - Tuesday, 03 November 2009, 08:19 GMT
Not sure why i2sin worked if we hadn't set the clock source .. I committed this to SVN with a bit of cleanup

If we need to disable all interrupts this surely means there is something wrong so we shouldn't do that.
BTW to achieve this you could have used disable_irq_save() / restore_irq() : they work at a lower level, you can check the code in firmware/target/arm/system-arm.h

I think we should check what's wrong with DMA : the repeating spike every 2048 sample and the errors on file closing; because DMA is better than isr copy (more friendly with the CPU)
Comment by Fred Bauer (freddyb) - Tuesday, 03 November 2009, 15:21 GMT
Disabling interrupts should be ok if we only do it while changing IRQ settings because it's just a few instructions. I really think lots of weird things can happen if we don't disable interrupts while configuring them. In this case VIC_INT_EN_CLEAR=I2SIN was causing the unhandled IRQ panic.

This is my theory of what was happening:
Higher priority IRQ lands when clearing I2SIN in the VIC
While servicing the higher IRQ I2SIN trips
When exiting the higher IRQ the processor finds itself interrupted by a cleared interrupt, generating a panic.

I'll have to do a lot of digging to be any help with DMA and I'm wondering about diminishing returns if the processor clock is 1400x the sample rate. Are there slower ARMs out there that this would help? Will we need the DMA channels open for USB down the road?
Comment by Rafaël Carré (funman) - Tuesday, 03 November 2009, 15:49 GMT
The problem is having interrupts disabled for a long time reduce system latency (the kernel tick happens HZ=100 times per second, and can't happen if interrupts are disabled)

I think I understand what you mean, I should check what the OF does : if it disables interrupts when modifying VIC_* (I looked at Linux code but I couldn't understand it)

I think using DMA would be simpler (this is what the OF does) but of course we need to understand the problems specific to it ;)

I don't understand what you mean by CPU clock = 1400 * sample rate

Here is the relation with rounding removed : divider (valid values = from 1 to 512) = AS3525_PLLA_FREQ / (128 * frequency )

About USB, afaiu when the USB connection is made, you can run no plugin, no playing, no recording, so channel 1 would be available (channel 0 is for storage)
Comment by Fred Bauer (freddyb) - Tuesday, 03 November 2009, 18:33 GMT
I don't mean to have interrupts disabled for a long time, just long enough to set some registers. I assumed that any interrupts would vector once they are re-enabled. (I mean not get discarded.)

I meant that if the system is at 62MHz and the sample rate is at 44100Hz we might not get a big reduction in CPU usage by eliminating ISR because it's not much to begin with. (If it takes 14 cycles on average to service an IRQ generated sample it would represent 1% of the CPU time.) I'd be more interested in looking at whatever higher interrupt is apparently blocking us for so long because if I up the I2SIN_MASK to almost full instead of half I get push errors.
Comment by Rafaël Carré (funman) - Tuesday, 03 November 2009, 19:57 GMT
the kernel tick might take some time, this is why I think DMA is better because it doesn't let the FIFO become full
Comment by Fred Bauer (freddyb) - Tuesday, 03 November 2009, 20:36 GMT
In the rec-dma-v2.diff you have this line in the callback:
if(!more_ready || more_ready(0) < 0)

Should it be
if(!more_ready || more_ready(0) <= 0) instead?
Comment by Rafaël Carré (funman) - Tuesday, 03 November 2009, 20:47 GMT
I just copied this code from other targets and they all use < 0 (I didn't check what the function does)
Comment by Fred Bauer (freddyb) - Friday, 06 November 2009, 15:31 GMT
Funman, would it be possible to commit rec-v11.patch with recording enabled for Fuze? I still haven't had any crashes from it.
Comment by Fred Bauer (freddyb) - Monday, 09 November 2009, 06:54 GMT
I'm posting this interrupt driven recording patch in case someone wants to use it until DMA gets ironed out. (This is not meant for development.) This fixes an issue with rec-v11 resuming playback on recent svn.
Comment by Fred Bauer (freddyb) - Thursday, 12 November 2009, 15:49 GMT
This is an update to the last patch. It reads half of the FIFO in one unrolled loop. (IRQ triggers at half.) I checked the assembly output and it's pretty small. Most FIFO entries gets handled as one ldr/str pair. This is not meant for development but it's stable if someone needs recording until DMA works.
Comment by Rafaël Carré (funman) - Thursday, 12 November 2009, 20:05 GMT
sorry i was a bit afk these last 7 days

i don't want to commit patch v11 because we don't understand fully why this interrupt with unknown source appear

in fact i'm reluctant to all patches which are not fully understood (the patch to enable MMU / data cache stayed on flyspray for several months before being committed, even if we hadn't understood 100% of the issues we had made a lot of progress)

i'll try to look at how OF handles interrupts and setup the VIC etc


about the last patch i'm not sure why some i2sout code (and some i2sin clock setup) needs to be modified (it's not in the description)

when we get this working it will be committed even if it's not committed (i just though DMA would be more resistant to buffer overruns but this should work because we don't use much CPU when recording - compressing at high bitrates needs to be checked of course but we have a pretty good CPU -)
Comment by Fred Bauer (freddyb) - Thursday, 12 November 2009, 22:54 GMT
Funman, thanks, don't let me rush you too much. Mine is recording now and that's what matters. ;)

The changes for rec-v13.diff from svn follow:

1. play_start_pcm():
shut off CGU_PERI:CGU_I2SIN_APB_CLOCK_ENABLE and
CGU_AUDIO:I2SI_MCLK_EN
I will retest this part to see if it's unnecessary.
(I think I added this before I discovered the i2sout stereo flag was missing.)

2. pcm_play_dma_start():
don't bother turning on I2SOUT_CONTROL:DMA because it's never turned off
don't set CGU_PERI and CGU_AUDIO because the next line calls play_start_pcm() which overwrites them

3. pcm_play_dma_init():
reverted to how it was before I started messing with it. CGU_AUDIO:i2sout_divider = max and clocksource = PLLa.
I don't why they initially set the slowest clock or if it's necessary.
Added the stereo flag for I2SOUT_CONTROL

4. pcm_dma_apply_settings():
set I2SIN clocksource to PLLa

5. Changed record record locking as discussed before.
I2SIN irq was occasionally popping at the same time as VIC_INT_EN_CLEAR=INTERRUPT_I2SIN, which is likely over time considering the frequency of pcm_rec_lock().
That would cause a panic because clearing VIC_INT would complete and then by the time the irq_handler saves the state i2sin is masked_out (unhandled).
My understanding is that on an interrupt the current instruction finishes and then jumps. Like this:

instruction i2sin_stat vic_mask
.... no i2sin
vic_int_clear no i2sin
irq_handler yes n/a < panic

6. INT_I2SIN():
Changed the isr to read half of FIFO in an unrolled loop. (This is the only diff between v12 & v13.)
I don't know if timing is that critical but it's already written.

7. pcm_rec_dma_init():
Took out a line setting CGU_AUDIO because the next line is pcm_dma_apply_settings() which overwrites them.
irq_handler yes n/a < panic

6. INT_I2SIN():
Changed the isr to read half of FIFO in an unrolled loop. (This is the only diff between v12 & v13.)
I don't know if timing is that critical but it's already written.

7. pcm_rec_dma_init():
Took out a line setting CGU_AUDIO because the next line is pcm_dma_apply_settings() which overwrites them.
Comment by Fred Bauer (freddyb) - Friday, 13 November 2009, 20:01 GMT
Cleaned up the previous patch, removing some unnecessary changes.
Item #1 above is gone, no longer turn off i2sin in play_start_pcm()
Item #3 is gone, no longer set output to max divider.
Comment by Fred Bauer (freddyb) - Sunday, 22 November 2009, 19:16 GMT
Changed pcm_rec_lock and unlock to use disable_irq_save() & restore_irq(). Seems ok after 20 minutes of recording.
Comment by Rafaël Carré (funman) - Sunday, 22 November 2009, 21:04 GMT
code to enable i2sout clocks should be left in the init routine : play_start_pcm() is called at each DMA transfer

i2sin clock divider doesn't need to be changed in pcm_dma_apply_settings : once it's set in the rec init routine it doesn't need to be changed

disable_irq_save() doesn't return the VIC register bits, but the irq & fiq enable bits of the ARM CPU (CPSR register), so this code is incorrect
Comment by Thomas Martitz (kugel.) - Monday, 23 November 2009, 01:46 GMT
Isn't DMA used? The last hunk where I2SIN is copied 16 times looks a bit strange to me.

Notice: I've now kowledge of this all, I'm just curious.

EDIT: Ok after reading through the task a bit, I see dma doesn't work for some reason. Maybe cache coherency?
Comment by Fred Bauer (freddyb) - Monday, 23 November 2009, 06:04 GMT
Kugel, I was trying to get interrupt driven recording smoothed out until DMA got going. (IRQ works, it's just not right...)

On interrupt driven recording we were getting panics from unhandled interrupts when the VIC bit for I2SIN was cleared in the locking code and I was trying to disable the interrupts while clearing the VIC flag for I2SIN. I see now I wasn't really clearing the VIC.

From what I can tell both the VIC bit and I2SIN_MASK need to be cleared in the lock to keep it from crashing. I'm going to let it record this way overnight to see how it does.

The 16x reads of I2SIN data was a loop I unrolled inside the ISR. The interrupt trips on a half full FIFO (16).
Comment by Fred Bauer (freddyb) - Monday, 23 November 2009, 16:14 GMT
Fixed pcm_rec_lock() and unlock. They now clear I2SIN_MASK while locked. I hope this fixes all of the edge conditions. It still hasn't crashed.
Moved I2SIN setup to pcm_rec_dma_init()
Removed the unnecessary reflexive bit math when setting VIC registers. (Low bits have no effect.)
I2SOUT setup is still in play_start_pcm(). I will look at it later to figure how to move it without breaking playback resume.
Comment by Rafaël Carré (funman) - Monday, 23 November 2009, 16:19 GMT
- VIC_INT_ENABLE |= INTERRUPT_DMAC;
+ VIC_INT_ENABLE = INTERRUPT_DMAC;

This effectively disables all interrupts but DMA ..

for play_start_pcm() you can revert all your diffs in i2sout functions (have a window with svn diff opened and another one with the .c file)

Also the pcm_rec_unlock() in pcm_rec_dma_stop() looks suspicious I think you should remove it

For the unrolled loop in the I2SIN isr, we could perhaps use SIMD with 8 registers (ldmia/stmia)

Rest of the patch looks ok
Comment by Fred Bauer (freddyb) - Monday, 23 November 2009, 20:04 GMT
These two instructions are the same. VIC_INT_ENABLE and VIC_INT_EN_CLEAR only change high bits written to them. They don't take a drop in replacement mask. (PL-190 datasheet 3.3.5-6)
Can I move the I2SOUT setup code to pcm_play_dma_start() instead of init? I can do that without breaking playback. I2SOUT is turned off in pcm_play_dma_stop().
Removed the suspicious pcm_rec_unlock(). It wasn't supposed to be there.
I don't know ARM assembly. :(
Comment by Rafaël Carré (funman) - Monday, 23 November 2009, 20:12 GMT
Hm I had understodo that for the clear register but not for the enable register, but indeed that makes sense.

Did you check that after VIC_INT_ENABLE = 0; when reading back it has the correct value ?

Ideally the code that activates clock (and cause power consumption) should be added to pcm_play_dma_start() which is called once : when playback begins.

Init code that is only needed once and has no incidence on power should go in pcm_play_dma_init().

The optimisation of the isr can come at the end when all the rest is OK (and I think we're pretty near now!), I can do it if you want
Comment by Fred Bauer (freddyb) - Monday, 23 November 2009, 22:47 GMT
I checked the VIC_INT_ENABLE operation by putting in a panic if it was equal to what was written. I also checked by writing zero and checking for a zero value. It's doing what the spec sheet says.

I have the I2SOUT clock enablers in pcm_play_dma_start() now and that works well. Given your description of pcm_play_dma_init() there's some code that could be moved to pcm_play_dma_start().

Re: ISR: "I can do it if you want" That would be great. ;) (If you don't want to, you could say that saving and restoring the registers would eat most of the gains.)

I think this current patch is pretty solid. I'm going to resync and let it record over night.
Comment by Fred Bauer (freddyb) - Tuesday, 24 November 2009, 16:56 GMT
Funman: I let this record over night and everything went smoothly. I think that locking is correct now and it can record indefinitely without a FIFO push error.

Loading...