[PATCH 0/7] ALSA: emu10k1: refactoring of the playback voice management

[PATCH 0/7] ALSA: emu10k1: refactoring of the playback voice management

[Oswald Buddenhagen]

Oswald Buddenhagen (7):
  ALSA: emu10k1: simplify freeing synth voices
  ALSA: emu10k1: don't forget to reset reclaimed synth voices
  ALSA: emu10k1: improve voice status display in /proc
  ALSA: emu10k1: make freeing untouched playback voices cheap
  ALSA: emu10k1: centralize freeing PCM voices
  ALSA: emu10k1: make snd_emu10k1_voice_alloc() assign voices' epcm
  ALSA: emu10k1: revamp playback voice allocator

 include/sound/emu10k1.h              |  18 ++--
 sound/pci/emu10k1/emu10k1_callback.c |   8 +-
 sound/pci/emu10k1/emumixer.c         |  24 ++---
 sound/pci/emu10k1/emupcm.c           |  88 ++++++++---------
 sound/pci/emu10k1/emuproc.c          |  18 ++--
 sound/pci/emu10k1/voice.c            | 136 +++++++++++++--------------
 6 files changed, 136 insertions(+), 156 deletions(-)

-- 
2.40.0.152.g15d061e6df

[PATCH 1/7] ALSA: emu10k1: simplify freeing synth voices

[Oswald Buddenhagen]

snd_emu10k1_voice_free() resets the hardware itself, so doing that
in the calling function as well is redundant.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_callback.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 06440b97b5d7..4a8b2df06a28 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -160,11 +160,6 @@ free_voice(struct snd_emux_voice *vp)
 	/* Problem apparent on plug, unplug then plug */
 	/* on the Audigy 2 ZS Notebook. */
 	if (hw && (vp->ch >= 0)) {
-		snd_emu10k1_ptr_write(hw, IFATN, vp->ch, 0xff00);
-		snd_emu10k1_ptr_write(hw, DCYSUSV, vp->ch, 0x807f | DCYSUSV_CHANNELENABLE_MASK);
-		// snd_emu10k1_ptr_write(hw, DCYSUSV, vp->ch, 0);
-		snd_emu10k1_ptr_write(hw, VTFT, vp->ch, 0xffff);
-		snd_emu10k1_ptr_write(hw, CVCF, vp->ch, 0xffff);
 		snd_emu10k1_voice_free(hw, &hw->voices[vp->ch]);
 		vp->emu->num_voices--;
 		vp->ch = -1;
-- 
2.40.0.152.g15d061e6df

[PATCH 2/7] ALSA: emu10k1: don't forget to reset reclaimed synth voices

[Oswald Buddenhagen]

The subsequent allocation may still fail after freeing some voices, so
we shouldn't leave them in their programmed state.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/voice.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/sound/pci/emu10k1/voice.c b/sound/pci/emu10k1/voice.c
index cbeb8443492c..a602df9117f6 100644
--- a/sound/pci/emu10k1/voice.c
+++ b/sound/pci/emu10k1/voice.c
@@ -98,6 +98,15 @@ static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 	return 0;
 }
 
