On Thu, 18 Jun 2020, Mark Hills wrote:
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.
Ok so the implementation would look something like this below, which I will run for the rest of the day:
* Clear separation of the period notification from position updates; only syncronising is around dma_counter, no need for locks
* All counting is accumulated to avoid bugs in the cases of wrapping and non-alignment
It's easier to see in the end results but of course I'll work on a clear diff.
---
/****************************************************************************** IRQ Handling ******************************************************************************/ /* Check if a period has elapsed since last interrupt * * Don't make any updates to state; PCM core handles this with the * correct locks. * * \return true if a period has elapsed, otherwise false */ static bool period_has_elapsed(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct audiopipe *pipe = runtime->private_data; u32 counter, step; size_t period_bytes;
if (pipe->state != PIPE_STATE_STARTED) return false;
period_bytes = frames_to_bytes(runtime, runtime->period_size);
counter = le32_to_cpu(*pipe->dma_counter); /* presumed atomic */
step = counter - pipe->last_period; /* handles wrapping */ step -= step % period_bytes; /* acknowledge whole periods only */
if (step == 0) return false; /* haven't advanced a whole period yet */
pipe->last_period += step; /* used exclusively by us */ return true; }
static irqreturn_t snd_echo_interrupt(int irq, void *dev_id) { struct echoaudio *chip = dev_id; int ss, st;
spin_lock(&chip->lock); st = service_irq(chip); if (st < 0) { spin_unlock(&chip->lock); return IRQ_NONE; } /* The hardware doesn't tell us which substream caused the irq, thus we have to check all running substreams. */ for (ss = 0; ss < DSP_MAXPIPES; ss++) { struct snd_pcm_substream *substream;
substream = chip->substream[ss]; if (substream && period_has_elapsed(substream)) { spin_unlock(&chip->lock); snd_pcm_period_elapsed(substream); spin_lock(&chip->lock); } } spin_unlock(&chip->lock);
#ifdef ECHOCARD_HAS_MIDI if (st > 0 && chip->midi_in) { snd_rawmidi_receive(chip->midi_in, chip->midi_buffer, st); dev_dbg(chip->card->dev, "rawmidi_iread=%d\n", st); } #endif return IRQ_HANDLED; }
static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct audiopipe *pipe = runtime->private_data; u32 counter, step;
/* * IRQ handling runs concurrently. Do not share tracking of * counter with it, which would race or require locking */
counter = le32_to_cpu(*pipe->dma_counter); /* presumed atomic */
step = counter - pipe->last_counter; /* handles wrapping */ pipe->last_counter = counter;
/* counter doesn't neccessarily wrap on a multiple of * buffer_size, so can't derive the position; must * accumulate */
pipe->position += step; pipe->position %= frames_to_bytes(runtime, runtime->buffer_size); /* wrap */
return bytes_to_frames(runtime, pipe->position); }