[5/5] ALSA: firewire: use nonatomic PCM operation
Checks
Commit Message
In the former commits, the callback of isochronous context runs on usual
work process. In the case, ALSA PCM device has a flag, nonatomic, to
acquire mutex lock instead of spin lock for PCM substream group.
This commit uses the flag. It has an advantage in the case that ALSA PCM
application uses the large size of intermediate buffer, since it takes
too long time even in tasklet softIRQ to process many of isochronous
packets, then result in the delay of system event due to disabled IRQ so
long. It is avertible to switch to nonatomic operation.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
sound/firewire/amdtp-stream.c | 34 +++++++++++++++++++-----
sound/firewire/bebob/bebob_pcm.c | 1 +
sound/firewire/dice/dice-pcm.c | 1 +
sound/firewire/digi00x/digi00x-pcm.c | 1 +
sound/firewire/fireface/ff-pcm.c | 1 +
sound/firewire/fireworks/fireworks_pcm.c | 1 +
sound/firewire/isight.c | 1 +
sound/firewire/motu/motu-pcm.c | 1 +
sound/firewire/oxfw/oxfw-pcm.c | 1 +
sound/firewire/tascam/tascam-pcm.c | 1 +
10 files changed, 36 insertions(+), 7 deletions(-)
Comments
On Wed, 04 Sep 2024 14:51:54 +0200,
Takashi Sakamoto wrote:
>
> In the former commits, the callback of isochronous context runs on usual
> work process. In the case, ALSA PCM device has a flag, nonatomic, to
> acquire mutex lock instead of spin lock for PCM substream group.
>
> This commit uses the flag. It has an advantage in the case that ALSA PCM
> application uses the large size of intermediate buffer, since it takes
> too long time even in tasklet softIRQ to process many of isochronous
> packets, then result in the delay of system event due to disabled IRQ so
> long. It is avertible to switch to nonatomic operation.
>
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Feel free to take my ack:
Reviewed-by: Takashi Iwai <tiwai@suse.de>
thanks,
Takashi
On Wed, Sep 04, 2024 at 06:07:54PM +0200, Takashi Iwai wrote:
> On Wed, 04 Sep 2024 14:51:54 +0200,
> Takashi Sakamoto wrote:
> >
> > In the former commits, the callback of isochronous context runs on usual
> > work process. In the case, ALSA PCM device has a flag, nonatomic, to
> > acquire mutex lock instead of spin lock for PCM substream group.
> >
> > This commit uses the flag. It has an advantage in the case that ALSA PCM
> > application uses the large size of intermediate buffer, since it takes
> > too long time even in tasklet softIRQ to process many of isochronous
> > packets, then result in the delay of system event due to disabled IRQ so
> > long. It is avertible to switch to nonatomic operation.
> >
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>
> Feel free to take my ack:
>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
Thanks for your ack ;)
Regards
Takashi Sakamoto
@@ -615,6 +615,22 @@ static void update_pcm_pointers(struct amdtp_stream *s,
// The program in user process should periodically check the status of intermediate
// buffer associated to PCM substream to process PCM frames in the buffer, instead
// of receiving notification of period elapsed by poll wait.
+ //
+ // Use another work item for period elapsed event to prevent the following AB/BA
+ // deadlock:
+ //
+ // thread 1 thread 2
+ // ================================= =================================
+ // A.work item (process) pcm ioctl (process)
+ // v v
+ // process_rx_packets() B.PCM stream lock
+ // process_tx_packets() v
+ // v callbacks in snd_pcm_ops
+ // update_pcm_pointers() v
+ // snd_pcm_elapsed() fw_iso_context_flush_completions()
+ // snd_pcm_stream_lock_irqsave() disable_work_sync()
+ // v v
+ // wait until release of B wait until A exits
if (!pcm->runtime->no_period_wakeup)
queue_work(system_highpri_wq, &s->period_work);
}
@@ -1055,8 +1071,15 @@ static void generate_rx_packet_descs(struct amdtp_stream *s, struct pkt_desc *de
static inline void cancel_stream(struct amdtp_stream *s)
{
+ struct work_struct *work = current_work();
+
s->packet_index = -1;
- if (in_softirq())
+
+ // Detect work items for any isochronous context. The work item for pcm_period_work()
+ // should be avoided since the call of snd_pcm_period_elapsed() can reach via
+ // snd_pcm_ops.pointer() under acquiring PCM stream(group) lock and causes dead lock at
+ // snd_pcm_stop_xrun().
+ if (work && work != &s->period_work)
amdtp_stream_pcm_abort(s);
WRITE_ONCE(s->pcm_buffer_pointer, SNDRV_PCM_POS_XRUN);
}
@@ -1856,12 +1879,9 @@ unsigned long amdtp_domain_stream_pcm_pointer(struct amdtp_domain *d,
struct amdtp_stream *irq_target = d->irq_target;
if (irq_target && amdtp_stream_running(irq_target)) {
- // use wq to prevent AB/BA deadlock competition for
- // substream lock:
- // fw_iso_context_flush_completions() acquires
- // lock by ohci_flush_iso_completions(),
- // amdtp-stream process_rx_packets() attempts to
- // acquire same lock by snd_pcm_elapsed()
+ // The work item to call snd_pcm_period_elapsed() can reach here by the call of
+ // snd_pcm_ops.pointer(), however less packets would be available then. Therefore
+ // the following call is just for user process contexts.
if (current_work() != &s->period_work)
fw_iso_context_flush_completions(irq_target->context);
}
@@ -367,6 +367,7 @@ int snd_bebob_create_pcm_devices(struct snd_bebob *bebob)
goto end;
pcm->private_data = bebob;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", bebob->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
@@ -441,6 +441,7 @@ int snd_dice_create_pcm(struct snd_dice *dice)
if (err < 0)
return err;
pcm->private_data = dice;
+ pcm->nonatomic = true;
strcpy(pcm->name, dice->card->shortname);
if (capture > 0)
@@ -350,6 +350,7 @@ int snd_dg00x_create_pcm_devices(struct snd_dg00x *dg00x)
return err;
pcm->private_data = dg00x;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", dg00x->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
@@ -390,6 +390,7 @@ int snd_ff_create_pcm_devices(struct snd_ff *ff)
return err;
pcm->private_data = ff;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", ff->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &pcm_playback_ops);
@@ -397,6 +397,7 @@ int snd_efw_create_pcm_devices(struct snd_efw *efw)
goto end;
pcm->private_data = efw;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name), "%s PCM", efw->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &capture_ops);
@@ -454,6 +454,7 @@ static int isight_create_pcm(struct isight *isight)
if (err < 0)
return err;
pcm->private_data = isight;
+ pcm->nonatomic = true;
strcpy(pcm->name, "iSight");
isight->pcm = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream;
isight->pcm->ops = &ops;
@@ -360,6 +360,7 @@ int snd_motu_create_pcm_devices(struct snd_motu *motu)
if (err < 0)
return err;
pcm->private_data = motu;
+ pcm->nonatomic = true;
strcpy(pcm->name, motu->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &capture_ops);
@@ -440,6 +440,7 @@ int snd_oxfw_create_pcm(struct snd_oxfw *oxfw)
return err;
pcm->private_data = oxfw;
+ pcm->nonatomic = true;
strcpy(pcm->name, oxfw->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
if (cap > 0)
@@ -279,6 +279,7 @@ int snd_tscm_create_pcm_devices(struct snd_tscm *tscm)
return err;
pcm->private_data = tscm;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", tscm->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);