On Thu, 18 Jun 2020, Takashi Iwai wrote:
> On Wed, 17 Jun 2020 12:51:05 +0200,
> Mark Hills wrote:
> >
> > On Tue, 16 Jun 2020, Takashi Iwai wrote:
> >
> > > On Tue, 16 Jun 2020 16:01:11 +0200,
> > > Mark Hills wrote:
[...]
> >
> > /* Update software pointer to match the hardware
> > *
> > * \pre chip->lock is held
> > */
> > static void snd_echo_update_substream_position(struct echoaudio *chip,
> > 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;
> >
> > period_bytes = frames_to_bytes(runtime, runtime->period_size);
> >
> > counter = le32_to_cpu(*pipe->dma_counter);
> >
> > step = counter - pipe->last_counter; /* handles wrapping of counter */
> > step -= step % period_bytes; /* acknowledge whole periods only */
> >
> > if (step == 0)
> > return; /* haven't advanced a whole period yet */
> > pipe->last_counter += step; /* does not always wrap on a period */
> > pipe->position += step;
> >
> > /* wraparound the buffer */
> > pipe->position %= frames_to_bytes(runtime, runtime->buffer_size);
> >
> > /* notify only once, even if multiple periods elapsed */
> > spin_unlock(&chip->lock);
> > snd_pcm_period_elapsed(substream);
> > spin_lock(&chip->lock);
> > }
> >
> > 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)
> > snd_echo_update_substream_position(chip, substream);
> > }
> > 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;
> >
> > return bytes_to_frames(runtime, pipe->position);
>
> I guess this misses the update of the precise position; in your code,
> pipe->position gets updated only with the period size at irq handler.
>
>
> IMO, we should have the code like:
>
> static bool update_stream_position(struct snd_pcm_substream *substream)
> {
> // update pipe->position and others, returns true if period elapsed
> }
>
> static irqreturn_t snd_echo_interrupt()
> {
> spin_lock(&chip->lock);
> ....
> if (update_stream_position(substream)) {
> spin_unlock(&chip->lock);
> snd_pcm_period_elapsed(substream);
> spin_lock(&chip->lock);
> }
> ....
> spin_unlock(&chip->lock);
> return IRQ_HANDLED;
> }
>
> static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream)
> {
> ....
> update_stream_position(substream);
> return bytes_to_frames(runtime, pipe->position);
> }
Thanks.
I certainly understand this in isolation.
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.
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.
* interrupt handler must make atomic update of pipe->position
This could have been a solution, but not if we expect pcm_pointer to
also invoke the position update. Now we have a race: both the interrupt
handler and pcm_position want to read dma_counter and write
pipe->position after.
So either do everthing in interrupt, everything in the pointer callback
(though there isn't the API for this), but doing both does not seem to
work well (though probably can be made to work if necessary)
If we can clarify the requirements then I do not think it would be hard
for me to write the code.
[...]
> Takashi
Thanks again,
--
Mark
