Accurate delay reporting from dshare

Accurate delay reporting from dshare

[Alan Young]

Hi,

When the kernel reports current (playback) delay via a call to 
snd_pcm_status() or snd_pcm_delay() for a normal hardware PCM, then the 
delay value reported is the sum of space used in the ring buffer plus 
any delay reported from the underlying runtime driver.

snd_pcm_dshare_status() and snd_pcm_dshare_delay() discard this 
refinement and simply report the use of some ring buffer. Why does it do 
this and how could the reporting be improved? In particular I am 
struggling to understand the relationship between a dshare instance's 
(view of a) ring buffer and the slave (hw) PCM's ring buffer.

I understand that the *slowptr* configuration item can be set to get 
more accurate position updates as part of this reporting but I get the 
feeling that this will not on its own solve the problem. If it did then 
one could simply use the delay value reported by the slave PCM.

The context of these questions is using dshare to expose 4 logical 
stereo devices on a platform using an ARM-based SoC with a single 
8-channel hardware driver. I need to get the same level of delay report 
accuracy as I would do without using dshare.

Alan.

Re: Accurate delay reporting from dshare

[Takashi Iwai]

On Wed, 26 Oct 2016 16:30:17 +0200,
Alan Young wrote:
> 
> Hi,
> 
> When the kernel reports current (playback) delay via a call to
> snd_pcm_status() or snd_pcm_delay() for a normal hardware PCM, then
> the delay value reported is the sum of space used in the ring buffer
> plus any delay reported from the underlying runtime driver.
> 
> snd_pcm_dshare_status() and snd_pcm_dshare_delay() discard this
> refinement and simply report the use of some ring buffer. Why does it
> do this and how could the reporting be improved?

The lack of delay calculation is just for simplicity.  We're tracking
the different hw_avail per each d-* PCM, the delay value has to be
re-calculated for each as in the current way. But we may put the
additional delay computed from the slave PCM, indeed.

> In particular I am
> struggling to understand the relationship between a dshare instance's
> (view of a) ring buffer and the slave (hw) PCM's ring buffer.

Basically d*-plugins share the same ring buffer as the underlying
slave PCM hw layer.  The d-plugins have the buffers in shared memory
in addition for keeping the 32bit data for clipping.  But in general
the ring buffer size and the position are same as the hw.

> I understand that the *slowptr* configuration item can be set to get
> more accurate position updates as part of this reporting but I get the
> feeling that this will not on its own solve the problem. If it did
> then one could simply use the delay value reported by the slave PCM.

Actually slowptr option is enabled as default.  It calls the hwsync of
the slave hw at each status update, instead of the passive update from
the hw PCM itself.

> The context of these questions is using dshare to expose 4 logical
> stereo devices on a platform using an ARM-based SoC with a single
> 8-channel hardware driver. I need to get the same level of delay
> report accuracy as I would do without using dshare.

Well, basically the additional delay can be deduced from
  delay - buffer_size - avail
