[PATCH 00/18] ALSA: emu10k1 & emux: fixes related to wavetable playback
this fixes the regression i introduced (though arguably, i just made it broken in a different way), and then some more.
Oswald Buddenhagen (18): ALSA: emux: fix /proc teardown at module unload ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch() ALSA: emux: fix validation of snd_emux.num_ports ALSA: emux: fix init of patch_info.truesize in load_data() ALSA: emu10k1: prune vestiges of SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support ALSA: emux: centralize & improve patch info validation ALSA: emux: improve patch ioctl data validation ALSA: emu10k1: move patch loader assertions into low-level functions ALSA: emu10k1: fix sample signedness issues in wavetable loader ALSA: emu10k1: fix playback of 8-bit wavetable samples ALSA: emu10k1: make wavetable sample playback start position exact ALSA: emu10k1: shrink blank space in front of wavetable samples ALSA: emu10k1: merge conditions in patch loader ALSA: emu10k1: fix wavetable offset recalculation ALSA: emu10k1: de-duplicate size calculations for 16-bit samples ALSA: emu10k1: improve cache behavior documentation ALSA: emu10k1: fix playback of short wavetable samples ALSA: emux: simplify snd_sf_list.callback handling
include/sound/emu10k1.h | 32 +++-- include/sound/soundfont.h | 2 +- sound/isa/sb/emu8000_patch.c | 13 -- sound/pci/emu10k1/emu10k1_callback.c | 10 +- sound/pci/emu10k1/emu10k1_patch.c | 207 +++++++++++---------------- sound/pci/emu10k1/memory.c | 55 +++++-- sound/synth/emux/emux.c | 6 +- sound/synth/emux/emux_hwdep.c | 3 +- sound/synth/emux/emux_oss.c | 3 +- sound/synth/emux/emux_proc.c | 1 + sound/synth/emux/emux_seq.c | 6 +- sound/synth/emux/soundfont.c | 73 +++++++--- 12 files changed, 216 insertions(+), 195 deletions(-)
-- 2.42.0.419.g70bf8a5751
We forgot to remember the wavetable /proc entry, so we'd fail to free it at module unload.
This matters only when only the synth module is unloaded, as unloading the card driver would tear down the sub-entry anyway.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/synth/emux/emux_proc.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/synth/emux/emux_proc.c b/sound/synth/emux/emux_proc.c index 7993e6a01e54..820351f52551 100644 --- a/sound/synth/emux/emux_proc.c +++ b/sound/synth/emux/emux_proc.c @@ -102,6 +102,7 @@ void snd_emux_proc_init(struct snd_emux *emu, struct snd_card *card, int device) entry->content = SNDRV_INFO_CONTENT_TEXT; entry->private_data = emu; entry->c.text.read = snd_emux_proc_info_read; + emu->proc = entry; }
void snd_emux_proc_free(struct snd_emux *emu) -- 2.42.0.419.g70bf8a5751
The `client` parameter was not used, so eliminate it from the call chain.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- include/sound/soundfont.h | 2 +- sound/synth/emux/emux_hwdep.c | 3 +-- sound/synth/emux/emux_oss.c | 3 +-- sound/synth/emux/soundfont.c | 7 +++---- 4 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/include/sound/soundfont.h b/include/sound/soundfont.h index e445688a4f4f..98ed98d89d6d 100644 --- a/include/sound/soundfont.h +++ b/include/sound/soundfont.h @@ -89,7 +89,7 @@ struct snd_sf_list { int snd_soundfont_load(struct snd_sf_list *sflist, const void __user *data, long count, int client); int snd_soundfont_load_guspatch(struct snd_sf_list *sflist, const char __user *data, - long count, int client); + long count); int snd_soundfont_close_check(struct snd_sf_list *sflist, int client);
struct snd_sf_list *snd_sf_new(struct snd_sf_callback *callback, diff --git a/sound/synth/emux/emux_hwdep.c b/sound/synth/emux/emux_hwdep.c index 81719bfb8ed7..fd8f978cde1c 100644 --- a/sound/synth/emux/emux_hwdep.c +++ b/sound/synth/emux/emux_hwdep.c @@ -27,8 +27,7 @@ snd_emux_hwdep_load_patch(struct snd_emux *emu, void __user *arg)
if (patch.key == GUS_PATCH) return snd_soundfont_load_guspatch(emu->sflist, arg, - patch.len + sizeof(patch), - TMP_CLIENT_ID); + patch.len + sizeof(patch));
if (patch.type >= SNDRV_SFNT_LOAD_INFO && patch.type <= SNDRV_SFNT_PROBE_DATA) { diff --git a/sound/synth/emux/emux_oss.c b/sound/synth/emux/emux_oss.c index d8d32671f703..04df46b269d3 100644 --- a/sound/synth/emux/emux_oss.c +++ b/sound/synth/emux/emux_oss.c @@ -205,8 +205,7 @@ snd_emux_load_patch_seq_oss(struct snd_seq_oss_arg *arg, int format, return -ENXIO;
if (format == GUS_PATCH) - rc = snd_soundfont_load_guspatch(emu->sflist, buf, count, - SF_CLIENT_NO(p->chset.port)); + rc = snd_soundfont_load_guspatch(emu->sflist, buf, count); else if (format == SNDRV_OSS_SOUNDFONT_PATCH) { struct soundfont_patch_info patch; if (count < (int)sizeof(patch)) diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c index 16f00097cb95..e1e47518ac92 100644 --- a/sound/synth/emux/soundfont.c +++ b/sound/synth/emux/soundfont.c @@ -941,8 +941,7 @@ int snd_sf_vol_table[128] = {
/* load GUS patch */ static int -load_guspatch(struct snd_sf_list *sflist, const char __user *data, - long count, int client) +load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count) { struct patch_info patch; struct snd_soundfont *sf; @@ -1122,11 +1121,11 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, /* load GUS patch */ int snd_soundfont_load_guspatch(struct snd_sf_list *sflist, const char __user *data, - long count, int client) + long count) { int rc; lock_preset(sflist); - rc = load_guspatch(sflist, data, count, client); + rc = load_guspatch(sflist, data, count); unlock_preset(sflist); return rc; } -- 2.42.0.419.g70bf8a5751
Both bounds had off-by-one errors.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/synth/emux/emux_seq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/synth/emux/emux_seq.c b/sound/synth/emux/emux_seq.c index b227c7e0bc2a..1adaa75df2f6 100644 --- a/sound/synth/emux/emux_seq.c +++ b/sound/synth/emux/emux_seq.c @@ -65,11 +65,11 @@ snd_emux_init_seq(struct snd_emux *emu, struct snd_card *card, int index) return -ENODEV; }
- if (emu->num_ports < 0) { + if (emu->num_ports <= 0) { snd_printk(KERN_WARNING "seqports must be greater than zero\n"); emu->num_ports = 1; - } else if (emu->num_ports >= SNDRV_EMUX_MAX_PORTS) { - snd_printk(KERN_WARNING "too many ports." + } else if (emu->num_ports > SNDRV_EMUX_MAX_PORTS) { + snd_printk(KERN_WARNING "too many ports. " "limited max. ports %d\n", SNDRV_EMUX_MAX_PORTS); emu->num_ports = SNDRV_EMUX_MAX_PORTS; } -- 2.42.0.419.g70bf8a5751
The field is explicitly documented to be initialized by the driver (which it actually is). Also, using patch_info.size would be actually wrong for 16-bit data, as one field counts samples, while the other counts bytes.
load_guspatch() already did it right.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/synth/emux/soundfont.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c index e1e47518ac92..ad0231d7a39d 100644 --- a/sound/synth/emux/soundfont.c +++ b/sound/synth/emux/soundfont.c @@ -735,7 +735,7 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count) sp->v = sample_info; sp->v.sf_id = sf->id; sp->v.dummy = 0; - sp->v.truesize = sp->v.size; + sp->v.truesize = 0;
/* * If there is wave data then load it. -- 2.42.0.419.g70bf8a5751
This is required only to implement WAVE_BIDIR_LOOP and WAVE_LOOP_BACK in the GUS patch loader. It has not worked on emu10k1 since before ALSA hit mainline, yet nobody appears to have complained. And as it isn't super easy to implement, just admit defeat and clean up the code.
If somebody wanted to resurrect the feature, the emu8k driver could serve as a template, but the code would be quite different. But arguably, this should be done in user space in the first place, as this doesn't represent a hardware feature (somewhat ironically, the actual GUS driver has no synth support, and therefore no GUS patch loader).
Note that instead of properly rejecting affected samples, we continue to just pretend that the feature wasn't requested. This is extremely questionable behavior, but avoids that possibly unused instruments suddenly prevent loading the entire file, which would break backwards compatibility. But at least we log a warning now.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_patch.c | 73 ++++--------------------------- 1 file changed, 8 insertions(+), 65 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index 89890f24509f..49214c226808 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -28,8 +28,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, { int offset; int truesize, size, blocksize; - __maybe_unused int loopsize; - int loopend, sampleend; unsigned int start_addr; struct snd_emu10k1 *emu;
@@ -43,32 +41,24 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, return 0; }
+ if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP | SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) { + /* should instead return -ENOTSUPP; but compatibility */ + printk(KERN_WARNING "Emu10k1 wavetable patch %d with unsupported loop feature\n", + sp->v.sample); + } + /* recalculate address offset */ sp->v.end -= sp->v.start; sp->v.loopstart -= sp->v.start; sp->v.loopend -= sp->v.start; sp->v.start = 0;
- /* some samples have invalid data. the addresses are corrected in voice info */ - sampleend = sp->v.end; - if (sampleend > sp->v.size) - sampleend = sp->v.size; - loopend = sp->v.loopend; - if (loopend > sampleend) - loopend = sampleend; - /* be sure loop points start < end */ if (sp->v.loopstart >= sp->v.loopend) swap(sp->v.loopstart, sp->v.loopend);
/* compute true data size to be loaded */ truesize = sp->v.size + BLANK_HEAD_SIZE; - loopsize = 0; -#if 0 /* not supported */ - if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) - loopsize = sp->v.loopend - sp->v.loopstart; - truesize += loopsize; -#endif if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) truesize += BLANK_LOOP_SIZE;
@@ -96,65 +86,18 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, snd_emu10k1_synth_bzero(emu, sp->block, offset, size); offset += size;
- /* copy start->loopend */ - size = loopend; + /* copy provided samples */ + size = sp->v.size; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) size *= 2; if (offset + size > blocksize) return -EINVAL; if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) { snd_emu10k1_synth_free(emu, sp->block); sp->block = NULL; return -EFAULT; } offset += size; - data += size; - -#if 0 /* not supported yet */ - /* handle reverse (or bidirectional) loop */ - if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) { - /* copy loop in reverse */ - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) { - int woffset; - unsigned short *wblock = (unsigned short*)block; - woffset = offset / 2; - if (offset + loopsize * 2 > blocksize) - return -EINVAL; - for (i = 0; i < loopsize; i++) - wblock[woffset + i] = wblock[woffset - i -1]; - offset += loopsize * 2; - } else { - if (offset + loopsize > blocksize) - return -EINVAL; - for (i = 0; i < loopsize; i++) - block[offset + i] = block[offset - i -1]; - offset += loopsize; - } - - /* modify loop pointers */ - if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_BIDIR_LOOP) { - sp->v.loopend += loopsize; - } else { - sp->v.loopstart += loopsize; - sp->v.loopend += loopsize; - } - /* add sample pointer */ - sp->v.end += loopsize; - } -#endif - - /* loopend -> sample end */ - size = sp->v.size - loopend; - if (size < 0) - return -EINVAL; - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) - size *= 2; - if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) { - snd_emu10k1_synth_free(emu, sp->block); - sp->block = NULL; - return -EFAULT; - } - offset += size;
/* clear rest of samples (if any) */ if (offset < blocksize) -- 2.42.0.419.g70bf8a5751
This does several closely related things: - Move the code from the drivers into the SoundFont loader, which de-duplicates it. - Sort of explain the weird "recalculate address offset" feature. Note that I don't think it actually makes any sense - the calling user space code should do that. The background is certainly that the source data (the SoundFont format) uses pointers into a single wave block (and the API allows doing the same for on-board ROM), but the API expects the wave data from user space to be pre-chopped into individual patches anyway. - Make sure that the specified offsets actually lie within the supplied wave data. Note that we don't validate ROM offsets, so one can play back anything within the sound card's address space. - In load_guspatch(), don't call the sample_new callback anymore when the patch size is zero, as was already the case in load_data(). The callbacks would instantly return in that case anyway; these checks are now removed.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/isa/sb/emu8000_patch.c | 13 ----------- sound/pci/emu10k1/emu10k1_patch.c | 16 ------------- sound/synth/emux/soundfont.c | 37 ++++++++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/sound/isa/sb/emu8000_patch.c b/sound/isa/sb/emu8000_patch.c index 8c1e7f2bfc34..ab4f988f080d 100644 --- a/sound/isa/sb/emu8000_patch.c +++ b/sound/isa/sb/emu8000_patch.c @@ -148,13 +148,6 @@ snd_emu8000_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, if (snd_BUG_ON(!sp)) return -EINVAL;
- if (sp->v.size == 0) - return 0; - - /* be sure loop points start < end */ - if (sp->v.loopstart > sp->v.loopend) - swap(sp->v.loopstart, sp->v.loopend); - /* compute true data size to be loaded */ truesize = sp->v.size; if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) @@ -177,12 +170,6 @@ snd_emu8000_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, return -EFAULT; }
- /* recalculate address offset */ - sp->v.end -= sp->v.start; - sp->v.loopstart -= sp->v.start; - sp->v.loopend -= sp->v.start; - sp->v.start = 0; - /* dram position (in word) -- mem_offset is byte */ dram_offset = EMU8000_DRAM_OFFSET + (sp->block->offset >> 1); dram_start = dram_offset; diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index 49214c226808..47d69a0e44bc 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -35,28 +35,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, if (snd_BUG_ON(!sp || !hdr)) return -EINVAL;
- if (sp->v.size == 0) { - dev_dbg(emu->card->dev, - "emu: rom font for sample %d\n", sp->v.sample); - return 0; - } - if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP | SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) { /* should instead return -ENOTSUPP; but compatibility */ printk(KERN_WARNING "Emu10k1 wavetable patch %d with unsupported loop feature\n", sp->v.sample); }
- /* recalculate address offset */ - sp->v.end -= sp->v.start; - sp->v.loopstart -= sp->v.start; - sp->v.loopend -= sp->v.start; - sp->v.start = 0; - - /* be sure loop points start < end */ - if (sp->v.loopstart >= sp->v.loopend) - swap(sp->v.loopstart, sp->v.loopend); - /* compute true data size to be loaded */ truesize = sp->v.size + BLANK_HEAD_SIZE; if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c index ad0231d7a39d..6d6f0102ed5b 100644 --- a/sound/synth/emux/soundfont.c +++ b/sound/synth/emux/soundfont.c @@ -689,6 +689,21 @@ find_sample(struct snd_soundfont *sf, int sample_id) }
+static int +validate_sample_info(struct soundfont_sample_info *si) +{ + if (si->end < 0 || si->end > si->size) + return -EINVAL; + if (si->loopstart < 0 || si->loopstart > si->end) + return -EINVAL; + if (si->loopend < 0 || si->loopend > si->end) + return -EINVAL; + /* be sure loop points start < end */ + if (si->loopstart > si->loopend) + swap(si->loopstart, si->loopend); + return 0; +} + /* * Load sample information, this can include data to be loaded onto * the soundcard. It can also just be a pointer into soundcard ROM. @@ -727,6 +742,21 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count) return -EINVAL; }
+ if (sample_info.size > 0) { + if (sample_info.start < 0) + return -EINVAL; + + // Here we "rebase out" the start address, because the + // real start is the start of the provided sample data. + sample_info.end -= sample_info.start; + sample_info.loopstart -= sample_info.start; + sample_info.loopend -= sample_info.start; + sample_info.start = 0; + + if (validate_sample_info(&sample_info) < 0) + return -EINVAL; + } + /* Allocate a new sample structure */ sp = sf_sample_new(sflist, sf); if (!sp) @@ -974,6 +1004,11 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count) smp->v.loopend = patch.loop_end; smp->v.size = patch.len;
+ if (validate_sample_info(&smp->v) < 0) { + sf_sample_delete(sflist, sf, smp); + return -EINVAL; + } + /* set up mode flags */ smp->v.mode_flags = 0; if (!(patch.mode & WAVE_16_BITS)) @@ -1011,7 +1046,7 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count) /* * load wave data */ - if (sflist->callback.sample_new) { + if (smp->v.size > 0 && sflist->callback.sample_new) { rc = sflist->callback.sample_new (sflist->callback.private_data, smp, sflist->memhdr, data, count); -- 2.42.0.419.g70bf8a5751
In load_data(), make the validation of and skipping over the main info block match that in load_guspatch().
In load_guspatch(), add checking that the specified patch length matches the actually supplied data, like load_data() already did.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/synth/emux/soundfont.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c index 6d6f0102ed5b..4edc693da8e7 100644 --- a/sound/synth/emux/soundfont.c +++ b/sound/synth/emux/soundfont.c @@ -716,22 +716,25 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count) struct snd_soundfont *sf; struct soundfont_sample_info sample_info; struct snd_sf_sample *sp; - long off;
/* patch must be opened */ sf = sflist->currsf; if (!sf) return -EINVAL;
if (is_special_type(sf->type)) return -EINVAL;
+ if (count < (long)sizeof(sample_info)) { + return -EINVAL; + } if (copy_from_user(&sample_info, data, sizeof(sample_info))) return -EFAULT; + data += sizeof(sample_info); + count -= sizeof(sample_info);
- off = sizeof(sample_info); - - if (sample_info.size != (count-off)/2) + // SoundFont uses S16LE samples. + if (sample_info.size * 2 != count) return -EINVAL;
/* Check for dup */ @@ -774,7 +777,7 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count) int rc; rc = sflist->callback.sample_new (sflist->callback.private_data, sp, sflist->memhdr, - data + off, count - off); + data, count); if (rc < 0) { sf_sample_delete(sflist, sf, sp); return rc; @@ -986,10 +989,12 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count) } if (copy_from_user(&patch, data, sizeof(patch))) return -EFAULT; - count -= sizeof(patch); data += sizeof(patch);
+ if ((patch.len << (patch.mode & WAVE_16_BITS ? 1 : 0)) != count) + return -EINVAL; + sf = newsf(sflist, SNDRV_SFNT_PAT_TYPE_GUS|SNDRV_SFNT_PAT_SHARED, NULL); if (sf == NULL) return -ENOMEM; -- 2.42.0.419.g70bf8a5751
Convert some checks in snd_emu10k1_sample_new() back into assertions (as they were prior to da3cec35dd (ALSA: Kill snd_assert() in sound/pci/*, 2008-08-08)), and move them into the low-level memory access functions they protect.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
---
Side note: this eliminates the memory leaks in the now gone error paths. I don't think it was actually possible to trigger these even before the foregoing cleanups. But if it were, it would allow a user with access to the audio device a scope-limited DoS attack on it. This would be only a very minor security hole, given that on modern systems it would merely enable the current seat owner to be a nuisance to their successor, by making a reboot necessary. --- sound/pci/emu10k1/emu10k1_patch.c | 4 ---- sound/pci/emu10k1/memory.c | 6 ++++++ 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index 47d69a0e44bc..55bb60d31fe4 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -65,17 +65,13 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, size = BLANK_HEAD_SIZE; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) size *= 2; - if (offset + size > blocksize) - return -EINVAL; snd_emu10k1_synth_bzero(emu, sp->block, offset, size); offset += size;
/* copy provided samples */ size = sp->v.size; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) size *= 2; - if (offset + size > blocksize) - return -EINVAL; if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) { snd_emu10k1_synth_free(emu, sp->block); sp->block = NULL; diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c index 20b07117574b..fc9444404151 100644 --- a/sound/pci/emu10k1/memory.c +++ b/sound/pci/emu10k1/memory.c @@ -574,6 +574,9 @@ int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk void *ptr; struct snd_emu10k1_memblk *p = (struct snd_emu10k1_memblk *)blk;
+ if (snd_BUG_ON(offset + size > p->mem.size)) + return -EFAULT; + offset += blk->offset & (PAGE_SIZE - 1); end_offset = offset + size; page = get_aligned_page(offset); @@ -604,6 +607,9 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me void *ptr; struct snd_emu10k1_memblk *p = (struct snd_emu10k1_memblk *)blk;
+ if (snd_BUG_ON(offset + size > p->mem.size)) + return -EFAULT; + offset += blk->offset & (PAGE_SIZE - 1); end_offset = offset + size; page = get_aligned_page(offset); -- 2.42.0.419.g70bf8a5751
The hardware supports S16LE and U8 samples, while U16LE and S8 (which the driver implicitly claims to support) require sign flipping.
Note that this matters only for the GUS patch loader, as the implemented SoundFont v2.01 spec is limited to S16LE.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- include/sound/emu10k1.h | 4 +-- sound/pci/emu10k1/emu10k1_patch.c | 30 ++++++++----------- sound/pci/emu10k1/memory.c | 49 +++++++++++++++++++++++++------ 3 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h index 1af9e6819392..9e3bd4f81460 100644 --- a/include/sound/emu10k1.h +++ b/include/sound/emu10k1.h @@ -1882,8 +1882,8 @@ int snd_emu10k1_alloc_pages_maybe_wider(struct snd_emu10k1 *emu, size_t size, struct snd_dma_buffer *dmab); struct snd_util_memblk *snd_emu10k1_synth_alloc(struct snd_emu10k1 *emu, unsigned int size); int snd_emu10k1_synth_free(struct snd_emu10k1 *emu, struct snd_util_memblk *blk); -int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, int size); -int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, const char __user *data, int size); +int snd_emu10k1_synth_memset(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, int size, u8 value); +int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, const char __user *data, int size, u32 xor); int snd_emu10k1_memblk_map(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk);
/* voice allocation */ diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index 55bb60d31fe4..eb3d1ef8a33a 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -26,6 +26,8 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, struct snd_util_memhdr *hdr, const void __user *data, long count) { + u8 fill; + u32 xor; int offset; int truesize, size, blocksize; unsigned int start_addr; @@ -41,6 +43,14 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, sp->v.sample); }
+ if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS) { + fill = 0x80; + xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0 : 0x80808080; + } else { + fill = 0; + xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0x80008000 : 0; + } + /* compute true data size to be loaded */ truesize = sp->v.size + BLANK_HEAD_SIZE; if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) @@ -65,46 +75,32 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, size = BLANK_HEAD_SIZE; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) size *= 2; - snd_emu10k1_synth_bzero(emu, sp->block, offset, size); + snd_emu10k1_synth_memset(emu, sp->block, offset, size, fill); offset += size;
/* copy provided samples */ size = sp->v.size; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) size *= 2; - if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) { + if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) { snd_emu10k1_synth_free(emu, sp->block); sp->block = NULL; return -EFAULT; } offset += size;
/* clear rest of samples (if any) */ if (offset < blocksize) - snd_emu10k1_synth_bzero(emu, sp->block, offset, blocksize - offset); + snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) { /* if no blank loop is attached in the sample, add it */ if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) { sp->v.loopstart = sp->v.end + BLANK_LOOP_START; sp->v.loopend = sp->v.end + BLANK_LOOP_END; } }
-#if 0 /* not supported yet */ - if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) { - /* unsigned -> signed */ - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) { - unsigned short *wblock = (unsigned short*)block; - for (i = 0; i < truesize; i++) - wblock[i] ^= 0x8000; - } else { - for (i = 0; i < truesize; i++) - block[i] ^= 0x80; - } - } -#endif - /* recalculate offset */ start_addr = BLANK_HEAD_SIZE * 2; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c index fc9444404151..d29711777161 100644 --- a/sound/pci/emu10k1/memory.c +++ b/sound/pci/emu10k1/memory.c @@ -565,10 +565,10 @@ static inline void *offset_ptr(struct snd_emu10k1 *emu, int page, int offset) }
/* - * bzero(blk + offset, size) + * memset(blk + offset, value, size) */ -int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, - int offset, int size) +int snd_emu10k1_synth_memset(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, + int offset, int size, u8 value) { int page, nextofs, end_offset, temp, temp1; void *ptr; @@ -588,20 +588,47 @@ int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk temp = temp1; ptr = offset_ptr(emu, page + p->first_page, offset); if (ptr) - memset(ptr, 0, temp); + memset(ptr, value, temp); offset = nextofs; page++; } while (offset < end_offset); return 0; }
-EXPORT_SYMBOL(snd_emu10k1_synth_bzero); +EXPORT_SYMBOL(snd_emu10k1_synth_memset); + +// Note that the value is assumed to be suitably repetitive. +static void xor_range(void *ptr, int size, u32 value) +{ + if ((long)ptr & 1) { + *(u8 *)ptr ^= (u8)value; + ptr++; + size--; + } + if (size > 1 && ((long)ptr & 2)) { + *(u16 *)ptr ^= (u16)value; + ptr += 2; + size -= 2; + } + while (size > 3) { + *(u32 *)ptr ^= value; + ptr += 4; + size -= 4; + } + if (size > 1) { + *(u16 *)ptr ^= (u16)value; + ptr += 2; + size -= 2; + } + if (size > 0) + *(u8 *)ptr ^= (u8)value; +}
/* - * copy_from_user(blk + offset, data, size) + * copy_from_user(blk + offset, data, size) ^ xor */ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, - int offset, const char __user *data, int size) + int offset, const char __user *data, int size, u32 xor) { int page, nextofs, end_offset, temp, temp1; void *ptr; @@ -620,8 +647,12 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me if (temp1 < temp) temp = temp1; ptr = offset_ptr(emu, page + p->first_page, offset); - if (ptr && copy_from_user(ptr, data, temp)) - return -EFAULT; + if (ptr) { + if (copy_from_user(ptr, data, temp)) + return -EFAULT; + if (xor) + xor_range(ptr, temp, xor); + } offset = nextofs; data += temp; page++; -- 2.42.0.419.g70bf8a5751
Samples are byte-sized in this mode, and thus the offset calculation needs no shifting.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_callback.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c index d36234b88fb4..2ed72bea1d8f 100644 --- a/sound/pci/emu10k1/emu10k1_callback.c +++ b/sound/pci/emu10k1/emu10k1_callback.c @@ -310,27 +310,29 @@ start_voice(struct snd_emux_voice *vp) { unsigned int temp; int ch; + bool w_16; u32 psst, dsl, map, ccca, vtarget; unsigned int addr, mapped_offset; struct snd_midi_channel *chan; struct snd_emu10k1 *hw; struct snd_emu10k1_memblk *emem;
hw = vp->hw; ch = vp->ch; if (snd_BUG_ON(ch < 0)) return -EINVAL; chan = vp->chan; + w_16 = !(vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_8BITS);
emem = (struct snd_emu10k1_memblk *)vp->block; if (emem == NULL) return -EINVAL; emem->map_locked++; if (snd_emu10k1_memblk_map(hw, emem) < 0) { /* dev_err(hw->card->devK, "emu: cannot map!\n"); */ return -ENOMEM; } - mapped_offset = snd_emu10k1_memblk_offset(emem) >> 1; + mapped_offset = snd_emu10k1_memblk_offset(emem) >> w_16; vp->reg.start += mapped_offset; vp->reg.end += mapped_offset; vp->reg.loopstart += mapped_offset; @@ -371,7 +373,7 @@ start_voice(struct snd_emux_voice *vp) unsigned int shift = (vp->apitch - 0xe000) >> 10; ccca |= shift << 25; } - if (vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_8BITS) + if (!w_16) ccca |= CCCA_8BITSELECT;
vtarget = (unsigned int)vp->vtarget << 16; -- 2.42.0.419.g70bf8a5751
This amends df335e9a8b (ALSA: emu10k1: fix synthesizer sample playback position and caching, 2023-05-18), because now I know that the samples are preceded by a blank block anyway, so we can just compensate for the interpolator read-ahead without any additional fiddling.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_callback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c index 2ed72bea1d8f..ef26e4d3e2a3 100644 --- a/sound/pci/emu10k1/emu10k1_callback.c +++ b/sound/pci/emu10k1/emu10k1_callback.c @@ -255,7 +255,7 @@ lookup_voices(struct snd_emux *emu, struct snd_emu10k1 *hw, /* check if sample is finished playing (non-looping only) */ if (bp != best + V_OFF && bp != best + V_FREE && (vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_SINGLESHOT)) { - val = snd_emu10k1_ptr_read(hw, CCCA_CURRADDR, vp->ch) - 64; + val = snd_emu10k1_ptr_read(hw, CCCA_CURRADDR, vp->ch) - 64 + 3; if (val >= vp->reg.loopstart) bp = best + V_OFF; } @@ -364,7 +364,7 @@ start_voice(struct snd_emux_voice *vp)
map = (hw->silent_page.addr << hw->address_mode) | (hw->address_mode ? MAP_PTI_MASK1 : MAP_PTI_MASK0);
- addr = vp->reg.start + 64; + addr = vp->reg.start + 64 - 3; temp = vp->reg.parm.filterQ; ccca = (temp << 28) | addr; if (vp->apitch < 0xe400) -- 2.42.0.419.g70bf8a5751
There is no need for it to be 32 samples - 3 will do just fine (which is the interpolator's epsilon). The old size was presumably meant to compensate for the cache's presence, but we're now handling that properly.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_patch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index eb3d1ef8a33a..a2ba6246dbc7 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -16,7 +16,7 @@ #define BLANK_LOOP_START 4 #define BLANK_LOOP_END 8 #define BLANK_LOOP_SIZE 12 -#define BLANK_HEAD_SIZE 32 +#define BLANK_HEAD_SIZE 3
/* * allocate a sample block and copy data from userspace -- 2.42.0.419.g70bf8a5751
This de-duplicates the code slightly. But the real reason is that it moves the code up, which the next patch will depend on.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_patch.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index a2ba6246dbc7..c7d54f38d28c 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -53,8 +53,14 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
/* compute true data size to be loaded */ truesize = sp->v.size + BLANK_HEAD_SIZE; - if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) + if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) { truesize += BLANK_LOOP_SIZE; + /* if no blank loop is attached in the sample, add it */ + if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) { + sp->v.loopstart = sp->v.end + BLANK_LOOP_START; + sp->v.loopend = sp->v.end + BLANK_LOOP_END; + } + }
/* try to allocate a memory block */ blocksize = truesize; @@ -93,14 +99,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, if (offset < blocksize) snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
- if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) { - /* if no blank loop is attached in the sample, add it */ - if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) { - sp->v.loopstart = sp->v.end + BLANK_LOOP_START; - sp->v.loopend = sp->v.end + BLANK_LOOP_END; - } - } - /* recalculate offset */ start_addr = BLANK_HEAD_SIZE * 2; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) -- 2.42.0.419.g70bf8a5751
The offsets are counted in samples, not in bytes.
While the code block is being rewritten, also move it up a bit, to avoid churn in a subsequent patch.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_patch.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index c7d54f38d28c..eb8365650bd4 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -30,7 +30,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, u32 xor; int offset; int truesize, size, blocksize; - unsigned int start_addr; struct snd_emu10k1 *emu;
emu = rec->hw; @@ -62,6 +61,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, } }
+ /* recalculate offset */ + sp->v.start += BLANK_HEAD_SIZE; + sp->v.end += BLANK_HEAD_SIZE; + sp->v.loopstart += BLANK_HEAD_SIZE; + sp->v.loopend += BLANK_HEAD_SIZE; + /* try to allocate a memory block */ blocksize = truesize; if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) @@ -99,15 +104,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, if (offset < blocksize) snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
- /* recalculate offset */ - start_addr = BLANK_HEAD_SIZE * 2; - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) - start_addr >>= 1; - sp->v.start += start_addr; - sp->v.end += start_addr; - sp->v.loopstart += start_addr; - sp->v.loopend += start_addr; - return 0; }
-- 2.42.0.419.g70bf8a5751
Instead of repeatedly checking the sample width, assign a size shift centrally.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_patch.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index eb8365650bd4..699aa0fec97b 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -28,6 +28,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, { u8 fill; u32 xor; + int shift; int offset; int truesize, size, blocksize; struct snd_emu10k1 *emu; @@ -43,9 +44,11 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, }
if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS) { + shift = 0; fill = 0x80; xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0 : 0x80808080; } else { + shift = 1; fill = 0; xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0x80008000 : 0; } @@ -68,9 +71,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, sp->v.loopend += BLANK_HEAD_SIZE;
/* try to allocate a memory block */ - blocksize = truesize; - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) - blocksize *= 2; + blocksize = truesize << shift; sp->block = snd_emu10k1_synth_alloc(emu, blocksize); if (sp->block == NULL) { dev_dbg(emu->card->dev, @@ -83,16 +84,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
/* write blank samples at head */ offset = 0; - size = BLANK_HEAD_SIZE; - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) - size *= 2; + size = BLANK_HEAD_SIZE << shift; snd_emu10k1_synth_memset(emu, sp->block, offset, size, fill); offset += size;
/* copy provided samples */ - size = sp->v.size; - if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) - size *= 2; + size = sp->v.size << shift; if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) { snd_emu10k1_synth_free(emu, sp->block); sp->block = NULL; -- 2.42.0.419.g70bf8a5751
Resulting from more reverse engineering in the course of debugging.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- include/sound/emu10k1.h | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h index 9e3bd4f81460..12c7dc760724 100644 --- a/include/sound/emu10k1.h +++ b/include/sound/emu10k1.h @@ -598,17 +598,25 @@ SUB_REG(PEFE, FILTERAMOUNT, 0x000000ff) /* Filter envlope amount */ // 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). +// All registers mentioned below count in frames. Shortcuts: +// CA = CCCA_CURRADDR, CRA = CCR_READADDRESS, +// CLA = CCR_CACHELOOPADDRHI:CLP_CACHELOOPADDR, +// CIS = CCR_CACHEINVALIDSIZE, LIS = CCR_LOOPINVALSIZE, +// CLF = CCR_CACHELOOPFLAG, LF = CCR_LOOPFLAG +// The cache is a ring buffer; CRA operates modulo 64. +// The cache is filled from (CA - CIS) into (CRA - CIS). // 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. +// CIS 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 CRA, which is incremented afterwards, +// along with CA and CIS. This implies that the actual playback +// position always lags CA by exactly 64 frames. +// When CA reaches DSL_LOOPENDADDR, LF is set for one frame's time. +// LF's rising edge causes the current values of CA and CIS to be +// copied into CLA and LIS, resp., and CLF to be set. +// If CLF is set, the first LIS of the CIS frames are instead +// filled from (CLA - LIS), and CLF is subsequently reset. #define CD0 0x20 /* Cache data registers 0 .. 0x1f */
#define PTB 0x40 /* Page table base register */ -- 2.42.0.419.g70bf8a5751
As already anticipated in the linked commit, playback was broken for very short samples. That's because we'd set the current position beyond the end of the loop, so while the start would now be correct (due to the cache lag), we'd run off the end of the sample and play garbage.
Fixing the playback position itself wouldn't be that hard (just modulo it), but this wouldn't address pre-filling the cache with the right data.
We could pre-fill the cache manually, but that's slow, requires additional code for each sample width, and is made even more complex by the driver's virtual address space having no contiguous mapping for the CPU.
We could have the engine fill the cache piece-wise (which is really what happens when playback is running), but that would also be complex, and we'd need to wait for the engine to handle each piece, so it wouldn't be that much faster than the manual fill.
For the case of requiring only one loop iteration prior to reaching the cache size, one could leverage the engine's looping mechanism around CCR_CACHELOOPFLAG, but this special case doesn't seem worth the complexity.
So we just unroll the loop as far as necessary to be able to play back the sample without any fiddling.
Pedantically, this would be incorrect for loop-until-release samples with a low loop end which are released very quickly, but that would be relatively harmless, is not a plausible use case in the first place, and SoundFont sample mode 3 isn't actually implemented anyway (it's conflated with mode 1, infinite looping).
Fixes: df335e9a8b (ALSA: emu10k1: fix synthesizer sample playback position and caching, 2023-05-18) Link: https://bugzilla.kernel.org/show_bug.cgi?id=218625 Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/pci/emu10k1/emu10k1_patch.c | 53 ++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 5 deletions(-)
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c index 699aa0fec97b..dbfa89435ac2 100644 --- a/sound/pci/emu10k1/emu10k1_patch.c +++ b/sound/pci/emu10k1/emu10k1_patch.c @@ -31,6 +31,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, int shift; int offset; int truesize, size, blocksize; + int loop_start, loop_end, loop_size, data_end, unroll; struct snd_emu10k1 *emu;
emu = rec->hw; @@ -64,12 +65,35 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, } }
+ loop_start = sp->v.loopstart; + loop_end = sp->v.loopend; + loop_size = loop_end - loop_start; + if (!loop_size) + return -EINVAL; + data_end = sp->v.end; + /* recalculate offset */ sp->v.start += BLANK_HEAD_SIZE; sp->v.end += BLANK_HEAD_SIZE; sp->v.loopstart += BLANK_HEAD_SIZE; sp->v.loopend += BLANK_HEAD_SIZE;
+ // Automatic pre-filling of the cache does not work in the presence + // of loops (*), and we don't want to fill it manually, as that is + // fiddly and slow. So we unroll the loop until the loop end is + // beyond the cache size. + // (*) Strictly speaking, a single iteration is supported (that's + // how it works when the playback engine runs), but handling this + // special case is not worth it. + unroll = 0; + while (sp->v.loopend < 64) { + truesize += loop_size; + sp->v.loopstart += loop_size; + sp->v.loopend += loop_size; + sp->v.end += loop_size; + unroll++; + } + /* try to allocate a memory block */ blocksize = truesize << shift; sp->block = snd_emu10k1_synth_alloc(emu, blocksize); @@ -89,19 +113,38 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp, offset += size;
/* copy provided samples */ - size = sp->v.size << shift; - if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) { - snd_emu10k1_synth_free(emu, sp->block); - sp->block = NULL; - return -EFAULT; + if (unroll && loop_end <= data_end) { + size = loop_end << shift; + if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) + goto faulty; + offset += size; + + data += loop_start << shift; + while (--unroll > 0) { + size = loop_size << shift; + if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) + goto faulty; + offset += size; + } + + size = (data_end - loop_start) << shift; + } else { + size = data_end << shift; } + if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) + goto faulty; offset += size;
/* clear rest of samples (if any) */ if (offset < blocksize) snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
return 0; + +faulty: + snd_emu10k1_synth_free(emu, sp->block); + sp->block = NULL; + return -EFAULT; }
/* -- 2.42.0.419.g70bf8a5751
Both drivers provide both sample_new and sample_free, and it makes no sense to pretend that they could not. In fact, load_data() would already crash if sample_new was null. So remove the remaining null checks.
Contrary to that, the emu10k1 driver actually has a null sample_reset, though I'm not convinced that this inconsistency is justified.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/synth/emux/emux.c | 6 ++---- sound/synth/emux/soundfont.c | 12 +++++------- 2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c index a82af9374852..01444fc960d0 100644 --- a/sound/synth/emux/emux.c +++ b/sound/synth/emux/emux.c @@ -94,10 +94,8 @@ int snd_emux_register(struct snd_emux *emu, struct snd_card *card, int index, ch /* create soundfont list */ memset(&sf_cb, 0, sizeof(sf_cb)); sf_cb.private_data = emu; - if (emu->ops.sample_new) - sf_cb.sample_new = sf_sample_new; - if (emu->ops.sample_free) - sf_cb.sample_free = sf_sample_free; + sf_cb.sample_new = sf_sample_new; + sf_cb.sample_free = sf_sample_free; if (emu->ops.sample_reset) sf_cb.sample_reset = sf_sample_reset; emu->sflist = snd_sf_new(&sf_cb, emu->memhdr); diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c index 4edc693da8e7..2373ed580bf8 100644 --- a/sound/synth/emux/soundfont.c +++ b/sound/synth/emux/soundfont.c @@ -1051,7 +1051,7 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count) /* * load wave data */ - if (smp->v.size > 0 && sflist->callback.sample_new) { + if (smp->v.size > 0) { rc = sflist->callback.sample_new (sflist->callback.private_data, smp, sflist->memhdr, data, count); @@ -1416,9 +1416,8 @@ snd_sf_clear(struct snd_sf_list *sflist) } for (sp = sf->samples; sp; sp = nextsp) { nextsp = sp->next; - if (sflist->callback.sample_free) - sflist->callback.sample_free(sflist->callback.private_data, - sp, sflist->memhdr); + sflist->callback.sample_free(sflist->callback.private_data, + sp, sflist->memhdr); kfree(sp); } kfree(sf); @@ -1520,9 +1519,8 @@ snd_soundfont_remove_unlocked(struct snd_sf_list *sflist) nextsp = sp->next; sf->samples = nextsp; sflist->mem_used -= sp->v.truesize; - if (sflist->callback.sample_free) - sflist->callback.sample_free(sflist->callback.private_data, - sp, sflist->memhdr); + sflist->callback.sample_free(sflist->callback.private_data, + sp, sflist->memhdr); kfree(sp); } } -- 2.42.0.419.g70bf8a5751
On Mon, 01 Apr 2024 12:07:24 +0200, Oswald Buddenhagen wrote:
this fixes the regression i introduced (though arguably, i just made it broken in a different way), and then some more.
Oswald Buddenhagen (18): ALSA: emux: fix /proc teardown at module unload ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch() ALSA: emux: fix validation of snd_emux.num_ports ALSA: emux: fix init of patch_info.truesize in load_data() ALSA: emu10k1: prune vestiges of SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support ALSA: emux: centralize & improve patch info validation ALSA: emux: improve patch ioctl data validation ALSA: emu10k1: move patch loader assertions into low-level functions ALSA: emu10k1: fix sample signedness issues in wavetable loader ALSA: emu10k1: fix playback of 8-bit wavetable samples ALSA: emu10k1: make wavetable sample playback start position exact ALSA: emu10k1: shrink blank space in front of wavetable samples ALSA: emu10k1: merge conditions in patch loader ALSA: emu10k1: fix wavetable offset recalculation ALSA: emu10k1: de-duplicate size calculations for 16-bit samples ALSA: emu10k1: improve cache behavior documentation ALSA: emu10k1: fix playback of short wavetable samples ALSA: emux: simplify snd_sf_list.callback handling
Could you give Fixes tag in each commit if it's a regression fix for the corresponding commit?
thanks,
Takashi
On Mon, Apr 01, 2024 at 12:51:32PM +0200, Takashi Iwai wrote:
Could you give Fixes tag in each commit if it's a regression fix for the corresponding commit?
i did. you'll see it when the later patches arrive (minor hiccup with mail delivery on my end ...).
of course this won't help a lot with picking to stable, because the fix actually depends on several of the prior patches. i can re-arrange the series to minimize the hard dependency chain, but it will still be ~10 patches.
an alternative approach would be just reverting the offending patch and re-fixing it as part of the subsequent series. the revert would be easily pickable, but that merely replaces the current problem with the (admittedly less audible) previous problem. your choice.
On Mon, 01 Apr 2024 13:18:36 +0200, Oswald Buddenhagen wrote:
On Mon, Apr 01, 2024 at 12:51:32PM +0200, Takashi Iwai wrote:
Could you give Fixes tag in each commit if it's a regression fix for the corresponding commit?
i did. you'll see it when the later patches arrive (minor hiccup with mail delivery on my end ...).
of course this won't help a lot with picking to stable, because the fix actually depends on several of the prior patches. i can re-arrange the series to minimize the hard dependency chain, but it will still be ~10 patches.
an alternative approach would be just reverting the offending patch and re-fixing it as part of the subsequent series. the revert would be easily pickable, but that merely replaces the current problem with the (admittedly less audible) previous problem. your choice.
Judging from the amount of patches, I prefer a quicker "fix" for the known regression, so a revert-and-rewrite sounds more like a reasonable approach. Care to resubmit with that?
thanks,
Takashi
participants (2)
-
Oswald Buddenhagen -
Takashi Iwai