+static void voice_free(struct snd_emu10k1 *emu,
+		       struct snd_emu10k1_voice *pvoice)
+{
+	snd_emu10k1_voice_init(emu, pvoice->number);
+	pvoice->interrupt = NULL;
+	pvoice->use = pvoice->pcm = pvoice->synth = pvoice->midi = pvoice->efx = 0;
+	pvoice->epcm = NULL;
+}
+
 int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 			    struct snd_emu10k1_voice **rvoice)
 {
@@ -118,12 +127,8 @@ int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 		/* free a voice from synth */
 		if (emu->get_synth_voice) {
 			result = emu->get_synth_voice(emu);
-			if (result >= 0) {
-				struct snd_emu10k1_voice *pvoice = &emu->voices[result];
-				pvoice->interrupt = NULL;
-				pvoice->use = pvoice->pcm = pvoice->synth = pvoice->midi = pvoice->efx = 0;
-				pvoice->epcm = NULL;
-			}
+			if (result >= 0)
+				voice_free(emu, &emu->voices[result]);
 		}
 		if (result < 0)
 			break;
@@ -143,10 +148,7 @@ int snd_emu10k1_voice_free(struct snd_emu10k1 *emu,
 	if (snd_BUG_ON(!pvoice))
 		return -EINVAL;
 	spin_lock_irqsave(&emu->voice_lock, flags);
-	pvoice->interrupt = NULL;
-	pvoice->use = pvoice->pcm = pvoice->synth = pvoice->midi = pvoice->efx = 0;
-	pvoice->epcm = NULL;
-	snd_emu10k1_voice_init(emu, pvoice->number);
+	voice_free(emu, pvoice);
 	spin_unlock_irqrestore(&emu->voice_lock, flags);
 	return 0;
 }
-- 
2.40.0.152.g15d061e6df

[PATCH 3/7] ALSA: emu10k1: improve voice status display in /proc

[Oswald Buddenhagen]

Eliminate the MIDI type, as there is no such thing - the MPU401 port
doesn't have anything to do with voices.

For clarity, differentiate between regular and extra voices.

Don't atomize the enum into bits in the table display.

Simplify/optimize the storage.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h     | 13 ++++++-------
 sound/pci/emu10k1/emupcm.c  |  2 +-
 sound/pci/emu10k1/emuproc.c | 16 ++++++++--------
 sound/pci/emu10k1/voice.c   | 20 +++-----------------
 4 files changed, 18 insertions(+), 33 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 9c5de1f45566..2d247f8f635b 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1437,21 +1437,20 @@ SUB_REG_NC(A_EHC, A_I2S_CAPTURE_RATE, 0x00000e00)  /* This sets the capture PCM
 /* ------------------- STRUCTURES -------------------- */
 
 enum {
+	EMU10K1_UNUSED,  // This must be zero
 	EMU10K1_EFX,
+	EMU10K1_EFX_IRQ,
 	EMU10K1_PCM,
+	EMU10K1_PCM_IRQ,
 	EMU10K1_SYNTH,
-	EMU10K1_MIDI
+	EMU10K1_NUM_TYPES
 };
 
 struct snd_emu10k1;
 
 struct snd_emu10k1_voice {
-	int number;
-	unsigned int use: 1,
-	    pcm: 1,
-	    efx: 1,
-	    synth: 1,
-	    midi: 1;
+	unsigned char number;
+	unsigned char use;
 	void (*interrupt)(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *pvoice);
 
 	struct snd_emu10k1_pcm *epcm;
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index 1ca16f0ddbed..aa3d6d573f05 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -117,7 +117,7 @@ static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voic
 		// 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,
+					      epcm->type == PLAYBACK_EMUVOICE ? EMU10K1_PCM_IRQ : EMU10K1_EFX_IRQ,
 					      1,
 					      &epcm->extra);
 		if (err < 0) {
diff --git a/sound/pci/emu10k1/emuproc.c b/sound/pci/emu10k1/emuproc.c
index 708aff6cf09a..c423a56ebf9e 100644
--- a/sound/pci/emu10k1/emuproc.c
+++ b/sound/pci/emu10k1/emuproc.c
@@ -367,17 +367,17 @@ static void snd_emu10k1_proc_voices_read(struct snd_info_entry *entry,
 	struct snd_emu10k1 *emu = entry->private_data;
 	struct snd_emu10k1_voice *voice;
 	int idx;
-	
-	snd_iprintf(buffer, "ch\tuse\tpcm\tefx\tsynth\tmidi\n");
+	static const char * const types[] = {
+		"Unused", "EFX", "EFX IRQ", "PCM", "PCM IRQ", "Synth"
+	};
+	static_assert(ARRAY_SIZE(types) == EMU10K1_NUM_TYPES);
+
+	snd_iprintf(buffer, "ch\tuse\n");
 	for (idx = 0; idx < NUM_G; idx++) {
 		voice = &emu->voices[idx];
-		snd_iprintf(buffer, "%i\t%i\t%i\t%i\t%i\t%i\n",
+		snd_iprintf(buffer, "%i\t%s\n",
 			idx,
-			voice->use,
-			voice->pcm,
-			voice->efx,
-			voice->synth,
-			voice->midi);
+			types[voice->use]);
 	}
 }
 
diff --git a/sound/pci/emu10k1/voice.c b/sound/pci/emu10k1/voice.c
index a602df9117f6..ac89d09ed9bc 100644
--- a/sound/pci/emu10k1/voice.c
+++ b/sound/pci/emu10k1/voice.c
@@ -78,32 +78,18 @@ static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 		dev_dbg(emu->card->dev, "voice alloc - %i, %i of %i\n",
 		       voice->number, idx-first_voice+1, number);
 		*/
-		voice->use = 1;
-		switch (type) {
-		case EMU10K1_PCM:
-			voice->pcm = 1;
-			break;
-		case EMU10K1_SYNTH:
-			voice->synth = 1;
-			break;
-		case EMU10K1_MIDI:
-			voice->midi = 1;
-			break;
-		case EMU10K1_EFX:
-			voice->efx = 1;
-			break;
-		}
+		voice->use = type;
 	}
 	*rvoice = &emu->voices[first_voice];
 	return 0;
 }
 
 static void voice_free(struct snd_emu10k1 *emu,
 		       struct snd_emu10k1_voice *pvoice)
 {
 	snd_emu10k1_voice_init(emu, pvoice->number);
 	pvoice->interrupt = NULL;
-	pvoice->use = pvoice->pcm = pvoice->synth = pvoice->midi = pvoice->efx = 0;
+	pvoice->use = 0;
 	pvoice->epcm = NULL;
 }
 
@@ -121,7 +107,7 @@ int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 	spin_lock_irqsave(&emu->voice_lock, flags);
 	for (;;) {
 		result = voice_alloc(emu, type, number, rvoice);
-		if (result == 0 || type == EMU10K1_SYNTH || type == EMU10K1_MIDI)
+		if (result == 0 || type == EMU10K1_SYNTH)
 			break;
 
 		/* free a voice from synth */
-- 
2.40.0.152.g15d061e6df

[PATCH 4/7] ALSA: emu10k1: make freeing untouched playback voices cheap

[Oswald Buddenhagen]

This allows us to drop the code that tries to preserve already allocated
voices upon repeated hw_param callback invocations. Getting it right for
multi-channel voices would otherwise get a bit hairy.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h              |  1 +
 sound/pci/emu10k1/emu10k1_callback.c |  1 +
 sound/pci/emu10k1/emupcm.c           | 13 ++-----------
 sound/pci/emu10k1/emuproc.c          |  5 +++--
 sound/pci/emu10k1/voice.c            |  5 +++--
 5 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 2d247f8f635b..6ec2079534d4 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1451,6 +1451,7 @@ struct snd_emu10k1;
 struct snd_emu10k1_voice {
 	unsigned char number;
 	unsigned char use;
+	unsigned char dirty;
 	void (*interrupt)(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *pvoice);
 
 	struct snd_emu10k1_pcm *epcm;
diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 4a8b2df06a28..d70ca1f50705 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -458,6 +458,7 @@ start_voice(struct snd_emux_voice *vp)
 	}
 #endif
 
+	hw->voices[ch].dirty = 1;
 	return 0;
 }
 
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index aa3d6d573f05..c4696c127028 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -80,17 +80,6 @@ static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voic
 {
 	int err, i;
 
-	if (epcm->voices[1] != NULL && voices < 2) {
-		snd_emu10k1_voice_free(epcm->emu, epcm->voices[1]);
-		epcm->voices[1] = NULL;
-	}
-	for (i = 0; i < voices; i++) {
-		if (epcm->voices[i] == NULL)
-			break;
-	}
-	if (i == voices)
-		return 0; /* already allocated */
-
 	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
 		if (epcm->voices[i]) {
 			snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]);