(This is applied for playback.  For capture, it's slightly different.)

A patch like below *might* work (totally untested!)


thanks,

Takashi

---
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index c5b3178a4990..8e21a6ec5fc2 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -214,6 +214,7 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
 static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 {
 	snd_pcm_direct_t *dshare = pcm->private_data;
+	snd_pcm_status_t slave_status;
 
 	switch (dshare->state) {
 	case SNDRV_PCM_STATE_DRAINING:
@@ -225,12 +226,15 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 	}
 	memset(status, 0, sizeof(*status));
 	snd_pcm_status(dshare->spcm, status);
+	slave_status = *status;
 	status->state = snd_pcm_state(dshare->spcm);
 	status->trigger_tstamp = dshare->trigger_tstamp;
 	status->avail = snd_pcm_mmap_playback_avail(pcm);
 	status->avail_max = status->avail > dshare->avail_max ? status->avail : dshare->avail_max;
 	dshare->avail_max = 0;
 	status->delay = snd_pcm_mmap_playback_delay(pcm);
+	status->delay += slave_status.delay + slave_status.avail -
+		dshare->spcm->buffer_size;
 	return 0;
 }
 

Re: Accurate delay reporting from dshare

[Alan Young]

On 27/10/16 11:52, Takashi Iwai wrote:
> On Wed, 26 Oct 2016 16:30:17 +0200,
> Alan Young wrote:
>> Hi,
>>
>> When the kernel reports current (playback) delay via a call to
>> snd_pcm_status() or snd_pcm_delay() for a normal hardware PCM, then
>> the delay value reported is the sum of space used in the ring buffer
>> plus any delay reported from the underlying runtime driver.
>>
>> snd_pcm_dshare_status() and snd_pcm_dshare_delay() discard this
>> refinement and simply report the use of some ring buffer. Why does it
>> do this and how could the reporting be improved?
> The lack of delay calculation is just for simplicity.  We're tracking
> the different hw_avail per each d-* PCM, the delay value has to be
> re-calculated for each as in the current way. But we may put the
> additional delay computed from the slave PCM, indeed.

Why does it have to be re-calculated? Under what circumstances would 
"snd_pcm_mmap_playback_delay(pcm)" and "slave_status.avail -  
dshare->spcm->buffer_size" (from your patched source) yield different 
results?
> Basically d*-plugins share the same ring buffer as the underlying
> slave PCM hw layer.  The d-plugins have the buffers in shared memory
> in addition for keeping the 32bit data for clipping.
This is only the case for dmix, right?
> But in general
> the ring buffer size and the position are same as the hw.

> Well, basically the additional delay can be deduced from
>    delay - buffer_size - avail
> (This is applied for playback.  For capture, it's slightly different.)
>
> A patch like below *might* work (totally untested!)
>

Thanks, this does seem to help but is not totally reliable.

I am wondering if the earlier call to snd_pcm_dshare_sync_ptr(pcm), 
which I guess forms the basis for the result of 
snd_pcm_mmap_playback_delay(pcm), may be operating on a different set of 
data (because of an intervening interrupt) to that returned by 
snd_pcm_status().

Thanks,
Alan.

> thanks,
>
> Takashi
>
> ---
> diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
> index c5b3178a4990..8e21a6ec5fc2 100644
> --- a/src/pcm/pcm_dshare.c
> +++ b/src/pcm/pcm_dshare.c
> @@ -214,6 +214,7 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
>   static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
>   {
>   	snd_pcm_direct_t *dshare = pcm->private_data;
> +	snd_pcm_status_t slave_status;
>   
>   	switch (dshare->state) {
>   	case SNDRV_PCM_STATE_DRAINING:
> @@ -225,12 +226,15 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
>   	}
>   	memset(status, 0, sizeof(*status));
>   	snd_pcm_status(dshare->spcm, status);
> +	slave_status = *status;
>   	status->state = snd_pcm_state(dshare->spcm);
>   	status->trigger_tstamp = dshare->trigger_tstamp;
>   	status->avail = snd_pcm_mmap_playback_avail(pcm);
>   	status->avail_max = status->avail > dshare->avail_max ? status->avail : dshare->avail_max;
>   	dshare->avail_max = 0;
>   	status->delay = snd_pcm_mmap_playback_delay(pcm);
> +	status->delay += slave_status.delay + slave_status.avail -
> +		dshare->spcm->buffer_size;
>   	return 0;
>   }
>   

Re: Accurate delay reporting from dshare

[Alan Young]

On 02/11/16 14:17, Alan Young wrote:
> I am wondering if the earlier call to snd_pcm_dshare_sync_ptr(pcm), 
> which I guess forms the basis for the result of 
> snd_pcm_mmap_playback_delay(pcm), may be operating on a different set 
> of data (because of an intervening interrupt) to that returned by 
> snd_pcm_status().

How about this for a revised patch. It seems to be working well for me 
(so far). I am not suggesting that this should be considered final yet.

Alan.

diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index 58e47bb..568448c 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -157,23 +157,14 @@ static void snd_pcm_dshare_sync_area(snd_pcm_t *pcm)
  /*
   *  synchronize hardware pointer (hw_ptr) with ours
   */
-static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr)
  {
  	snd_pcm_direct_t *dshare = pcm->private_data;
-	snd_pcm_uframes_t slave_hw_ptr, old_slave_hw_ptr, avail;
+	snd_pcm_uframes_t old_slave_hw_ptr, avail;
  	snd_pcm_sframes_t diff;
  	
-	switch (snd_pcm_state(dshare->spcm)) {
-	case SND_PCM_STATE_DISCONNECTED:
-		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
-		return -ENODEV;
-	default:
-		break;
-	}
-	if (dshare->slowptr)
-		snd_pcm_hwsync(dshare->spcm);
  	old_slave_hw_ptr = dshare->slave_hw_ptr;
-	slave_hw_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr;
+	dshare->slave_hw_ptr = slave_hw_ptr;
  	diff = slave_hw_ptr - old_slave_hw_ptr;
  	if (diff == 0)		/* fast path */
  		return 0;
@@ -207,6 +198,24 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
  	return 0;
  }
  
+static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+{
+	snd_pcm_direct_t *dshare = pcm->private_data;
+
+	switch (snd_pcm_state(dshare->spcm)) {
+	case SND_PCM_STATE_DISCONNECTED:
+		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
+		return -ENODEV;
+	default:
+		break;
+	}
+
+	if (dshare->slowptr)
+		snd_pcm_hwsync(dshare->spcm);
+
+	return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);
+}
+
  /*
   *  plugin implementation
   */
