[PATCH 0/3] ALSA: pcm:firewire: allow to operate for period elapse event in process context

[PATCH 0/3] ALSA: pcm:firewire: allow to operate for period elapse event in process context

[Takashi Sakamoto]

Hi,

This patchset comes from my former RFC:

[RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context
 * https://lore.kernel.org/alsa-devel/20210606091838.80812-1-o-takashi@sakamocchi.jp/

All of drivers in ALSA firewire stack have two chances to process
isochronous packets of any isochronous context; in software IRQ context
for 1394 OHCI, and in process context of ALSA PCM application.

In the process context, callbacks of .pointer and .ack are utilized. The
callbacks are done by ALSA PCM core under acquiring lock of PCM substream,

In design of ALSA PCM core, call of snd_pcm_period_elapsed() is used for
drivers to awaken user processes from waiting for available frames. The
function voluntarily acquires lock of PCM substream, therefore it is not
called in the process context since it causes dead lock. As a workaround
to avoid the dead lock, all of drivers in ALSA firewire stack use workqueue
to delegate the call.

This patchset is my attempt for the issue. A variant of 
'snd_pcm_period_elapsed()' without lock acquisition is going to be added,
named 'snd_pcm_period_elapsed_under_stream_lock()'. The call is available
in callbacks of .pointer and .ack of snd_pcm_ops structure.

Changes from RFC:
 * dismiss inlining
 * rename function name
 * improve function comments

I tested the patchset with ALSA OXFW driver and ftrace, covering axfer and
jackd cases since pulseaudio and pipewire are programmed with timer-based
scheduling model and ALSA runtime expects drivers not to call
snd_pcm_period_elapsed().

The configuration of tracer and filters is:

```
$ sudo trace-cmd record -p function_graph \
    -l :mod:snd_firewire_lib \
    -l fw_iso_context_queue:mod:firewire_core \
    -l snd_pcm_ioctl:mod:snd_pcm \
    -l snd_pcm_period_elapsed*:mod:snd_pcm 
```

The runtime of axfer without '-M' option can often call .pointer and .ack.
Below is a sample about .pointer case via SNDRV_PCM_IOCT_HWSYNC. The new
snd_pcm_period_elapsed_under_stream_lock() should be called.

```
  3929.769359: funcgraph_entry:           | snd_pcm_ioctl() {
  3929.769360: funcgraph_entry:           |   amdtp_domain_stream_pcm_pointer() {
  3929.769361: funcgraph_entry:           |     irq_target_callback() {
  3929.769361: funcgraph_entry:           |       process_rx_packets() {
  3929.769362: funcgraph_entry:           |         process_ctx_payloads() {
  3929.769363: funcgraph_entry: 0.391 us  |           process_it_ctx_payloads();
  3929.769363: funcgraph_entry:           |           snd_pcm_period_elapsed_under_stream_lock() {
  3929.769364: funcgraph_entry: 0.411 us  |             amdtp_domain_stream_pcm_pointer();
  3929.769365: funcgraph_exit:  1.723 us  |           }
  3929.769365: funcgraph_exit:  3.106 us  |         }
  3929.769366: funcgraph_entry:           |         queue_packet() {
  3929.769366: funcgraph_entry: 0.561 us  |           fw_iso_context_queue();
  3929.769367: funcgraph_exit:  1.122 us  |         }
  3929.769367: funcgraph_exit:  5.731 us  |       }
  3929.769367: funcgraph_entry:           |       process_ctxs_in_domain() {
  3929.769368: funcgraph_entry:           |         process_tx_packets() {
  3929.769369: funcgraph_entry:           |           process_ctx_payloads() {
  3929.769369: funcgraph_entry: 0.321 us  |             process_ir_ctx_payloads();
  3929.769369: funcgraph_exit:  0.962 us  |           }
  3929.769370: funcgraph_entry: 0.491 us  |           fw_iso_context_queue();
  3929.769371: funcgraph_exit:  2.364 us  |         }
  3929.769371: funcgraph_exit:  3.427 us  |       }
  3929.769371: funcgraph_exit:  10.038 us |     }
  3929.769372: funcgraph_exit:  11.271 us |   }
  3929.769372: funcgraph_exit:  13.606 us | }
```

The runtime of jackd heavily relies on period wakeup scheduled in
invocation of interrupt handler. Below is a sample about software IRQ case
of 1394 OHCI. The snd_pcm_period_elapsed() should be called.

```
  5318.980502: funcgraph_entry:           | irq_target_callback() {
  5318.980503: funcgraph_entry:           |   process_rx_packets() {
  5318.980503: funcgraph_entry:           |     process_ctx_payloads() {
  5318.980503: funcgraph_entry: 0.531 us  |       process_it_ctx_payloads();
  5318.980505: funcgraph_entry:           |       snd_pcm_period_elapsed() {
  5318.980505: funcgraph_entry:           |         snd_pcm_period_elapsed_under_stream_lock() {
  5318.980505: funcgraph_entry: 0.180 us  |           amdtp_domain_stream_pcm_pointer();
  5318.980508: funcgraph_exit:  3.106 us  |         }
  5318.980508: funcgraph_exit:  3.497 us  |       }
  5318.980508: funcgraph_exit:  4.990 us  |     }
  5318.980508: funcgraph_entry:           |     queue_packet() {
  5318.980509: funcgraph_entry: 0.350 us  |       fw_iso_context_queue();
  5318.980509: funcgraph_exit:  0.702 us  |     }
  5318.980509: funcgraph_entry:           |     queue_packet() {
  5318.980509: funcgraph_entry: 0.291 us  |       fw_iso_context_queue();
  5318.980510: funcgraph_exit:  0.651 us  |     }
  5318.980510: funcgraph_entry:           |     queue_packet() {
  5318.980510: funcgraph_entry: 0.280 us  |       fw_iso_context_queue();
  5318.980511: funcgraph_exit:  0.641 us  |     }
  5318.980511: funcgraph_exit:  8.416 us  |   }
  5318.980511: funcgraph_entry:           |   process_ctxs_in_domain() {
  5318.980512: funcgraph_entry:           |     process_tx_packets() {
  5318.980512: funcgraph_entry:           |       process_ctx_payloads() {
  5318.980512: funcgraph_entry: 0.531 us  |         process_ir_ctx_payloads();
  5318.980513: funcgraph_entry:           |         snd_pcm_period_elapsed() {
  5318.980513: funcgraph_entry:           |           snd_pcm_period_elapsed_under_stream_lock() {
  5318.980513: funcgraph_entry: 0.170 us  |             amdtp_domain_stream_pcm_pointer();
  5318.980514: funcgraph_exit:  0.842 us  |           }
  5318.980514: funcgraph_exit:  1.242 us  |         }
  5318.980514: funcgraph_exit:  2.335 us  |       }
  5318.980515: funcgraph_entry: 0.301 us  |       fw_iso_context_queue();
  5318.980515: funcgraph_entry: 0.291 us  |       fw_iso_context_queue();
  5318.980516: funcgraph_entry: 0.290 us  |       fw_iso_context_queue();
  5318.980516: funcgraph_exit:  4.198 us  |     }
  5318.980516: funcgraph_exit:  5.119 us  |   }
  5318.980516: funcgraph_exit:  14.077 us | }
```

ALSA OXFW driver works well to select appropriate kernel API by
distinguishing running context.

I also tested for error cases that the sequence of tx packets is invalid
or packet queueing fails. In both cases, the error doesn't bring system
corruption.


Takashi Sakamoto (3):
  ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock
    of PCM substream
  ALSA: firewire-lib: operate for period elapse event in process
    context
  ALSA: firewire-lib: obsolete workqueue for period update

 include/sound/pcm.h           |  1 +
 sound/core/pcm_lib.c          | 68 +++++++++++++++++++++++++++--------
 sound/firewire/amdtp-stream.c | 46 ++++++++----------------
 sound/firewire/amdtp-stream.h |  1 -
 4 files changed, 68 insertions(+), 48 deletions(-)

-- 
2.27.0

[PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream

[Takashi Sakamoto]

Current implementation of ALSA PCM core has a kernel API,
snd_pcm_period_elapsed(), for drivers to awaken processes from waiting for
available frames. The function voluntarily acquires lock of PCM substream,
therefore it is not called in process context for any PCM operation since
the lock is already acquired.

The call in process context is convenient for packet-oriented driver, at
least for drivers to audio and music unit in IEEE 1394 bus. The drivers
are allowed by Linux FireWire subsystem to process isochronous packets
queued till recent isochronous cycle in process context in any time.

This commit adds snd_pcm_period_elapsed() variant,
snd_pcm_period_elapsed_under_stream_lock(), for drivers to queue the event
in the process context.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/pcm.h  |  1 +
 sound/core/pcm_lib.c | 68 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2e1200d17d0c..bae90696cd06 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -1066,6 +1066,7 @@ void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
 void snd_pcm_set_sync(struct snd_pcm_substream *substream);
 int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
 		      unsigned int cmd, void *arg);                      
+void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream);
 void snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
 snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
 				     void *buf, bool interleaved,
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index b7e3d8f44511..6f01b0c805ca 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1778,27 +1778,40 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
 EXPORT_SYMBOL(snd_pcm_lib_ioctl);
 
 /**
- * snd_pcm_period_elapsed - update the pcm status for the next period
- * @substream: the pcm substream instance
+ * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
+ *						under acquired lock of PCM substream.
+ * @substream: the instance of pcm substream.
+ *
+ * The function is called when the batch of audio data frames as the same size as the period of
+ * buffer is already processed in audio data transmission.
+ *
+ * The call of function updates the status of runtime with the latest position of audio data
+ * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
+ * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
+ * substream according to configured threshold.
+ *
+ * The function is intended to use for the case that PCM driver operates audio data frames under
+ * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
+ * context. In any interrupt context, it's preferable to use ``snd_pcm_period_elapsed()`` instead
+ * since lock of PCM substream should be acquired in advance.
  *
- * This function is called from the interrupt handler when the
- * PCM has processed the period size.  It will update the current
- * pointer, wake up sleepers, etc.
+ * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
+ * function:
  *
- * Even if more than one periods have elapsed since the last call, you
- * have to call this only once.
+ * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
+ * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
+ * - .get_time_info - to retrieve audio time stamp if needed.
+ *
+ * Even if more than one periods have elapsed since the last call, you have to call this only once.
+ *
+ * Context: Any context under acquired lock of PCM substream. This function may not sleep.
  */