@@ -306,6 +295,8 @@ static void snd_emu10k1_pcm_init_voice(struct snd_emu10k1 *emu,
 				      snd_emu10k1_compose_send_routing(send_routing));
 	}
 
+	emu->voices[voice].dirty = 1;
+
 	spin_unlock_irqrestore(&emu->reg_lock, flags);
 }
 
diff --git a/sound/pci/emu10k1/emuproc.c b/sound/pci/emu10k1/emuproc.c
index c423a56ebf9e..abcec8a01760 100644
--- a/sound/pci/emu10k1/emuproc.c
+++ b/sound/pci/emu10k1/emuproc.c
@@ -372,11 +372,12 @@ static void snd_emu10k1_proc_voices_read(struct snd_info_entry *entry,
 	};
 	static_assert(ARRAY_SIZE(types) == EMU10K1_NUM_TYPES);
 
-	snd_iprintf(buffer, "ch\tuse\n");
+	snd_iprintf(buffer, "ch\tdirty\tuse\n");
 	for (idx = 0; idx < NUM_G; idx++) {
 		voice = &emu->voices[idx];
-		snd_iprintf(buffer, "%i\t%s\n",
+		snd_iprintf(buffer, "%i\t%u\t%s\n",
 			idx,
+			voice->dirty,
 			types[voice->use]);
 	}
 }
diff --git a/sound/pci/emu10k1/voice.c b/sound/pci/emu10k1/voice.c
index ac89d09ed9bc..25e78fc188bf 100644
--- a/sound/pci/emu10k1/voice.c
+++ b/sound/pci/emu10k1/voice.c
@@ -87,9 +87,10 @@ static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 static void voice_free(struct snd_emu10k1 *emu,
 		       struct snd_emu10k1_voice *pvoice)
 {
-	snd_emu10k1_voice_init(emu, pvoice->number);
+	if (pvoice->dirty)
+		snd_emu10k1_voice_init(emu, pvoice->number);
 	pvoice->interrupt = NULL;
-	pvoice->use = 0;
+	pvoice->use = pvoice->dirty = 0;
 	pvoice->epcm = NULL;
 }
 
-- 
2.40.0.152.g15d061e6df

[PATCH 5/7] ALSA: emu10k1: centralize freeing PCM voices

[Oswald Buddenhagen]

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emupcm.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index c4696c127028..63e085aa65fe 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -76,16 +76,22 @@ static void snd_emu10k1_pcm_efx_interrupt(struct snd_emu10k1 *emu,
 	snd_pcm_period_elapsed(emu->pcm_capture_efx_substream);
 }	 
 
-static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
+static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
 {
-	int err, i;
-
-	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
+	for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
 		if (epcm->voices[i]) {
 			snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]);
 			epcm->voices[i] = NULL;
 		}
 	}
+}
+
+static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
+{
+	int err, i;
+
+	snd_emu10k1_pcm_free_voices(epcm);
+
 	err = snd_emu10k1_voice_alloc(epcm->emu,
 				      epcm->type == PLAYBACK_EMUVOICE ? EMU10K1_PCM : EMU10K1_EFX,
 				      voices,
@@ -115,15 +121,13 @@ static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voic
 			       "failed extra: voices=%d, frame=%d\n",
 			       voices, frame);
 			*/
-			for (i = 0; i < voices; i++) {
-				snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]);
-				epcm->voices[i] = NULL;
-			}
+			snd_emu10k1_pcm_free_voices(epcm);
 			return err;
 		}
 		epcm->extra->epcm = epcm;
 		epcm->extra->interrupt = snd_emu10k1_pcm_interrupt;
 	}
+
 	return 0;
 }
 
@@ -342,21 +346,15 @@ static int snd_emu10k1_playback_hw_free(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;
-	int i;
 
 	if (runtime->private_data == NULL)
 		return 0;
 	epcm = runtime->private_data;
 	if (epcm->extra) {
 		snd_emu10k1_voice_free(epcm->emu, epcm->extra);
 		epcm->extra = NULL;
 	}
-	for (i = 0; i < NUM_EFX_PLAYBACK; i++) {
-		if (epcm->voices[i]) {
-			snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]);
-			epcm->voices[i] = NULL;
-		}
-	}
+	snd_emu10k1_pcm_free_voices(epcm);
 	if (epcm->memblk) {
 		snd_emu10k1_free_pages(emu, epcm->memblk);
 		epcm->memblk = NULL;
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 5/7] ALSA: emu10k1: centralize freeing PCM voices

