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