@@ -215,22 +224,24 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
  {
  	snd_pcm_direct_t *dshare = pcm->private_data;
  
+	memset(status, 0, sizeof(*status));
+	snd_pcm_status(dshare->spcm, status);
+
  	switch (dshare->state) {
  	case SNDRV_PCM_STATE_DRAINING:
  	case SNDRV_PCM_STATE_RUNNING:
-		snd_pcm_dshare_sync_ptr(pcm);
+		snd_pcm_dshare_sync_ptr0(pcm, status->hw_ptr);
+		status->delay += snd_pcm_mmap_playback_delay(pcm)
+				+ status->avail - dshare->spcm->buffer_size;
  		break;
  	default:
  		break;
  	}
-	memset(status, 0, sizeof(*status));
-	snd_pcm_status(dshare->spcm, status);
-	status->state = snd_pcm_state(dshare->spcm);
+
  	status->trigger_tstamp = dshare->trigger_tstamp;
  	status->avail = snd_pcm_mmap_playback_avail(pcm);
  	status->avail_max = status->avail > dshare->avail_max ? status->avail : dshare->avail_max;
  	dshare->avail_max = 0;
-	status->delay = snd_pcm_mmap_playback_delay(pcm);
  	return 0;
  }
  

[PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Alan Young]

snd_pcm_dshare_status() gets the underlying status from the slave PCM.
This may contain a delay value that includes elements such as codec and
other transfer delays. Use this as the base for the returned delay
value, adjusted for any frames buffered locally (within the dshare
plugin).

Note: snd_pcm_dshare_delay() is not updated.
---
  src/pcm/pcm_dshare.c | 45 ++++++++++++++++++++++++++++-----------------
  1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index c5b3178..9b478a7 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -157,23 +157,14 @@ static void snd_pcm_dshare_sync_area(snd_pcm_t *pcm)
  /*
   *  synchronize hardware pointer (hw_ptr) with ours
   */
-static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr)
  {
  	snd_pcm_direct_t *dshare = pcm->private_data;
-	snd_pcm_uframes_t slave_hw_ptr, old_slave_hw_ptr, avail;
+	snd_pcm_uframes_t old_slave_hw_ptr, avail;
  	snd_pcm_sframes_t diff;
  	
-	switch (snd_pcm_state(dshare->spcm)) {
-	case SND_PCM_STATE_DISCONNECTED:
-		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
-		return -ENODEV;
-	default:
-		break;
-	}
-	if (dshare->slowptr)
-		snd_pcm_hwsync(dshare->spcm);
  	old_slave_hw_ptr = dshare->slave_hw_ptr;
-	slave_hw_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr;
+	dshare->slave_hw_ptr = slave_hw_ptr;
  	diff = slave_hw_ptr - old_slave_hw_ptr;
  	if (diff == 0)		/* fast path */
  		return 0;
@@ -207,6 +198,24 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
  	return 0;
  }
  
+static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+{
+	snd_pcm_direct_t *dshare = pcm->private_data;
+
+	switch (snd_pcm_state(dshare->spcm)) {
+	case SND_PCM_STATE_DISCONNECTED:
+		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
+		return -ENODEV;
+	default:
+		break;
+	}
+
+	if (dshare->slowptr)
+		snd_pcm_hwsync(dshare->spcm);
+
+	return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);
+}
+
  /*
   *  plugin implementation
   */
