| Message ID | 20230630144542.664190-1-oswald.buddenhagen@gmx.de |
|---|---|
| Headers | show |
| Series | ALSA: emu10k1: add support for high-bitrate modes of E-MU cards | expand |
On Mon, Jul 10, 2023 at 05:06:36PM +0200, Takashi Iwai wrote: >On Fri, 30 Jun 2023 16:45:34 +0200, >Oswald Buddenhagen wrote: >> >> This series is what all the work was about: support the "dual-/quad-pumped" >> modes of the E-MU cards. >> >> Oswald Buddenhagen (8): >> ALSA: emu10k1: introduce alternative E-MU D.A.S. mode >> ALSA: emu10k1: improve mixer control naming in E-MU D.A.S. mode >> ALSA: emu10k1: set the "no filtering" bits on PCM voices >> ALSA: emu10k1: make playback in E-MU D.A.S. mode 32-bit >> ALSA: add snd_ctl_add_locked() >> ALSA: emu10k1: add support for 2x/4x word clocks in E-MU D.A.S. mode >> ALSA: emu10k1: add high-rate capture in E-MU D.A.S. mode >> ALSA: emu10k1: add high-rate playback in E-MU D.A.S. mode > >I still can't agree with the basic design using the dynamic kctl >addition / deletion in kcontrol's put action. > you are not being constructive. please provide specific, actionable responses to _all_ challenges/questions. regards
On Tue, Jul 11, 2023 at 01:08:05PM +0200, Takashi Iwai wrote: >On Tue, 11 Jul 2023 12:15:31 +0200, >Oswald Buddenhagen wrote: >> >> On Mon, Jul 10, 2023 at 05:06:36PM +0200, Takashi Iwai wrote: >> > I still can't agree with the basic design using the dynamic kctl >> > addition / deletion in kcontrol's put action. >> > >> you are not being constructive. please provide specific, actionable >> responses to _all_ challenges/questions. > >The fundamental idea to add / delete the kctls from the put callback >is unacceptable; as repeated many times, this is known to break >existing applications. As long as you are sticking with this idea, it >can go [no] further. Please avoid it and use the (more or less) static >configuration instead. > to put the implications of this in clear words: you want me to spend additional time on a driver barely anyone still cares about to actively degrade the (my!) user experience to avoid hypothetical / likely obsolete crashes that would happen upon a rare user-controlled event in unspecified buggy (mixer? (!)) applications, while a known-good fallback exists (alsamixer). i fail to see how that is even _remotely_ a reasonable request, and therefore have no intention whatsoever to follow it. regards
On Mon, 17 Jul 2023 12:19:49 +0200, Oswald Buddenhagen wrote: > > On Tue, Jul 11, 2023 at 01:08:05PM +0200, Takashi Iwai wrote: > > On Tue, 11 Jul 2023 12:15:31 +0200, > > Oswald Buddenhagen wrote: > >> > >> On Mon, Jul 10, 2023 at 05:06:36PM +0200, Takashi Iwai wrote: > >> > I still can't agree with the basic design using the dynamic kctl > >> > addition / deletion in kcontrol's put action. > >> > you are not being constructive. please provide specific, > >> actionable > >> responses to _all_ challenges/questions. > > > > The fundamental idea to add / delete the kctls from the put callback > > is unacceptable; as repeated many times, this is known to break > > existing applications. As long as you are sticking with this idea, it > > can go [no] further. Please avoid it and use the (more or less) static > > configuration instead. > > > to put the implications of this in clear words: > you want me to spend additional time > on a driver barely anyone still cares about > to actively degrade the (my!) user experience > to avoid hypothetical / likely obsolete crashes > that would happen upon a rare user-controlled event > in unspecified buggy (mixer? (!)) applications, > while a known-good fallback exists (alsamixer). Simply put, YES. It's breaking applications pretty easily. This already happened in the past, so it's no hypothesis. If you've ever programmed applications that deal with ALSA mixer/control stuff by yourself, you'll notice that it's really tough to deal with the dynamic deletion/addition. alsamixer can accept it in the limited manner, but it's no fallback for everything, of course. Takashi
On Mon, Jul 17, 2023 at 02:53:19PM +0200, Takashi Iwai wrote: >On Mon, 17 Jul 2023 12:19:49 +0200, >Oswald Buddenhagen wrote: >> >> you want me to spend additional time >> on a driver barely anyone still cares about >> to actively degrade the (my!) user experience >> to avoid hypothetical / likely obsolete crashes >> that would happen upon a rare user-controlled event >> in unspecified buggy (mixer? (!)) applications, >> while a known-good fallback exists (alsamixer). > >Simply put, YES. > well, your priorities don't align with the needs of actual users (that would be me, in this case). >If you've ever programmed applications that deal with ALSA >mixer/control stuff by yourself, you'll notice that it's really tough >to deal with the dynamic deletion/addition. > hot-plugging always requires some care to handle. i don't consider this a showstopper, esp. in the year 2023, when udev and pulseaudio/pipewire go all crazy on us (and yes, that crashed kmix - big deal). i don't think it's sane to set the bar at 1995 standards. even less so when the class of potentially affected apps holds no user data of note. >alsamixer can accept it in the limited manner, >but it's no fallback for everything, of course. > i have no clue what point you're trying to make. regards
This series is what all the work was about: support the "dual-/quad-pumped" modes of the E-MU cards. Oswald Buddenhagen (8): ALSA: emu10k1: introduce alternative E-MU D.A.S. mode ALSA: emu10k1: improve mixer control naming in E-MU D.A.S. mode ALSA: emu10k1: set the "no filtering" bits on PCM voices ALSA: emu10k1: make playback in E-MU D.A.S. mode 32-bit ALSA: add snd_ctl_add_locked() ALSA: emu10k1: add support for 2x/4x word clocks in E-MU D.A.S. mode ALSA: emu10k1: add high-rate capture in E-MU D.A.S. mode ALSA: emu10k1: add high-rate playback in E-MU D.A.S. mode include/sound/control.h | 1 + include/sound/emu10k1.h | 11 + sound/core/control.c | 31 ++ sound/pci/emu10k1/emu10k1.c | 29 +- sound/pci/emu10k1/emu10k1_main.c | 19 +- sound/pci/emu10k1/emufx.c | 109 +++- sound/pci/emu10k1/emumixer.c | 901 +++++++++++++++++++++++++++---- sound/pci/emu10k1/emupcm.c | 422 +++++++++++++-- sound/pci/emu10k1/emuproc.c | 5 + sound/pci/emu10k1/io.c | 30 +- sound/pci/emu10k1/voice.c | 6 + 11 files changed, 1383 insertions(+), 181 deletions(-) Range-diff against v1: 1: c8c1fbba5e52 = 1: 7cb49147164f ALSA: emu10k1: introduce alternative E-MU D.A.S. mode 2: 879066428bee = 2: aa9ead00e045 ALSA: emu10k1: improve mixer control naming in E-MU D.A.S. mode 3: b663229a0f46 = 3: ac830e67322f ALSA: emu10k1: set the "no filtering" bits on PCM voices 4: a9de9a73571e = 4: 26e0b7c02c1d ALSA: emu10k1: make playback in E-MU D.A.S. mode 32-bit 5: 0f1950b03193 ! 5: 94b64e6c123d ALSA: add snd_ctl_add_locked() @@ Commit message This is in fact more symmetrical to snd_ctl_remove() than snd_ctl_add() is - the former could be named snd_ctl_remove_locked() just as well. + One may argue that this is going in the wrong direction, as drivers have + no business managing the lock. This may be true in principle, but in + practice the vast majority of controls is created even before the device + was registered, and therefore before any locking is necessary at all. + That means that an even more radical approach of changing snd_ctl_add() + do do no locking, and converting the call sites that actually need + locking to a new function, would better match reality, and would be + somewhat more efficient at that. However, that seems a bit risky and way + too much work. + This will be used to dynamically change the available controls from another control's put() callback, which is already locked. One might want to add snd_ctl_replace_locked() for completeness, but I have no use for it now. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> + --- + v2: + - extended commit message ## include/sound/control.h ## @@ include/sound/control.h: void snd_ctl_notify_one(struct snd_card * card, unsigned int mask, struct snd_kc 6: 511efe4ac7ad ! 6: 7bc314bae7f2 ALSA: emu10k1: add support for 2x/4x word clocks in E-MU D.A.S. mode @@ Commit message on playback, and throw away the excess ones on capture. Input-to-output monitoring does actually use the full sample rate, though. - Notably, add_ctls() now uses snd_ctl_add_locked(), so it doesn't - deadlock when called from snd_emu1010_clock_shift_put(). This also - affects the initial creation of the controls, which is OK, as that is - done before the card is registered, so no concurrent access can occur. + Due to hardware constraints, changing the clock multiplier (CM) changes + the available audio ports and the number of available channels. This has + an impact on the channel routing mixer controls. One way to deal with + this would be presenting a union of all possibilities, and simply + ignoring currently inapplicable settings. However, this would be a + terrible user experience, and go against the spirit of prior patches + aimed at decluttering the mixer. Therefore, we do dynamic + reconfiguration (DR) of the mixer in response to changing the CM. + + DR is somewhat controversial, as it has the potential to crash poorly + programmed applications. But that in itself isn't a very convincing + argument against it, as by that logic we'd have to ban all hot-plugging. + Such crashes would also not really qualify as regressions, as the D.A.S. + mode is a new opt-in feature, and therefore no previously stable setups + would be impacted. Also, pendantically, the driver already had DR via + SNDRV_EMU10K1_IOCTL_CODE_POKE. A somewhat valid concern is that changing + mixer settings is a non-privileged operation and therefore potential + crashes could be exploited for a somewhat more impactful nuisance attack + on another user than messing with the mixer per se. However, systemd & + co. limit device access to the user currently logged in on the seat + owning the device. + + There is a specific concern about doing DR in response to changing a + mixer control's value, as an application may legitimately react to DR by + updating all mixer settings in turn. However, that update should write + the same value to the clock multiplier, thus terminating the recursion. + + One may limit DR to merely disabling inapplicable controls, in the hope + that this would be better handled than completely tearing down and + re-creating controls as we do. However, there is no guarantee for that. + And because it is impossible to disable particular enum values within a + control, it would be necessary to have three complete sets of + per-channel controls. This would yield an extremely cluttered and + confusing UI if the application (reasonably) chose to merely visually + disable inactive controls rather than hiding them. + + We do the DR synchronously from within snd_emu1010_clock_shift_put(). + This was enabled by commit 5bbb1ab5bd ("control: use counting semaphore + as write lock for ELEM_WRITE operation"); we merely need to make + add_ctls() use snd_ctl_add_locked() instead of snd_ctl_add(), so it + doesn't deadlock. That also affects the initial creation of the + controls, which is OK, as that is done before the card is registered, so + no concurrent access can occur. + + It would be possible to do the DR in a tasklet after the ioctl finishes. + However, it is not obvious what actual problem that would solve, and the + added asynchronicity would significantly complicate matters, esp. wrt. + the batch updates expected during mixer state restoration. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> + --- + v2: + - expanded commit message ## include/sound/emu10k1.h ## @@ include/sound/emu10k1.h: struct snd_emu1010 { 7: d5cb50ca707f = 7: 72a156fb32cd ALSA: emu10k1: add high-rate capture in E-MU D.A.S. mode 8: 319425a4ccb6 ! 8: 6d35891832b3 ALSA: emu10k1: add high-rate playback in E-MU D.A.S. mode @@ Commit message d948035a92 ("Remove PCM xfer_align sw params"). Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> + --- + v2: + - fixed `sparse` warning re missing __user annotation ## sound/pci/emu10k1/emufx.c ## @@ sound/pci/emu10k1/emufx.c: static int _snd_emu10k1_das_init_efx(struct snd_emu10k1 *emu) @@ sound/pci/emu10k1/emupcm.c: static int snd_emu10k1_efx_playback_trigger(struct s + for (i = 0; i < channels; i++) { + for (j = 0; j < subchans; j++) { + u32 *dst = get_dma_ptr_x(runtime, shift, i, j, hwoff); -+ u32 *src = (u32 *)buf + j * channels + i; ++ u32 __user *src = (u32 __user *)buf + j * channels + i; + for (k = 0; k < frames; k++, dst++, src += voices) + unsafe_get_user(*dst, src, faulted); + }