-void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
+void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime;
-	unsigned long flags;
-
-	if (snd_BUG_ON(!substream))
-		return;
 
-	snd_pcm_stream_lock_irqsave(substream, flags);
 	if (PCM_RUNTIME_CHECK(substream))
-		goto _unlock;
+		return;
 	runtime = substream->runtime;
 
 	if (!snd_pcm_running(substream) ||
@@ -1811,7 +1824,32 @@ void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
 #endif
  _end:
 	kill_fasync(&runtime->fasync, SIGIO, POLL_IN);
- _unlock:
+}
+EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock);
+
+/**
+ * snd_pcm_period_elapsed() - update the status of runtime for the next period by acquiring lock of
+ *			      PCM substream.
+ * @substream: the instance of PCM substream.
+ *
+ * The function is mostly similar to ``snd_pcm_period_elapsed_under_stream_lock()`` except for
+ * acquiring lock of PCM substream voluntarily.
+ *
+ * It's typically called by any type of IRQ handler when hardware IRQ occurs to notify event that
+ * the batch of audio data frames as the same size as the period of buffer is already processed in
+ * audio data transmission.
+ *
+ * Context: Interrupt context without acquired lock of PCM substream. This function may not sleep.
+ */
+void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
+{
+	unsigned long flags;
+
+	if (snd_BUG_ON(!substream))
+		return;
+
+	snd_pcm_stream_lock_irqsave(substream, flags);
+	snd_pcm_period_elapsed_under_stream_lock(substream);
 	snd_pcm_stream_unlock_irqrestore(substream, flags);
 }
 EXPORT_SYMBOL(snd_pcm_period_elapsed);