@@ -215,22 +224,24 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
  {
  	snd_pcm_direct_t *dshare = pcm->private_data;
  
+	memset(status, 0, sizeof(*status));
+	snd_pcm_status(dshare->spcm, status);
+
  	switch (dshare->state) {
  	case SNDRV_PCM_STATE_DRAINING:
  	case SNDRV_PCM_STATE_RUNNING:
-		snd_pcm_dshare_sync_ptr(pcm);
+		snd_pcm_dshare_sync_ptr0(pcm, status->hw_ptr);
+		status->delay += snd_pcm_mmap_playback_delay(pcm)
+				+ status->avail - dshare->spcm->buffer_size;
  		break;
  	default:
  		break;
  	}
-	memset(status, 0, sizeof(*status));
-	snd_pcm_status(dshare->spcm, status);
-	status->state = snd_pcm_state(dshare->spcm);
+
  	status->trigger_tstamp = dshare->trigger_tstamp;
  	status->avail = snd_pcm_mmap_playback_avail(pcm);
  	status->avail_max = status->avail > dshare->avail_max ? status->avail : dshare->avail_max;
  	dshare->avail_max = 0;
-	status->delay = snd_pcm_mmap_playback_delay(pcm);
  	return 0;
  }
  
-- 
2.5.5

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Takashi Iwai]

On Thu, 17 Nov 2016 09:20:16 +0100,
Alan Young wrote:
> 
> snd_pcm_dshare_status() gets the underlying status from the slave PCM.
> This may contain a delay value that includes elements such as codec and
> other transfer delays. Use this as the base for the returned delay
> value, adjusted for any frames buffered locally (within the dshare
> plugin).
> 
> Note: snd_pcm_dshare_delay() is not updated.

Thanks for the patch, but it doesn't look like a proper patch to be
applied to the latest git tree.  I guess you created a patch on top of
the modified tree.

Please rebase and resubmit.

Also, don't forget to add your sign-off.  We prefer having it in
alsa-lib code like the kernel code (although it's not strictly
needed).


Takashi


> ---
>  src/pcm/pcm_dshare.c | 45 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
> index c5b3178..9b478a7 100644
> --- a/src/pcm/pcm_dshare.c
> +++ b/src/pcm/pcm_dshare.c
> @@ -157,23 +157,14 @@ static void snd_pcm_dshare_sync_area(snd_pcm_t *pcm)
>  /*
>   *  synchronize hardware pointer (hw_ptr) with ours
>   */
> -static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
> +static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr)
>  {
>  	snd_pcm_direct_t *dshare = pcm->private_data;
> -	snd_pcm_uframes_t slave_hw_ptr, old_slave_hw_ptr, avail;
> +	snd_pcm_uframes_t old_slave_hw_ptr, avail;
>  	snd_pcm_sframes_t diff;
>  	
> -	switch (snd_pcm_state(dshare->spcm)) {
> -	case SND_PCM_STATE_DISCONNECTED:
> -		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
> -		return -ENODEV;
> -	default:
> -		break;
> -	}
> -	if (dshare->slowptr)
> -		snd_pcm_hwsync(dshare->spcm);
>  	old_slave_hw_ptr = dshare->slave_hw_ptr;
> -	slave_hw_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr;
> +	dshare->slave_hw_ptr = slave_hw_ptr;
>  	diff = slave_hw_ptr - old_slave_hw_ptr;
>  	if (diff == 0)		/* fast path */
>  		return 0;
> @@ -207,6 +198,24 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
>  	return 0;
>  }
>  +static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
> +{
> +	snd_pcm_direct_t *dshare = pcm->private_data;
> +
> +	switch (snd_pcm_state(dshare->spcm)) {
> +	case SND_PCM_STATE_DISCONNECTED:
> +		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
> +		return -ENODEV;
> +	default:
> +		break;
> +	}
> +
> +	if (dshare->slowptr)
> +		snd_pcm_hwsync(dshare->spcm);
> +
> +	return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);
> +}
> +
>  /*
>   *  plugin implementation
>   */
> @@ -215,22 +224,24 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
>  {
>  	snd_pcm_direct_t *dshare = pcm->private_data;
>  +	memset(status, 0, sizeof(*status));
> +	snd_pcm_status(dshare->spcm, status);
> +
>  	switch (dshare->state) {
>  	case SNDRV_PCM_STATE_DRAINING:
>  	case SNDRV_PCM_STATE_RUNNING:
> -		snd_pcm_dshare_sync_ptr(pcm);
> +		snd_pcm_dshare_sync_ptr0(pcm, status->hw_ptr);
> +		status->delay += snd_pcm_mmap_playback_delay(pcm)
> +				+ status->avail - dshare->spcm->buffer_size;
>  		break;
>  	default:
>  		break;
>  	}
> -	memset(status, 0, sizeof(*status));
> -	snd_pcm_status(dshare->spcm, status);
> -	status->state = snd_pcm_state(dshare->spcm);
> +
>  	status->trigger_tstamp = dshare->trigger_tstamp;
>  	status->avail = snd_pcm_mmap_playback_avail(pcm);
>  	status->avail_max = status->avail > dshare->avail_max ? status->avail : dshare->avail_max;
>  	dshare->avail_max = 0;
> -	status->delay = snd_pcm_mmap_playback_delay(pcm);
>  	return 0;
>  }
>  -- 
> 2.5.5
> 

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Alan Young]

