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:
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() :)
It's fine to use any name for a local function.
+{
- 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);
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); }
Note that snd_pcm_period_elapsed() isn't needed (must not be done) from the pointer callback. The PCM core would detect and handle properly if the period gets elapsed there.
thanks,
Takashi