All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context
@ 2021-06-06  9:18 Takashi Sakamoto
  2021-06-06  9:18 ` [RFC][PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream Takashi Sakamoto
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2021-06-06  9:18 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel, clemens

Hi,

All of drivers in ALSA firewire stack processes two chances to process
isochronous packets in 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.

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_without_lock()'. This is used in callbacks
of .pointer and .ack of snd_pcm_ops structure.

The patchset is still under my test, but it looks to work well in my
easy and rough test. Before posting for merge, I'd like to get your
comment to the idea. When evaluating, please merge below two histories:
 * 64584f329352 (for-next)
 * 9981b20a5e36 (for-linus)

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

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

-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC][PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream
  2021-06-06  9:18 [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Sakamoto
@ 2021-06-06  9:18 ` Takashi Sakamoto
  2021-06-06  9:18 ` [RFC][PATCH 2/3] ALSA: firewire-lib: queue event of period elapse in process context Takashi Sakamoto
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2021-06-06  9:18 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel, clemens

Current implementation of ALSA PCM core has a kernel API,
snd_pcm_period_elapsed(), for drivers to queue event 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.

It 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_without_lock(), for drivers to queue the event in
the process context.
---
 include/sound/pcm.h  | 53 +++++++++++++++++++++++++++++++++++++++++++-
 sound/core/pcm_lib.c | 25 ++++-----------------
 2 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2e1200d17d0c..a4eaa48584da 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -11,6 +11,7 @@
 #include <sound/asound.h>
 #include <sound/memalloc.h>
 #include <sound/minors.h>
+#include <sound/core.h>
 #include <linux/poll.h>
 #include <linux/mm.h>
 #include <linux/bitops.h>
@@ -1066,7 +1067,57 @@ 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(struct snd_pcm_substream *substream);
+
+void __snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
+
+/**
+ * snd_pcm_period_elapsed - update the pcm status for the next period
+ * @substream: the pcm substream instance
+ *
+ * This function is called when the batch of PCM frames as the same as period of PCM buffer are
+ * processed in audio data transmission. It's typically called by any type of IRQ handler when
+ * hardware IRQ occurs to notify the event. It acquires lock of PCM substream, then 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:
+ *
+ * - .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.
+ *
+ * Even if more than one periods have elapsed since the last call, you have to call this only once.
+ */
+static inline 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(substream);
+	snd_pcm_stream_unlock_irqrestore(substream, flags);
+}
+
+/**
+ * snd_pcm_period_elapsed_without_lock() - update the pcm status for the next period without
+ *					   acquiring lock of PCM substream.
+ * @substream: the pcm substream instance
+ *
+ * This function is variant of ``snd_pcm_period_elapsed()`` without voluntarily acquiring lock of
+ * PCM substream. It's intended to use for the case that PCM driver operates PCM frames under
+ * acquiring lock of PCM substream; e.g. in callback of any operation of &snd_pcm_ops in process
+ * context, then queueing period wakeup event.
+ */
+static inline void snd_pcm_period_elapsed_without_lock(struct snd_pcm_substream *substream)
+{
+	if (snd_BUG_ON(!substream))
+		return;
+
+	__snd_pcm_period_elapsed(substream);
+}
+
 snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
 				     void *buf, bool interleaved,
 				     snd_pcm_uframes_t frames, bool in_kernel);
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index b7e3d8f44511..33ad4ab0ec3a 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1777,28 +1777,13 @@ 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
- *
- * 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.
- *
- * Even if more than one periods have elapsed since the last call, you
- * have to call this only once.
- */
-void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
+// The pointer to PCM substream should not be NULL as precondition.
+void __snd_pcm_period_elapsed(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,10 +1796,8 @@ void snd_pcm_period_elapsed(struct snd_pcm_substream *substream)
 #endif
  _end:
 	kill_fasync(&runtime->fasync, SIGIO, POLL_IN);
- _unlock:
-	snd_pcm_stream_unlock_irqrestore(substream, flags);
 }
-EXPORT_SYMBOL(snd_pcm_period_elapsed);
+EXPORT_SYMBOL(__snd_pcm_period_elapsed);
 
 /*
  * Wait until avail_min data becomes available
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC][PATCH 2/3] ALSA: firewire-lib: queue event of period elapse in process context
  2021-06-06  9:18 [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Sakamoto
  2021-06-06  9:18 ` [RFC][PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream Takashi Sakamoto
@ 2021-06-06  9:18 ` Takashi Sakamoto
  2021-06-06  9:18 ` [RFC][PATCH 3/3] ALSA: firewire-lib: obsolete workqueue for period update Takashi Sakamoto
  2021-06-06 10:20 ` [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Iwai
  3 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2021-06-06  9:18 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel, clemens

All of drivers in ALSA firewire stack processes two chances to process
isochronous packets in 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.
---
 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..b39328eab67e 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 acquiring lock
+				// of PCM substream.
+				snd_pcm_period_elapsed_without_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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC][PATCH 3/3] ALSA: firewire-lib: obsolete workqueue for period update
  2021-06-06  9:18 [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Sakamoto
  2021-06-06  9:18 ` [RFC][PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream Takashi Sakamoto
  2021-06-06  9:18 ` [RFC][PATCH 2/3] ALSA: firewire-lib: queue event of period elapse in process context Takashi Sakamoto
@ 2021-06-06  9:18 ` Takashi Sakamoto
  2021-06-06 10:20 ` [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Iwai
  3 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2021-06-06  9:18 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel, clemens

The workqueue to notify PCM period elapse is not used anymore.
---
 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 b39328eab67e..7946d5e800c1 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context
  2021-06-06  9:18 [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Sakamoto
                   ` (2 preceding siblings ...)
  2021-06-06  9:18 ` [RFC][PATCH 3/3] ALSA: firewire-lib: obsolete workqueue for period update Takashi Sakamoto
@ 2021-06-06 10:20 ` Takashi Iwai
  2021-06-07  3:05   ` Takashi Sakamoto
  3 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2021-06-06 10:20 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, clemens

On Sun, 06 Jun 2021 11:18:35 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> All of drivers in ALSA firewire stack processes two chances to process
> isochronous packets in 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.
> 
> 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_without_lock()'. This is used in callbacks
> of .pointer and .ack of snd_pcm_ops structure.
> 
> The patchset is still under my test, but it looks to work well in my
> easy and rough test. Before posting for merge, I'd like to get your
> comment to the idea. When evaluating, please merge below two histories:
>  * 64584f329352 (for-next)
>  * 9981b20a5e36 (for-linus)
> 
> Takashi Sakamoto (3):
>   ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock
>     of PCM substream
>   ALSA: firewire-lib: queue event of period elapse in process context
>   ALSA: firewire-lib: obsolete workqueue for period update

The idea is fine, but moving snd_pcm_period_elapsed() as inline static
doesn't give much benefit, IMO.  Although it can avoid an exported
symbol, its cost is much higher, since it'd expand the code at each
place of snd_pcm_period_elapsed(), i.e. almost in all driver code.
Just provide two exported functions instead in a more straightforward
way.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context
  2021-06-06 10:20 ` [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Iwai
@ 2021-06-07  3:05   ` Takashi Sakamoto
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2021-06-07  3:05 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, clemens

Hi,

On Sun, Jun 06, 2021 at 12:20:57PM +0200, Takashi Iwai wrote:
> On Sun, 06 Jun 2021 11:18:35 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > All of drivers in ALSA firewire stack processes two chances to process
> > isochronous packets in 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.
> > 
> > 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_without_lock()'. This is used in callbacks
> > of .pointer and .ack of snd_pcm_ops structure.
> > 
> > The patchset is still under my test, but it looks to work well in my
> > easy and rough test. Before posting for merge, I'd like to get your
> > comment to the idea. When evaluating, please merge below two histories:
> >  * 64584f329352 (for-next)
> >  * 9981b20a5e36 (for-linus)
> > 
> > Takashi Sakamoto (3):
> >   ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock
> >     of PCM substream
> >   ALSA: firewire-lib: queue event of period elapse in process context
> >   ALSA: firewire-lib: obsolete workqueue for period update
> 
> The idea is fine, but moving snd_pcm_period_elapsed() as inline static
> doesn't give much benefit, IMO.  Although it can avoid an exported
> symbol, its cost is much higher, since it'd expand the code at each
> place of snd_pcm_period_elapsed(), i.e. almost in all driver code.
> Just provide two exported functions instead in a more straightforward
> way.

Thanks for your positive comment.

I agree with it. When adding parameters for function internal, we will
discuss about the inlining for variations of kernel API again, I guess.

After merging for-linus branch into for-next branch, I'm going to post
it again. At the time, I may finish enough test.


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-07  3:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-06  9:18 [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Sakamoto
2021-06-06  9:18 ` [RFC][PATCH 1/3] ALSA: pcm: add snd_pcm_period_elapsed() variant without acquiring lock of PCM substream Takashi Sakamoto
2021-06-06  9:18 ` [RFC][PATCH 2/3] ALSA: firewire-lib: queue event of period elapse in process context Takashi Sakamoto
2021-06-06  9:18 ` [RFC][PATCH 3/3] ALSA: firewire-lib: obsolete workqueue for period update Takashi Sakamoto
2021-06-06 10:20 ` [RFC][PATCH 0/3] ALSA: pcm/firewire: allow to queue period elapse event in process context Takashi Iwai
2021-06-07  3:05   ` Takashi Sakamoto

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.