On 17/11/16 10:31, Takashi Iwai wrote:
> On Thu, 17 Nov 2016 09:20:16 +0100,
> Alan Young wrote:
>> snd_pcm_dshare_status() gets the underlying status from the slave PCM.
>> This may contain a delay value that includes elements such as codec and
>> other transfer delays. Use this as the base for the returned delay
>> value, adjusted for any frames buffered locally (within the dshare
>> plugin).
>>
>> Note: snd_pcm_dshare_delay() is not updated.
> Thanks for the patch, but it doesn't look like a proper patch to be
> applied to the latest git tree.  I guess you created a patch on top of
> the modified tree.
>
> Please rebase and resubmit.
>
> Also, don't forget to add your sign-off.  We prefer having it in
> alsa-lib code like the kernel code (although it's not strictly
> needed).

I can add a signoff but first I need to understand what is wrong with 
the patch.

I did a git pull of master from git://git.alsa-project.org/alsa-lib.git, 
committed my patch on top (previous commit a668a94238d: "mixer: Don't 
install smixer modules unless python is enabled") and did a git 
format-patch to generate the supplied patch. What did I do wrong?

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Takashi Iwai]

On Thu, 17 Nov 2016 15:18:04 +0100,
Alan Young wrote:
> 
> On 17/11/16 10:31, Takashi Iwai wrote:
> > On Thu, 17 Nov 2016 09:20:16 +0100,
> > Alan Young wrote:
> >> snd_pcm_dshare_status() gets the underlying status from the slave PCM.
> >> This may contain a delay value that includes elements such as codec and
> >> other transfer delays. Use this as the base for the returned delay
> >> value, adjusted for any frames buffered locally (within the dshare
> >> plugin).
> >>
> >> Note: snd_pcm_dshare_delay() is not updated.
> > Thanks for the patch, but it doesn't look like a proper patch to be
> > applied to the latest git tree.  I guess you created a patch on top of
> > the modified tree.
> >
> > Please rebase and resubmit.
> >
> > Also, don't forget to add your sign-off.  We prefer having it in
> > alsa-lib code like the kernel code (although it's not strictly
> > needed).
> 
> I can add a signoff but first I need to understand what is wrong with
> the patch.
> 
> I did a git pull of master from
> git://git.alsa-project.org/alsa-lib.git, committed my patch on top
> (previous commit a668a94238d: "mixer: Don't install smixer modules
> unless python is enabled") and did a git format-patch to generate the
> supplied patch. What did I do wrong?

Try a clean alsa-lib.git checkout and apply your patch manually there.
Then you'll see what I meant.


Takashi

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Alan Young]

snd_pcm_dshare_status() gets the underlying status from the slave PCM.
This may contain a delay value that includes elements such as codec and
other transfer delays. Use this as the base for the returned delay
value, adjusted for any frames buffered locally (within the dshare
plugin).

Note: snd_pcm_dshare_delay() is not updated.

Signed-off-by: Alan Young <consult.awy@gmail.com>
---
  src/pcm/pcm_dshare.c | 45 ++++++++++++++++++++++++++++-----------------
  1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index c5b3178..9b478a7 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -157,23 +157,14 @@ static void snd_pcm_dshare_sync_area(snd_pcm_t *pcm)
  /*
   *  synchronize hardware pointer (hw_ptr) with ours
   */
