More snd_pcm_ioplug_avail_update() questions

More snd_pcm_ioplug_avail_update() questions

[Rob Duncan]

Is it required that all available data has been committed between calls
to snd_pcm_ioplug_avail_update() on an IO plugin capture stream?

If this is not a requirement then there can still be uncommitted data in
the mmap after the current appl_ptr.  This data will get overwritten by
the transfer call.

If it is a requirement then snd_pcm_rate_avail_update() seems to be
doing the wrong thing, because it only commits in units of the period
size, which means that there could be a partial period left in the mmap.

Can someone clarify what the contract is for these calls?

Thanks,

Rob.

Re: More snd_pcm_ioplug_avail_update() questions

[Takashi Iwai]

On Wed, 25 Jul 2018 23:52:36 +0200,
Rob Duncan wrote:
> 
> Is it required that all available data has been committed between calls
> to snd_pcm_ioplug_avail_update() on an IO plugin capture stream?

Yes, the capture data is committed.
The snd_pcm_ioplug_hw_ptr_update() should have reported the amount of
data the slave PCM can actually transfer.  Hence the transfer call
thereafter must fulfill the whole requested data.

The capture in ioplug is a sort of mmap emulation.  On real hardware,
the mmap data is already committed before update_avail is called,
update_avail just follows the already committed amount.  In the
emulation mode, it's other way round; the data is committed at the
point hwptr is update, i.e. avail_update is called.


Takashi

> If this is not a requirement then there can still be uncommitted data in
> the mmap after the current appl_ptr.  This data will get overwritten by
> the transfer call.
> 
> If it is a requirement then snd_pcm_rate_avail_update() seems to be
> doing the wrong thing, because it only commits in units of the period
> size, which means that there could be a partial period left in the mmap.
> 
> Can someone clarify what the contract is for these calls?
> 
> Thanks,
> 
> Rob.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: More snd_pcm_ioplug_avail_update() questions

[Rob Duncan]

At 00:04 on Thu, Jul 26 2018, Takashi wrote:
>> Is it required that all available data has been committed between calls
>> to snd_pcm_ioplug_avail_update() on an IO plugin capture stream?
>
> Yes, the capture data is committed.
> The snd_pcm_ioplug_hw_ptr_update() should have reported the amount of
> data the slave PCM can actually transfer.  Hence the transfer call
> thereafter must fulfill the whole requested data.

But what I see is that snd_pcm_rate_avail_update() commits data in the
slave PCM in units of slave->period_size (via
snd_pcm_rate_grab_next_period()), which means that if there is a partial
period in the slave PCM mmap it will not be committed.

In other words, not all the available data will be commmitted before the
next call to snd_pcm_ioplug_avail_update().  This means that the data
will be discarded the next time that snd_pcm_ioplug_avail_update() is
called.

Rob.

Re: More snd_pcm_ioplug_avail_update() questions

[Takashi Iwai]

On Thu, 26 Jul 2018 16:19:19 +0200,
Rob Duncan wrote:
> 
> 
> At 00:04 on Thu, Jul 26 2018, Takashi wrote:
> >> Is it required that all available data has been committed between calls
> >> to snd_pcm_ioplug_avail_update() on an IO plugin capture stream?
> >
> > Yes, the capture data is committed.
> > The snd_pcm_ioplug_hw_ptr_update() should have reported the amount of
> > data the slave PCM can actually transfer.  Hence the transfer call
> > thereafter must fulfill the whole requested data.
> 
> But what I see is that snd_pcm_rate_avail_update() commits data in the
> slave PCM in units of slave->period_size (via
> snd_pcm_rate_grab_next_period()), which means that if there is a partial
> period in the slave PCM mmap it will not be committed.
> 
> In other words, not all the available data will be commmitted before the
> next call to snd_pcm_ioplug_avail_update().  This means that the data
> will be discarded the next time that snd_pcm_ioplug_avail_update() is
> called.

OK, I seem to have misunderstood about what you meant as committed in
the context.  Yes, if the available is partial, it might be not
committed.  But I don't understand the next part.

How will it be discarded at the next snd_pcm_ioplug_avail_update()?
The data remains on the buffer, and applptr isn't changed.


Takashi

Re: More snd_pcm_ioplug_avail_update() questions

[Rob Duncan]

At 07:37 on Thu, Jul 26 2018, Takashi wrote:
> OK, I seem to have misunderstood about what you meant as committed in
> the context.  Yes, if the available is partial, it might be not
> committed.  But I don't understand the next part.
>
> How will it be discarded at the next snd_pcm_ioplug_avail_update()?
> The data remains on the buffer, and applptr isn't changed.

Yes, that's the problem.  Because when snd_pcm_ioplug_avail_update() is
subsequently called it uses snd_pcm_mmap_begin() to get the offset into
the mmap for the destination of the capture transfer operation.  This is
essentially appl_ptr, which means that the data that has not yet been
commited will be overwritten by the transfer.

I guess that the offset could somehow be adjusted to point to after the
uncommitted data but I don't see a straightforward way to do that.  A
scheme along these lines would also have to adjust the size parameter
accordingly, of course.  This would sometimes mean that we cannot
transfer all the available data from the IO plugin.  Would that cause
any issues?

Rob.

Re: More snd_pcm_ioplug_avail_update() questions

[Takashi Iwai]

