diff mbox series

ALSA: emu10k1: macro-ize snd_emu10k1_ptr_{read,write}()

Message ID 20230428095941.1706263-1-oswald.buddenhagen@gmx.de (mailing list archive)
State Superseded
Headers show
Series ALSA: emu10k1: macro-ize snd_emu10k1_ptr_{read,write}() | expand

Commit Message

Oswald Buddenhagen April 28, 2023, 9:59 a.m. UTC 
The idea to encode the bitfield manipulation in the register address is
quite clever, but it's somewhat wasteful to do these calculations at
runtime, given that they are all constants. Change that.

The added bitfield manipulation macros will be used separately as well.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h     |  29 ++++++++-
 sound/pci/emu10k1/emupcm.c  |  31 +++++-----
 sound/pci/emu10k1/emuproc.c |   4 +-
 sound/pci/emu10k1/io.c      | 116 +++++++++++++++++-------------------
 4 files changed, 99 insertions(+), 81 deletions(-)

Comments

Takashi Iwai April 28, 2023, 10:32 a.m. UTC | #1 
On Fri, 28 Apr 2023 11:59:41 +0200,
Oswald Buddenhagen wrote:
> 
> The idea to encode the bitfield manipulation in the register address is
> quite clever, but it's somewhat wasteful to do these calculations at
> runtime, given that they are all constants. Change that.
> 
> The added bitfield manipulation macros will be used separately as well.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

JFYI, I postpone applying this and the rest emu10k1 changes, since
it's in a merge window now.  They will be reviewed and merged for 6.5
after the 6.4 merge window.

The patch "[PATCH RESEND] ALSA: emu10k1: use more existing defines
instead of open-coded numbers" was already taken as it was a resent
patch.

So, if you have patches that fix bugs and should be applied soonish,
submit those.


thanks,

Takashi
Takashi Iwai May 8, 2023, 7:14 a.m. UTC | #2 
On Fri, 28 Apr 2023 11:59:41 +0200,
Oswald Buddenhagen wrote:
> +#define snd_emu10k1_ptr_read(emu, reg, voice) \
> +	({ \
> +		u32 data = snd_emu10k1_ptr_read_raw(emu, REG_ADDR(reg, voice)); \
> +		if (REG_SIZE(reg)) \
> +			data = REG_VAL_GET(reg, data); \
> +		data; \
> +	})
> +#define snd_emu10k1_ptr_write(emu, reg, voice, data) \
> +	do { \
> +		if (REG_SIZE(reg)) \
> +			snd_emu10k1_ptr_modify(emu, REG_ADDR(reg, voice), \
> +					       ~REG_MASK(reg), REG_VAL_PUT(reg, data)); \
> +		else \
> +			snd_emu10k1_ptr_write_raw(emu, REG_ADDR(reg, voice), data); \
> +	} while (0)

Must those be macros?  Not only that macro isn't really safe to use
for obvious reasons, the expansion would cost significantly as they
are called in many places.

> --- a/sound/pci/emu10k1/io.c
> +++ b/sound/pci/emu10k1/io.c
> @@ -18,72 +18,64 @@
>  #include <linux/export.h>
>  #include "p17v.h"
>  
> -unsigned int snd_emu10k1_ptr_read(struct snd_emu10k1 * emu, unsigned int reg, unsigned int chn)
> +static inline int check_ptr_reg(struct snd_emu10k1 *emu, u32 reg)

Should be bool.


thanks,

Takashi
Oswald Buddenhagen May 8, 2023, 11:14 a.m. UTC | #3 
On Mon, May 08, 2023 at 09:14:01AM +0200, Takashi Iwai wrote:
>Must those be macros?
>
yes. at least with the additional checks i've added in v2.

>Not only that macro isn't really safe to use for obvious reasons,
>
i don't see anything obviously bad. please be specific.

>the expansion would cost significantly as they are called in many 
>places.
>
after i fixed the calls with a non-const register in v2, the code size 
increases by less than 400 bytes (with ~95 call sites). i'd say that's 
negligible.