-static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t 
slave_hw_ptr)
  {
      snd_pcm_direct_t *dshare = pcm->private_data;
-    snd_pcm_uframes_t slave_hw_ptr, old_slave_hw_ptr, avail;
+    snd_pcm_uframes_t old_slave_hw_ptr, avail;
      snd_pcm_sframes_t diff;

-    switch (snd_pcm_state(dshare->spcm)) {
-    case SND_PCM_STATE_DISCONNECTED:
-        dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
-        return -ENODEV;
-    default:
-        break;
-    }
-    if (dshare->slowptr)
-        snd_pcm_hwsync(dshare->spcm);
      old_slave_hw_ptr = dshare->slave_hw_ptr;
-    slave_hw_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr;
+    dshare->slave_hw_ptr = slave_hw_ptr;
      diff = slave_hw_ptr - old_slave_hw_ptr;
      if (diff == 0)        /* fast path */
          return 0;
@@ -207,6 +198,24 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
      return 0;
  }

+static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+{
+    snd_pcm_direct_t *dshare = pcm->private_data;
+
+    switch (snd_pcm_state(dshare->spcm)) {
+    case SND_PCM_STATE_DISCONNECTED:
+        dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
+        return -ENODEV;
+    default:
+        break;
+    }
+
+    if (dshare->slowptr)
+        snd_pcm_hwsync(dshare->spcm);
+
+    return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);
+}
+
  /*
   *  plugin implementation
   */
@@ -215,22 +224,24 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, 
snd_pcm_status_t * status)
  {
      snd_pcm_direct_t *dshare = pcm->private_data;

+    memset(status, 0, sizeof(*status));
+    snd_pcm_status(dshare->spcm, status);
+
      switch (dshare->state) {
      case SNDRV_PCM_STATE_DRAINING:
      case SNDRV_PCM_STATE_RUNNING:
-        snd_pcm_dshare_sync_ptr(pcm);
+        snd_pcm_dshare_sync_ptr0(pcm, status->hw_ptr);
+        status->delay += snd_pcm_mmap_playback_delay(pcm)
+                + status->avail - dshare->spcm->buffer_size;
          break;
      default:
          break;
      }
-    memset(status, 0, sizeof(*status));
-    snd_pcm_status(dshare->spcm, status);
-    status->state = snd_pcm_state(dshare->spcm);
+
      status->trigger_tstamp = dshare->trigger_tstamp;
      status->avail = snd_pcm_mmap_playback_avail(pcm);
      status->avail_max = status->avail > dshare->avail_max ? 
status->avail : dshare->avail_max;
      dshare->avail_max = 0;
-    status->delay = snd_pcm_mmap_playback_delay(pcm);
      return 0;
  }

-- 
2.5.5

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Takashi Iwai]

On Thu, 17 Nov 2016 15:35:55 +0100,
Alan Young wrote:
> 
> snd_pcm_dshare_status() gets the underlying status from the slave PCM.
> This may contain a delay value that includes elements such as codec and
> other transfer delays. Use this as the base for the returned delay
> value, adjusted for any frames buffered locally (within the dshare
> plugin).
> 
> Note: snd_pcm_dshare_delay() is not updated.
> 
> Signed-off-by: Alan Young <consult.awy@gmail.com>

Unfortunately, your MUA seems malforming the texts and does
line-break and replacing the tabs, so the patch is broken.
Please either fix your MUA setup or use an attachment as a fallback.


thanks,

Takashi

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Alan Young]

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

On 17/11/16 15:12, Takashi Iwai wrote:
> On Thu, 17 Nov 2016 15:35:55 +0100,
> Alan Young wrote:
>> snd_pcm_dshare_status() gets the underlying status from the slave PCM.
>> This may contain a delay value that includes elements such as codec and
>> other transfer delays. Use this as the base for the returned delay
>> value, adjusted for any frames buffered locally (within the dshare
>> plugin).
>>
>> Note: snd_pcm_dshare_delay() is not updated.
>>
>> Signed-off-by: Alan Young <consult.awy@gmail.com>
> Unfortunately, your MUA seems malforming the texts and does
> line-break and replacing the tabs, so the patch is broken.
> Please either fix your MUA setup or use an attachment as a fallback.
Let's try an attachment.

