| Message ID | 20210609143145.146680-2-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: pcm:firewire: allow to operate for period elapse event in process context | expand |
On Wed, 09 Jun 2021 16:31:43 +0200, Takashi Sakamoto wrote: > > Current implementation of ALSA PCM core has a kernel API, > snd_pcm_period_elapsed(), 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 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 call in the > process context. > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > --- > include/sound/pcm.h | 1 + > sound/core/pcm_lib.c | 71 ++++++++++++++++++++++++++++++++++---------- > 2 files changed, 57 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..3488ec1e3674 100644 > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -1778,27 +1778,41 @@ 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. > + * > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > + * sleep. Hm, this text still remains here. Overlooked? Takashi
On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > On Wed, 09 Jun 2021 16:31:43 +0200, > Takashi Sakamoto wrote: > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > index b7e3d8f44511..3488ec1e3674 100644 > > --- a/sound/core/pcm_lib.c > > +++ b/sound/core/pcm_lib.c > > @@ -1778,27 +1778,41 @@ 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. > > + * > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > + * sleep. > > Hm, this text still remains here. Overlooked? It's my intension for documentation of snd_pcm_period_elapsed_under_stream_lock() since it's expected to call it under acquired lock. Its implementation doesn't yield processor voluntarily by itself. If it yielded, it would depend on implementation of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, but it's not preferable implementation of driver, in my opinion. Hm. Addition of context section seems to bring more discussion since we should consider about several types of context; e.g. threadirqs. Although the documentation for the detail is the part of my intension in the patchset, it's not the center. I'm sorry to reviewers but let me delete the section in next version... Thanks Takashi Sakamoto
On Thu, 10 Jun 2021 01:16:23 +0200, Takashi Sakamoto wrote: > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > On Wed, 09 Jun 2021 16:31:43 +0200, > > Takashi Sakamoto wrote: > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > index b7e3d8f44511..3488ec1e3674 100644 > > > --- a/sound/core/pcm_lib.c > > > +++ b/sound/core/pcm_lib.c > > > @@ -1778,27 +1778,41 @@ 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. > > > + * > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > + * sleep. > > > > Hm, this text still remains here. Overlooked? > > It's my intension for documentation of > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > it under acquired lock. Its implementation doesn't yield processor > voluntarily by itself. If it yielded, it would depend on implementation > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > but it's not preferable implementation of driver, in my opinion. My point is again about the sleep. This function may sleep in the nonatomic mode. The type of the PCM stream lock depends on it. Takashi
On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote: > On Thu, 10 Jun 2021 01:16:23 +0200, > Takashi Sakamoto wrote: > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > > On Wed, 09 Jun 2021 16:31:43 +0200, > > > Takashi Sakamoto wrote: > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > --- a/sound/core/pcm_lib.c > > > > +++ b/sound/core/pcm_lib.c > > > > @@ -1778,27 +1778,41 @@ 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. > > > > + * > > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > > + * sleep. > > > > > > Hm, this text still remains here. Overlooked? > > > > It's my intension for documentation of > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > > it under acquired lock. Its implementation doesn't yield processor > > voluntarily by itself. If it yielded, it would depend on implementation > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > > but it's not preferable implementation of driver, in my opinion. > > My point is again about the sleep. This function may sleep in the > nonatomic mode. The type of the PCM stream lock depends on it. Would I simply request you to show how the added function yields except for the driver implementation? The lock of stream is expected to be acquired already. Regards Takashi Sakamoto
On Thu, 10 Jun 2021 10:05:21 +0200, Takashi Sakamoto wrote: > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote: > > On Thu, 10 Jun 2021 01:16:23 +0200, > > Takashi Sakamoto wrote: > > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > > > On Wed, 09 Jun 2021 16:31:43 +0200, > > > > Takashi Sakamoto wrote: > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > > --- a/sound/core/pcm_lib.c > > > > > +++ b/sound/core/pcm_lib.c > > > > > @@ -1778,27 +1778,41 @@ 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. > > > > > + * > > > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > > > + * sleep. > > > > > > > > Hm, this text still remains here. Overlooked? > > > > > > It's my intension for documentation of > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > > > it under acquired lock. Its implementation doesn't yield processor > > > voluntarily by itself. If it yielded, it would depend on implementation > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > > > but it's not preferable implementation of driver, in my opinion. > > > > My point is again about the sleep. This function may sleep in the > > nonatomic mode. The type of the PCM stream lock depends on it. > > Would I simply request you to show how the added function yields except > for the driver implementation? The lock of stream is expected to be > acquired already. In the nonatomic mode, the PCM stream lock is a mutex (no spin_lock_irqsave), hence it can sleep -- which contradicts with the added description above. Or do I misunderstand your question...? Takashi
On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote: > On Thu, 10 Jun 2021 10:05:21 +0200, > Takashi Sakamoto wrote: > > > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote: > > > On Thu, 10 Jun 2021 01:16:23 +0200, > > > Takashi Sakamoto wrote: > > > > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > > > > On Wed, 09 Jun 2021 16:31:43 +0200, > > > > > Takashi Sakamoto wrote: > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > > > --- a/sound/core/pcm_lib.c > > > > > > +++ b/sound/core/pcm_lib.c > > > > > > @@ -1778,27 +1778,41 @@ 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. > > > > > > + * > > > > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > > > > + * sleep. > > > > > > > > > > Hm, this text still remains here. Overlooked? > > > > > > > > It's my intension for documentation of > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > > > > it under acquired lock. Its implementation doesn't yield processor > > > > voluntarily by itself. If it yielded, it would depend on implementation > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > > > > but it's not preferable implementation of driver, in my opinion. > > > > > > My point is again about the sleep. This function may sleep in the > > > nonatomic mode. The type of the PCM stream lock depends on it. > > > > Would I simply request you to show how the added function yields except > > for the driver implementation? The lock of stream is expected to be > > acquired already. > > In the nonatomic mode, the PCM stream lock is a mutex (no > spin_lock_irqsave), hence it can sleep -- which contradicts with the > added description above. > > Or do I misunderstand your question...? Thanks to clarify the role of PCM stream lock, and I'm ease that we have the same understanding about the lock. Here, let us see deleted/added line again. > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > index b7e3d8f44511..3488ec1e3674 100644 > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -1778,27 +1778,41 @@ 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. > + ... > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > + * sleep. The issued documentation is for the new function. Inner the function, the lock of PCM substream is not acquired again since it causes dead lock (it's not nest-able lock) regardless of usage of mutex or spin_lock. The well-known function, snd_pcm_period_elapsed(), is rewritten to call the new function between lock/unlock operations: ->snd_pcm_period_elapsed() ->snd_pcm_stream_lock_irqsave() ->snd_pcm_period_elapsed_under_stream_lock() ->snd_pcm_stream_unlock_irqrestore() Or the new function can acquire the lock somewhere I overlook? However I think it is unlikely since it necessarily causes dead lock or corruption of irq context... Thanks Takashi Sakamoto
On Thu, 10 Jun 2021 10:26:22 +0200, Takashi Sakamoto wrote: > > On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote: > > On Thu, 10 Jun 2021 10:05:21 +0200, > > Takashi Sakamoto wrote: > > > > > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote: > > > > On Thu, 10 Jun 2021 01:16:23 +0200, > > > > Takashi Sakamoto wrote: > > > > > > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > > > > > On Wed, 09 Jun 2021 16:31:43 +0200, > > > > > > Takashi Sakamoto wrote: > > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > > > > --- a/sound/core/pcm_lib.c > > > > > > > +++ b/sound/core/pcm_lib.c > > > > > > > @@ -1778,27 +1778,41 @@ 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. > > > > > > > + * > > > > > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > > > > > + * sleep. > > > > > > > > > > > > Hm, this text still remains here. Overlooked? > > > > > > > > > > It's my intension for documentation of > > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > > > > > it under acquired lock. Its implementation doesn't yield processor > > > > > voluntarily by itself. If it yielded, it would depend on implementation > > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > > > > > but it's not preferable implementation of driver, in my opinion. > > > > > > > > My point is again about the sleep. This function may sleep in the > > > > nonatomic mode. The type of the PCM stream lock depends on it. > > > > > > Would I simply request you to show how the added function yields except > > > for the driver implementation? The lock of stream is expected to be > > > acquired already. > > > > In the nonatomic mode, the PCM stream lock is a mutex (no > > spin_lock_irqsave), hence it can sleep -- which contradicts with the > > added description above. > > > > Or do I misunderstand your question...? > > Thanks to clarify the role of PCM stream lock, and I'm ease that we have > the same understanding about the lock. > > Here, let us see deleted/added line again. > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > index b7e3d8f44511..3488ec1e3674 100644 > > --- a/sound/core/pcm_lib.c > > +++ b/sound/core/pcm_lib.c > > @@ -1778,27 +1778,41 @@ 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. > > + ... > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > + * sleep. > > The issued documentation is for the new function. Inner the function, the > lock of PCM substream is not acquired again since it causes dead lock > (it's not nest-able lock) regardless of usage of mutex or spin_lock. > > The well-known function, snd_pcm_period_elapsed(), is rewritten to call > the new function between lock/unlock operations: > > ->snd_pcm_period_elapsed() > ->snd_pcm_stream_lock_irqsave() > ->snd_pcm_period_elapsed_under_stream_lock() > ->snd_pcm_stream_unlock_irqrestore() > > Or the new function can acquire the lock somewhere I overlook? However I > think it is unlikely since it necessarily causes dead lock or corruption > of irq context... Again, my *only* point is about the sleep. You addition was: + * Context: Any context in which lock of PCM substream is already acquired. This function may not + * sleep. where "This function may not sleep" is stated incorrectly. Takashi
On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote: > On Thu, 10 Jun 2021 10:26:22 +0200, > Takashi Sakamoto wrote: > > > > On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote: > > > On Thu, 10 Jun 2021 10:05:21 +0200, > > > Takashi Sakamoto wrote: > > > > > > > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote: > > > > > On Thu, 10 Jun 2021 01:16:23 +0200, > > > > > Takashi Sakamoto wrote: > > > > > > > > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > > > > > > On Wed, 09 Jun 2021 16:31:43 +0200, > > > > > > > Takashi Sakamoto wrote: > > > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > > > > > --- a/sound/core/pcm_lib.c > > > > > > > > +++ b/sound/core/pcm_lib.c > > > > > > > > @@ -1778,27 +1778,41 @@ 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. > > > > > > > > + * > > > > > > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > > > > > > + * sleep. > > > > > > > > > > > > > > Hm, this text still remains here. Overlooked? > > > > > > > > > > > > It's my intension for documentation of > > > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > > > > > > it under acquired lock. Its implementation doesn't yield processor > > > > > > voluntarily by itself. If it yielded, it would depend on implementation > > > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > > > > > > but it's not preferable implementation of driver, in my opinion. > > > > > > > > > > My point is again about the sleep. This function may sleep in the > > > > > nonatomic mode. The type of the PCM stream lock depends on it. > > > > > > > > Would I simply request you to show how the added function yields except > > > > for the driver implementation? The lock of stream is expected to be > > > > acquired already. > > > > > > In the nonatomic mode, the PCM stream lock is a mutex (no > > > spin_lock_irqsave), hence it can sleep -- which contradicts with the > > > added description above. > > > > > > Or do I misunderstand your question...? > > > > Thanks to clarify the role of PCM stream lock, and I'm ease that we have > > the same understanding about the lock. > > > > Here, let us see deleted/added line again. > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > index b7e3d8f44511..3488ec1e3674 100644 > > > --- a/sound/core/pcm_lib.c > > > +++ b/sound/core/pcm_lib.c > > > @@ -1778,27 +1778,41 @@ 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. > > > + ... > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > > + * sleep. > > > > The issued documentation is for the new function. Inner the function, the > > lock of PCM substream is not acquired again since it causes dead lock > > (it's not nest-able lock) regardless of usage of mutex or spin_lock. > > > > The well-known function, snd_pcm_period_elapsed(), is rewritten to call > > the new function between lock/unlock operations: > > > > ->snd_pcm_period_elapsed() > > ->snd_pcm_stream_lock_irqsave() > > ->snd_pcm_period_elapsed_under_stream_lock() > > ->snd_pcm_stream_unlock_irqrestore() > > > > Or the new function can acquire the lock somewhere I overlook? However I > > think it is unlikely since it necessarily causes dead lock or corruption > > of irq context... > > Again, my *only* point is about the sleep. You addition was: > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > + * sleep. > > where "This function may not sleep" is stated incorrectly. Hm. Would I request you to show the detail case that the call of function (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for driver-side implementation of snd_pcm_ops.{pointer, trigger, get_time_info}? At least, in callgraph I find no function call to yield... Regards Takashi Sakamoto
On Thu, 10 Jun 2021 12:12:43 +0200, Takashi Sakamoto wrote: > > On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote: > > On Thu, 10 Jun 2021 10:26:22 +0200, > > Takashi Sakamoto wrote: > > > > > > On Thu, Jun 10, 2021 at 10:08:39AM +0200, Takashi Iwai wrote: > > > > On Thu, 10 Jun 2021 10:05:21 +0200, > > > > Takashi Sakamoto wrote: > > > > > > > > > > On Thu, Jun 10, 2021 at 09:39:37AM +0200, Takashi Iwai wrote: > > > > > > On Thu, 10 Jun 2021 01:16:23 +0200, > > > > > > Takashi Sakamoto wrote: > > > > > > > > > > > > > > On Wed, Jun 09, 2021 at 05:27:29PM +0200, Takashi Iwai wrote: > > > > > > > > On Wed, 09 Jun 2021 16:31:43 +0200, > > > > > > > > Takashi Sakamoto wrote: > > > > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > > > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > > > > > > --- a/sound/core/pcm_lib.c > > > > > > > > > +++ b/sound/core/pcm_lib.c > > > > > > > > > @@ -1778,27 +1778,41 @@ 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. > > > > > > > > > + * > > > > > > > > > + * This 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 preferrable 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 in which lock of PCM substream is already acquired. This function may not > > > > > > > > > + * sleep. > > > > > > > > > > > > > > > > Hm, this text still remains here. Overlooked? > > > > > > > > > > > > > > It's my intension for documentation of > > > > > > > snd_pcm_period_elapsed_under_stream_lock() since it's expected to call > > > > > > > it under acquired lock. Its implementation doesn't yield processor > > > > > > > voluntarily by itself. If it yielded, it would depend on implementation > > > > > > > of each driver for struct snd_pcm_ops.{pointer, trigger, get_time_info}, > > > > > > > but it's not preferable implementation of driver, in my opinion. > > > > > > > > > > > > My point is again about the sleep. This function may sleep in the > > > > > > nonatomic mode. The type of the PCM stream lock depends on it. > > > > > > > > > > Would I simply request you to show how the added function yields except > > > > > for the driver implementation? The lock of stream is expected to be > > > > > acquired already. > > > > > > > > In the nonatomic mode, the PCM stream lock is a mutex (no > > > > spin_lock_irqsave), hence it can sleep -- which contradicts with the > > > > added description above. > > > > > > > > Or do I misunderstand your question...? > > > > > > Thanks to clarify the role of PCM stream lock, and I'm ease that we have > > > the same understanding about the lock. > > > > > > Here, let us see deleted/added line again. > > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > > index b7e3d8f44511..3488ec1e3674 100644 > > > > --- a/sound/core/pcm_lib.c > > > > +++ b/sound/core/pcm_lib.c > > > > @@ -1778,27 +1778,41 @@ 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. > > > > + ... > > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > > > + * sleep. > > > > > > The issued documentation is for the new function. Inner the function, the > > > lock of PCM substream is not acquired again since it causes dead lock > > > (it's not nest-able lock) regardless of usage of mutex or spin_lock. > > > > > > The well-known function, snd_pcm_period_elapsed(), is rewritten to call > > > the new function between lock/unlock operations: > > > > > > ->snd_pcm_period_elapsed() > > > ->snd_pcm_stream_lock_irqsave() > > > ->snd_pcm_period_elapsed_under_stream_lock() > > > ->snd_pcm_stream_unlock_irqrestore() > > > > > > Or the new function can acquire the lock somewhere I overlook? However I > > > think it is unlikely since it necessarily causes dead lock or corruption > > > of irq context... > > > > Again, my *only* point is about the sleep. You addition was: > > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > + * sleep. > > > > where "This function may not sleep" is stated incorrectly. > > Hm. Would I request you to show the detail case that the call of function > (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for > driver-side implementation of snd_pcm_ops.{pointer, trigger, > get_time_info}? At least, in callgraph I find no function call to > yield... True. But the fact that those callbacks may sleep means that the function would go sleeping after all. Takashi
Hi, On Thu, Jun 10, 2021 at 01:03:19PM +0200, Takashi Iwai wrote: > On Thu, 10 Jun 2021 12:12:43 +0200, Takashi Sakamoto wrote: > > > > On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote: > > > Again, my *only* point is about the sleep. You addition was: > > > > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > > + * sleep. > > > > > > where "This function may not sleep" is stated incorrectly. > > > > Hm. Would I request you to show the detail case that the call of function > > (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for > > driver-side implementation of snd_pcm_ops.{pointer, trigger, > > get_time_info}? At least, in callgraph I find no function call to > > yield... > > True. But the fact that those callbacks may sleep means that the > function would go sleeping after all. Thanks. After all, our discussion comes from the ambiguity that what has responsibility at yielding processor under the lock. I think it helpful to describe devide responsibilities about the yielding. I'm glad for you to review patch below: ======== 8< -------- From 98e1b8332a95935ae875c637d3ddc27e68689aa0 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto <o-takashi@sakamocchi.jp> Date: Fri, 11 Jun 2021 11:03:46 +0900 Subject: [PATCH] ALSA: pcm: add context section for documentation about period-elapsed kernel APIs This commit fulfils documentation of period-elapsed kernel APIs with their context section. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/core/pcm_lib.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 7d5883432085..5d28d63a3216 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1803,6 +1803,10 @@ EXPORT_SYMBOL(snd_pcm_lib_ioctl); * - .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 in which lock of PCM substream is already acquired. The function may not + * sleep by ALSA PCM core. The function may sleep in the above callbacks by driver which should + * configures PCM device for it (@snd_pcm.nonatomic). */ void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream) { @@ -1836,6 +1840,10 @@ EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock); * 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: Any context in which lock of PCM substream is not acquired yet. It depends on + * configuration of PCM device (@snd_pcm.nonatomic) by driver whether the function may or may not + * sleep by operating lock of PCM substream. */ void snd_pcm_period_elapsed(struct snd_pcm_substream *substream) {
On Fri, 11 Jun 2021 05:38:16 +0200, Takashi Sakamoto wrote: > > Hi, > > On Thu, Jun 10, 2021 at 01:03:19PM +0200, Takashi Iwai wrote: > > On Thu, 10 Jun 2021 12:12:43 +0200, Takashi Sakamoto wrote: > > > > > > On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote: > > > > Again, my *only* point is about the sleep. You addition was: > > > > > > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > > > + * sleep. > > > > > > > > where "This function may not sleep" is stated incorrectly. > > > > > > Hm. Would I request you to show the detail case that the call of function > > > (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for > > > driver-side implementation of snd_pcm_ops.{pointer, trigger, > > > get_time_info}? At least, in callgraph I find no function call to > > > yield... > > > > True. But the fact that those callbacks may sleep means that the > > function would go sleeping after all. > > Thanks. After all, our discussion comes from the ambiguity that what > has responsibility at yielding processor under the lock. I think it helpful > to describe devide responsibilities about the yielding. I'm glad for you > to review patch below: Well, I don't think it's worth to mention "ALSA core may not sleep". It's just casually so for now, but it doesn't mean that this will be guaranteed in future. After all, this function call may sleep in the nonatomic mode (that's the very reason for that mode!), and the caller has to be prepared for that, no matter whether you do sleep in the callbacks or not. thanks, Takashi > > ======== 8< -------- > > >From 98e1b8332a95935ae875c637d3ddc27e68689aa0 Mon Sep 17 00:00:00 2001 > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Date: Fri, 11 Jun 2021 11:03:46 +0900 > Subject: [PATCH] ALSA: pcm: add context section for documentation about > period-elapsed kernel APIs > > This commit fulfils documentation of period-elapsed kernel APIs with their > context section. > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > --- > sound/core/pcm_lib.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > index 7d5883432085..5d28d63a3216 100644 > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -1803,6 +1803,10 @@ EXPORT_SYMBOL(snd_pcm_lib_ioctl); > * - .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 in which lock of PCM substream is already acquired. The function may not > + * sleep by ALSA PCM core. The function may sleep in the above callbacks by driver which should > + * configures PCM device for it (@snd_pcm.nonatomic). > */ > void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream) > { > @@ -1836,6 +1840,10 @@ EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock); > * 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: Any context in which lock of PCM substream is not acquired yet. It depends on > + * configuration of PCM device (@snd_pcm.nonatomic) by driver whether the function may or may not > + * sleep by operating lock of PCM substream. > */ > void snd_pcm_period_elapsed(struct snd_pcm_substream *substream) > { > -- > 2.27.0 > > ======== 8< -------- > > Thanks > > Takashi Sakamoto >
On Fri, Jun 11, 2021 at 08:47:59AM +0200, Takashi Iwai wrote: > On Fri, 11 Jun 2021 05:38:16 +0200, > Takashi Sakamoto wrote: > > > > Hi, > > > > On Thu, Jun 10, 2021 at 01:03:19PM +0200, Takashi Iwai wrote: > > > On Thu, 10 Jun 2021 12:12:43 +0200, Takashi Sakamoto wrote: > > > > > > > > On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote: > > > > > Again, my *only* point is about the sleep. You addition was: > > > > > > > > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > > > > + * sleep. > > > > > > > > > > where "This function may not sleep" is stated incorrectly. > > > > > > > > Hm. Would I request you to show the detail case that the call of function > > > > (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for > > > > driver-side implementation of snd_pcm_ops.{pointer, trigger, > > > > get_time_info}? At least, in callgraph I find no function call to > > > > yield... > > > > > > True. But the fact that those callbacks may sleep means that the > > > function would go sleeping after all. > > > > Thanks. After all, our discussion comes from the ambiguity that what > > has responsibility at yielding processor under the lock. I think it helpful > > to describe devide responsibilities about the yielding. I'm glad for you > > to review patch below: > > Well, I don't think it's worth to mention "ALSA core may not sleep". > It's just casually so for now, but it doesn't mean that this will be > guaranteed in future. After all, this function call may sleep in > the nonatomic mode (that's the very reason for that mode!), and the > caller has to be prepared for that, no matter whether you do sleep in > the callbacks or not. I have an opinion that we should guarantee it as long as maintaining existent in-kernel drivers, which call it in hw/sw IRQ context. This is not the issue 'casually so for now'. If you had a plan to rewrite or drop the drivers near future, you could say it. > > ======== 8< -------- > > > > >From 98e1b8332a95935ae875c637d3ddc27e68689aa0 Mon Sep 17 00:00:00 2001 > > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > Date: Fri, 11 Jun 2021 11:03:46 +0900 > > Subject: [PATCH] ALSA: pcm: add context section for documentation about > > period-elapsed kernel APIs > > > > This commit fulfils documentation of period-elapsed kernel APIs with their > > context section. > > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > --- > > sound/core/pcm_lib.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > index 7d5883432085..5d28d63a3216 100644 > > --- a/sound/core/pcm_lib.c > > +++ b/sound/core/pcm_lib.c > > @@ -1803,6 +1803,10 @@ EXPORT_SYMBOL(snd_pcm_lib_ioctl); > > * - .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 in which lock of PCM substream is already acquired. The function may not > > + * sleep by ALSA PCM core. The function may sleep in the above callbacks by driver which should > > + * configures PCM device for it (@snd_pcm.nonatomic). > > */ > > void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream) > > { > > @@ -1836,6 +1840,10 @@ EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock); > > * 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: Any context in which lock of PCM substream is not acquired yet. It depends on > > + * configuration of PCM device (@snd_pcm.nonatomic) by driver whether the function may or may not > > + * sleep by operating lock of PCM substream. > > */ > > void snd_pcm_period_elapsed(struct snd_pcm_substream *substream) > > { > > -- > > 2.27.0 > > > > ======== 8< -------- > > > > Thanks > > > > Takashi Sakamoto Regards Takashi Sakamoto
On Fri, 11 Jun 2021 09:07:57 +0200, Takashi Sakamoto wrote: > > On Fri, Jun 11, 2021 at 08:47:59AM +0200, Takashi Iwai wrote: > > On Fri, 11 Jun 2021 05:38:16 +0200, > > Takashi Sakamoto wrote: > > > > > > Hi, > > > > > > On Thu, Jun 10, 2021 at 01:03:19PM +0200, Takashi Iwai wrote: > > > > On Thu, 10 Jun 2021 12:12:43 +0200, Takashi Sakamoto wrote: > > > > > > > > > > On Thu, Jun 10, 2021 at 10:36:57AM +0200, Takashi Iwai wrote: > > > > > > Again, my *only* point is about the sleep. You addition was: > > > > > > > > > > > > + * Context: Any context in which lock of PCM substream is already acquired. This function may not > > > > > > + * sleep. > > > > > > > > > > > > where "This function may not sleep" is stated incorrectly. > > > > > > > > > > Hm. Would I request you to show the detail case that the call of function > > > > > (snd_pcm_period_elapsed_under_stream_lock()) goes sleep except for > > > > > driver-side implementation of snd_pcm_ops.{pointer, trigger, > > > > > get_time_info}? At least, in callgraph I find no function call to > > > > > yield... > > > > > > > > True. But the fact that those callbacks may sleep means that the > > > > function would go sleeping after all. > > > > > > Thanks. After all, our discussion comes from the ambiguity that what > > > has responsibility at yielding processor under the lock. I think it helpful > > > to describe devide responsibilities about the yielding. I'm glad for you > > > to review patch below: > > > > Well, I don't think it's worth to mention "ALSA core may not sleep". > > It's just casually so for now, but it doesn't mean that this will be > > guaranteed in future. After all, this function call may sleep in > > the nonatomic mode (that's the very reason for that mode!), and the > > caller has to be prepared for that, no matter whether you do sleep in > > the callbacks or not. > > I have an opinion that we should guarantee it as long as maintaining > existent in-kernel drivers, which call it in hw/sw IRQ context. This is > not the issue 'casually so for now'. It *is* casually so for now, and I see no big merit for the ALSA core about such a limitation. The PCM core might need to introduce another lock in future for some reason, and that'll be a mutex in nonatomic mode. If we guarantee the current behavior, it would become impossible. After all, the preempt is still allowed even if there is no sleeper in snd_pcm_period*() itself. For atomic mode, it's under the stream spin lock, so it's clearly no sleep / no preempt. For non-atomic mode, it's under the stream mutex lock, and that's all. There should be no other restriction there. We don't want to choke ourselves unnecessarily. thanks, Takashi > > If you had a plan to rewrite or drop the drivers near future, you could say > it. > > > > ======== 8< -------- > > > > > > >From 98e1b8332a95935ae875c637d3ddc27e68689aa0 Mon Sep 17 00:00:00 2001 > > > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > > Date: Fri, 11 Jun 2021 11:03:46 +0900 > > > Subject: [PATCH] ALSA: pcm: add context section for documentation about > > > period-elapsed kernel APIs > > > > > > This commit fulfils documentation of period-elapsed kernel APIs with their > > > context section. > > > > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > > --- > > > sound/core/pcm_lib.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > > > index 7d5883432085..5d28d63a3216 100644 > > > --- a/sound/core/pcm_lib.c > > > +++ b/sound/core/pcm_lib.c > > > @@ -1803,6 +1803,10 @@ EXPORT_SYMBOL(snd_pcm_lib_ioctl); > > > * - .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 in which lock of PCM substream is already acquired. The function may not > > > + * sleep by ALSA PCM core. The function may sleep in the above callbacks by driver which should > > > + * configures PCM device for it (@snd_pcm.nonatomic). > > > */ > > > void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream) > > > { > > > @@ -1836,6 +1840,10 @@ EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock); > > > * 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: Any context in which lock of PCM substream is not acquired yet. It depends on > > > + * configuration of PCM device (@snd_pcm.nonatomic) by driver whether the function may or may not > > > + * sleep by operating lock of PCM substream. > > > */ > > > void snd_pcm_period_elapsed(struct snd_pcm_substream *substream) > > > { > > > -- > > > 2.27.0 > > > > > > ======== 8< -------- > > > > > > Thanks > > > > > > Takashi Sakamoto > > > Regards > > Takashi Sakamoto >
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..3488ec1e3674 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1778,27 +1778,41 @@ 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. + * + * This 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 preferrable 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 in which lock of PCM substream is already acquired. 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 +1825,34 @@ 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. + * + * This 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: Any context in which lock of PCM substream is not acquired yet. It depends on + * configuration of PCM device (@snd_pcm.nonatomic) by each driver whether this function may or + * may not sleep due to internal call of ``snd_pcm_stream_lock_irqsave()``. + */ +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);
Current implementation of ALSA PCM core has a kernel API, snd_pcm_period_elapsed(), 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 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 call in the process context. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 71 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 15 deletions(-)