-- 
2.27.0

[PATCH 2/3] ALSA: firewire-lib: operate for period elapse event in process context

[Takashi Sakamoto]

All of drivers in ALSA firewire stack processes two chances to process
isochronous packets of any isochronous context; in software IRQ context
for 1394 OHCI, and in process context of ALSA PCM application.

In the process context, callbacks of .pointer and .ack are utilized. The
callbacks are done by ALSA PCM core under acquiring lock of PCM substream,

In design of ALSA PCM core, call of snd_pcm_period_elapsed() is used for
drivers to awaken user processes from waiting for available frames. The
function voluntarily acquires lock of PCM substream, therefore it is not
called in the process context since it causes dead lock.

As a workaround to avoid the dead lock, all of drivers in ALSA firewire
stack uses workqueue to delegate the call.  A variant of
snd_pcm_period_elapsed() without lock acquisition can obsolete the
workqueue.

An extra care is needed for the callback of .pointer since it's called
from snd_pcm_period_elapsed(). The isochronous context in Linux FireWire
subsystem is safe mostly for nested call except in software IRQ context.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 150ee0b9e707..426a85b56cf1 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -613,8 +613,16 @@ 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.
-		if (!pcm->runtime->no_period_wakeup)
-			queue_work(system_highpri_wq, &s->period_work);
+		if (!pcm->runtime->no_period_wakeup) {
+			if (in_interrupt()) {
+				// In software IRQ context for 1394 OHCI.
+				snd_pcm_period_elapsed(pcm);
+			} else {
+				// In process context of ALSA PCM application under acquired lock of
+				// PCM substream.
+				snd_pcm_period_elapsed_under_stream_lock(pcm);
+			}
+		}
 	}
 }
 