On Thu, 26 Jul 2018 17:02:33 +0200,
Rob Duncan wrote:
> 
> 
> At 07:37 on Thu, Jul 26 2018, Takashi wrote:
> > OK, I seem to have misunderstood about what you meant as committed in
> > the context.  Yes, if the available is partial, it might be not
> > committed.  But I don't understand the next part.
> >
> > How will it be discarded at the next snd_pcm_ioplug_avail_update()?
> > The data remains on the buffer, and applptr isn't changed.
> 
> Yes, that's the problem.  Because when snd_pcm_ioplug_avail_update() is
> subsequently called it uses snd_pcm_mmap_begin() to get the offset into
> the mmap for the destination of the capture transfer operation.  This is
> essentially appl_ptr, which means that the data that has not yet been
> commited will be overwritten by the transfer.

OK, point taken.  Yeah, if the ioplug driver expects that the transfer
happens only once, it's a problem.

> I guess that the offset could somehow be adjusted to point to after the
> uncommitted data but I don't see a straightforward way to do that.  A
> scheme along these lines would also have to adjust the size parameter
> accordingly, of course.  This would sometimes mean that we cannot
> transfer all the available data from the IO plugin.  Would that cause
> any issues?

Maybe we can track the own applptr (e.g. transfer_ptr or such) and
calculate the transfer size from it instead of snd_pcm_mmap_begin();
i.e. write some open codes of alternative snd_pcm_mmap_begin() there.

Then transfer_ptr is updated again in snd_pcm_ioplug_mmap_commit() as
well when applptr is updated.

Of course, there is a smarter way, I'd happily take another approach.


thanks,

Takashi

Re: More snd_pcm_ioplug_avail_update() questions

[Rob Duncan]

At 08:14 on Thu, Jul 26 2018, Takashi wrote:

> Maybe we can track the own applptr (e.g. transfer_ptr or such) and
> calculate the transfer size from it instead of snd_pcm_mmap_begin();
> i.e. write some open codes of alternative snd_pcm_mmap_begin() there.
>
> Then transfer_ptr is updated again in snd_pcm_ioplug_mmap_commit() as
> well when applptr is updated.
>
> Of course, there is a smarter way, I'd happily take another approach.

Thanks for the hint.  I'm not sure I 100% understand your suggestion,
though.  Unfortunately I don't think I'll have an opportunity to try
this for a few weeks.

Rob.

Re: More snd_pcm_ioplug_avail_update() questions

[Takashi Iwai]

On Mon, 30 Jul 2018 20:55:45 +0200,
Rob Duncan wrote:
> 
> At 08:14 on Thu, Jul 26 2018, Takashi wrote:
> 
> > Maybe we can track the own applptr (e.g. transfer_ptr or such) and
> > calculate the transfer size from it instead of snd_pcm_mmap_begin();
> > i.e. write some open codes of alternative snd_pcm_mmap_begin() there.
> >
> > Then transfer_ptr is updated again in snd_pcm_ioplug_mmap_commit() as
> > well when applptr is updated.
> >
> > Of course, there is a smarter way, I'd happily take another approach.
> 
> Thanks for the hint.  I'm not sure I 100% understand your suggestion,
> though.

Well, something like below (totally untested).


Takashi

---
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -44,6 +44,7 @@ typedef struct snd_pcm_ioplug_priv {
 	struct snd_ext_parm params[SND_PCM_IOPLUG_HW_PARAMS];
 	snd_pcm_uframes_t last_hw;
 	snd_pcm_uframes_t avail_max;
+	snd_pcm_uframes_t transfer_offset;
 	snd_htimestamp_t trigger_tstamp;
 } ioplug_priv_t;
 
@@ -154,6 +155,7 @@ static int snd_pcm_ioplug_reset(snd_pcm_t *pcm)
 	io->data->hw_ptr = 0;
 	io->last_hw = 0;
 	io->avail_max = 0;
+	io->transfer_offset = 0;
 	return 0;
 }
 
@@ -595,7 +597,10 @@ static snd_pcm_sframes_t snd_pcm_ioplug_rewindable(snd_pcm_t *pcm)
 
 static snd_pcm_sframes_t snd_pcm_ioplug_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
 {
+	ioplug_priv_t *io = pcm->private_data;
+
 	snd_pcm_mmap_appl_backward(pcm, frames);
+	io->transfer_offset = io->data->appl_ptr;
 	return frames;
 }
 
@@ -606,7 +611,10 @@ static snd_pcm_sframes_t snd_pcm_ioplug_forwardable(snd_pcm_t *pcm)
 
 static snd_pcm_sframes_t snd_pcm_ioplug_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
 {
+	ioplug_priv_t *io = pcm->private_data;
+
 	snd_pcm_mmap_appl_forward(pcm, frames);
+	io->transfer_offset = io->data->appl_ptr;
 	return frames;
 }
 
@@ -723,10 +731,16 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
 	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED) {
 		if (io->data->callback->transfer) {
 			const snd_pcm_channel_area_t *areas;
-			snd_pcm_uframes_t offset, size = UINT_MAX;
+			snd_pcm_uframes_t offset, size;
 			snd_pcm_sframes_t result;
 
-			__snd_pcm_mmap_begin(pcm, &areas, &offset, &size);
+			areas = snd_pcm_mmap_areas(pcm);
+			if (!areas)
+				return -EBADFD;
+			offset = io->transfer_offset;
+			size = pcm->buffer_size - offset;
+			if (avail < size)
+				size = avail;
 			result = io->data->callback->transfer(io->data, areas, offset, size);
 			if (result < 0)
 				return result;
@@ -741,6 +755,9 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
 				if (result < 0)
 					return result;
 			}
+
+			io->transfer_offset += avail;
+			io->transfer_offset %= pcm->buffer_size;
 		}
 	}
 	if (avail > io->avail_max)