[Takashi Iwai]

On Thu, 18 May 2023 16:09:45 +0200,
Oswald Buddenhagen wrote:
> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
>  {
> -	int err, i;
> -
> -	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
> +	for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) {

We don't use this style.  Declare the variable outside the for().

Also, as usual, it'd be still helpful if you show this is merely a
code simplification without any functional change in the commit log.


thanks,

Takashi

Re: [PATCH 5/7] ALSA: emu10k1: centralize freeing PCM voices

[Oswald Buddenhagen]

On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote:
>On Thu, 18 May 2023 16:09:45 +0200,
>Oswald Buddenhagen wrote:
>> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
>> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
>>  {
>> -	int err, i;
>> -
>> -	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
>> +	for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
>
>We don't use this style.  Declare the variable outside the for().
>
ehm ...
- "we" seems to be mostly true for alsa. but looking at the kernel as a 
   whole, that ship has sailed since the adoption of c11. maybe time to 
   adapt?
- you're noticing this a bit late, after already merging 8 instances.

how should i proceed?

>Also, as usual, it'd be still helpful if you show this is merely a
>code simplification without any functional change in the commit log.
>
right. i don't always remember to pre-emptively amend the patches i 
wrote quite a while ago ...

regards

Re: [PATCH 5/7] ALSA: emu10k1: centralize freeing PCM voices

[Takashi Iwai]

On Fri, 19 May 2023 12:43:24 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote:
> > On Thu, 18 May 2023 16:09:45 +0200,
> > Oswald Buddenhagen wrote:
> >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
> >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
> >>  {
> >> -	int err, i;
> >> -
> >> -	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
> >> +	for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
> > 
> > We don't use this style.  Declare the variable outside the for().
> > 
> ehm ...
> - "we" seems to be mostly true for alsa. but looking at the kernel as
> a   whole, that ship has sailed since the adoption of c11. maybe time
> to   adapt?
> - you're noticing this a bit late, after already merging 8 instances.
> 
> how should i proceed?

I'm not super-strict about it, but as checkpatch complaints, it's
still not so widely adapted.  Unless there is a reason that must be
written in that way, let's avoid it as much as possible.

That said, the already merged stuff, it's OK-ish, and we can correct
only when anyone complains.  For the new stuff, let's be careful from
now on :)

If we want really adapting this style, the coding style rule should be
officially updated at first, followed by the update of tools.


thanks,

Takashi

Re: [PATCH 5/7] ALSA: emu10k1: centralize freeing PCM voices

[Oswald Buddenhagen]

On Fri, May 19, 2023 at 01:04:32PM +0200, Takashi Iwai wrote:
>On Fri, 19 May 2023 12:43:24 +0200,
>Oswald Buddenhagen wrote:
>> 
>> On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote:
>> > On Thu, 18 May 2023 16:09:45 +0200,
>> > Oswald Buddenhagen wrote:
>> >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
>> >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
>> >>  {
>> >> -	int err, i;
>> >> -
>> >> -	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
>> >> +	for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
>> > 
>I'm not super-strict about it, but

>as checkpatch complaints,
>
it doesn't, so from that side it's settled. it's really just about the 
alsa-local policy.

what it actually *does* complain about is the use of bare "unsigned". i 
don't like the excessively verbose "unsigned int", so i'll switch my 
uses over to "uint", which already has some use in alsa. ok?

regards,
ossi

Re: [PATCH 5/7] ALSA: emu10k1: centralize freeing PCM voices

[Takashi Iwai]

On Fri, 19 May 2023 13:53:01 +0200,
Oswald Buddenhagen wrote:
> 
> On Fri, May 19, 2023 at 01:04:32PM +0200, Takashi Iwai wrote:
> > On Fri, 19 May 2023 12:43:24 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote:
> >> > On Thu, 18 May 2023 16:09:45 +0200,
> >> > Oswald Buddenhagen wrote:
> >> >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
> >> >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
> >> >>  {
> >> >> -	int err, i;
> >> >> -
> >> >> -	for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
> >> >> +	for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) {
> >> > 
> > I'm not super-strict about it, but
> 
> > as checkpatch complaints,
> > 
> it doesn't, so from that side it's settled. it's really just about the
> alsa-local policy.
> 
> what it actually *does* complain about is the use of bare
> "unsigned".

Ah that's OK, then.

> i don't like the excessively verbose "unsigned int", so
> i'll switch my uses over to "uint", which already has some use in
> alsa. ok?

I don't mind much about the use of unsigned without int.  Or it could
be a simple int there, as that's nothing but a counter that is used
locally that can't over 31bit.

But the patch description could be still improved.


thanks,

Takashi

[PATCH 6/7] ALSA: emu10k1: make snd_emu10k1_voice_alloc() assign voices' epcm

[Oswald Buddenhagen]

The voice allocator clearly knows about the field (it resets it), so
it's more consistent (and leads to less duplicated code) to have the
constructor take it as a parameter.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h              | 3 ++-
 sound/pci/emu10k1/emu10k1_callback.c | 2 +-
 sound/pci/emu10k1/emupcm.c           | 7 ++-----
 sound/pci/emu10k1/voice.c            | 7 ++++---
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 6ec2079534d4..42959ba9c9f4 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1848,7 +1848,8 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me
 int snd_emu10k1_memblk_map(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk);
 
 /* voice allocation */
-int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int pair, struct snd_emu10k1_voice **rvoice);
+int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int pair,
+			    struct snd_emu10k1_pcm *epcm, struct snd_emu10k1_voice **rvoice);
 int snd_emu10k1_voice_free(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *pvoice);
 
 /* MIDI uart */
diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index d70ca1f50705..e256d3c22ee4 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -282,7 +282,7 @@ get_voice(struct snd_emux *emu, struct snd_emux_port *port)
 			if (vp->ch < 0) {
 				/* allocate a voice */
 				struct snd_emu10k1_voice *hwvoice;
-				if (snd_emu10k1_voice_alloc(hw, EMU10K1_SYNTH, 1, &hwvoice) < 0 || hwvoice == NULL)
+				if (snd_emu10k1_voice_alloc(hw, EMU10K1_SYNTH, 1, NULL, &hwvoice) < 0 || hwvoice == NULL)
 					continue;
 				vp->ch = hwvoice->number;
 				emu->num_voices++;
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index 63e085aa65fe..8bf0a4d8aaf1 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -95,36 +95,33 @@ static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voic
 	err = snd_emu10k1_voice_alloc(epcm->emu,
 				      epcm->type == PLAYBACK_EMUVOICE ? EMU10K1_PCM : EMU10K1_EFX,
 				      voices,
-				      &epcm->voices[0]);
+				      epcm, &epcm->voices[0]);
 	
 	if (err < 0)
 		return err;
-	epcm->voices[0]->epcm = epcm;
 	if (voices > 1) {
 		for (i = 1; i < voices; i++) {
 			epcm->voices[i] = &epcm->emu->voices[(epcm->voices[0]->number + i) % NUM_G];
-			epcm->voices[i]->epcm = epcm;
 		}
 	}
 	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_IRQ : EMU10K1_EFX_IRQ,
 					      1,
-					      &epcm->extra);
+					      epcm, &epcm->extra);
 		if (err < 0) {
 			/*
 			dev_dbg(emu->card->dev, "pcm_channel_alloc: "
 			       "failed extra: voices=%d, frame=%d\n",
 			       voices, frame);
 			*/
 			snd_emu10k1_pcm_free_voices(epcm);
 			return err;
 		}