regards
Takashi Iwai May 8, 2023, 11:40 a.m. UTC | #4 
On Mon, 08 May 2023 13:14:12 +0200,
Oswald Buddenhagen wrote:
> 
> On Mon, May 08, 2023 at 09:14:01AM +0200, Takashi Iwai wrote:
> > Must those be macros?
> > 
> yes. at least with the additional checks i've added in v2.

Hmm, it's not clear why it must be a macro.  Isn't it a snd_BUG_ON()
and the check itself is non-inlined, right?

> > Not only that macro isn't really safe to use for obvious reasons,
> > 
> i don't see anything obviously bad. please be specific.

It's a generic problem of a macro; you're referring the arguments that
are expanded in multiple times.  At least, it could be a static
inline?

> > the expansion would cost significantly as they are called in many
> > places.
> > 
> after i fixed the calls with a non-const register in v2, the code size
> increases by less than 400 bytes (with ~95 call sites). i'd say that's
> negligible.

But it's more.  I'd like to know why those have to be expanded
inevitably with a macro with that cost, and if any, it should be
described.


Takashi
diff mbox series

Patch

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 8fe80dcee71b..fd1fe49578ce 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1773,8 +1773,33 @@  int snd_emu10k1_fx8010_tram_setup(struct snd_emu10k1 *emu, u32 size);
 int snd_emu10k1_done(struct snd_emu10k1 * emu);
 
 /* I/O functions */
-unsigned int snd_emu10k1_ptr_read(struct snd_emu10k1 * emu, unsigned int reg, unsigned int chn);
-void snd_emu10k1_ptr_write(struct snd_emu10k1 *emu, unsigned int reg, unsigned int chn, unsigned int data);
+
+#define REG_SHIFT(r) (((r) >> 16) & 0x1f)
+#define REG_SIZE(r) (((r) >> 24) & 0x1f)
+#define REG_MASK0(r) ((1U << REG_SIZE(r)) - 1U)
+#define REG_MASK(r) (REG_MASK0(r) << REG_SHIFT(r))
+#define REG_ADDR(r, v) ((((r) & 0xffff) << 16) | (v))
+#define REG_VAL_GET(r, v) ((v & REG_MASK(r)) >> REG_SHIFT(r))
+#define REG_VAL_PUT(r, v) ((v) << REG_SHIFT(r))
+#define snd_emu10k1_ptr_read(emu, reg, voice) \
+	({ \
+		u32 data = snd_emu10k1_ptr_read_raw(emu, REG_ADDR(reg, voice)); \
+		if (REG_SIZE(reg)) \
+			data = REG_VAL_GET(reg, data); \
+		data; \
+	})
+#define snd_emu10k1_ptr_write(emu, reg, voice, data) \
+	do { \
+		if (REG_SIZE(reg)) \
+			snd_emu10k1_ptr_modify(emu, REG_ADDR(reg, voice), \
+					       ~REG_MASK(reg), REG_VAL_PUT(reg, data)); \
+		else \
+			snd_emu10k1_ptr_write_raw(emu, REG_ADDR(reg, voice), data); \
+	} while (0)
+
+u32 snd_emu10k1_ptr_read_raw(struct snd_emu10k1 *emu, u32 reg);
+void snd_emu10k1_ptr_write_raw(struct snd_emu10k1 *emu, u32 reg, u32 data);
+void snd_emu10k1_ptr_modify(struct snd_emu10k1 *emu, u32 reg, u32 and_mask, u32 or_mask);
 unsigned int snd_emu10k1_ptr20_read(struct snd_emu10k1 * emu, unsigned int reg, unsigned int chn);
 void snd_emu10k1_ptr20_write(struct snd_emu10k1 *emu, unsigned int reg, unsigned int chn, unsigned int data);
 int snd_emu10k1_spi_write(struct snd_emu10k1 * emu, unsigned int data);
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index b0c0ef342756..2baa7f01eb5b 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -528,7 +528,7 @@  static int snd_emu10k1_capture_prepare(struct snd_pcm_substream *substream)
 	int idx;
 
 	/* zeroing the buffer size will stop capture */
