[alsa-devel] [PATCH 1/2] ALSA: update sync header when streams are linked/unlinked
Previous code only reported card number and was not updated when devices were linked/unlinked. This makes alsa-lib snd_pcm_info_get_sync totally useless. Add hooks to force update of sync header when devices are linked/unlinked, and provide more information such as number of devices and indices of capture/playback devices linked to
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/core/pcm_lib.c | 37 ++++++++++++++++++++++++++++++++----- sound/core/pcm_native.c | 6 ++++++ 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index faedb14..ae46d75 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -533,11 +533,38 @@ EXPORT_SYMBOL(snd_pcm_set_ops); void snd_pcm_set_sync(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; - - runtime->sync.id32[0] = substream->pcm->card->number; - runtime->sync.id32[1] = -1; - runtime->sync.id32[2] = -1; - runtime->sync.id32[3] = -1; + struct snd_pcm_substream *s; + int dev_c, dev_p; + + if (snd_pcm_stream_linked(substream)) { + runtime->sync.id32[0] = substream->pcm->card->number; + /* save number of devices linked */ + runtime->sync.id32[1] = substream->group->count-1; + dev_c = dev_p = 0; + + snd_pcm_group_for_each_entry(s, substream) { + if (s == substream) + continue; + if (s->stream == SNDRV_PCM_STREAM_PLAYBACK) { + dev_p++; + if (dev_p == 1) + runtime->sync.id32[2] = 0; + /* save mask of linked playback devices */ + runtime->sync.id32[2] |= (1<<s->number); + } else { + dev_c++; + if (dev_c == 1) + runtime->sync.id32[3] = 0; + /* save mask of linked capture devices */ + runtime->sync.id32[3] |= (1<<s->number); + } + } + } else { + runtime->sync.id32[0] = -1; + runtime->sync.id32[1] = -1; + runtime->sync.id32[2] = -1; + runtime->sync.id32[3] = -1; + } }
EXPORT_SYMBOL(snd_pcm_set_sync); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 3fe99e6..e92bc62 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1619,6 +1619,9 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd) list_add_tail(&substream1->link_list, &substream->group->substreams); substream->group->count++; substream1->group = substream->group; + snd_pcm_set_sync(substream); + snd_pcm_set_sync(substream1); + _end: write_unlock_irq(&snd_pcm_link_rwlock); up_write(&snd_pcm_link_rwsem); @@ -1652,11 +1655,14 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream) if (substream->group->count == 1) { /* detach the last stream, too */ snd_pcm_group_for_each_entry(s, substream) { relink_to_local(s); + snd_pcm_set_sync(s); break; } kfree(substream->group); } relink_to_local(substream); + snd_pcm_set_sync(substream); + _end: write_unlock_irq(&snd_pcm_link_rwlock); up_write(&snd_pcm_link_rwsem);
Group read of hw_ptr, tstamp and jiffies in a sequence for better correlation. Previous code took timestamp at the end, which could introduce delays between audio time and system time.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/core/pcm_lib.c | 23 ++++++++++++++++++----- 1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index ae46d75..3f3097d 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -313,9 +313,22 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, snd_pcm_uframes_t old_hw_ptr, new_hw_ptr, hw_base; snd_pcm_sframes_t hdelta, delta; unsigned long jdelta; + unsigned long curr_jiffies; + struct timespec curr_tstamp;
old_hw_ptr = runtime->status->hw_ptr; + + /* + * group pointer, time and jiffies reads to allow for more + * accurate correlations/corrections. + * The values are stored at the end of this routine after + * corrections for hw_ptr position + */ pos = substream->ops->pointer(substream); + curr_jiffies = jiffies; + if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) + snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp); + if (pos == SNDRV_PCM_POS_XRUN) { xrun(substream); return -EPIPE; @@ -343,7 +356,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, delta = runtime->hw_ptr_interrupt + runtime->period_size; if (delta > new_hw_ptr) { /* check for double acknowledged interrupts */ - hdelta = jiffies - runtime->hw_ptr_jiffies; + hdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (hdelta > runtime->hw_ptr_buffer_jiffies/2) { hw_base += runtime->buffer_size; if (hw_base >= runtime->boundary) @@ -388,7 +401,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, * Without regular period interrupts, we have to check * the elapsed time to detect xruns. */ - jdelta = jiffies - runtime->hw_ptr_jiffies; + jdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (jdelta < runtime->hw_ptr_buffer_jiffies / 2) goto no_delta_check; hdelta = jdelta - delta * HZ / runtime->rate; @@ -430,7 +443,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, if (hdelta < runtime->delay) goto no_jiffies_check; hdelta -= runtime->delay; - jdelta = jiffies - runtime->hw_ptr_jiffies; + jdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) { delta = jdelta / (((runtime->period_size * HZ) / runtime->rate) @@ -492,9 +505,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, } runtime->hw_ptr_base = hw_base; runtime->status->hw_ptr = new_hw_ptr; - runtime->hw_ptr_jiffies = jiffies; + runtime->hw_ptr_jiffies = curr_jiffies; if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) - snd_pcm_gettime(runtime, (struct timespec *)&runtime->status->tstamp); + runtime->status->tstamp = curr_tstamp;
return snd_pcm_update_state(substream, runtime); }
At Tue, 22 May 2012 14:54:02 -0500, Pierre-Louis Bossart wrote:
Group read of hw_ptr, tstamp and jiffies in a sequence for better correlation. Previous code took timestamp at the end, which could introduce delays between audio time and system time.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Applied this one. Thanks.
Takashi
sound/core/pcm_lib.c | 23 ++++++++++++++++++----- 1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index ae46d75..3f3097d 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -313,9 +313,22 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, snd_pcm_uframes_t old_hw_ptr, new_hw_ptr, hw_base; snd_pcm_sframes_t hdelta, delta; unsigned long jdelta;
unsigned long curr_jiffies;
struct timespec curr_tstamp;
old_hw_ptr = runtime->status->hw_ptr;
/*
* group pointer, time and jiffies reads to allow for more* accurate correlations/corrections.* The values are stored at the end of this routine after* corrections for hw_ptr position*/pos = substream->ops->pointer(substream);
curr_jiffies = jiffies;
if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE)
snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);if (pos == SNDRV_PCM_POS_XRUN) { xrun(substream); return -EPIPE;
@@ -343,7 +356,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, delta = runtime->hw_ptr_interrupt + runtime->period_size; if (delta > new_hw_ptr) { /* check for double acknowledged interrupts */
hdelta = jiffies - runtime->hw_ptr_jiffies;
hdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (hdelta > runtime->hw_ptr_buffer_jiffies/2) { hw_base += runtime->buffer_size; if (hw_base >= runtime->boundary)@@ -388,7 +401,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, * Without regular period interrupts, we have to check * the elapsed time to detect xruns. */
jdelta = jiffies - runtime->hw_ptr_jiffies;
jdelta = curr_jiffies - runtime->hw_ptr_jiffies;@@ -430,7 +443,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, if (hdelta < runtime->delay) goto no_jiffies_check; hdelta -= runtime->delay;
- jdelta = jiffies - runtime->hw_ptr_jiffies;
- jdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) { delta = jdelta / (((runtime->period_size * HZ) / runtime->rate)
@@ -492,9 +505,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, } runtime->hw_ptr_base = hw_base; runtime->status->hw_ptr = new_hw_ptr;
- runtime->hw_ptr_jiffies = jiffies;
- runtime->hw_ptr_jiffies = curr_jiffies; if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE)
snd_pcm_gettime(runtime, (struct timespec *)&runtime->status->tstamp);
runtime->status->tstamp = curr_tstamp;return snd_pcm_update_state(substream, runtime);
}
1.7.6.5
At Tue, 22 May 2012 14:54:01 -0500, Pierre-Louis Bossart wrote:
Previous code only reported card number and was not updated when devices were linked/unlinked. This makes alsa-lib snd_pcm_info_get_sync totally useless. Add hooks to force update of sync header when devices are linked/unlinked,
This looks like a right "fix", but ...
and provide more information such as number of devices and indices of capture/playback devices linked to
This is a completely different issue, so please don't mix up in a single patch.
And...
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/core/pcm_lib.c | 37 ++++++++++++++++++++++++++++++++----- sound/core/pcm_native.c | 6 ++++++ 2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index faedb14..ae46d75 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -533,11 +533,38 @@ EXPORT_SYMBOL(snd_pcm_set_ops); void snd_pcm_set_sync(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime;
- runtime->sync.id32[0] = substream->pcm->card->number;
- runtime->sync.id32[1] = -1;
- runtime->sync.id32[2] = -1;
- runtime->sync.id32[3] = -1;
- struct snd_pcm_substream *s;
- int dev_c, dev_p;
- if (snd_pcm_stream_linked(substream)) {
runtime->sync.id32[0] = substream->pcm->card->number;/* save number of devices linked */runtime->sync.id32[1] = substream->group->count-1;dev_c = dev_p = 0;snd_pcm_group_for_each_entry(s, substream) {if (s == substream)continue;if (s->stream == SNDRV_PCM_STREAM_PLAYBACK) {dev_p++;if (dev_p == 1)runtime->sync.id32[2] = 0;/* save mask of linked playback devices */runtime->sync.id32[2] |= (1<<s->number);
... this isn's safe. There are more than 32 substreams. And there are multiple streams with the same substream index.
Also, the point of the sync id is that it's shared with all linked streams. Your patch breaks it. It updates only the last added sync id.
The fact that the driver currently sets only the card number is actually problematic. It's not unique enough. This should be fixed. But, exposing the substream bitmask doesn't help much because it can't be fully implemented in the sync id size. If you need to know which streams are linked, loop over all streams and check the sync id.
thanks,
Takashi
} else {dev_c++;if (dev_c == 1)runtime->sync.id32[3] = 0;/* save mask of linked capture devices */runtime->sync.id32[3] |= (1<<s->number);}}- } else {
runtime->sync.id32[0] = -1;runtime->sync.id32[1] = -1;runtime->sync.id32[2] = -1;runtime->sync.id32[3] = -1;- }
}
EXPORT_SYMBOL(snd_pcm_set_sync); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 3fe99e6..e92bc62 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1619,6 +1619,9 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd) list_add_tail(&substream1->link_list, &substream->group->substreams); substream->group->count++; substream1->group = substream->group;
- snd_pcm_set_sync(substream);
- snd_pcm_set_sync(substream1);
- _end: write_unlock_irq(&snd_pcm_link_rwlock); up_write(&snd_pcm_link_rwsem);
@@ -1652,11 +1655,14 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream) if (substream->group->count == 1) { /* detach the last stream, too */ snd_pcm_group_for_each_entry(s, substream) { relink_to_local(s);
snd_pcm_set_sync(s); break;- snd_pcm_set_sync(substream);
_end:-- 1.7.6.5
Thanks for reviewing Takashi. Comments below.
and provide more information such as number of devices and indices of capture/playback devices linked to
This is a completely different issue, so please don't mix up in a single patch.
No problem. I did this on purpose to see the reaction and understand what the expectation was...I don't really care about the contents of this structure as long as it's consistent.
... this isn's safe. There are more than 32 substreams. And there are multiple streams with the same substream index.
The only HW I know of that supports linked streams is HDAudio, and it uses a 32-bit mask for SSYNC... But I guess you're right this doesn't scale.
Also, the point of the sync id is that it's shared with all linked streams. Your patch breaks it. It updates only the last added sync id.
The fact that the driver currently sets only the card number is actually problematic. It's not unique enough. This should be fixed. But, exposing the substream bitmask doesn't help much because it can't be fully implemented in the sync id size. If you need to know which streams are linked, loop over all streams and check the sync id.
If I understand you well, the sync id should be a unique identifier shared by all linked streams in the same group. Since devices can be linked/unlinked, this id cannot use anything related to device or subdevice number. Maybe a pid-like value incremented when a group is created would do? Thanks, -Pierre
Pierre-Louis Bossart wrote:
The fact that the driver currently sets only the card number is actually problematic. It's not unique enough. This should be fixed. But, exposing the substream bitmask doesn't help much because it can't be fully implemented in the sync id size. If you need to know which streams are linked, loop over all streams and check the sync id.
If I understand you well, the sync id should be a unique identifier shared by all linked streams in the same group.
Just to clarify: does the sync id identify streams that are linked, or streams that can be started atomically when linked? Because at the moment, all drivers implement the latter. Furthermore, it's possible to link completely unrelated devices, so not even the card number could be used for the former.
Regards, Clemens
If I understand you well, the sync id should be a unique identifier shared by all linked streams in the same group.
Just to clarify: does the sync id identify streams that are linked, or streams that can be started atomically when linked? Because at the moment, all drivers implement the latter. Furthermore, it's possible to link completely unrelated devices, so not even the card number could be used for the former.
I may have a very Intel-centric view, but it should be the former, identify all streams currently linked. All devices controlled by the same HDAudio controller can be linked at any time, providing the list of possible streams to link to does help anyone since the information is known by default. However, I've also seen that a lot of non-HDAudio drivers seem to provide the ability to link only playback and capture for synchronized full-duplex operation. This is a much simpler case than multiple playback/capture devices, most serial links (SSI, McBSP, SSP, etc) provide such capabilities. Bottom line is that maybe the sync information needs to provide both the devices that can be linked and the devices currently linked.
Regarding the card #, linking between devices handled by different controllers is not supported in hardware, only 32 devices controlled with the same SSYNC register can be linked. In addition there are tests in the HDAudio code to yank devices with a different card number from the group (see azx_pcm_trigger() in hda_intel.c) -Pierre
At Wed, 23 May 2012 13:57:34 -0500, Pierre-Louis Bossart wrote:
Also, the point of the sync id is that it's shared with all linked streams. Your patch breaks it. It updates only the last added sync id.
The fact that the driver currently sets only the card number is actually problematic. It's not unique enough. This should be fixed. But, exposing the substream bitmask doesn't help much because it can't be fully implemented in the sync id size. If you need to know which streams are linked, loop over all streams and check the sync id.
If I understand you well, the sync id should be a unique identifier shared by all linked streams in the same group. Since devices can be linked/unlinked, this id cannot use anything related to device or subdevice number. Maybe a pid-like value incremented when a group is created would do?
Yes, that sounds like a reasonable solution.
thanks,
Takashi
participants (3)
-
Clemens Ladisch -
Pierre-Louis Bossart -
Takashi Iwai