-		epcm->extra->epcm = epcm;
 		epcm->extra->interrupt = snd_emu10k1_pcm_interrupt;
 	}
 
diff --git a/sound/pci/emu10k1/voice.c b/sound/pci/emu10k1/voice.c
index 25e78fc188bf..6c58e3bd14f7 100644
--- a/sound/pci/emu10k1/voice.c
+++ b/sound/pci/emu10k1/voice.c
@@ -32,7 +32,7 @@
  */
 
 static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
-		       struct snd_emu10k1_voice **rvoice)
+		       struct snd_emu10k1_pcm *epcm, struct snd_emu10k1_voice **rvoice)
 {
 	struct snd_emu10k1_voice *voice;
 	int i, j, k, first_voice, last_voice, skip;
@@ -79,6 +79,7 @@ static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 		       voice->number, idx-first_voice+1, number);
 		*/
 		voice->use = type;
+		voice->epcm = epcm;
 	}
 	*rvoice = &emu->voices[first_voice];
 	return 0;
@@ -95,19 +96,19 @@ static void voice_free(struct snd_emu10k1 *emu,
 }
 
 int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int number,
-			    struct snd_emu10k1_voice **rvoice)
+			    struct snd_emu10k1_pcm *epcm, struct snd_emu10k1_voice **rvoice)
 {
 	unsigned long flags;
 	int result;
 
 	if (snd_BUG_ON(!rvoice))
 		return -EINVAL;
 	if (snd_BUG_ON(!number))
 		return -EINVAL;
 
 	spin_lock_irqsave(&emu->voice_lock, flags);
 	for (;;) {
-		result = voice_alloc(emu, type, number, rvoice);
+		result = voice_alloc(emu, type, number, epcm, rvoice);
 		if (result == 0 || type == EMU10K1_SYNTH)
 			break;
 
-- 
2.40.0.152.g15d061e6df

[PATCH 7/7] ALSA: emu10k1: revamp playback voice allocator

[Oswald Buddenhagen]

Instead of separate voices, we now allocate non-interleaved channels,
which may in turn contain two interleaved voices each. The higher-level
code keeps only one pointer per channel. The channels are not allocated
in one block any more, as there is no reason to do that. As a
consequence of that, and because it is cleaner regardless, we now let
the allocator store these pointers at a specified location, rather than
returning only the first one and having the calling code deduce the
remaining ones.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h              |   3 +-
 sound/pci/emu10k1/emu10k1_callback.c |   2 +-
 sound/pci/emu10k1/emumixer.c         |  24 +++---
 sound/pci/emu10k1/emupcm.c           |  44 ++++++-----
 sound/pci/emu10k1/emuproc.c          |   5 +-
 sound/pci/emu10k1/voice.c            | 110 ++++++++++++++-------------
 6 files changed, 97 insertions(+), 91 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 42959ba9c9f4..50fb242656f7 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1452,6 +1452,7 @@ struct snd_emu10k1_voice {
 	unsigned char number;
 	unsigned char use;
 	unsigned char dirty;
+	unsigned char last;
 	void (*interrupt)(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *pvoice);
 
 	struct snd_emu10k1_pcm *epcm;
@@ -1848,7 +1849,7 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me
 int snd_emu10k1_memblk_map(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk);
 
 /* voice allocation */
-int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int pair,
+int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int count, int channels,
 			    struct snd_emu10k1_pcm *epcm, struct snd_emu10k1_voice **rvoice);
 int snd_emu10k1_voice_free(struct snd_emu10k1 *emu, struct snd_emu10k1_voice *pvoice);
 
diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index e256d3c22ee4..5691f2b610c4 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -282,7 +282,7 @@ get_voice(struct snd_emux *emu, struct snd_emux_port *port)
 			if (vp->ch < 0) {
 				/* allocate a voice */
 				struct snd_emu10k1_voice *hwvoice;
-				if (snd_emu10k1_voice_alloc(hw, EMU10K1_SYNTH, 1, NULL, &hwvoice) < 0 || hwvoice == NULL)
+				if (snd_emu10k1_voice_alloc(hw, EMU10K1_SYNTH, 1, 1, NULL, &hwvoice) < 0)
 					continue;
 				vp->ch = hwvoice->number;
 				emu->num_voices++;
diff --git a/sound/pci/emu10k1/emumixer.c b/sound/pci/emu10k1/emumixer.c
index 183051e846f2..a7dd88404ae1 100644
--- a/sound/pci/emu10k1/emumixer.c
+++ b/sound/pci/emu10k1/emumixer.c
@@ -1467,13 +1467,13 @@ static int snd_emu10k1_send_routing_put(struct snd_kcontrol *kcontrol,
 				change = 1;
 			}
 		}	
-	if (change && mix->epcm) {
-		if (mix->epcm->voices[0] && mix->epcm->voices[1]) {
+	if (change && mix->epcm && mix->epcm->voices[0]) {
+		if (!mix->epcm->voices[0]->last) {
 			update_emu10k1_fxrt(emu, mix->epcm->voices[0]->number,
 					    &mix->send_routing[1][0]);
-			update_emu10k1_fxrt(emu, mix->epcm->voices[1]->number,
+			update_emu10k1_fxrt(emu, mix->epcm->voices[0]->number + 1,
 					    &mix->send_routing[2][0]);
-		} else if (mix->epcm->voices[0]) {
+		} else {
 			update_emu10k1_fxrt(emu, mix->epcm->voices[0]->number,
 					    &mix->send_routing[0][0]);
 		}
@@ -1535,13 +1535,13 @@ static int snd_emu10k1_send_volume_put(struct snd_kcontrol *kcontrol,
 			change = 1;
 		}
 	}
-	if (change && mix->epcm) {
-		if (mix->epcm->voices[0] && mix->epcm->voices[1]) {
+	if (change && mix->epcm && mix->epcm->voices[0]) {
+		if (!mix->epcm->voices[0]->last) {
 			update_emu10k1_send_volume(emu, mix->epcm->voices[0]->number,
 						   &mix->send_volume[1][0]);
-			update_emu10k1_send_volume(emu, mix->epcm->voices[1]->number,
+			update_emu10k1_send_volume(emu, mix->epcm->voices[0]->number + 1,
 						   &mix->send_volume[2][0]);
-		} else if (mix->epcm->voices[0]) {
+		} else {
 			update_emu10k1_send_volume(emu, mix->epcm->voices[0]->number,
 						   &mix->send_volume[0][0]);
 		}
@@ -1601,11 +1601,11 @@ static int snd_emu10k1_attn_put(struct snd_kcontrol *kcontrol,
 			change = 1;
 		}
 	}
-	if (change && mix->epcm) {
-		if (mix->epcm->voices[0] && mix->epcm->voices[1]) {
+	if (change && mix->epcm && mix->epcm->voices[0]) {
+		if (!mix->epcm->voices[0]->last) {
 			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[0]->number, mix->attn[1]);
-			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[1]->number, mix->attn[2]);
-		} else if (mix->epcm->voices[0]) {
+			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[0]->number + 1, mix->attn[2]);
+		} else {
 			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[0]->number, mix->attn[0]);
 		}
 	}
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index 8bf0a4d8aaf1..ab3f81dd439e 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -86,32 +86,26 @@ static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm)
 	}
 }
 
-static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices)
+static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm *epcm,
+					 int type, int count, int channels)
 {
-	int err, i;
+	int err;
 
 	snd_emu10k1_pcm_free_voices(epcm);
 
 	err = snd_emu10k1_voice_alloc(epcm->emu,
-				      epcm->type == PLAYBACK_EMUVOICE ? EMU10K1_PCM : EMU10K1_EFX,
-				      voices,
+				      type, count, channels,
 				      epcm, &epcm->voices[0]);
-	
 	if (err < 0)
 		return err;
-	if (voices > 1) {
-		for (i = 1; i < voices; i++) {
-			epcm->voices[i] = &epcm->emu->voices[(epcm->voices[0]->number + i) % NUM_G];
-		}
-	}
+
 	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_IRQ : EMU10K1_EFX_IRQ,
-					      1,
+					      type + 1, 1, 1,
 					      epcm, &epcm->extra);
 		if (err < 0) {
 			/*
@@ -308,9 +302,19 @@ static int snd_emu10k1_playback_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_emu10k1_pcm *epcm = runtime->private_data;
 	size_t alloc_size;
+	int type, channels, count;
 	int err;
 
-	err = snd_emu10k1_pcm_channel_alloc(epcm, params_channels(hw_params));
+	if (epcm->type == PLAYBACK_EMUVOICE) {
+		type = EMU10K1_PCM;
+		channels = 1;
+		count = params_channels(hw_params);
+	} else {
+		type = EMU10K1_EFX;
+		channels = params_channels(hw_params);
+		count = 1;
+	}
+	err = snd_emu10k1_pcm_channel_alloc(epcm, type, count, channels);
 	if (err < 0)
 		return err;
 
@@ -380,8 +384,8 @@ static int snd_emu10k1_playback_prepare(struct snd_pcm_substream *substream)
 	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], w_16, true,
+	if (stereo)
+		snd_emu10k1_pcm_init_voice(emu, 0, 0, epcm->voices[0] + 1, w_16, true,
 					   start_addr, end_addr,
 					   &emu->pcm_mixer[substream->number]);
 	return 0;
@@ -572,18 +576,14 @@ static void snd_emu10k1_playback_unmute_voice(struct snd_emu10k1 *emu,
 	unsigned int vattn;
 	unsigned int tmp;
 
-	if (evoice == NULL)	/* skip second voice for mono */
-		return;
 	tmp = stereo ? (master ? 1 : 2) : 0;
 	vattn = mix->attn[tmp] << 16;
 	snd_emu10k1_playback_commit_volume(emu, evoice, vattn);
 }	
 
 static void snd_emu10k1_playback_mute_voice(struct snd_emu10k1 *emu,
 					    struct snd_emu10k1_voice *evoice)
 {
-	if (evoice == NULL)
-		return;
 	snd_emu10k1_playback_commit_volume(emu, evoice, 0);
 }
 
@@ -655,19 +655,21 @@ static int snd_emu10k1_playback_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_RESUME:
 		mix = &emu->pcm_mixer[substream->number];
 		snd_emu10k1_playback_unmute_voice(emu, epcm->voices[0], stereo, true, mix);
-		snd_emu10k1_playback_unmute_voice(emu, epcm->voices[1], stereo, false, mix);
+		if (stereo)
+			snd_emu10k1_playback_unmute_voice(emu, epcm->voices[0] + 1, true, 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);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		snd_emu10k1_playback_stop_voice(emu, epcm->voices[0]);
 		snd_emu10k1_playback_stop_voice(emu, epcm->extra);
 		snd_emu10k1_playback_set_stopped(emu, epcm);
 		snd_emu10k1_playback_mute_voice(emu, epcm->voices[0]);
-		snd_emu10k1_playback_mute_voice(emu, epcm->voices[1]);
+		if (stereo)
+			snd_emu10k1_playback_mute_voice(emu, epcm->voices[0] + 1);
 		break;
 	default:
 		result = -EINVAL;
diff --git a/sound/pci/emu10k1/emuproc.c b/sound/pci/emu10k1/emuproc.c
index abcec8a01760..89ea3adff322 100644
--- a/sound/pci/emu10k1/emuproc.c
+++ b/sound/pci/emu10k1/emuproc.c
@@ -372,12 +372,13 @@ static void snd_emu10k1_proc_voices_read(struct snd_info_entry *entry,
 	};
 	static_assert(ARRAY_SIZE(types) == EMU10K1_NUM_TYPES);
 
-	snd_iprintf(buffer, "ch\tdirty\tuse\n");
+	snd_iprintf(buffer, "ch\tdirty\tlast\tuse\n");
 	for (idx = 0; idx < NUM_G; idx++) {
 		voice = &emu->voices[idx];
-		snd_iprintf(buffer, "%i\t%u\t%s\n",
+		snd_iprintf(buffer, "%i\t%u\t%u\t%s\n",
 			idx,
 			voice->dirty,
+			voice->last,
 			types[voice->use]);
 	}
 }
diff --git a/sound/pci/emu10k1/voice.c b/sound/pci/emu10k1/voice.c
index 6c58e3bd14f7..6939498e26f0 100644
--- a/sound/pci/emu10k1/voice.c
+++ b/sound/pci/emu10k1/voice.c
@@ -23,103 +23,101 @@
  * allocator uses a round robin scheme.  The next free voice is tracked in 
  * the card record and each allocation begins where the last left off.  The 
  * hardware requires stereo interleaved voices be aligned to an even/odd 
- * boundary.  For multichannel voice allocation we ensure than the block of 
- * voices does not cross the 32 voice boundary.  This simplifies the 
- * multichannel support and ensures we can use a single write to the 
- * (set|clear)_loop_stop registers.  Otherwise (for example) the voices would 
- * get out of sync when pausing/resuming a stream.
+ * boundary.
  *							--rlrevell
  */
 
 static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 		       struct snd_emu10k1_pcm *epcm, struct snd_emu10k1_voice **rvoice)
 {
 	struct snd_emu10k1_voice *voice;
-	int i, j, k, first_voice, last_voice, skip;
+	int i, j, k, skip;
 
-	*rvoice = NULL;
-	first_voice = last_voice = 0;
-	for (i = emu->next_free_voice, j = 0; j < NUM_G ; i += number, j += number) {
+	for (i = emu->next_free_voice, j = 0; j < NUM_G; i = (i + skip) % NUM_G, j += skip) {
 		/*
 		dev_dbg(emu->card->dev, "i %d j %d next free %d!\n",
 		       i, j, emu->next_free_voice);
 		*/
-		i %= NUM_G;
 
 		/* stereo voices must be even/odd */
-		if ((number == 2) && (i % 2)) {
-			i++;
+		if ((number > 1) && (i % 2)) {
+			skip = 1;
 			continue;
 		}
-			
-		skip = 0;
+
 		for (k = 0; k < number; k++) {
-			voice = &emu->voices[(i+k) % NUM_G];
+			voice = &emu->voices[i + k];
 			if (voice->use) {
-				skip = 1;
-				break;
+				skip = k + 1;
+				goto next;
 			}
 		}
-		if (!skip) {
-			/* dev_dbg(emu->card->dev, "allocated voice %d\n", i); */
-			first_voice = i;
-			last_voice = (i + number) % NUM_G;
-			emu->next_free_voice = last_voice;
-			break;
+
+		for (k = 0; k < number; k++) {
+			voice = &emu->voices[i + k];
+			voice->use = type;
+			voice->epcm = epcm;
+			/* dev_dbg(emu->card->dev, "allocated voice %d\n", i + k); */
 		}
+		voice->last = 1;
+
+		*rvoice = &emu->voices[i];
+		emu->next_free_voice = (i + number) % NUM_G;
+		return 0;
+
+	next: ;
 	}
-	
-	if (first_voice == last_voice)
-		return -ENOMEM;
-	
-	for (i = 0; i < number; i++) {
-		voice = &emu->voices[(first_voice + i) % NUM_G];
-		/*
-		dev_dbg(emu->card->dev, "voice alloc - %i, %i of %i\n",
-		       voice->number, idx-first_voice+1, number);
-		*/
-		voice->use = type;
-		voice->epcm = epcm;
-	}
-	*rvoice = &emu->voices[first_voice];
-	return 0;
+	return -ENOMEM;  // -EBUSY would have been better
 }
 
 static void voice_free(struct snd_emu10k1 *emu,
 		       struct snd_emu10k1_voice *pvoice)
 {
 	if (pvoice->dirty)
 		snd_emu10k1_voice_init(emu, pvoice->number);
 	pvoice->interrupt = NULL;
-	pvoice->use = pvoice->dirty = 0;
+	pvoice->use = pvoice->dirty = pvoice->last = 0;
 	pvoice->epcm = NULL;
 }
 
-int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int number,
+int snd_emu10k1_voice_alloc(struct snd_emu10k1 *emu, int type, int count, int channels,
 			    struct snd_emu10k1_pcm *epcm, struct snd_emu10k1_voice **rvoice)
 {
 	unsigned long flags;
 	int result;
 
 	if (snd_BUG_ON(!rvoice))
 		return -EINVAL;
-	if (snd_BUG_ON(!number))
+	if (snd_BUG_ON(!count))
+		return -EINVAL;
+	if (snd_BUG_ON(!channels))
 		return -EINVAL;
 
 	spin_lock_irqsave(&emu->voice_lock, flags);
-	for (;;) {
-		result = voice_alloc(emu, type, number, epcm, rvoice);
-		if (result == 0 || type == EMU10K1_SYNTH)
-			break;
-
-		/* free a voice from synth */
-		if (emu->get_synth_voice) {
-			result = emu->get_synth_voice(emu);
-			if (result >= 0)
-				voice_free(emu, &emu->voices[result]);
+	for (int got = 0; got < channels; ) {
+		result = voice_alloc(emu, type, count, epcm, &rvoice[got]);
+		if (result == 0) {
+			got++;
+			/*
+			dev_dbg(emu->card->dev, "voice alloc - %i, %i of %i\n",
+			        rvoice[got - 1]->number, got, want);
+			*/
+			continue;
 		}
-		if (result < 0)
-			break;
+		if (type != EMU10K1_SYNTH && emu->get_synth_voice) {
+			/* free a voice from synth */
+			result = emu->get_synth_voice(emu);
+			if (result >= 0) {
+				voice_free(emu, &emu->voices[result]);
+				continue;
+			}
+		}
+		for (int i = 0; i < got; i++) {
+			for (int j = 0; j < count; j++)
+				voice_free(emu, rvoice[i] + j);
+			rvoice[i] = NULL;
+		}
+		break;
 	}
 	spin_unlock_irqrestore(&emu->voice_lock, flags);
 
@@ -132,11 +130,15 @@ int snd_emu10k1_voice_free(struct snd_emu10k1 *emu,
 			   struct snd_emu10k1_voice *pvoice)
 {
 	unsigned long flags;
+	int last;
 
 	if (snd_BUG_ON(!pvoice))
 		return -EINVAL;
 	spin_lock_irqsave(&emu->voice_lock, flags);
-	voice_free(emu, pvoice);
+	do {
+		last = pvoice->last;
+		voice_free(emu, pvoice++);
+	} while (!last);
 	spin_unlock_irqrestore(&emu->voice_lock, flags);
 	return 0;
 }
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 0/7] ALSA: emu10k1: refactoring of the playback voice management

[Takashi Iwai]

On Thu, 18 May 2023 16:09:40 +0200,
Oswald Buddenhagen wrote:
> 
> 
> Oswald Buddenhagen (7):
>   ALSA: emu10k1: simplify freeing synth voices
>   ALSA: emu10k1: don't forget to reset reclaimed synth voices
>   ALSA: emu10k1: improve voice status display in /proc
>   ALSA: emu10k1: make freeing untouched playback voices cheap
>   ALSA: emu10k1: centralize freeing PCM voices
>   ALSA: emu10k1: make snd_emu10k1_voice_alloc() assign voices' epcm
>   ALSA: emu10k1: revamp playback voice allocator

I applied partially, patches 1-4.
For the rest, please resubmit with corrections.


thanks,

Takashi