-	snd_emu10k1_ptr_write(emu, epcm->capture_bs_reg, 0, 0);
+	snd_emu10k1_ptr_write_raw(emu, epcm->capture_bs_reg, 0);
 	switch (epcm->type) {
 	case CAPTURE_AC97ADC:
 		snd_emu10k1_ptr_write(emu, ADCCR, 0, 0);
@@ -543,7 +543,7 @@  static int snd_emu10k1_capture_prepare(struct snd_pcm_substream *substream)
 	default:
 		break;
 	}	
-	snd_emu10k1_ptr_write(emu, epcm->capture_ba_reg, 0, runtime->dma_addr);
+	snd_emu10k1_ptr_write_raw(emu, epcm->capture_ba_reg, runtime->dma_addr);
 	epcm->capture_bufsize = snd_pcm_lib_buffer_bytes(substream);
 	epcm->capture_bs_val = 0;
 	for (idx = 0; idx < 31; idx++) {
@@ -771,16 +771,16 @@  static int snd_emu10k1_capture_trigger(struct snd_pcm_substream *substream,
 		default:	
 			break;
 		}
-		snd_emu10k1_ptr_write(emu, epcm->capture_bs_reg, 0, epcm->capture_bs_val);
+		snd_emu10k1_ptr_write_raw(emu, epcm->capture_bs_reg, epcm->capture_bs_val);
 		epcm->running = 1;
 		epcm->first_ptr = 1;
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		epcm->running = 0;
 		snd_emu10k1_intr_disable(emu, epcm->capture_inte);
 		outl(epcm->capture_ipr, emu->port + IPR);
-		snd_emu10k1_ptr_write(emu, epcm->capture_bs_reg, 0, 0);
+		snd_emu10k1_ptr_write_raw(emu, epcm->capture_bs_reg, 0);
 		switch (epcm->type) {
 		case CAPTURE_AC97ADC:
 			snd_emu10k1_ptr_write(emu, ADCCR, 0, 0);
@@ -812,7 +812,7 @@  static snd_pcm_uframes_t snd_emu10k1_playback_pointer(struct snd_pcm_substream *
 
 	if (!epcm->running)
 		return 0;
-	ptr = snd_emu10k1_ptr_read(emu, CCCA, epcm->voices[0]->number) & 0x00ffffff;
+	ptr = snd_emu10k1_ptr_read(emu, CCCA_CURRADDR, epcm->voices[0]->number);
 #if 0	/* Perex's code */
 	ptr += runtime->buffer_size;
 	ptr -= epcm->ccca_start_addr;
@@ -899,7 +899,8 @@  static snd_pcm_uframes_t snd_emu10k1_capture_pointer(struct snd_pcm_substream *s
 		udelay(50);	/* hack, it takes awhile until capture is started */
 		epcm->first_ptr = 0;
 	}
-	ptr = snd_emu10k1_ptr_read(emu, epcm->capture_idx_reg, 0) & 0x0000ffff;
+	ptr = REG_VAL_GET(FXIDX_IDX,  // All *IDX_MASKs are assumed to be equal
+			snd_emu10k1_ptr_read_raw(emu, epcm->capture_idx_reg));
 	return bytes_to_frames(runtime, ptr);
 }
 
@@ -1128,9 +1129,9 @@  static int snd_emu10k1_capture_open(struct snd_pcm_substream *substream)
 	epcm->substream = substream;
 	epcm->capture_ipr = IPR_ADCBUFFULL|IPR_ADCBUFHALFFULL;
 	epcm->capture_inte = INTE_ADCBUFENABLE;
-	epcm->capture_ba_reg = ADCBA;
-	epcm->capture_bs_reg = ADCBS;
-	epcm->capture_idx_reg = emu->audigy ? A_ADCIDX : ADCIDX;
+	epcm->capture_ba_reg = REG_ADDR(ADCBA, 0);
+	epcm->capture_bs_reg = REG_ADDR(ADCBS, 0);
+	epcm->capture_idx_reg = emu->audigy ? REG_ADDR(A_ADCIDX, 0) : REG_ADDR(ADCIDX, 0);
 	runtime->private_data = epcm;
 	runtime->private_free = snd_emu10k1_pcm_free_substream;
 	runtime->hw = snd_emu10k1_capture;
@@ -1164,9 +1165,9 @@  static int snd_emu10k1_capture_mic_open(struct snd_pcm_substream *substream)
 	epcm->substream = substream;
 	epcm->capture_ipr = IPR_MICBUFFULL|IPR_MICBUFHALFFULL;
 	epcm->capture_inte = INTE_MICBUFENABLE;
-	epcm->capture_ba_reg = MICBA;
-	epcm->capture_bs_reg = MICBS;
-	epcm->capture_idx_reg = emu->audigy ? A_MICIDX : MICIDX;
+	epcm->capture_ba_reg = REG_ADDR(MICBA, 0);
+	epcm->capture_bs_reg = REG_ADDR(MICBS, 0);
+	epcm->capture_idx_reg = emu->audigy ? REG_ADDR(A_MICIDX, 0) : REG_ADDR(MICIDX, 0);
 	substream->runtime->private_data = epcm;
 	substream->runtime->private_free = snd_emu10k1_pcm_free_substream;
 	runtime->hw = snd_emu10k1_capture;
@@ -1204,9 +1205,9 @@  static int snd_emu10k1_capture_efx_open(struct snd_pcm_substream *substream)
 	epcm->substream = substream;
 	epcm->capture_ipr = IPR_EFXBUFFULL|IPR_EFXBUFHALFFULL;
 	epcm->capture_inte = INTE_EFXBUFENABLE;
-	epcm->capture_ba_reg = FXBA;
-	epcm->capture_bs_reg = FXBS;
-	epcm->capture_idx_reg = FXIDX;
+	epcm->capture_ba_reg = REG_ADDR(FXBA, 0);
+	epcm->capture_bs_reg = REG_ADDR(FXBS, 0);
+	epcm->capture_idx_reg = REG_ADDR(FXIDX, 0);
 	substream->runtime->private_data = epcm;
 	substream->runtime->private_free = snd_emu10k1_pcm_free_substream;
 	runtime->hw = snd_emu10k1_capture_efx;
diff --git a/sound/pci/emu10k1/emuproc.c b/sound/pci/emu10k1/emuproc.c
index bec72dc60a41..3a7f60d64c27 100644
--- a/sound/pci/emu10k1/emuproc.c
+++ b/sound/pci/emu10k1/emuproc.c
@@ -23,8 +23,8 @@ 
 static void snd_emu10k1_proc_spdif_status(struct snd_emu10k1 * emu,
 					  struct snd_info_buffer *buffer,
 					  char *title,
-					  int status_reg,
-					  int rate_reg)
+					  u8 status_reg,
+					  u8 rate_reg)
 {
 	static const char * const clkaccy[4] = { "1000ppm", "50ppm", "variable", "unknown" };
 	static const int samplerate[16] = { 44100, 1, 48000, 32000, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
diff --git a/sound/pci/emu10k1/io.c b/sound/pci/emu10k1/io.c
index c60573f14ea8..373c633bd88b 100644
--- a/sound/pci/emu10k1/io.c
+++ b/sound/pci/emu10k1/io.c
@@ -18,72 +18,64 @@ 
 #include <linux/export.h>
 #include "p17v.h"
 
-unsigned int snd_emu10k1_ptr_read(struct snd_emu10k1 * emu, unsigned int reg, unsigned int chn)
+static inline int check_ptr_reg(struct snd_emu10k1 *emu, u32 reg)
 {
-	unsigned long flags;
-	unsigned int regptr, val;
-	unsigned int mask;
-
-	mask = emu->audigy ? A_PTR_ADDRESS_MASK : PTR_ADDRESS_MASK;
-	regptr = ((reg << 16) & mask) | (chn & PTR_CHANNELNUM_MASK);
-
-	if (reg & 0xff000000) {
-		unsigned char size, offset;
-		
-		size = (reg >> 24) & 0x3f;
-		offset = (reg >> 16) & 0x1f;
-		mask = ((1 << size) - 1) << offset;
-		
-		spin_lock_irqsave(&emu->emu_lock, flags);
-		outl(regptr, emu->port + PTR);
-		val = inl(emu->port + DATA);
-		spin_unlock_irqrestore(&emu->emu_lock, flags);
-		
-		return (val & mask) >> offset;
-	} else {
-		spin_lock_irqsave(&emu->emu_lock, flags);
-		outl(regptr, emu->port + PTR);
-		val = inl(emu->port + DATA);
-		spin_unlock_irqrestore(&emu->emu_lock, flags);
-		return val;
-	}
-}
-
-EXPORT_SYMBOL(snd_emu10k1_ptr_read);
-
-void snd_emu10k1_ptr_write(struct snd_emu10k1 *emu, unsigned int reg, unsigned int chn, unsigned int data)
-{
-	unsigned int regptr;
-	unsigned long flags;
-	unsigned int mask;
-
 	if (snd_BUG_ON(!emu))
-		return;
-	mask = emu->audigy ? A_PTR_ADDRESS_MASK : PTR_ADDRESS_MASK;
-	regptr = ((reg << 16) & mask) | (chn & PTR_CHANNELNUM_MASK);
-
-	if (reg & 0xff000000) {
-		unsigned char size, offset;
-
-		size = (reg >> 24) & 0x3f;
-		offset = (reg >> 16) & 0x1f;
-		mask = ((1 << size) - 1) << offset;
-		data = (data << offset) & mask;
-
-		spin_lock_irqsave(&emu->emu_lock, flags);
-		outl(regptr, emu->port + PTR);
-		data |= inl(emu->port + DATA) & ~mask;
-		outl(data, emu->port + DATA);
-		spin_unlock_irqrestore(&emu->emu_lock, flags);		
-	} else {
-		spin_lock_irqsave(&emu->emu_lock, flags);
-		outl(regptr, emu->port + PTR);
-		outl(data, emu->port + DATA);
-		spin_unlock_irqrestore(&emu->emu_lock, flags);
-	}
+		return 0;
+	if (snd_BUG_ON(reg & 0xffff0000 & (emu->audigy ? ~A_PTR_ADDRESS_MASK : ~PTR_ADDRESS_MASK)))
+		return 0;
+	if (snd_BUG_ON(reg & 0xffff & ~PTR_CHANNELNUM_MASK))
+		return 0;
+	return 1;
 }
 
-EXPORT_SYMBOL(snd_emu10k1_ptr_write);
+unsigned int snd_emu10k1_ptr_read_raw(struct snd_emu10k1 *emu, u32 reg)
+{
+	unsigned long flags;
+	u32 val;
+
+	if (!check_ptr_reg(emu, reg))
+		return 0;
+
+	spin_lock_irqsave(&emu->emu_lock, flags);
+	outl(reg, emu->port + PTR);
+	val = inl(emu->port + DATA);
+	spin_unlock_irqrestore(&emu->emu_lock, flags);
+	return val;
+}
+
+EXPORT_SYMBOL(snd_emu10k1_ptr_read_raw);
+
+void snd_emu10k1_ptr_write_raw(struct snd_emu10k1 *emu, u32 reg, u32 data)
+{
+	unsigned long flags;
+
+	if (!check_ptr_reg(emu, reg))
+		return;
+
+	spin_lock_irqsave(&emu->emu_lock, flags);
+	outl(reg, emu->port + PTR);
+	outl(data, emu->port + DATA);
+	spin_unlock_irqrestore(&emu->emu_lock, flags);
+}
+
+EXPORT_SYMBOL(snd_emu10k1_ptr_write_raw);
+
+void snd_emu10k1_ptr_modify(struct snd_emu10k1 *emu, u32 reg, u32 and_mask, u32 or_mask)
+{
+	unsigned long flags;
+
+	if (!check_ptr_reg(emu, reg))
+		return;
+
+	spin_lock_irqsave(&emu->emu_lock, flags);
+	outl(reg, emu->port + PTR);
+	or_mask |= inl(emu->port + DATA) & and_mask;
+	outl(or_mask, emu->port + DATA);
+	spin_unlock_irqrestore(&emu->emu_lock, flags);
+}
+
+EXPORT_SYMBOL(snd_emu10k1_ptr_modify);
 
 unsigned int snd_emu10k1_ptr20_read(struct snd_emu10k1 * emu, 
 					  unsigned int reg,