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