[PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[Pierre-Louis Bossart]

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);
-- 
1.7.6.5

[PATCH 2/2] ALSA: core: group read of pointer, tstamp and jiffies

[Pierre-Louis Bossart]

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);
 }
-- 
1.7.6.5

Re: [PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[Takashi Iwai]

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;
>  		}
>  		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);
> -- 
> 1.7.6.5
> 

Re: [PATCH 2/2] ALSA: core: group read of pointer, tstamp and jiffies

[Takashi Iwai]

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;
>  		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);
>  }
> -- 
> 1.7.6.5
> 

Re: [PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[Pierre-Louis Bossart]

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

Re: [PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[Clemens Ladisch]

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

Re: [PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[Pierre-Louis Bossart]

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

Re: [PATCH 1/2] ALSA: update sync header when streams are linked/unlinked

[Takashi Iwai]

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