[alsa-devel] [patch] snd-pcsp fixes
Hi.
The attached patch fixes the problems introduced in this commit: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
- Fix nForce workaround by honouring the pointer_update var - Revert "ns" to u64, as per the hrtimer API - Revert to the zero-delay timer startup, since I can't reproduce any problem with it (please, give me the hint!)
Signed-off-by: Stas Sergeev stsp@aknet.ru
Takashi, could you please apply or tell me where the problem is/was? Or, alternatively, we can revert the commit entirely, and then re-do the cleanups.
At Wed, 21 Oct 2009 21:43:03 +0400, Stas Sergeev wrote:
[1 <text/plain; UTF-8 (7bit)>] Hi.
The attached patch fixes the problems introduced in this commit: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
- Fix nForce workaround by honouring
the pointer_update var
- Revert "ns" to u64, as per the hrtimer
API
- Revert to the zero-delay timer startup,
since I can't reproduce any problem with it (please, give me the hint!)
Signed-off-by: Stas Sergeev stsp@aknet.ru
Takashi, could you please apply or tell me where the problem is/was? Or, alternatively, we can revert the commit entirely, and then re-do the cleanups.
I applied the patch now as is, as I suppose you tested it well :)
I don't remember exactly the changes for pcsp driver right now, but the change of the initial delay was due to the change of start logic. At least, at the time HRTIMER_CB_IRQSAFE was removed, starting with zero didn't work at all on my machine. Maybe something got fixed in the core side.
But, I still don't understand why it has to be zero. The first bit-flip is done inside the trigger-start, so the next wakeup should be after the calculated ns. Isn't it?
thanks,
Takashi
Hi.
Takashi Iwai wrote:
I applied the patch now as is, as I suppose you tested it well :)
Well, no! :) I only tested it on 2 machines yet. Please give it more testing if you can.
I don't remember exactly the changes for pcsp driver right now, but the change of the initial delay was due to the change of start logic. At least, at the time HRTIMER_CB_IRQSAFE was removed, starting with zero didn't work at all on my machine. Maybe something got fixed in the core side.
OK, it would be nice if you can check whether or not the problem is there on that particular machine...
But, I still don't understand why it has to be zero. The first bit-flip is done inside the trigger-start, so the next wakeup should be after the calculated ns. Isn't it?
That was the logic you invented, but initially, and with my patch, there is no bitflip on start trigger. Or, at least, not supposed to be - there was only a timer mode setup, or if not - that was a bug. :) Also, you added some caching variables:
+ unsigned int fmt_size; + unsigned int is_signed;
What was the intention? Is this a speed-up? If not - reverting that too?
At Fri, 30 Oct 2009 15:22:39 +0300, Stas Sergeev wrote:
Hi.
Takashi Iwai wrote:
I applied the patch now as is, as I suppose you tested it well :)
Well, no! :) I only tested it on 2 machines yet. Please give it more testing if you can.
OK, will check later. But I think it should be irrelevant with machine since it's the core part of hrtimer.
I don't remember exactly the changes for pcsp driver right now, but the change of the initial delay was due to the change of start logic. At least, at the time HRTIMER_CB_IRQSAFE was removed, starting with zero didn't work at all on my machine. Maybe something got fixed in the core side.
OK, it would be nice if you can check whether or not the problem is there on that particular machine...
Maybe I have no more that machine. Anyway, more testing would be needed.
But, I still don't understand why it has to be zero. The first bit-flip is done inside the trigger-start, so the next wakeup should be after the calculated ns. Isn't it?
That was the logic you invented, but initially, and with my patch, there is no bitflip on start trigger. Or, at least, not supposed to be - there was only a timer mode setup, or if not - that was a bug. :)
OK, then we should revert the zero-delay logic again. The zero-delay start means to do the bitflip again immediately. This is wrong, of course.
Also, you added some caching variables:
unsigned int fmt_size;unsigned int is_signed;What was the intention? Is this a speed-up? If not - reverting that too?
It's for speeding up. We should minimize any calculations in the hrtimer callback.
thanks,
Takashi
Takashi Iwai wrote:
But, I still don't understand why it has to be zero. The first bit-flip is done inside the trigger-start, so the next wakeup should be after the calculated ns. Isn't it?
That was the logic you invented, but initially, and with my patch, there is no bitflip on start trigger. Or, at least, not supposed to be - there was only a timer mode setup, or if not - that was a bug. :)
OK, then we should revert the zero-delay logic again. The zero-delay start means to do the bitflip again immediately. This is wrong, of course.
What do you mean? As I said above, there seems to be no first bitflip on a start trigger, neither initially, nor with my current patch. There was only the timer _mode_ setup, the GATE input of the timer was not supposed to be touched. The logic you invented, OTOH, does the first bitflip there, and then advances the timer, which is also correct, but is more complicated. Do you think there was a bug that actually did the first bitflip anyway?
It's for speeding up. We should minimize any calculations in the hrtimer callback.
Ok but snd_pcm_format_signed() and snd_pcm_format_width() are really trivial functions, so I just thought this is not worth an efforts. But if you think this have to be optimized, then fine.
At Fri, 30 Oct 2009 17:23:09 +0300, Stas Sergeev wrote:
Takashi Iwai wrote:
But, I still don't understand why it has to be zero. The first bit-flip is done inside the trigger-start, so the next wakeup should be after the calculated ns. Isn't it?
That was the logic you invented, but initially, and with my patch, there is no bitflip on start trigger. Or, at least, not supposed to be - there was only a timer mode setup, or if not - that was a bug. :)
OK, then we should revert the zero-delay logic again. The zero-delay start means to do the bitflip again immediately. This is wrong, of course.
What do you mean? As I said above, there seems to be no first bitflip on a start trigger, neither initially, nor with my current patch. There was only the timer _mode_ setup, the GATE input of the timer was not supposed to be touched. The logic you invented, OTOH, does the first bitflip there, and then advances the timer, which is also correct, but is more complicated. Do you think there was a bug that actually did the first bitflip anyway?
The problem was that the first call with zero delay didn't happen and hrtimer stopped totally on my tests. So, it had to be implemented in my way.
I'm not sure whether this was fixed. Keep testing on your other machines as I have really no time for checking this at all for the time being. Still over 8,000 mails left in my inbox.
thanks,
Takashi
participants (2)
-
Stas Sergeev -
Takashi Iwai