Re: [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

Re: [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Raymond Yau]

[-- Attachment #1.1: Type: text/plain, Size: 2158 bytes --]

> >
> >     PCM Devices which have the BATCH flag set update the PCM pointer
> only with
> >     period size granularity. Using timer based scheduling does not have
> any
> >     advantage in this mode. For one devices which have that flag set
> usually
> >     update
> >     the position pointer in software after getting the period interrupt.
> So
> >     disabling the period interrupt is not possible for this kind of
> devices.
> >     Furthermore writing to or reading from the buffer slice for the
> current
> >     period
> >     is not possible since the position inside the buffer is not known. On
> >     the other
> >     hand the tsched algorithm seems to get easily confused for this kind
> of
> >     hardware, which results in garbled audio output. This typically means
> >     that timer
> >     based scheduling needs to be manually disabled on systems with such
> devices.
> >     Auto disabling tsched in this case allows these systems to run with
> the
> >     default
> >     configuration.
> >
> >
> > If the playback position is reported in steps instead of monotonic
> increasing ?
> >
> > does this mean that you also need to increase rewind_safeguard to one
> period
> > or modifiy snd_pcm_rewindable ?
>
>
> Yes that makes sense. The safeguard should probably be 1.5 periods or 1
> period + fixed value, since it will always take some time from when the
> hardware reaches the next period to when userspace is notified about this.
>
> I'm unfortunately not that familiar with pulseaudio. Maybe there is a
> better
> way to fix this problem then to disable tsched. If anybody wants to
> recreate
> the issue it can easily be emulated by applying the following patch to your
> kernel:
>
>
does all alsa drivers with SNDRV_PCM_INFO_PATCH update the playback/capture
position in period interrupt ?

usb-audio seem update when each URB send/receive (in 1ms intervals)

ymfpci driver also update hwptr in 5ms interval but no SNDRV_PCM_INFO_PATCH
and no constraint in period time must be interval of 5 ms

the rewind_safeguard is affected by the size of FIFO buffer as you cannot
rewind any data in FIFO buffer of DSP/ sound controller

[-- Attachment #1.2: Type: text/html, Size: 2753 bytes --]

[-- Attachment #2: Type: text/plain, Size: 186 bytes --]

_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [alsa-devel] [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Takashi Iwai]

At Tue, 3 Dec 2013 08:47:07 +0800,
Raymond Yau wrote:
> 
> > >
> > >     PCM Devices which have the BATCH flag set update the PCM pointer
> > only with
> > >     period size granularity. Using timer based scheduling does not have
> > any
> > >     advantage in this mode. For one devices which have that flag set
> > usually
> > >     update
> > >     the position pointer in software after getting the period interrupt.
> > So
> > >     disabling the period interrupt is not possible for this kind of
> > devices.
> > >     Furthermore writing to or reading from the buffer slice for the
> > current
> > >     period
> > >     is not possible since the position inside the buffer is not known. On
> > >     the other
> > >     hand the tsched algorithm seems to get easily confused for this kind
> > of
> > >     hardware, which results in garbled audio output. This typically means
> > >     that timer
> > >     based scheduling needs to be manually disabled on systems with such
> > devices.
> > >     Auto disabling tsched in this case allows these systems to run with
> > the
> > >     default
> > >     configuration.
> > >
> > >
> > > If the playback position is reported in steps instead of monotonic
> > increasing ?
> > >
> > > does this mean that you also need to increase rewind_safeguard to one
> > period
> > > or modifiy snd_pcm_rewindable ?
> >
> >
> > Yes that makes sense. The safeguard should probably be 1.5 periods or 1
> > period + fixed value, since it will always take some time from when the
> > hardware reaches the next period to when userspace is notified about this.
> >
> > I'm unfortunately not that familiar with pulseaudio. Maybe there is a
> > better
> > way to fix this problem then to disable tsched. If anybody wants to
> > recreate
> > the issue it can easily be emulated by applying the following patch to your
> > kernel:
> >
> >
> does all alsa drivers with SNDRV_PCM_INFO_PATCH update the playback/capture
> position in period interrupt ?

In general, yes.

> usb-audio seem update when each URB send/receive (in 1ms intervals)

The usb-audio driver behaved in a BATCH way in the very early version,
but it was improved.  So we can get rid of the flag from this driver
now.

> ymfpci driver also update hwptr in 5ms interval but no SNDRV_PCM_INFO_PATCH
> and no constraint in period time must be interval of 5 ms

It's a subtle driver bug.  Feel free to fix it if you have a test
hardware :)  Otherwise, better not to touch such an old code.


Takashi

Re: [alsa-devel] [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Clemens Ladisch]

Takashi Iwai wrote:
> Raymond Yau wrote:
>> usb-audio seem update when each URB send/receive (in 1ms intervals)

The default is to use larger intervals.

> The usb-audio driver behaved in a BATCH way in the very early version,
> but it was improved.

The delay reporting is more precise, but the DMA position still jumps
for each URB.

> So we can get rid of the flag from this driver now.

How well does timer-scheduling work with large URBs?

>> ymfpci driver also update hwptr in 5ms interval but no SNDRV_PCM_INFO_PATCH
>> and no constraint in period time must be interval of 5 ms

The period length (in frames) is not constant for most sample rates.

However, if the noresample flag is set (which should be the case with
PA), the period length should be constant, and such a period constraint
might make sense.  I'll have to test it ...


Regards,
Clemens

Re: [alsa-devel] [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Takashi Iwai]

At Tue, 03 Dec 2013 10:02:44 +0100,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Raymond Yau wrote:
> >> usb-audio seem update when each URB send/receive (in 1ms intervals)
> 
> The default is to use larger intervals.
> 
> > The usb-audio driver behaved in a BATCH way in the very early version,
> > but it was improved.
> 
> The delay reporting is more precise, but the DMA position still jumps
> for each URB.
> 
> > So we can get rid of the flag from this driver now.
> 
> How well does timer-scheduling work with large URBs?

I thought PCM runtime delay should give the accurate sample position,
irrespective to URB size?  It should suffice for PA to control via
timer scheduling.


Takashi

Re: [alsa-devel] [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Clemens Ladisch]

Takashi Iwai wrote:
> Clemens Ladisch wrote:
>> Takashi Iwai wrote:
>>> Raymond Yau wrote:
>>>> usb-audio seem update when each URB send/receive (in 1ms intervals)
>>
>> The default is to use larger intervals.
>>
>>> The usb-audio driver behaved in a BATCH way in the very early version,
>>> but it was improved.
>>
>> The delay reporting is more precise, but the DMA position still jumps
>> for each URB.
>>
>>> So we can get rid of the flag from this driver now.
>>
>> How well does timer-scheduling work with large URBs?
>
> I thought PCM runtime delay should give the accurate sample position,
> irrespective to URB size?

The position of the sample that is currently being played is accurate
(rounded to a USB frame).  The DMA position, however, changes only in
URB-sized increments.  (And that is accurate too, because it describes
exactly how the data is copied out of the buffer.)

> It should suffice for PA to control via timer scheduling.

Large jumps in the DMA position mean that there is a large part of the
buffer which cannot be safely changed by PA.  If this is why PA wants to
disable tsched, then the BATCH flag should stay.


Regards,
Clemens

Re: [pulseaudio-discuss] [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Raymond Yau]

2013/12/6 Tanu Kaskinen <tanu.kaskinen@linux.intel.com>

> On Sat, 2013-11-30 at 18:07 +0100, Lars-Peter Clausen wrote:
> > PCM Devices which have the BATCH flag set update the PCM pointer only
> with
> > period size granularity. Using timer based scheduling does not have any
> > advantage in this mode. For one devices which have that flag set usually
> update
> > the position pointer in software after getting the period interrupt. So
> > disabling the period interrupt is not possible for this kind of devices.
> > Furthermore writing to or reading from the buffer slice for the current
> period
> > is not possible since the position inside the buffer is not known. On
> the other
> > hand the tsched algorithm seems to get easily confused for this kind of
> > hardware, which results in garbled audio output. This typically means
> that timer
> > based scheduling needs to be manually disabled on systems with such
> devices.
> > Auto disabling tsched in this case allows these systems to run with the
> default
> > configuration.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > ---
> >  src/modules/alsa/alsa-util.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
> > index 75f5858..4b24e47 100644
> > --- a/src/modules/alsa/alsa-util.c
> > +++ b/src/modules/alsa/alsa-util.c
> > @@ -245,6 +245,10 @@ int pa_alsa_set_hw_params(
> >      if (!pa_alsa_pcm_is_hw(pcm_handle))
> >          _use_tsched = false;
> >
> > +    /* The PCM pointer is only updated with period granularity */
> > +    if (snd_pcm_hw_params_is_batch(hwparams))
> > +        _use_tsched = false;
> > +
> >  #if (SND_LIB_VERSION >= ((1<<16)|(0<<8)|24)) /* API additions in 1.0.24
> */
> >      if (_use_tsched) {
> >
>
> Thanks! I applied the patch.
>
> --
>

Are you patch sufficient ?

In alsa-lib , snd_pcm_sw_params_set_avail_min always return zero

int snd_pcm_sw_params_set_avail_min(snd_pcm_t *pcm, snd_pcm_sw_params_t
*params, snd_pcm_uframes_t val)
{
         assert(pcm && params);
         /* Fix avail_min if it's below period size.  The period_size
          * defines the minimal wake-up timing accuracy, so it doesn't
          * make sense to set below that.
          */
         if (val < pcm->period_size)
                 val = pcm->period_size;
         params->avail_min = val;
         return 0;
}


http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/src/modules/alsa/alsa-sink.c?id=26a270a934f0f174f7bd7eb89e85301963721deb

/* We need at last one frame in the used part of the buffer */
- avail_min = (snd_pcm_uframes_t) u->hwbuf_unused_frames + 1;
+ avail_min = (snd_pcm_uframes_t) u->hwbuf_unused / u->frame_size + 1;

do pulseaudio need to keep at least one periods instead of more than 1
frames for those driver which update hwptr in periods ?

Re: [PATCH] alsa: Disable timer-scheduling for PCMs with the BATCH flag

[Tanu Kaskinen]

On Mon, 2013-12-09 at 13:23 +0800, Raymond Yau wrote:
> 2013/12/6 Tanu Kaskinen <tanu.kaskinen@linux.intel.com>
> 
> > On Sat, 2013-11-30 at 18:07 +0100, Lars-Peter Clausen wrote:
> > > PCM Devices which have the BATCH flag set update the PCM pointer only
> > with
> > > period size granularity. Using timer based scheduling does not have any
> > > advantage in this mode. For one devices which have that flag set usually
> > update
> > > the position pointer in software after getting the period interrupt. So
> > > disabling the period interrupt is not possible for this kind of devices.
> > > Furthermore writing to or reading from the buffer slice for the current
> > period
> > > is not possible since the position inside the buffer is not known. On
> > the other
> > > hand the tsched algorithm seems to get easily confused for this kind of
> > > hardware, which results in garbled audio output. This typically means
> > that timer
> > > based scheduling needs to be manually disabled on systems with such
> > devices.
> > > Auto disabling tsched in this case allows these systems to run with the
> > default
> > > configuration.
> > >
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > ---
> > >  src/modules/alsa/alsa-util.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
> > > index 75f5858..4b24e47 100644
> > > --- a/src/modules/alsa/alsa-util.c
> > > +++ b/src/modules/alsa/alsa-util.c
> > > @@ -245,6 +245,10 @@ int pa_alsa_set_hw_params(
> > >      if (!pa_alsa_pcm_is_hw(pcm_handle))
> > >          _use_tsched = false;
> > >
> > > +    /* The PCM pointer is only updated with period granularity */
> > > +    if (snd_pcm_hw_params_is_batch(hwparams))
> > > +        _use_tsched = false;
> > > +
> > >  #if (SND_LIB_VERSION >= ((1<<16)|(0<<8)|24)) /* API additions in 1.0.24
> > */
> > >      if (_use_tsched) {
> > >
> >
> > Thanks! I applied the patch.
> >
> > --
> >
> 
> Are you patch sufficient ?
> 
> In alsa-lib , snd_pcm_sw_params_set_avail_min always return zero
> 
> int snd_pcm_sw_params_set_avail_min(snd_pcm_t *pcm, snd_pcm_sw_params_t
> *params, snd_pcm_uframes_t val)
> {
>          assert(pcm && params);
>          /* Fix avail_min if it's below period size.  The period_size
>           * defines the minimal wake-up timing accuracy, so it doesn't
>           * make sense to set below that.
>           */
>          if (val < pcm->period_size)
>                  val = pcm->period_size;
>          params->avail_min = val;
>          return 0;
> }
> 
> 
> http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/src/modules/alsa/alsa-sink.c?id=26a270a934f0f174f7bd7eb89e85301963721deb
> 
> /* We need at last one frame in the used part of the buffer */
> - avail_min = (snd_pcm_uframes_t) u->hwbuf_unused_frames + 1;
> + avail_min = (snd_pcm_uframes_t) u->hwbuf_unused / u->frame_size + 1;
> 
> do pulseaudio need to keep at least one periods instead of more than 1
> frames for those driver which update hwptr in periods ?

I don't know.

-- 
Tanu