Re: [PATCH 3/3] echoaudio: Address bugs in the interrupt handling

[Takashi Iwai <tiwai@suse.de>]

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:
> > > 
> > > 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.

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.

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.

> * 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.

As mentioned, the update of position at query is essential in majority
of sound systems.


Takashi

> 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
>