[alsa-devel] [PATCH 1/2] ALSA: pcm - Ensure wakeup at each periodic interrupt

[jassi brar]

On Mon, Dec 21, 2009 at 8:28 PM, Takashi Iwai <tiwai at suse.de> wrote:
> At Thu, 17 Dec 2009 17:21:28 +0900,
> jassi brar wrote:
>>
>> On Thu, Dec 17, 2009 at 4:43 PM, Takashi Iwai <tiwai at suse.de> wrote:
>> > At Thu, 17 Dec 2009 16:31:08 +0900,
>> > jassi brar wrote:
>> >>
>> >> On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai <tiwai at suse.de> wrote:
>> >> > At Thu, 17 Dec 2009 15:00:02 +0900,
>> >> > jassisinghbrar at gmail.com wrote:
>> >> >>
>> >> >> From: Jassi Brar <jassi.brar at samsung.com>
>> >> >>
>> >> >> The check for at least 'avail_min' available data before calling wake_up
>> >> >> doesn't always hold good as it does not guarantee callbacks at each periodic
>> >> >> interrupt.
>> >> >
>> >> > Well, avail_min can be greater than period_size.  And, avail_min won't be
>> >> > less than period size.
>> >> >
>> >> > For example, when avail_min = 2.5 x period_size, the driver wakes up
>> >> > in periods like 3, 2, 3, 2, ...
>> >> correct, but if we ensure wake_up's after each period and let the 'sleepers'
>> >> track if the data available is enough or not, we will have more fine grained
>> >> control.
>> >> The point is:- Waking up _after_ avail_min is working, but does waking up before
>> >> avail_min(but at period boundary) break the system?
>> >
>> > PulseAudio may complain :)
>> I meant effects on ALSA state-machine within the kernel.
>> >From what i have seen, every use of sleep is just to kill some time,
>> i.e, wake_up
>> is not taken as indication of completion of the purpose.
>
> It's used also for poll.
>
> Meanwhile, we may change snd_pcm_*_poll() function itself, too...
How about employing a new snd_pcm_runtime member (say, need_min) which
denote the _exact_ minimum amount of data needed to serve purpose
of last sleep ?

Quick idea of what i mean, follows. If you like it, i will resend the
patches again
Thanks.

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index de6d981..b5a9e81 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -267,6 +267,7 @@ struct snd_pcm_runtime {
 	struct snd_pcm_substream *trigger_master;
 	struct timespec trigger_tstamp;	/* trigger timestamp */
 	int overrange;
+	snd_pcm_uframes_t need_min;	/* exact minimum needed before wakeup */
 	snd_pcm_uframes_t avail_max;
 	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
 	snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 30f4108..a689add 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -208,7 +208,7 @@ static int snd_pcm_update_hw_ptr_post(struct
snd_pcm_substream *substream,
 			return -EPIPE;
 		}
 	}
-	if (avail >= runtime->control->avail_min)
+	if (avail >= runtime->need_min)
 		wake_up(&runtime->sleep);
 	return 0;
 }
@@ -1681,6 +1681,7 @@ static int wait_for_avail_min(struct
snd_pcm_substream *substream,
 			break;
 		}
 		set_current_state(TASK_INTERRUPTIBLE);
+		runtime->need_min = *availp;
 		snd_pcm_stream_unlock_irq(substream);
 		tout = schedule_timeout(msecs_to_jiffies(10000));
 		snd_pcm_stream_lock_irq(substream);
@@ -1788,6 +1789,7 @@ static snd_pcm_sframes_t
snd_pcm_lib_write1(struct snd_pcm_substream *substream,
 				err = -EAGAIN;
 				goto _end_unlock;
 			}
+			avail = size;
 			err = wait_for_avail_min(substream, &avail);
 			if (err < 0)
 				goto _end_unlock;
@@ -2010,6 +2012,7 @@ static snd_pcm_sframes_t
snd_pcm_lib_read1(struct snd_pcm_substream *substream,
 				err = -EAGAIN;
 				goto _end_unlock;
 			}
+			avail = size;
 			err = wait_for_avail_min(substream, &avail);
 			if (err < 0)
 				goto _end_unlock;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ab73edf..6e5dfa2 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2892,6 +2892,10 @@ static unsigned int
snd_pcm_playback_poll(struct file *file, poll_table * wait)
 		return -ENXIO;
 	runtime = substream->runtime;

+	snd_pcm_stream_lock_irq(substream);
+	runtime->need_min = runtime->control->avail_min;
+	snd_pcm_stream_unlock_irq(substream);
+
 	poll_wait(file, &runtime->sleep, wait);

 	snd_pcm_stream_lock_irq(substream);
@@ -2931,6 +2935,10 @@ static unsigned int snd_pcm_capture_poll(struct
file *file, poll_table * wait)
 		return -ENXIO;
 	runtime = substream->runtime;

+	snd_pcm_stream_lock_irq(substream);
+	runtime->need_min = runtime->control->avail_min;
+	snd_pcm_stream_unlock_irq(substream);
+
 	poll_wait(file, &runtime->sleep, wait);

 	snd_pcm_stream_lock_irq(substream);