[patch] snd-pcsp fixes

[patch] snd-pcsp fixes

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

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=commitdiff;h=eea0579fc85e64e9f05361d5aacf496fe7a151aa

- 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.

[-- Attachment #2: pcsp_nfwa1.diff --]
[-- Type: text/plain, Size: 4751 bytes --]

diff --git a/sound/drivers/pcsp/pcsp_lib.c b/sound/drivers/pcsp/pcsp_lib.c
index 84cc265..587ea0c 100644
--- a/sound/drivers/pcsp/pcsp_lib.c
+++ b/sound/drivers/pcsp/pcsp_lib.c
@@ -39,25 +39,20 @@ static DECLARE_TASKLET(pcsp_pcm_tasklet, pcsp_call_pcm_elapsed, 0);
 /* write the port and returns the next expire time in ns;
  * called at the trigger-start and in hrtimer callback
  */
-static unsigned long pcsp_timer_update(struct hrtimer *handle)
+static u64 pcsp_timer_update(struct snd_pcsp *chip)
 {
 	unsigned char timer_cnt, val;
 	u64 ns;
 	struct snd_pcm_substream *substream;
 	struct snd_pcm_runtime *runtime;
-	struct snd_pcsp *chip = container_of(handle, struct snd_pcsp, timer);
 	unsigned long flags;
 
 	if (chip->thalf) {
 		outb(chip->val61, 0x61);
 		chip->thalf = 0;
-		if (!atomic_read(&chip->timer_active))
-			return 0;
 		return chip->ns_rem;
 	}
 
-	if (!atomic_read(&chip->timer_active))
-		return 0;
 	substream = chip->playback_substream;
 	if (!substream)
 		return 0;
@@ -88,24 +83,17 @@ static unsigned long pcsp_timer_update(struct hrtimer *handle)
 	return ns;
 }
 
-enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
+static void pcsp_pointer_update(struct snd_pcsp *chip)
 {
-	struct snd_pcsp *chip = container_of(handle, struct snd_pcsp, timer);
 	struct snd_pcm_substream *substream;
-	int periods_elapsed, pointer_update;
 	size_t period_bytes, buffer_bytes;
-	unsigned long ns;
+	int periods_elapsed;
 	unsigned long flags;
 
-	pointer_update = !chip->thalf;
-	ns = pcsp_timer_update(handle);
-	if (!ns)
-		return HRTIMER_NORESTART;
-
 	/* update the playback position */
 	substream = chip->playback_substream;
 	if (!substream)
-		return HRTIMER_NORESTART;
+		return;
 
 	period_bytes = snd_pcm_lib_period_bytes(substream);
 	buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
@@ -134,6 +122,26 @@ enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
 
 	if (periods_elapsed)
 		tasklet_schedule(&pcsp_pcm_tasklet);
+}
+
+enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
+{
+	struct snd_pcsp *chip = container_of(handle, struct snd_pcsp, timer);
+	int pointer_update;
+	u64 ns;
+
+	if (!atomic_read(&chip->timer_active) || !chip->playback_substream)
+		return HRTIMER_NORESTART;
+
+	pointer_update = !chip->thalf;
+	ns = pcsp_timer_update(chip);
+	if (!ns) {
+		printk(KERN_WARNING "PCSP: unexpected stop\n");
+		return HRTIMER_NORESTART;
+	}
+
+	if (pointer_update)
+	    pcsp_pointer_update(chip);
 
 	hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns));
 
@@ -142,8 +150,6 @@ enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
 
 static int pcsp_start_playing(struct snd_pcsp *chip)
 {
-	unsigned long ns;
-
 #if PCSP_DEBUG
 	printk(KERN_INFO "PCSP: start_playing called\n");
 #endif
@@ -159,11 +165,7 @@ static int pcsp_start_playing(struct snd_pcsp *chip)
 	atomic_set(&chip->timer_active, 1);
 	chip->thalf = 0;
 
-	ns = pcsp_timer_update(&pcsp_chip.timer);
-	if (!ns)
-		return -EIO;
-
-	hrtimer_start(&pcsp_chip.timer, ktime_set(0, ns), HRTIMER_MODE_REL);
+	hrtimer_start(&pcsp_chip.timer, ktime_set(0, 0), HRTIMER_MODE_REL);
 	return 0;
 }
 
@@ -232,21 +234,22 @@ static int snd_pcsp_playback_hw_free(struct snd_pcm_substream *substream)
 static int snd_pcsp_playback_prepare(struct snd_pcm_substream *substream)
 {
 	struct snd_pcsp *chip = snd_pcm_substream_chip(substream);
+	pcsp_sync_stop(chip);
+	chip->playback_ptr = 0;
+	chip->period_ptr = 0;
+	chip->fmt_size =
+		snd_pcm_format_physical_width(substream->runtime->format) >> 3;
+	chip->is_signed = snd_pcm_format_signed(substream->runtime->format);
 #if PCSP_DEBUG
 	printk(KERN_INFO "PCSP: prepare called, "
-			"size=%zi psize=%zi f=%zi f1=%i\n",
+			"size=%zi psize=%zi f=%zi f1=%i fsize=%i\n",
 			snd_pcm_lib_buffer_bytes(substream),
 			snd_pcm_lib_period_bytes(substream),
 			snd_pcm_lib_buffer_bytes(substream) /
 			snd_pcm_lib_period_bytes(substream),
-			substream->runtime->periods);
+			substream->runtime->periods,
+			chip->fmt_size);
 #endif
-	pcsp_sync_stop(chip);
-	chip->playback_ptr = 0;
-	chip->period_ptr = 0;
-	chip->fmt_size =
-		snd_pcm_format_physical_width(substream->runtime->format) >> 3;
-	chip->is_signed = snd_pcm_format_signed(substream->runtime->format);
 	return 0;
 }
 
diff --git a/sound/drivers/pcsp/pcsp_mixer.c b/sound/drivers/pcsp/pcsp_mixer.c
index 199b033..903bc84 100644
--- a/sound/drivers/pcsp/pcsp_mixer.c
+++ b/sound/drivers/pcsp/pcsp_mixer.c
@@ -72,7 +72,7 @@ static int pcsp_treble_put(struct snd_kcontrol *kcontrol,
 	if (treble != chip->treble) {
 		chip->treble = treble;
 #if PCSP_DEBUG
-		printk(KERN_INFO "PCSP: rate set to %i\n", PCSP_RATE());
+		printk(KERN_INFO "PCSP: rate set to %li\n", PCSP_RATE());
 #endif
 		changed = 1;
 	}

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [patch] snd-pcsp fixes

[Takashi Iwai]

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=commitdiff;h=eea0579fc85e64e9f05361d5aacf496fe7a151aa
> 
> - 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

Re: [patch] snd-pcsp fixes

[Stas Sergeev]

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?

Re: [patch] snd-pcsp fixes

[Takashi Iwai]

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

Re: [patch] snd-pcsp fixes

[Stas Sergeev]

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.

Re: [patch] snd-pcsp fixes

[Takashi Iwai]

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