@@ -1740,22 +1748,11 @@ unsigned long amdtp_domain_stream_pcm_pointer(struct amdtp_domain *d,
 {
 	struct amdtp_stream *irq_target = d->irq_target;
 
+	// Process isochronous packets queued till recent isochronous cycle to handle PCM frames.
 	if (irq_target && amdtp_stream_running(irq_target)) {
-		// This function is called in software IRQ context of
-		// period_work or process context.
-		//
-		// When the software IRQ context was scheduled by software IRQ
-		// context of IT contexts, queued packets were already handled.
-		// Therefore, no need to flush the queue in buffer furthermore.
-		//
-		// When the process context reach here, some packets will be
-		// already queued in the buffer. These packets should be handled
-		// immediately to keep better granularity of PCM pointer.
-		//
-		// Later, the process context will sometimes schedules software
-		// IRQ context of the period_work. Then, no need to flush the
-		// queue by the same reason as described in the above
-		if (current_work() != &s->period_work)
+		// In software IRQ context, the call causes dead-lock to disable the tasklet
+		// synchronously.
+		if (!in_interrupt())
 			fw_iso_context_flush_completions(irq_target->context);
 	}
 
-- 
2.27.0

[PATCH 3/3] ALSA: firewire-lib: obsolete workqueue for period update

[Takashi Sakamoto]

The workqueue to notify PCM period elapse is not used anymore.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 15 ---------------
 sound/firewire/amdtp-stream.h |  1 -
 2 files changed, 16 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 426a85b56cf1..1d9bc7b07df1 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -77,8 +77,6 @@
 // overrun. Actual device can skip more, then this module stops the packet streaming.
 #define IR_JUMBO_PAYLOAD_MAX_SKIP_CYCLES	5
 
-static void pcm_period_work(struct work_struct *work);
-
 /**
  * amdtp_stream_init - initialize an AMDTP stream structure
  * @s: the AMDTP stream to initialize
@@ -107,7 +105,6 @@ int amdtp_stream_init(struct amdtp_stream *s, struct fw_unit *unit,
 	s->flags = flags;
 	s->context = ERR_PTR(-1);
 	mutex_init(&s->mutex);
-	INIT_WORK(&s->period_work, pcm_period_work);
 	s->packet_index = 0;
 
 	init_waitqueue_head(&s->ready_wait);
@@ -346,7 +343,6 @@ EXPORT_SYMBOL(amdtp_stream_get_max_payload);
  */
 void amdtp_stream_pcm_prepare(struct amdtp_stream *s)
 {
-	cancel_work_sync(&s->period_work);
 	s->pcm_buffer_pointer = 0;
 	s->pcm_period_pointer = 0;
 }
@@ -626,16 +622,6 @@ static void update_pcm_pointers(struct amdtp_stream *s,
 	}
 }
 
-static void pcm_period_work(struct work_struct *work)
-{
-	struct amdtp_stream *s = container_of(work, struct amdtp_stream,
-					      period_work);
-	struct snd_pcm_substream *pcm = READ_ONCE(s->pcm);
-
-	if (pcm)
-		snd_pcm_period_elapsed(pcm);
-}
-
 static int queue_packet(struct amdtp_stream *s, struct fw_iso_packet *params,
 			bool sched_irq)
 {
@@ -1808,7 +1794,6 @@ static void amdtp_stream_stop(struct amdtp_stream *s)
 		return;
 	}
 
-	cancel_work_sync(&s->period_work);
 	fw_iso_context_stop(s->context);
 	fw_iso_context_destroy(s->context);
 	s->context = ERR_PTR(-1);
diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h
index b25592d5f6af..1f957c946c95 100644
--- a/sound/firewire/amdtp-stream.h
+++ b/sound/firewire/amdtp-stream.h
@@ -186,7 +186,6 @@ struct amdtp_stream {
 
 	/* For a PCM substream processing. */
 	struct snd_pcm_substream *pcm;
-	struct work_struct period_work;
 	snd_pcm_uframes_t pcm_buffer_pointer;
 	unsigned int pcm_period_pointer;
 
-- 
2.27.0

Re: [PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream

[Takashi Iwai]

On Wed, 09 Jun 2021 03:22:42 +0200,
Takashi Sakamoto wrote:
> 
> Current implementation of ALSA PCM core has a kernel API,
> snd_pcm_period_elapsed(), for drivers to awaken processes from waiting for
> available frames. The function voluntarily acquires lock of PCM substream,
> therefore it is not called in process context for any PCM operation since
> the lock is already acquired.
> 
> The call in process context is convenient for packet-oriented driver, at
> least for drivers to audio and music unit in IEEE 1394 bus. The drivers
> are allowed by Linux FireWire subsystem to process isochronous packets
> queued till recent isochronous cycle in process context in any time.
> 
> This commit adds snd_pcm_period_elapsed() variant,
> snd_pcm_period_elapsed_under_stream_lock(), for drivers to queue the event
> in the process context.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/sound/pcm.h  |  1 +
>  sound/core/pcm_lib.c | 68 ++++++++++++++++++++++++++++++++++----------
>  2 files changed, 54 insertions(+), 15 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 2e1200d17d0c..bae90696cd06 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -1066,6 +1066,7 @@ void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
>  void snd_pcm_set_sync(struct snd_pcm_substream *substream);
>  int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  		      unsigned int cmd, void *arg);                      
> +void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream);
>  void snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
>  snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
>  				     void *buf, bool interleaved,
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index b7e3d8f44511..6f01b0c805ca 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1778,27 +1778,40 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
>  
>  /**
> - * snd_pcm_period_elapsed - update the pcm status for the next period
> - * @substream: the pcm substream instance
> + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> + *						under acquired lock of PCM substream.
> + * @substream: the instance of pcm substream.
> + *
> + * The function is called when the batch of audio data frames as the same size as the period of
> + * buffer is already processed in audio data transmission.
> + *
> + * The call of function updates the status of runtime with the latest position of audio data
> + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> + * substream according to configured threshold.
> + *
> + * The function is intended to use for the case that PCM driver operates audio data frames under
> + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> + * context. In any interrupt context, it's preferable to use ``snd_pcm_period_elapsed()`` instead
> + * since lock of PCM substream should be acquired in advance.
>   *
> - * This function is called from the interrupt handler when the
> - * PCM has processed the period size.  It will update the current
> - * pointer, wake up sleepers, etc.
> + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> + * function:
>   *
> - * Even if more than one periods have elapsed since the last call, you
> - * have to call this only once.
> + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> + * - .get_time_info - to retrieve audio time stamp if needed.
> + *
> + * Even if more than one periods have elapsed since the last call, you have to call this only once.
> + *
> + * Context: Any context under acquired lock of PCM substream. This function may not sleep.

Actually it may sleep if the PCM is nonatomic mode; then the stream
lock is a mutex instead of a spinlock.


Takashi

Re: [PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream

[Takashi Sakamoto]

On Wed, Jun 09, 2021 at 09:12:20AM +0200, Takashi Iwai wrote:
> On Wed, 09 Jun 2021 03:22:42 +0200,
> Takashi Sakamoto wrote:
> > 
> > Current implementation of ALSA PCM core has a kernel API,
> > snd_pcm_period_elapsed(), for drivers to awaken processes from waiting for
> > available frames. The function voluntarily acquires lock of PCM substream,
> > therefore it is not called in process context for any PCM operation since
> > the lock is already acquired.
> > 
> > The call in process context is convenient for packet-oriented driver, at
> > least for drivers to audio and music unit in IEEE 1394 bus. The drivers
> > are allowed by Linux FireWire subsystem to process isochronous packets
> > queued till recent isochronous cycle in process context in any time.
> > 
> > This commit adds snd_pcm_period_elapsed() variant,
> > snd_pcm_period_elapsed_under_stream_lock(), for drivers to queue the event
> > in the process context.
> > 
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > ---
> >  include/sound/pcm.h  |  1 +
> >  sound/core/pcm_lib.c | 68 ++++++++++++++++++++++++++++++++++----------
> >  2 files changed, 54 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index 2e1200d17d0c..bae90696cd06 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -1066,6 +1066,7 @@ void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
> >  void snd_pcm_set_sync(struct snd_pcm_substream *substream);
> >  int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> >  		      unsigned int cmd, void *arg);                      
> > +void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream);
> >  void snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
> >  snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
> >  				     void *buf, bool interleaved,
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index b7e3d8f44511..6f01b0c805ca 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -1778,27 +1778,40 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
> >  EXPORT_SYMBOL(snd_pcm_lib_ioctl);
> >  
> >  /**
> > - * snd_pcm_period_elapsed - update the pcm status for the next period
> > - * @substream: the pcm substream instance
> > + * snd_pcm_period_elapsed_under_stream_lock() - update the status of runtime for the next period
> > + *						under acquired lock of PCM substream.
> > + * @substream: the instance of pcm substream.
> > + *
> > + * The function is called when the batch of audio data frames as the same size as the period of
> > + * buffer is already processed in audio data transmission.
> > + *
> > + * The call of function updates the status of runtime with the latest position of audio data
> > + * transmission, checks overrun and underrun over buffer, awaken user processes from waiting for
> > + * available audio data frames, sampling audio timestamp, and performs stop or drain the PCM
> > + * substream according to configured threshold.
> > + *
> > + * The function is intended to use for the case that PCM driver operates audio data frames under
> > + * acquired lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
> > + * context. In any interrupt context, it's preferable to use ``snd_pcm_period_elapsed()`` instead
> > + * since lock of PCM substream should be acquired in advance.
> >   *
> > - * This function is called from the interrupt handler when the
> > - * PCM has processed the period size.  It will update the current
> > - * pointer, wake up sleepers, etc.
> > + * Developer should pay enough attention that some callbacks in &snd_pcm_ops are done by the call of
> > + * function:
> >   *
> > - * Even if more than one periods have elapsed since the last call, you
> > - * have to call this only once.
> > + * - .pointer - to retrieve current position of audio data transmission by frame count or XRUN state.
> > + * - .trigger - with SNDRV_PCM_TRIGGER_STOP at XRUN or DRAINING state.
> > + * - .get_time_info - to retrieve audio time stamp if needed.
> > + *
> > + * Even if more than one periods have elapsed since the last call, you have to call this only once.
> > + *
> > + * Context: Any context under acquired lock of PCM substream. This function may not sleep.
> 
> Actually it may sleep if the PCM is nonatomic mode; then the stream
> lock is a mutex instead of a spinlock.
 
Ah... Yes. I have less care of the case when writing it. I'm going to post
revised version this night.


Thanks

Takashi Sakamoto