On Thu, 06 Apr 2023 09:33:41 +0200,
Oswald Buddenhagen wrote:
>
> On Thu, Apr 06, 2023 at 08:55:27AM +0200, Takashi Iwai wrote:
> > On Wed, 05 Apr 2023 22:12:20 +0200,
> > Oswald Buddenhagen wrote:
> >> @@ -2262,7 +2156,7 @@ Typical code would be like the following.
> >> /* over the period boundary? */
> >> if (chip->size >= runtime->period_size) {
> >> /* reset the accumulator */
> >> - chip->size %= runtime->period_size;
> >> + chip->size -= runtime->period_size;
> >> /* call updater */
> >> spin_unlock(&chip->lock);
> >> snd_pcm_period_elapsed(substream);
> >
> > Hmm, this kind of change shouldn't be sneaked in.
> > That's more than the typo fixes etc,
> >
> true, the patch grew too big and i missed this hunk.
> (i only kept it as one patch, because some pieces overlap and i didn't
> want to add churn.)
Maybe the changes could have been split from the first place, the
mechanical changes to drop empty lines before "::", typo fixes, and
text improvements, etc. But I guess it's too late and I'm fine to
apply this whole change as a single patch with the correction.
> > and even worse, it's a wrong replacement.
> >
> hmm, yeah, if the timer ints are configured to occur too rarely, this
> wouldn't do the right thing.
> but then, why would they be? that would basically defeat the point of
> using many periods in the first place. should i instead change the
> text to emphasize that the ints should occur at least once per period?
> (i've actually pondered the timer option in the context of the emu10k1
> driver as well, and concluded that there should be two timer ints per
> period, so snd_pcm_period_elapsed() is reliably called in the first
> half of the next period, which is critical when only two periods are
> configured.)
No, the point is that, if an irq handler misses the multiple period
updates, it has to handle the situation in a single shot, and the
offset gets corrected to the value within the period size; using "-="
instead of "%=" may leave the size over the period size in such a
case, and this may return an invalid PCM pointer value in the end.
The text about the irq handling could be improved, of course.
thanks,
Takashi
