At Thu, 29 May 2008 21:07:55 +0400, Stas Sergeev wrote:
Hello.
Takashi Iwai wrote:
really be better if just snd_pcm_period_elapsed() would raise a softirq. That's something you proposed in the past IIRC. Is this acceptable to have that change not in the driver, or do you think it is snd-pcsp-specific, and no other driver will benefit?
It would be good in general. However, better to be the next step after changing pcsp.
But why do you need both changes? If you are going to change snd_pcm_period_elapsed() itself, then how would the snd-pcsp change be justified?
snd-pcsp needs anyway the change to HRTIMER_IRQSAFE again.
And, moving snd_pcm_period_elapsed() to a tasklet isn't needed for RT kernels. It's basically a good thing, but just not mandatory like snd-pcsp.
One big disadvantage of the softirq is its (possible) timing inaccuracy. (Note that this is about the normal non-preempt kernel.) The thing like pcsp driver does, flipping the bits at a high rate, isn't for softirq per se.
OK, I see.
Well, it can be done in another away if you are so nervous about it. Simply add chip->timer_running atomic_t and reset it when the callback finished with HRTIMER_NORESTART. In another side, wait until chip->timer_running becomes zero. But, it's just an uneeded addition of the code.
But for some reasons it looks safer to me...
A spinlock is like a gun: you can protect yourself with it, and you can shoot others with it :)
Because the substream lock isn't necessary! It just makes things more complicated -- if we take substream lock, AB/BA deadlocks occur again.
Even if the callback is called within the softirq?
The runtime callbacks, pointer and trigger, are called already inside they substream lock held by the PCM core. Thus it doesn't matter whether it's in a hard or a softirq.
The difference is that in a softirq, the hrtimer code doesn't take a lock.
But it may cause automatically latencies for beep controls. The latency isn't a big issue for snd_pcm_period_elapsed() (unless you want realtime thingy with snd-pcsp), but for beep controls, it does.
The AB/BA problem was because of the hrtimer' lock, which is not there with the softirq callback. So what exactly A and B do you mean here?
That's what I meant for, too. Note that in the above text, I presume that the hrtimer is called in HRTIMER_IRQSAFE.
and yes, its nasty. But to me its nastiness is only because it needs another lock to protect the substream itself.
No, you don't need to protect the substream at all.
But that was suggested by you in the past. :)
Yeah, that's my failure, obviously. I just thought your hrtimer callback must be constantly called and hard to sync.
But adding a syncronization point should be better I agree. I'll see if some minimal change is possible that will look sane to you and wont make me ask 100 questions about its safety at the same time. :)
OK.
thanks,
Takashi