[PATCH 1/9] ALSA: emu10k1: pass frame instead of byte addresses

[PATCH 1/9] ALSA: emu10k1: pass frame instead of byte addresses

[Oswald Buddenhagen]

... 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);
-- 
2.40.0.152.g15d061e6df

[PATCH 2/9] Revert "ALSA: emu10k1 - delay the PCM interrupts (add pcm_irq_delay parameter)"

[Oswald Buddenhagen]

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;
-- 
2.40.0.152.g15d061e6df

[PATCH 3/9] ALSA: emu10k1: remove pointless displacement of the extra voices

[Oswald Buddenhagen]

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
-- 
2.40.0.152.g15d061e6df

[PATCH 4/9] ALSA: emu10k1: skip pointless cache setup for extra voices

[Oswald Buddenhagen]

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:
-- 
2.40.0.152.g15d061e6df

[PATCH 5/9] ALSA: emu10k1: fix PCM playback cache and interrupt handling

[Oswald Buddenhagen]

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,
-- 
2.40.0.152.g15d061e6df

[PATCH 6/9] ALSA: emu10k1: fix PCM playback buffer size constraints

[Oswald Buddenhagen]

The period_bytes_min parameter made no sense at all, as it didn't
correlate with any hardware limitation.
The same can be said of the buffer_bytes minimum constraint.
Instead, apply a buffer_size constraint, which works on frames.

Sync up the constraints of the EFX playback with those of the regular
playback, as there is no reason for them to diverge.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 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 feb575922738..5226f0978408 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -457,9 +457,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,
@@ -851,7 +850,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,
@@ -956,25 +954,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_BUFFER_SIZE, 256, 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++)
@@ -1005,12 +1024,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;
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 6/9] ALSA: emu10k1: fix PCM playback buffer size constraints

[Takashi Iwai]

On Wed, 17 May 2023 19:42:53 +0200,
Oswald Buddenhagen wrote:
> 
> The period_bytes_min parameter made no sense at all, as it didn't
> correlate with any hardware limitation.

Does the device really work with less than 64 bytes period size?
I meant not in theory but in practice.  Without any value set,
dumb applications may try to set 4 bytes for period size, so setting
some practical limit still makes sense.


Takashi

> The same can be said of the buffer_bytes minimum constraint.
> Instead, apply a buffer_size constraint, which works on frames.
> 
> Sync up the constraints of the EFX playback with those of the regular
> playback, as there is no reason for them to diverge.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  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 feb575922738..5226f0978408 100644
> --- a/sound/pci/emu10k1/emupcm.c
> +++ b/sound/pci/emu10k1/emupcm.c
> @@ -457,9 +457,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,
> @@ -851,7 +850,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,
> @@ -956,25 +954,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_BUFFER_SIZE, 256, 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++)
> @@ -1005,12 +1024,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;
> -- 
> 2.40.0.152.g15d061e6df
> 

Re: [PATCH 6/9] ALSA: emu10k1: fix PCM playback buffer size constraints

[Oswald Buddenhagen]

On Wed, May 17, 2023 at 10:25:00PM +0200, Takashi Iwai wrote:
>On Wed, 17 May 2023 19:42:53 +0200,
>Oswald Buddenhagen wrote:
>> 
>> The period_bytes_min parameter made no sense at all, as it didn't
>> correlate with any hardware limitation.
>
>Does the device really work with less than 64 bytes period size?
>I meant not in theory but in practice.
>
somewhat predictably, not.

>Without any value set,
>dumb applications may try to set 4 bytes for period size,
>
the "try to" is key here. it will fail, because the frame-based 
constraint will prevent it from doing so.

alsa's constraint system is really quite impressive, probably the 
technically most interesting part of the whole. :-)

regards

Re: [PATCH 6/9] ALSA: emu10k1: fix PCM playback buffer size constraints

[Takashi Iwai]

On Thu, 18 May 2023 09:57:51 +0200,
Oswald Buddenhagen wrote:
> 
> On Wed, May 17, 2023 at 10:25:00PM +0200, Takashi Iwai wrote:
> > On Wed, 17 May 2023 19:42:53 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> The period_bytes_min parameter made no sense at all, as it didn't
> >> correlate with any hardware limitation.
> > 
> > Does the device really work with less than 64 bytes period size?
> > I meant not in theory but in practice.
> > 
> somewhat predictably, not.
> 
> > Without any value set,
> > dumb applications may try to set 4 bytes for period size,
> > 
> the "try to" is key here. it will fail, because the frame-based
> constraint will prevent it from doing so.

Ah OK, this should be commented that the lower bound is set in a
different way.


Takashi

[PATCH 7/9] ALSA: emu10k1: improve API of low-level voice manipulation functions

[Oswald Buddenhagen]

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 5226f0978408..7924d185cf92 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;
@@ -512,16 +511,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).
@@ -551,20 +548,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);
 }	
@@ -627,23 +619,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);
@@ -781,13 +775,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);
-- 
2.40.0.152.g15d061e6df

[PATCH 8/9] ALSA: emu10k1: refactor PCM playback cache filling

[Oswald Buddenhagen]

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 7924d185cf92..2af210961a87 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -510,16 +510,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).
 
@@ -538,6 +534,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)
@@ -631,7 +643,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:
@@ -773,10 +785,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:
-- 
2.40.0.152.g15d061e6df

[PATCH 9/9] ALSA: emu10k1: refactor PCM playback address handling

[Oswald Buddenhagen]

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 2af210961a87..0094f629ad47 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]);
@@ -539,13 +527,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.
 }
@@ -745,24 +765,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",
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 1/9] ALSA: emu10k1: pass frame instead of byte addresses

[Takashi Iwai]

On Wed, 17 May 2023 19:42:48 +0200,
Oswald Buddenhagen wrote:
> 
> ... to snd_emu10k1_pcm_init_voice(). This makes the code arguably less
> convoluted.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Now applied all patches except for patch#6 ("ALSA: emu10k1: fix PCM
playback buffer size constraints").


thanks,

Takashi