On Thu, 18 Jun 2020, Takashi Iwai wrote: > On Thu, 18 Jun 2020 13:07:33 +0200, > Mark Hills wrote: > > > > On Thu, 18 Jun 2020, Takashi Iwai wrote: > > > > > On Wed, 17 Jun 2020 12:51:05 +0200, > > > Mark Hills wrote: > > [...] > > But could I please ask for help with the bigger picture? As it feels > > mismatched. > > > > * Code should take every possible opportunity to update the stream > > position; interrupts, or explicit pcm_pointer calls (whereas the docs > > guide towards doing it in the interrupt handler) > > > > * I critiqued (elsewhere in thread) the older interrupt handler for > > checking the counter, unlocking, calling back into PCM core and checking > > again a moment later. Whereas this is considered good behaviour. > > > > * Seems like the overall aim is for userland to be able (if it wants to) > > to poll the soundcard, even outside of periods. > > Right, the user-space can query the current position at any time, and > the driver should return the position as precisely as possible. > > Some applications (like PulseAudio) sets the interrupt as minimum as > possible while it does schedule the update by itself, judging the > position via the ioctl. In such operational mode, the accuracy of the > current position query is vital. > > > If all the above is true, I would expect interrupt handling to be very > > simple -- it would straight away call into PCM core, join existing if the > > codepaths (to take locks) and do a position update. PCM core would decide > > if a period really elapsed, not the driver. But this is not how it works. > > > > This now relates strongly to a question of locking: > > > > I ran the code (top of this message) all day, with a few instances in > > dmesg (at irregular intervals, not wrapping): > > > > [161644.076666] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64 > > [163232.392501] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64 > > [164976.098069] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64 > > [165054.946225] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64 > > [165312.141545] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64 > > > > A definite bug, of course. > > > > However (and I am happy to be corrected) the function never finishes with > > position == buffer size. Only way is a race between interrupt handler and > > pcm_pointer. > > > > Therefore one of these is needed: > > > > * pcm_pointer locks chip->lock > > > > Even though the docs emphasise PCM core has exclusivity, it it not worth > > much as it does not protect against the interrupt handler. > > > > But now interrupt handler becomes ugly in total: take chip->lock, check > > the counter, release chip->lock, go to PCM core (which takes middle > > layer locks), take chip->lock again, check counter again, release > > chip->lock again. > > Yes, that's the most robust way to go. If the lock is really costing > too much, you can consider a fast-path by some flag for the irq -> > snd_pcm_period_elapsed() case. I don't understand how a fast path could be made to work, as it can't pass data across snd_pcm_period_elapsed() and it still must syncronise access between reading dma_counter and writing pipe->position. Hence questioning if a better design is simpler interrupt handlers that just enter PCM core. > Basically you can do everything in the pointer callback. The only > requirement in the interrupt handle is basically judge whether you need > the call of snd_pcm_period_elapsed() and call it. The rest update could > be done in the other places. Thanks, please just another clarification: I presume that calls to pcm_pointer are completely independent of the period notifications? ie. period notifications are at regular intervals, regardless of whether pcm_pointer is called inbetween. pcm_pointer must not reset any state used by the interrupt. Which means we must handle when non-interrupt call to pcm_pointer causes a period to elapse. The next interrupt handler must notify. I can see in the original code uses chip->last_period exclusively by the interrupt handler, and I removed it. Some comments around the intent would help. I'll cross reference the original code with my new understanding. My instinct here is that to preserve - regular period notifications - handle period_size not aligning with 32-bit counter - no races between interrupt and pcm_pointer that the clearest and bug-free implementation may be to separate the interrupt (notifications) and pointer updates to separate state. Then there's no lock and the only crossover is an atomic read of dma_counter. And that's what I will try -- thanks. -- Mark