[-- Attachment #2: 0001-pcm_dshare-Do-not-discard-slave-reported-delay-in-st.patch --]
[-- Type: text/x-patch, Size: 3304 bytes --]

>From f459991615254066f687ca8d0aef2bdfeff33964 Mon Sep 17 00:00:00 2001
From: Alan Young <consult.awy@gmail.com>
Date: Wed, 2 Nov 2016 17:40:32 +0000
Subject: [PATCH] pcm_dshare: Do not discard slave reported delay in status
 result.

snd_pcm_dshare_status() gets the underlying status from the slave PCM.
This may contain a delay value that includes elements such as codec and
other transfer delays. Use this as the base for the returned delay
value, adjusted for any frames buffered locally (within the dshare
plugin).

Note: snd_pcm_dshare_delay() is not updated.

Signed-off-by: Alan Young <consult.awy@gmail.com>
---
 src/pcm/pcm_dshare.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index c5b3178..9b478a7 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -157,23 +157,14 @@ static void snd_pcm_dshare_sync_area(snd_pcm_t *pcm)
 /*
  *  synchronize hardware pointer (hw_ptr) with ours
  */
-static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr)
 {
 	snd_pcm_direct_t *dshare = pcm->private_data;
-	snd_pcm_uframes_t slave_hw_ptr, old_slave_hw_ptr, avail;
+	snd_pcm_uframes_t old_slave_hw_ptr, avail;
 	snd_pcm_sframes_t diff;
 	
-	switch (snd_pcm_state(dshare->spcm)) {
-	case SND_PCM_STATE_DISCONNECTED:
-		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
-		return -ENODEV;
-	default:
-		break;
-	}
-	if (dshare->slowptr)
-		snd_pcm_hwsync(dshare->spcm);
 	old_slave_hw_ptr = dshare->slave_hw_ptr;
-	slave_hw_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr;
+	dshare->slave_hw_ptr = slave_hw_ptr;
 	diff = slave_hw_ptr - old_slave_hw_ptr;
 	if (diff == 0)		/* fast path */
 		return 0;
@@ -207,6 +198,24 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
 	return 0;
 }
 
+static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
+{
+	snd_pcm_direct_t *dshare = pcm->private_data;
+
+	switch (snd_pcm_state(dshare->spcm)) {
+	case SND_PCM_STATE_DISCONNECTED:
+		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
+		return -ENODEV;
+	default:
+		break;
+	}
+
+	if (dshare->slowptr)
+		snd_pcm_hwsync(dshare->spcm);
+
+	return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);
+}
+
 /*
  *  plugin implementation
  */
@@ -215,22 +224,24 @@ static int snd_pcm_dshare_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 {
 	snd_pcm_direct_t *dshare = pcm->private_data;
 
+	memset(status, 0, sizeof(*status));
+	snd_pcm_status(dshare->spcm, status);
+
 	switch (dshare->state) {
 	case SNDRV_PCM_STATE_DRAINING:
 	case SNDRV_PCM_STATE_RUNNING:
-		snd_pcm_dshare_sync_ptr(pcm);
+		snd_pcm_dshare_sync_ptr0(pcm, status->hw_ptr);
+		status->delay += snd_pcm_mmap_playback_delay(pcm)
+				+ status->avail - dshare->spcm->buffer_size;
 		break;
 	default:
 		break;
 	}
-	memset(status, 0, sizeof(*status));
-	snd_pcm_status(dshare->spcm, status);
-	status->state = snd_pcm_state(dshare->spcm);
+
 	status->trigger_tstamp = dshare->trigger_tstamp;
 	status->avail = snd_pcm_mmap_playback_avail(pcm);
 	status->avail_max = status->avail > dshare->avail_max ? status->avail : dshare->avail_max;
 	dshare->avail_max = 0;
-	status->delay = snd_pcm_mmap_playback_delay(pcm);
 	return 0;
 }
 
-- 
2.5.5


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

Re: [PATCH] pcm_dshare: Do not discard slave reported delay in status result.

[Takashi Iwai]

On Thu, 17 Nov 2016 16:18:53 +0100,
Alan Young wrote:
> 
> On 17/11/16 15:12, Takashi Iwai wrote:
> > On Thu, 17 Nov 2016 15:35:55 +0100,
> > Alan Young wrote:
> >> snd_pcm_dshare_status() gets the underlying status from the slave PCM.
> >> This may contain a delay value that includes elements such as codec and
> >> other transfer delays. Use this as the base for the returned delay
> >> value, adjusted for any frames buffered locally (within the dshare
> >> plugin).
> >>
> >> Note: snd_pcm_dshare_delay() is not updated.
> >>
> >> Signed-off-by: Alan Young <consult.awy@gmail.com>
> > Unfortunately, your MUA seems malforming the texts and does
> > line-break and replacing the tabs, so the patch is broken.
> > Please either fix your MUA setup or use an attachment as a fallback.
> Let's try an attachment.

OK, applied now.


thanks,

Takashi