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

From: Mark Hills <mark@xwax.org>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH 3/3] echoaudio: Address bugs in the interrupt handling
Date: Wed, 17 Jun 2020 11:51:05 +0100 (BST)	[thread overview]
Message-ID: <2006171134130.2561@stax.localdomain> (raw)
In-Reply-To: <s5hlfkn14cr.wl-tiwai@suse.de>

On Tue, 16 Jun 2020, Takashi Iwai wrote:

> On Tue, 16 Jun 2020 16:01:11 +0200,
> Mark Hills wrote:
> > 
> > On Tue, 16 Jun 2020, Takashi Iwai wrote:
> > 
> > > On Tue, 16 Jun 2020 15:17:43 +0200,
> > > Mark Hills wrote:
> > > > 
> > > > +/* Update software pointer to match the hardware
> > > > + *
> > > > + * Return: 1 if we crossed a period threshold, otherwise 0
> > > > + */
> > > > +static int snd_echo_poll_substream(struct snd_pcm_substream *substream)
> > > 
> > > This is a bit confusing name.  One would imagine from the function
> > > name as if it were about the handling of poll() syscall.
> > 
> > Poll felt intuitive to me; maybe from FreeBSD where network drivers can 
> > poll on a timer instead of interrupts. I do know about poll(), though.
> > 
> > How about snd_echo_update_substream()?
> 
> Yes, it's more self-explanatory.  Or even better
> snd_echo_update_substream_position() :)

Out of interest, these are static but the names are globally qualified. I 
see this elsewhere, so I followed, but is this agreed convention?

Because it could be update_substream_position() :)

> > > > +{
> > > > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > > > +	struct audiopipe *pipe = runtime->private_data;
> > > > +	unsigned counter, step, period, last_period;
> > > > +	size_t buffer_bytes;
> > > > +
> > > > +	if (pipe->state != PIPE_STATE_STARTED)
> > > > +		return 0;
> > > > +
> > > > +	counter = le32_to_cpu(*pipe->dma_counter);
> > > > +
> > > > +	period = bytes_to_frames(runtime, counter)
> > > > +		/ runtime->period_size;
> > > > +	last_period = bytes_to_frames(runtime, pipe->last_counter)
> > > > +		/ runtime->period_size;
> > > > +
> > > > +	if (period == last_period)
> > > > +		return 0;  /* don't do any work yet */
> > > > +
> > > > +	step = counter - pipe->last_counter;
> > > > +	pipe->last_counter = counter;
> > > 
> > > Dose this calculation consider the overlap of 32bit integer of the
> > > counter?  (I'm not sure whether the old code did it right, though.)
> > 
> > I believe it does, since (small - big) as unsigned gives small value. And 
> > period is checked only for equality (not greater than). I'll add a comment 
> > as such. I have run it with long streams.
> 
> The problem is that the last_period calculation can be wrong if the
> period size isn't aligned.  e.g. when period size is 44100, around the
> boundary position, it gets a different last_period value although it
> still in the same period.

Agreed, yes.

In which case I think it's clearer to not infer anything about periods 
from the current counter or position, and (effectively) accumulate it.

Would you please make suggestions on the code below?

Because it allowed for further simplification whilst fixing the bugs.

In the end, modulo became clearer than loops and I think it has the bonus 
of being less risky should the counter make a large step.

I'll run for a longer test today.

---

/* 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);
}

-- 
Mark