[PATCH v2 1/9] ALSA: emu10k1: pass frame instead of byte addresses
... to snd_emu10k1_pcm_init_voice(). This makes the code arguably less convoluted.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emupcm.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index d669f93d8930..9f151a0a7756 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -270,15 +270,6 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, stereo = runtime->channels == 2; w_16 = snd_pcm_format_width(runtime->format) == 16;
- if (!extra && stereo) { - start_addr >>= 1; - end_addr >>= 1; - } - if (w_16) { - start_addr >>= 1; - end_addr >>= 1; - } - spin_lock_irqsave(&emu->reg_lock, flags);
/* volume parameters */ @@ -424,19 +415,16 @@ static int snd_emu10k1_playback_prepare(struct snd_pcm_substream *substream) struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; struct snd_emu10k1_pcm *epcm = runtime->private_data; + bool w_16 = snd_pcm_format_width(runtime->format) == 16; + bool stereo = runtime->channels == 2; unsigned int start_addr, end_addr;
- start_addr = epcm->start_addr; - end_addr = snd_pcm_lib_period_bytes(substream); - if (runtime->channels == 2) { - start_addr >>= 1; - end_addr >>= 1; - } - end_addr += start_addr; + start_addr = epcm->start_addr >> w_16; + end_addr = start_addr + runtime->period_size; snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, start_addr, end_addr, NULL); - start_addr = epcm->start_addr; - end_addr = epcm->start_addr + snd_pcm_lib_buffer_bytes(substream); + start_addr >>= stereo; + end_addr = start_addr + runtime->buffer_size; snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], start_addr, end_addr, &emu->pcm_mixer[substream->number]); @@ -452,14 +440,13 @@ static int snd_emu10k1_efx_playback_prepare(struct snd_pcm_substream *substream) struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; struct snd_emu10k1_pcm *epcm = runtime->private_data; - unsigned int start_addr, end_addr; + unsigned int start_addr; unsigned int channel_size; int i;
- start_addr = epcm->start_addr; - end_addr = epcm->start_addr + snd_pcm_lib_buffer_bytes(substream); + start_addr = epcm->start_addr >> 1; // 16-bit voices
- channel_size = ( end_addr - start_addr ) / NUM_EFX_PLAYBACK; + channel_size = runtime->buffer_size;
snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, start_addr, start_addr + (channel_size / 2), NULL);
This workaround fails to address the underlying problem, which is actually wholly self-made. Subsequent patches will fix it.
This reverts commit 56385a12d9bb9e173751f74b6c430742018cafc0.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- include/sound/emu10k1.h | 1 - sound/core/pcm_native.c | 4 ---- sound/pci/emu10k1/emu10k1.c | 4 ---- sound/pci/emu10k1/emupcm.c | 25 ++----------------------- sound/pci/emu10k1/memory.c | 4 +--- 5 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h index 5ad2144fa530..2d64f07e3883 100644 --- a/include/sound/emu10k1.h +++ b/include/sound/emu10k1.h @@ -1670,7 +1670,6 @@ struct snd_emu10k1 { unsigned int address_mode; /* address mode */ unsigned long dma_mask; /* PCI DMA mask */ bool iommu_workaround; /* IOMMU workaround needed */ - unsigned int delay_pcm_irq; /* in samples */ int max_cache_pages; /* max memory size / PAGE_SIZE */ struct snd_dma_buffer silent_page; /* silent page */ struct snd_dma_buffer ptb_pages; /* page table pages */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 39a65d1415ab..95fc56e403b1 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1605,10 +1605,6 @@ static int snd_pcm_do_pause(struct snd_pcm_substream *substream, { if (substream->runtime->trigger_master != substream) return 0; - /* some drivers might use hw_ptr to recover from the pause - - update the hw_ptr now */ - if (pause_pushed(state)) - snd_pcm_update_hw_ptr(substream); /* The jiffies check in snd_pcm_update_hw_ptr*() is done by * a delta between the current jiffies, this gives a large enough * delta, effectively to skip the check once. diff --git a/sound/pci/emu10k1/emu10k1.c b/sound/pci/emu10k1/emu10k1.c index 0c97237af922..23adace1b969 100644 --- a/sound/pci/emu10k1/emu10k1.c +++ b/sound/pci/emu10k1/emu10k1.c @@ -34,7 +34,6 @@ static int max_synth_voices[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 64}; static int max_buffer_size[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 128}; static bool enable_ir[SNDRV_CARDS]; static uint subsystem[SNDRV_CARDS]; /* Force card subsystem model */ -static uint delay_pcm_irq[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 2};
module_param_array(index, int, NULL, 0444); MODULE_PARM_DESC(index, "Index value for the EMU10K1 soundcard."); @@ -56,8 +55,6 @@ module_param_array(enable_ir, bool, NULL, 0444); MODULE_PARM_DESC(enable_ir, "Enable IR."); module_param_array(subsystem, uint, NULL, 0444); MODULE_PARM_DESC(subsystem, "Force card subsystem model."); -module_param_array(delay_pcm_irq, uint, NULL, 0444); -MODULE_PARM_DESC(delay_pcm_irq, "Delay PCM interrupt by specified number of samples (default 0)."); /* * Class 0401: 1102:0008 (rev 00) Subsystem: 1102:1001 -> Audigy2 Value Model:SB0400 */ @@ -103,7 +100,6 @@ static int snd_card_emu10k1_probe(struct pci_dev *pci, enable_ir[dev], subsystem[dev]); if (err < 0) return err; - emu->delay_pcm_irq = delay_pcm_irq[dev] & 0x1f; err = snd_emu10k1_pcm(emu, 0); if (err < 0) return err; diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 9f151a0a7756..27977d03e323 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -290,7 +290,7 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, evoice->epcm->ccca_start_addr = start_addr + ccis; if (extra) { start_addr += ccis; - end_addr += ccis + emu->delay_pcm_irq; + end_addr += ccis; } } if (stereo && !extra) { @@ -315,9 +315,7 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, snd_emu10k1_ptr_write(emu, PTRX, voice, (send_amount[0] << 8) | send_amount[1]); // Stereo slaves don't need to have the addresses set, but it doesn't hurt snd_emu10k1_ptr_write(emu, DSL, voice, end_addr | (send_amount[3] << 24)); - snd_emu10k1_ptr_write(emu, PSST, voice, - (start_addr + (extra ? emu->delay_pcm_irq : 0)) | - (send_amount[2] << 24)); + snd_emu10k1_ptr_write(emu, PSST, voice, start_addr | (send_amount[2] << 24)); if (emu->card_capabilities->emu_model) pitch_target = PITCH_48000; /* Disable interpolators on emu1010 card */ else @@ -647,23 +645,6 @@ static void snd_emu10k1_playback_set_stopped(struct snd_emu10k1 *emu, epcm->running = 0; }
-static inline void snd_emu10k1_playback_mangle_extra(struct snd_emu10k1 *emu, - struct snd_emu10k1_pcm *epcm, - struct snd_pcm_substream *substream, - struct snd_pcm_runtime *runtime) -{ - unsigned int ptr, period_pos; - - /* try to sychronize the current position for the interrupt - source voice */ - period_pos = runtime->status->hw_ptr - runtime->hw_ptr_interrupt; - period_pos %= runtime->period_size; - ptr = snd_emu10k1_ptr_read(emu, CCCA, epcm->extra->number); - ptr &= ~0x00ffffff; - ptr |= epcm->ccca_start_addr + period_pos; - snd_emu10k1_ptr_write(emu, CCCA, epcm->extra->number, ptr); -} - static int snd_emu10k1_playback_trigger(struct snd_pcm_substream *substream, int cmd) { @@ -686,8 +667,6 @@ static int snd_emu10k1_playback_trigger(struct snd_pcm_substream *substream, fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: - if (cmd == SNDRV_PCM_TRIGGER_PAUSE_RELEASE) - snd_emu10k1_playback_mangle_extra(emu, epcm, substream, runtime); mix = &emu->pcm_mixer[substream->number]; snd_emu10k1_playback_unmute_voice(emu, epcm->voices[0], true, mix); snd_emu10k1_playback_unmute_voice(emu, epcm->voices[1], false, mix); diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c index edb3f1763719..20b07117574b 100644 --- a/sound/pci/emu10k1/memory.c +++ b/sound/pci/emu10k1/memory.c @@ -315,10 +315,8 @@ snd_emu10k1_alloc_pages(struct snd_emu10k1 *emu, struct snd_pcm_substream *subst if (snd_BUG_ON(!hdr)) return NULL;
- idx = runtime->period_size >= runtime->buffer_size ? - (emu->delay_pcm_irq * 2) : 0; mutex_lock(&hdr->block_mutex); - blk = search_empty(emu, runtime->dma_bytes + idx); + blk = search_empty(emu, runtime->dma_bytes); if (blk == NULL) { mutex_unlock(&hdr->block_mutex); return NULL;
The idea is to make the extra voice lag behind the "real" voices, but moving the buffer address around doesn't contribute to that, as the CCCA write below uses the same address. The exact address is unimportant, as the data is discarded anyway.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emupcm.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 27977d03e323..16e7d0ff97a4 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -288,10 +288,6 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, if (master) { evoice->epcm->ccca_start_addr = start_addr + ccis; - if (extra) { - start_addr += ccis; - end_addr += ccis; - } } if (stereo && !extra) { // Not really necessary for the slave, but it doesn't hurt
Given that the data is going to be ignored anyway, and that the cache does not influence interrupt timing (which is the purpose of the extra voices), it's pointless to pre-fill the cache.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emupcm.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 16e7d0ff97a4..a6c4f1895a08 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -528,14 +528,15 @@ static int snd_emu10k1_capture_prepare(struct snd_pcm_substream *substream) return 0; }
-static void snd_emu10k1_playback_invalidate_cache(struct snd_emu10k1 *emu, int extra, struct snd_emu10k1_voice *evoice) +static void snd_emu10k1_playback_invalidate_cache(struct snd_emu10k1 *emu, + struct snd_emu10k1_voice *evoice) { struct snd_pcm_runtime *runtime; unsigned int voice, stereo, i, ccis, cra = 64, cs, sample;
runtime = evoice->epcm->substream->runtime; voice = evoice->number; - stereo = (!extra && runtime->channels == 2); + stereo = (runtime->channels == 2); sample = snd_pcm_format_width(runtime->format) == 16 ? 0 : 0x80808080; ccis = emu10k1_ccis(stereo, sample == 0); /* set cs to 2 * number of cache registers beside the invalidated */ @@ -658,8 +659,7 @@ static int snd_emu10k1_playback_trigger(struct snd_pcm_substream *substream, spin_lock(&emu->reg_lock); switch (cmd) { case SNDRV_PCM_TRIGGER_START: - snd_emu10k1_playback_invalidate_cache(emu, 1, epcm->extra); /* do we need this? */ - snd_emu10k1_playback_invalidate_cache(emu, 0, epcm->voices[0]); + snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[0]); fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: @@ -803,9 +803,8 @@ static int snd_emu10k1_efx_playback_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_START: /* prepare voices */ for (i = 0; i < NUM_EFX_PLAYBACK; i++) { - snd_emu10k1_playback_invalidate_cache(emu, 0, epcm->voices[i]); + snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[i]); } - snd_emu10k1_playback_invalidate_cache(emu, 1, epcm->extra); fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME:
The cache causes a fixed delay regardless of stream parameters. Consequently, all that "cache invalidate size" calculation stuff was garbage (which can be traced right back to Creative's OSS driver).
This also removes the definitions of registers CD1..CDF, because they are accessed only relative to CD0 anyway.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- include/sound/emu10k1.h | 38 +++++++++++---------- sound/pci/emu10k1/emupcm.c | 67 +++++++++++++------------------------- 2 files changed, 43 insertions(+), 62 deletions(-)
diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h index 2d64f07e3883..ee662a1b0dc7 100644 --- a/include/sound/emu10k1.h +++ b/include/sound/emu10k1.h @@ -116,6 +116,10 @@ #define IPR_MIDITRANSBUFEMPTY 0x00000100 /* MIDI UART transmit buffer empty */ #define IPR_MIDIRECVBUFEMPTY 0x00000080 /* MIDI UART receive buffer empty */ #define IPR_CHANNELLOOP 0x00000040 /* Channel (half) loop interrupt(s) pending */ + /* The interrupt is triggered shortly after */ + /* CCR_READADDRESS has crossed the boundary; */ + /* due to the cache, this runs ahead of the */ + /* actual playback position. */ #define IPR_CHANNELNUMBERMASK 0x0000003f /* When IPR_CHANNELLOOP is set, indicates the */ /* highest set channel in CLIPL, CLIPH, HLIPL, */ /* or HLIPH. When IPR is written with CL set, */ @@ -586,24 +590,22 @@ SUB_REG(PEFE, FILTERAMOUNT, 0x000000ff) /* Filter envlope amount */
/* 0x1f: not used */
-#define CD0 0x20 /* Cache data 0 register */ -#define CD1 0x21 /* Cache data 1 register */ -#define CD2 0x22 /* Cache data 2 register */ -#define CD3 0x23 /* Cache data 3 register */ -#define CD4 0x24 /* Cache data 4 register */ -#define CD5 0x25 /* Cache data 5 register */ -#define CD6 0x26 /* Cache data 6 register */ -#define CD7 0x27 /* Cache data 7 register */ -#define CD8 0x28 /* Cache data 8 register */ -#define CD9 0x29 /* Cache data 9 register */ -#define CDA 0x2a /* Cache data A register */ -#define CDB 0x2b /* Cache data B register */ -#define CDC 0x2c /* Cache data C register */ -#define CDD 0x2d /* Cache data D register */ -#define CDE 0x2e /* Cache data E register */ -#define CDF 0x2f /* Cache data F register */ - -/* 0x30-3f seem to be the same as 0x20-2f */ +// 32 cache registers (== 128 bytes) per channel follow. +// In stereo mode, the two channels' caches are concatenated into one, +// and hold the interleaved frames. +// The cache holds 64 frames, so the upper half is not used in 8-bit mode. +// All registers mentioned below count in frames. +// The cache is a ring buffer; CCR_READADDRESS operates modulo 64. +// The cache is filled from (CCCA_CURRADDR - CCR_CACHEINVALIDSIZE) +// into (CCR_READADDRESS - CCR_CACHEINVALIDSIZE). +// The engine has a fetch threshold of 32 bytes, so it tries to keep +// CCR_CACHEINVALIDSIZE below 8 (16-bit stereo), 16 (16-bit mono, +// 8-bit stereo), or 32 (8-bit mono). The actual transfers are pretty +// unpredictable, especially if several voices are running. +// Frames are consumed at CCR_READADDRESS, which is incremented afterwards, +// along with CCCA_CURRADDR and CCR_CACHEINVALIDSIZE. This implies that the +// actual playback position always lags CCCA_CURRADDR by exactly 64 frames. +#define CD0 0x20 /* Cache data registers 0 .. 0x1f */
#define PTB 0x40 /* Page table base register */ #define PTB_MASK 0xfffff000 /* Physical address of the page table in host memory */ diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index a6c4f1895a08..feb575922738 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -112,6 +112,10 @@ static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voic } } if (epcm->extra == NULL) { + // The hardware supports only (half-)loop interrupts, so to support an + // arbitrary number of periods per buffer, we use an extra voice with a + // period-sized loop as the interrupt source. Additionally, the interrupt + // timing of the hardware is "suboptimal" and needs some compensation. err = snd_emu10k1_voice_alloc(epcm->emu, epcm->type == PLAYBACK_EMUVOICE ? EMU10K1_PCM : EMU10K1_EFX, 1, @@ -232,23 +236,6 @@ static unsigned int emu10k1_select_interprom(unsigned int pitch_target) return CCCA_INTERPROM_2; }
-/* - * calculate cache invalidate size - * - * stereo: channel is stereo - * w_16: using 16bit samples - * - * returns: cache invalidate size in samples - */ -static inline int emu10k1_ccis(int stereo, int w_16) -{ - if (w_16) { - return stereo ? 24 : 26; - } else { - return stereo ? 24*2 : 26*2; - } -} - static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, int master, int extra, struct snd_emu10k1_voice *evoice, @@ -264,7 +251,6 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, unsigned char send_routing[8]; unsigned long flags; unsigned int pitch_target; - unsigned int ccis;
voice = evoice->number; stereo = runtime->channels == 2; @@ -284,10 +270,8 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, memcpy(send_amount, &mix->send_volume[tmp][0], 8); }
- ccis = emu10k1_ccis(stereo, w_16); - if (master) { - evoice->epcm->ccca_start_addr = start_addr + ccis; + evoice->epcm->ccca_start_addr = start_addr + 64 - 3; } if (stereo && !extra) { // Not really necessary for the slave, but it doesn't hurt @@ -317,11 +301,11 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, else pitch_target = emu10k1_calc_pitch_target(runtime->rate); if (extra) - snd_emu10k1_ptr_write(emu, CCCA, voice, start_addr | + snd_emu10k1_ptr_write(emu, CCCA, voice, (end_addr - 3) | emu10k1_select_interprom(pitch_target) | (w_16 ? 0 : CCCA_8BITSELECT)); else - snd_emu10k1_ptr_write(emu, CCCA, voice, (start_addr + ccis) | + snd_emu10k1_ptr_write(emu, CCCA, voice, (start_addr + 64 - 3) | emu10k1_select_interprom(pitch_target) | (w_16 ? 0 : CCCA_8BITSELECT)); /* Clear filter delay memory */ @@ -532,35 +516,30 @@ static void snd_emu10k1_playback_invalidate_cache(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *evoice) { struct snd_pcm_runtime *runtime; - unsigned int voice, stereo, i, ccis, cra = 64, cs, sample; + unsigned voice, stereo, sample; + u32 ccr;
runtime = evoice->epcm->substream->runtime; voice = evoice->number; stereo = (runtime->channels == 2); sample = snd_pcm_format_width(runtime->format) == 16 ? 0 : 0x80808080; - ccis = emu10k1_ccis(stereo, sample == 0); - /* set cs to 2 * number of cache registers beside the invalidated */ - cs = (sample == 0) ? (32-ccis) : (64-ccis+1) >> 1; - if (cs > 16) cs = 16; - for (i = 0; i < cs; i++) { + + // We assume that the cache is resting at this point (i.e., + // CCR_CACHEINVALIDSIZE is very small). + + // Clear leading frames. For simplicitly, this does too much, + // except for 16-bit stereo. And the interpolator will actually + // access them at all only when we're pitch-shifting. + for (int i = 0; i < 3; i++) snd_emu10k1_ptr_write(emu, CD0 + i, voice, sample); - if (stereo) { - snd_emu10k1_ptr_write(emu, CD0 + i, voice + 1, sample); - } - } - /* reset cache */ - snd_emu10k1_ptr_write(emu, CCR_CACHEINVALIDSIZE, voice, 0); - snd_emu10k1_ptr_write(emu, CCR_READADDRESS, voice, cra); + + // Fill cache + ccr = (64 - 3) << REG_SHIFT(CCR_CACHEINVALIDSIZE); if (stereo) { - snd_emu10k1_ptr_write(emu, CCR_CACHEINVALIDSIZE, voice + 1, 0); - // The engine goes haywire if this one is out of sync - snd_emu10k1_ptr_write(emu, CCR_READADDRESS, voice + 1, cra); - } - /* fill cache */ - snd_emu10k1_ptr_write(emu, CCR_CACHEINVALIDSIZE, voice, ccis); - if (stereo) { - snd_emu10k1_ptr_write(emu, CCR_CACHEINVALIDSIZE, voice+1, ccis); + // The engine goes haywire if CCR_READADDRESS is out of sync + snd_emu10k1_ptr_write(emu, CCR, voice + 1, ccr); } + snd_emu10k1_ptr_write(emu, CCR, voice, ccr); }
static void snd_emu10k1_playback_commit_volume(struct snd_emu10k1 *emu,
Originally, there was a 1:1 relationship between the PCM streams' and the low-level voices' parameters. The addition of multi-channel playback partially invalidated that, but didn't introduce proper layering, so things kept working only by virtue of the multi-channel device never having two channels (yet). The upcoming addition of 32-bit playback would complete upending the relationships.
So this patch detaches the low-level parameters from the high-level ones: we pass pre-calculated bit width and stereo flags to the low-level manipulation functions instead of calculating them in-place from the stream parameters.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emupcm.c | 50 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index feb575922738..bbe054be2448 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -239,22 +239,21 @@ static unsigned int emu10k1_select_interprom(unsigned int pitch_target) static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, int master, int extra, struct snd_emu10k1_voice *evoice, + bool w_16, bool stereo, unsigned int start_addr, unsigned int end_addr, struct snd_emu10k1_pcm_mixer *mix) { struct snd_pcm_substream *substream = evoice->epcm->substream; struct snd_pcm_runtime *runtime = substream->runtime; unsigned int silent_page, tmp; - int voice, stereo, w_16; + int voice; unsigned char send_amount[8]; unsigned char send_routing[8]; unsigned long flags; unsigned int pitch_target;
voice = evoice->number; - stereo = runtime->channels == 2; - w_16 = snd_pcm_format_width(runtime->format) == 16;
spin_lock_irqsave(&emu->reg_lock, flags);
@@ -273,7 +272,7 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, if (master) { evoice->epcm->ccca_start_addr = start_addr + 64 - 3; } - if (stereo && !extra) { + if (stereo) { // Not really necessary for the slave, but it doesn't hurt snd_emu10k1_ptr_write(emu, CPF, voice, CPF_STEREO_MASK); } else { @@ -399,15 +398,15 @@ static int snd_emu10k1_playback_prepare(struct snd_pcm_substream *substream)
start_addr = epcm->start_addr >> w_16; end_addr = start_addr + runtime->period_size; - snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, + snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, w_16, false, start_addr, end_addr, NULL); start_addr >>= stereo; end_addr = start_addr + runtime->buffer_size; - snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], + snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], w_16, stereo, start_addr, end_addr, &emu->pcm_mixer[substream->number]); if (epcm->voices[1]) - snd_emu10k1_pcm_init_voice(emu, 0, 0, epcm->voices[1], + snd_emu10k1_pcm_init_voice(emu, 0, 0, epcm->voices[1], w_16, true, start_addr, end_addr, &emu->pcm_mixer[substream->number]); return 0; @@ -426,17 +425,17 @@ static int snd_emu10k1_efx_playback_prepare(struct snd_pcm_substream *substream)
channel_size = runtime->buffer_size;
- snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, + snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, true, false, start_addr, start_addr + (channel_size / 2), NULL);
/* only difference with the master voice is we use it for the pointer */ - snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], + snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], true, false, start_addr, start_addr + channel_size, &emu->efx_pcm_mixer[0]);
start_addr += channel_size; for (i = 1; i < NUM_EFX_PLAYBACK; i++) { - snd_emu10k1_pcm_init_voice(emu, 0, 0, epcm->voices[i], + snd_emu10k1_pcm_init_voice(emu, 0, 0, epcm->voices[i], true, false, start_addr, start_addr + channel_size, &emu->efx_pcm_mixer[i]); start_addr += channel_size; @@ -513,16 +512,14 @@ static int snd_emu10k1_capture_prepare(struct snd_pcm_substream *substream) }
static void snd_emu10k1_playback_invalidate_cache(struct snd_emu10k1 *emu, - struct snd_emu10k1_voice *evoice) + struct snd_emu10k1_voice *evoice, + bool w_16, bool stereo) { - struct snd_pcm_runtime *runtime; - unsigned voice, stereo, sample; + unsigned voice, sample; u32 ccr;
- runtime = evoice->epcm->substream->runtime; voice = evoice->number; - stereo = (runtime->channels == 2); - sample = snd_pcm_format_width(runtime->format) == 16 ? 0 : 0x80808080; + sample = w_16 ? 0 : 0x80808080;
// We assume that the cache is resting at this point (i.e., // CCR_CACHEINVALIDSIZE is very small). @@ -552,20 +549,15 @@ static void snd_emu10k1_playback_commit_volume(struct snd_emu10k1 *emu,
static void snd_emu10k1_playback_unmute_voice(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *evoice, - bool master, + bool stereo, bool master, struct snd_emu10k1_pcm_mixer *mix) { - struct snd_pcm_substream *substream; - struct snd_pcm_runtime *runtime; unsigned int vattn; unsigned int tmp;
if (evoice == NULL) /* skip second voice for mono */ return; - substream = evoice->epcm->substream; - runtime = substream->runtime; - - tmp = runtime->channels == 2 ? (master ? 1 : 2) : 0; + tmp = stereo ? (master ? 1 : 2) : 0; vattn = mix->attn[tmp] << 16; snd_emu10k1_playback_commit_volume(emu, evoice, vattn); } @@ -628,23 +620,25 @@ static int snd_emu10k1_playback_trigger(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime = substream->runtime; struct snd_emu10k1_pcm *epcm = runtime->private_data; struct snd_emu10k1_pcm_mixer *mix; + bool w_16 = snd_pcm_format_width(runtime->format) == 16; + bool stereo = runtime->channels == 2; int result = 0;
/* dev_dbg(emu->card->dev, "trigger - emu10k1 = 0x%x, cmd = %i, pointer = %i\n", (int)emu, cmd, substream->ops->pointer(substream)) */ spin_lock(&emu->reg_lock); switch (cmd) { case SNDRV_PCM_TRIGGER_START: - snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[0]); + snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[0], w_16, stereo); fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: mix = &emu->pcm_mixer[substream->number]; - snd_emu10k1_playback_unmute_voice(emu, epcm->voices[0], true, mix); - snd_emu10k1_playback_unmute_voice(emu, epcm->voices[1], false, mix); + snd_emu10k1_playback_unmute_voice(emu, epcm->voices[0], stereo, true, mix); + snd_emu10k1_playback_unmute_voice(emu, epcm->voices[1], stereo, false, mix); snd_emu10k1_playback_set_running(emu, epcm); snd_emu10k1_playback_trigger_voice(emu, epcm->voices[0]); snd_emu10k1_playback_trigger_voice(emu, epcm->extra); @@ -782,13 +776,13 @@ static int snd_emu10k1_efx_playback_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_START: /* prepare voices */ for (i = 0; i < NUM_EFX_PLAYBACK; i++) { - snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[i]); + snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[i], true, false); } fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: for (i = 0; i < NUM_EFX_PLAYBACK; i++) - snd_emu10k1_playback_unmute_voice(emu, epcm->voices[i], false, + snd_emu10k1_playback_unmute_voice(emu, epcm->voices[i], false, true, &emu->efx_pcm_mixer[i]);
snd_emu10k1_playback_set_running(emu, epcm);
Rename snd_emu10k1_playback_invalidate_cache() to the more apt snd_emu10k1_playback_fill_cache(), and factor out snd_emu10k1_playback_prepare_voices(), which calls the former for all channels.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emupcm.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index bbe054be2448..063918397a5f 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -511,16 +511,12 @@ static int snd_emu10k1_capture_prepare(struct snd_pcm_substream *substream) return 0; }
-static void snd_emu10k1_playback_invalidate_cache(struct snd_emu10k1 *emu, - struct snd_emu10k1_voice *evoice, - bool w_16, bool stereo) +static void snd_emu10k1_playback_fill_cache(struct snd_emu10k1 *emu, + unsigned voice, + u32 sample, bool stereo) { - unsigned voice, sample; u32 ccr;
- voice = evoice->number; - sample = w_16 ? 0 : 0x80808080; - // We assume that the cache is resting at this point (i.e., // CCR_CACHEINVALIDSIZE is very small).
@@ -539,6 +535,22 @@ static void snd_emu10k1_playback_invalidate_cache(struct snd_emu10k1 *emu, snd_emu10k1_ptr_write(emu, CCR, voice, ccr); }
+static void snd_emu10k1_playback_prepare_voices(struct snd_emu10k1 *emu, + struct snd_emu10k1_pcm *epcm, + bool w_16, bool stereo, + int channels) +{ + u32 sample = w_16 ? 0 : 0x80808080; + + for (int i = 0; i < channels; i++) { + unsigned voice = epcm->voices[i]->number; + snd_emu10k1_playback_fill_cache(emu, voice, sample, stereo); + } + + // It takes a moment until the cache fills complete, + // but the unmuting takes long enough for that. +} + static void snd_emu10k1_playback_commit_volume(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *evoice, unsigned int vattn) @@ -632,7 +644,7 @@ static int snd_emu10k1_playback_trigger(struct snd_pcm_substream *substream, spin_lock(&emu->reg_lock); switch (cmd) { case SNDRV_PCM_TRIGGER_START: - snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[0], w_16, stereo); + snd_emu10k1_playback_prepare_voices(emu, epcm, w_16, stereo, 1); fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: @@ -774,10 +786,7 @@ static int snd_emu10k1_efx_playback_trigger(struct snd_pcm_substream *substream, spin_lock(&emu->reg_lock); switch (cmd) { case SNDRV_PCM_TRIGGER_START: - /* prepare voices */ - for (i = 0; i < NUM_EFX_PLAYBACK; i++) { - snd_emu10k1_playback_invalidate_cache(emu, epcm->voices[i], true, false); - } + snd_emu10k1_playback_prepare_voices(emu, epcm, true, false, NUM_EFX_PLAYBACK); fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME:
Pull the special handling of extra voices out of snd_emu10k1_pcm_init_voice(), simplify snd_emu10k1_playback_pointer(), and make the logic overall clearer. Also, add verbose comments.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emupcm.c | 81 ++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 29 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 063918397a5f..89f7e85034b6 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -269,9 +269,6 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, memcpy(send_amount, &mix->send_volume[tmp][0], 8); }
- if (master) { - evoice->epcm->ccca_start_addr = start_addr + 64 - 3; - } if (stereo) { // Not really necessary for the slave, but it doesn't hurt snd_emu10k1_ptr_write(emu, CPF, voice, CPF_STEREO_MASK); @@ -299,12 +296,7 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu, pitch_target = PITCH_48000; /* Disable interpolators on emu1010 card */ else pitch_target = emu10k1_calc_pitch_target(runtime->rate); - if (extra) - snd_emu10k1_ptr_write(emu, CCCA, voice, (end_addr - 3) | - emu10k1_select_interprom(pitch_target) | - (w_16 ? 0 : CCCA_8BITSELECT)); - else - snd_emu10k1_ptr_write(emu, CCCA, voice, (start_addr + 64 - 3) | + snd_emu10k1_ptr_write(emu, CCCA, voice, emu10k1_select_interprom(pitch_target) | (w_16 ? 0 : CCCA_8BITSELECT)); /* Clear filter delay memory */ @@ -401,6 +393,7 @@ static int snd_emu10k1_playback_prepare(struct snd_pcm_substream *substream) snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, w_16, false, start_addr, end_addr, NULL); start_addr >>= stereo; + epcm->ccca_start_addr = start_addr; end_addr = start_addr + runtime->buffer_size; snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], w_16, stereo, start_addr, end_addr, @@ -428,13 +421,8 @@ static int snd_emu10k1_efx_playback_prepare(struct snd_pcm_substream *substream) snd_emu10k1_pcm_init_voice(emu, 1, 1, epcm->extra, true, false, start_addr, start_addr + (channel_size / 2), NULL);
- /* only difference with the master voice is we use it for the pointer */ - snd_emu10k1_pcm_init_voice(emu, 1, 0, epcm->voices[0], true, false, - start_addr, start_addr + channel_size, - &emu->efx_pcm_mixer[0]); - - start_addr += channel_size; - for (i = 1; i < NUM_EFX_PLAYBACK; i++) { + epcm->ccca_start_addr = start_addr; + for (i = 0; i < NUM_EFX_PLAYBACK; i++) { snd_emu10k1_pcm_init_voice(emu, 0, 0, epcm->voices[i], true, false, start_addr, start_addr + channel_size, &emu->efx_pcm_mixer[i]); @@ -540,13 +528,45 @@ static void snd_emu10k1_playback_prepare_voices(struct snd_emu10k1 *emu, bool w_16, bool stereo, int channels) { + struct snd_pcm_substream *substream = epcm->substream; + struct snd_pcm_runtime *runtime = substream->runtime; + unsigned eloop_start = epcm->start_addr >> w_16; + unsigned loop_start = eloop_start >> stereo; + unsigned eloop_size = runtime->period_size; + unsigned loop_size = runtime->buffer_size; u32 sample = w_16 ? 0 : 0x80808080;
+ // To make the playback actually start at the 1st frame, + // we need to compensate for two circumstances: + // - The actual position is delayed by the cache size (64 frames) + // - The interpolator is centered around the 4th frame + loop_start += 64 - 3; for (int i = 0; i < channels; i++) { unsigned voice = epcm->voices[i]->number; + snd_emu10k1_ptr_write(emu, CCCA_CURRADDR, voice, loop_start); + loop_start += loop_size; snd_emu10k1_playback_fill_cache(emu, voice, sample, stereo); }
+ // The interrupt is triggered when CCCA_CURRADDR (CA) wraps around, + // which is ahead of the actual playback position, so the interrupt + // source needs to be delayed. + // + // In principle, this wouldn't need to be the cache's entire size - in + // practice, CCR_CACHEINVALIDSIZE (CIS) > `fetch threshold` has never + // been observed, and assuming 40 _bytes_ should be safe. + // + // The cache fills are somewhat random, which makes it impossible to + // align them with the interrupts. This makes a non-delayed interrupt + // source not practical, as the interrupt handler would have to wait + // for (CA - CIS) >= period_boundary for every channel in the stream. + // + // This is why all other (open) drivers for these chips use timer-based + // interrupts. + // + eloop_start += eloop_size - 3; + snd_emu10k1_ptr_write(emu, CCCA_CURRADDR, epcm->extra->number, eloop_start); + // It takes a moment until the cache fills complete, // but the unmuting takes long enough for that. } @@ -746,24 +766,27 @@ static snd_pcm_uframes_t snd_emu10k1_playback_pointer(struct snd_pcm_substream * struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; struct snd_emu10k1_pcm *epcm = runtime->private_data; - unsigned int ptr; + int ptr;
if (!epcm->running) return 0; + ptr = snd_emu10k1_ptr_read(emu, CCCA, epcm->voices[0]->number) & 0x00ffffff; -#if 0 /* Perex's code */ - ptr += runtime->buffer_size; ptr -= epcm->ccca_start_addr; - ptr %= runtime->buffer_size; -#else /* EMU10K1 Open Source code from Creative */ - if (ptr < epcm->ccca_start_addr) - ptr += runtime->buffer_size - epcm->ccca_start_addr; - else { - ptr -= epcm->ccca_start_addr; - if (ptr >= runtime->buffer_size) - ptr -= runtime->buffer_size; - } -#endif + + // This is the size of the whole cache minus the interpolator read-ahead, + // which leads us to the actual playback position. + // + // The cache is constantly kept mostly filled, so in principle we could + // return a more advanced position representing how far the hardware has + // already read the buffer, and set runtime->delay accordingly. However, + // this would be slightly different for every channel (and remarkably slow + // to obtain), so only a fixed worst-case value would be practical. + // + ptr -= 64 - 3; + if (ptr < 0) + ptr += runtime->buffer_size; + /* dev_dbg(emu->card->dev, "ptr = 0x%lx, buffer_size = 0x%lx, period_size = 0x%lx\n",
The period_bytes_min parameter and the buffer_bytes minimum constraint made no sense at all, as they didn't reflect any hardware limitation. Instead, apply a frame-based period_size minimum constraint, which is derived from the cache size (it would be actually possible to go below that, but it would require special handling, and it would be practically impossible to keep up with the IRQ rate anyway).
Sync up the constraints of the EFX playback with those of the regular playback, as there is no reason for them to diverge.
N.b., the maximum buffer size is actually arbitrary - the hardware could go waay beyond 128 KiB.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- v2: - constrain period size instead of buffer size, as otherwise a stupid app could set a small buffer with many periods, thus making the period size too small. takashi was actually right! :-D --- sound/pci/emu10k1/emupcm.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 89f7e85034b6..9045359bb461 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -444,9 +444,8 @@ static const struct snd_pcm_hardware snd_emu10k1_efx_playback = .rate_max = 48000, .channels_min = NUM_EFX_PLAYBACK, .channels_max = NUM_EFX_PLAYBACK, - .buffer_bytes_max = (64*1024), - .period_bytes_min = 64, - .period_bytes_max = (64*1024), + .buffer_bytes_max = (128*1024), + .period_bytes_max = (128*1024), .periods_min = 2, .periods_max = 2, .fifo_size = 0, @@ -877,7 +876,6 @@ static const struct snd_pcm_hardware snd_emu10k1_playback = .channels_min = 1, .channels_max = 2, .buffer_bytes_max = (128*1024), - .period_bytes_min = 64, .period_bytes_max = (128*1024), .periods_min = 1, .periods_max = 1024, @@ -982,25 +980,46 @@ static int snd_emu10k1_efx_playback_close(struct snd_pcm_substream *substream) return 0; }
+static int snd_emu10k1_playback_set_constraints(struct snd_pcm_runtime *runtime) +{ + int err; + + // The buffer size must be a multiple of the period size, to avoid a + // mismatch between the extra voice and the regular voices. + err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); + if (err < 0) + return err; + // The hardware is typically the cache's size of 64 frames ahead. + // Leave enough time for actually filling up the buffer. + err = snd_pcm_hw_constraint_minmax( + runtime, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 128, UINT_MAX); + return err; +} + static int snd_emu10k1_efx_playback_open(struct snd_pcm_substream *substream) { struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_emu10k1_pcm *epcm; struct snd_emu10k1_pcm_mixer *mix; struct snd_pcm_runtime *runtime = substream->runtime; - int i, j; + int i, j, err;
epcm = kzalloc(sizeof(*epcm), GFP_KERNEL); if (epcm == NULL) return -ENOMEM; epcm->emu = emu; epcm->type = PLAYBACK_EFX; epcm->substream = substream; runtime->private_data = epcm; runtime->private_free = snd_emu10k1_pcm_free_substream; runtime->hw = snd_emu10k1_efx_playback; - + err = snd_emu10k1_playback_set_constraints(runtime); + if (err < 0) { + kfree(epcm); + return err; + } + for (i = 0; i < NUM_EFX_PLAYBACK; i++) { mix = &emu->efx_pcm_mixer[i]; for (j = 0; j < 8; j++) @@ -1031,12 +1050,7 @@ static int snd_emu10k1_playback_open(struct snd_pcm_substream *substream) runtime->private_data = epcm; runtime->private_free = snd_emu10k1_pcm_free_substream; runtime->hw = snd_emu10k1_playback; - err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); - if (err < 0) { - kfree(epcm); - return err; - } - err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 256, UINT_MAX); + err = snd_emu10k1_playback_set_constraints(runtime); if (err < 0) { kfree(epcm); return err;
sorry for the "flood", i forgot to adjust the declaration of the series in my tool. you can ignore the other patches; they didn't change from what you already applied.
regards
On Thu, 18 May 2023 11:29:00 +0200, Oswald Buddenhagen wrote:
sorry for the "flood", i forgot to adjust the declaration of the series in my tool. you can ignore the other patches; they didn't change from what you already applied.
Don't worry, now I applied this one.
Thanks!
Takashi
participants (2)
-
Oswald Buddenhagen -
Takashi Iwai