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.

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