[PATCH] aplay: support no period wakeup option in argument

[PATCH] aplay: support no period wakeup option in argument

[S.j. Wang]

In the case that alsa driver can't support period wakeup,
we need to set the no period wakeup flag

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 aplay/aplay.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/aplay/aplay.c b/aplay/aplay.c
index efc1eb4cae3a..4f562bfe2884 100644
--- a/aplay/aplay.c
+++ b/aplay/aplay.c
@@ -137,6 +137,7 @@ static int use_strftime = 0;
 volatile static int recycle_capture_file = 0;
 static long term_c_lflag = -1;
 static int dump_hw_params = 0;
+static int no_period_wakeup = 0;
 
 static int fd = -1;
 static off64_t pbrec_count = LLONG_MAX, fdcount;
@@ -243,6 +244,7 @@ _("Usage: %s [OPTION]... [FILE]...\n"
 "    --use-strftime      apply the strftime facility to the output file name\n"
 "    --dump-hw-params    dump hw_params of the device\n"
 "    --fatal-errors      treat all errors as fatal\n"
+"    --no-period-wakeup  set no period wakeup flag if necessary\n"
   )
 		, command);
 	printf(_("Recognized sample formats are:"));
@@ -429,6 +431,7 @@ enum {
 	OPT_USE_STRFTIME,
 	OPT_DUMP_HWPARAMS,
 	OPT_FATAL_ERRORS,
+	OPT_NO_PERIOD_WAKEUP,
 };
 
 /*
@@ -516,6 +519,7 @@ int main(int argc, char *argv[])
 		{"interactive", 0, 0, 'i'},
 		{"dump-hw-params", 0, 0, OPT_DUMP_HWPARAMS},
 		{"fatal-errors", 0, 0, OPT_FATAL_ERRORS},
+		{"no-period-wakeup", 0, 0, OPT_NO_PERIOD_WAKEUP},
 #ifdef CONFIG_SUPPORT_CHMAP
 		{"chmap", 1, 0, 'm'},
 #endif
@@ -799,6 +803,9 @@ int main(int argc, char *argv[])
 		case OPT_FATAL_ERRORS:
 			fatal_errors = 1;
 			break;
+		case OPT_NO_PERIOD_WAKEUP:
+			no_period_wakeup = 1;
+			break;
 #ifdef CONFIG_SUPPORT_CHMAP
 		case 'm':
 			channel_map = snd_pcm_chmap_parse_string(optarg);
@@ -1396,6 +1403,12 @@ static void set_params(void)
 							     &buffer_frames);
 	}
 	assert(err >= 0);
+
+	if (no_period_wakeup) {
+		err = snd_pcm_hw_params_set_period_wakeup(handle, params, 0);
+		assert(err >= 0);
+	}
+
 	monotonic = snd_pcm_hw_params_is_monotonic(params);
 	can_pause = snd_pcm_hw_params_can_pause(params);
 	err = snd_pcm_hw_params(handle, params);
-- 
1.9.1

Re: [PATCH] aplay: support no period wakeup option in argument

[Takashi Sakamoto]

Hi,

On Wed, Dec 26, 2018 at 11:28:11AM +0000, S.j. Wang wrote:
> In the case that alsa driver can't support period wakeup,
> we need to set the no period wakeup flag
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  aplay/aplay.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/aplay/aplay.c b/aplay/aplay.c
> index efc1eb4cae3a..4f562bfe2884 100644
> --- a/aplay/aplay.c
> +++ b/aplay/aplay.c
> @@ -137,6 +137,7 @@ static int use_strftime = 0;
>  volatile static int recycle_capture_file = 0;
>  static long term_c_lflag = -1;
>  static int dump_hw_params = 0;
> +static int no_period_wakeup = 0;
>  
>  static int fd = -1;
>  static off64_t pbrec_count = LLONG_MAX, fdcount;
> @@ -243,6 +244,7 @@ _("Usage: %s [OPTION]... [FILE]...\n"
>  "    --use-strftime      apply the strftime facility to the output file name\n"
>  "    --dump-hw-params    dump hw_params of the device\n"
>  "    --fatal-errors      treat all errors as fatal\n"
> +"    --no-period-wakeup  set no period wakeup flag if necessary\n"
>    )
>  		, command);
>  	printf(_("Recognized sample formats are:"));
> @@ -429,6 +431,7 @@ enum {
>  	OPT_USE_STRFTIME,
>  	OPT_DUMP_HWPARAMS,
>  	OPT_FATAL_ERRORS,
> +	OPT_NO_PERIOD_WAKEUP,
>  };
>  
>  /*
> @@ -516,6 +519,7 @@ int main(int argc, char *argv[])
>  		{"interactive", 0, 0, 'i'},
>  		{"dump-hw-params", 0, 0, OPT_DUMP_HWPARAMS},
>  		{"fatal-errors", 0, 0, OPT_FATAL_ERRORS},
> +		{"no-period-wakeup", 0, 0, OPT_NO_PERIOD_WAKEUP},
>  #ifdef CONFIG_SUPPORT_CHMAP
>  		{"chmap", 1, 0, 'm'},
>  #endif
> @@ -799,6 +803,9 @@ int main(int argc, char *argv[])
>  		case OPT_FATAL_ERRORS:
>  			fatal_errors = 1;
>  			break;
> +		case OPT_NO_PERIOD_WAKEUP:
> +			no_period_wakeup = 1;
> +			break;
>  #ifdef CONFIG_SUPPORT_CHMAP
>  		case 'm':
>  			channel_map = snd_pcm_chmap_parse_string(optarg);
> @@ -1396,6 +1403,12 @@ static void set_params(void)
>  							     &buffer_frames);
>  	}
>  	assert(err >= 0);
> +
> +	if (no_period_wakeup) {
> +		err = snd_pcm_hw_params_set_period_wakeup(handle, params, 0);
> +		assert(err >= 0);
> +	}
> +
>  	monotonic = snd_pcm_hw_params_is_monotonic(params);
>  	can_pause = snd_pcm_hw_params_can_pause(params);
>  	err = snd_pcm_hw_params(handle, params);
> -- 
> 1.9.1

As of v4.21-rc1, runtime of PCM substream can't run no_period_wakeup
mode unless two conditions are satisfied[1]:
 - driver supports SNDRV_PCM_INFO_NO_PERIOD_WAKEUP
 - PCM applications set SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP flag to
   hardware parameter structure.

Neither alsa-lib nor the most of existent userspace applications set
SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP without explicit call of helper
API. In this point, I can get your intention for this patch. If a driver
just supports no_period_wakeup mode, such driver can't work well.

However, such driver is problematic because apparently it can not run
with many existent userspace applications and alsa-lib. Before applying
this patch, we have enough discussion to prevent problems to introduce
such problematic drivers into Linux sound subsystem, in my opinion.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/pcm_native.c?h=sound-4.21-rc1#n727


Regards

Takashi Sakamoto

Re: [PATCH] aplay: support no period wakeup option in argument

[S.j. Wang]

Hi

> -----Original Message-----
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Sent: Wednesday, December 26, 2018 9:35 PM
> 
> Hi,
> 
> On Wed, Dec 26, 2018 at 11:28:11AM +0000, S.j. Wang wrote:
> > In the case that alsa driver can't support period wakeup, we need to
> > set the no period wakeup flag
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  aplay/aplay.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/aplay/aplay.c b/aplay/aplay.c index
> > efc1eb4cae3a..4f562bfe2884 100644
> > --- a/aplay/aplay.c
> > +++ b/aplay/aplay.c
> > @@ -137,6 +137,7 @@ static int use_strftime = 0;  volatile static int
> > recycle_capture_file = 0;  static long term_c_lflag = -1;  static int
> > dump_hw_params = 0;
> > +static int no_period_wakeup = 0;
> >
> >  static int fd = -1;
> >  static off64_t pbrec_count = LLONG_MAX, fdcount; @@ -243,6 +244,7
> @@
> > _("Usage: %s [OPTION]... [FILE]...\n"
> >  "    --use-strftime      apply the strftime facility to the output file name\n"
> >  "    --dump-hw-params    dump hw_params of the device\n"
> >  "    --fatal-errors      treat all errors as fatal\n"
> > +"    --no-period-wakeup  set no period wakeup flag if necessary\n"
> >    )
> >  		, command);
> >  	printf(_("Recognized sample formats are:")); @@ -429,6 +431,7 @@
> > enum {
> >  	OPT_USE_STRFTIME,
> >  	OPT_DUMP_HWPARAMS,
> >  	OPT_FATAL_ERRORS,
> > +	OPT_NO_PERIOD_WAKEUP,
> >  };
> >
> >  /*
> > @@ -516,6 +519,7 @@ int main(int argc, char *argv[])
> >  		{"interactive", 0, 0, 'i'},
> >  		{"dump-hw-params", 0, 0, OPT_DUMP_HWPARAMS},
> >  		{"fatal-errors", 0, 0, OPT_FATAL_ERRORS},
> > +		{"no-period-wakeup", 0, 0, OPT_NO_PERIOD_WAKEUP},
> >  #ifdef CONFIG_SUPPORT_CHMAP
> >  		{"chmap", 1, 0, 'm'},
> >  #endif
> > @@ -799,6 +803,9 @@ int main(int argc, char *argv[])
> >  		case OPT_FATAL_ERRORS:
> >  			fatal_errors = 1;
> >  			break;
> > +		case OPT_NO_PERIOD_WAKEUP:
> > +			no_period_wakeup = 1;
> > +			break;
> >  #ifdef CONFIG_SUPPORT_CHMAP
> >  		case 'm':
> >  			channel_map =
> snd_pcm_chmap_parse_string(optarg);
> > @@ -1396,6 +1403,12 @@ static void set_params(void)
> >  							     &buffer_frames);
> >  	}
> >  	assert(err >= 0);
> > +
> > +	if (no_period_wakeup) {
> > +		err = snd_pcm_hw_params_set_period_wakeup(handle,
> params, 0);
> > +		assert(err >= 0);
> > +	}
> > +
> >  	monotonic = snd_pcm_hw_params_is_monotonic(params);
> >  	can_pause = snd_pcm_hw_params_can_pause(params);
> >  	err = snd_pcm_hw_params(handle, params);
> > --
> > 1.9.1
> 
> As of v4.21-rc1, runtime of PCM substream can't run no_period_wakeup
> mode unless two conditions are satisfied[1]:
>  - driver supports SNDRV_PCM_INFO_NO_PERIOD_WAKEUP
>  - PCM applications set SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP
> flag to
>    hardware parameter structure.
> 
> Neither alsa-lib nor the most of existent userspace applications set
> SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP without explicit call of
> helper API. In this point, I can get your intention for this patch. If a driver
> just supports no_period_wakeup mode, such driver can't work well.
> 
> However, such driver is problematic because apparently it can not run with
> many existent userspace applications and alsa-lib. Before applying this
> patch, we have enough discussion to prevent problems to introduce such
> problematic drivers into Linux sound subsystem, in my opinion.

What is helper API?  I found that you have a similar function in axfer/xfer-libasound.c,
Which is disable_period_wakeup().  Do you think I need to porting this function to
Aplay? 

Best regards
Wang shengjiu
> 
> [1]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftiwai%2Fsound.git%2
> Ftree%2Fsound%2Fcore%2Fpcm_native.c%3Fh%3Dsound-4.21-
> rc1%23n727&amp;data=02%7C01%7Cshengjiu.wang%40nxp.com%7Cdaa8cb
> 93830245f7b27108d66b36fe91%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C636814281289991663&amp;sdata=Qv2Jfrd2qXc91gpwJhu28bhMC
> SJ5%2FgZJWDzP0gDT0Ws%3D&amp;reserved=0
> 
> 
> Regards
> 
> Takashi Sakamoto

Re: [PATCH] aplay: support no period wakeup option in argument

[Takashi Sakamoto]

On Thu, Dec 27, 2018 at 02:13:28AM +0000, S.j. Wang wrote:
> > -----Original Message-----
> > From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > Sent: Wednesday, December 26, 2018 9:35 PM
> > 
> > Hi,
> > 
> > On Wed, Dec 26, 2018 at 11:28:11AM +0000, S.j. Wang wrote:
> > > In the case that alsa driver can't support period wakeup, we need to
> > > set the no period wakeup flag
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > >  aplay/aplay.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/aplay/aplay.c b/aplay/aplay.c index
> > > efc1eb4cae3a..4f562bfe2884 100644
> > > --- a/aplay/aplay.c
> > > +++ b/aplay/aplay.c
> > > @@ -137,6 +137,7 @@ static int use_strftime = 0;  volatile static int
> > > recycle_capture_file = 0;  static long term_c_lflag = -1;  static int
> > > dump_hw_params = 0;
> > > +static int no_period_wakeup = 0;
> > >
> > >  static int fd = -1;
> > >  static off64_t pbrec_count = LLONG_MAX, fdcount; @@ -243,6 +244,7
> > @@
> > > _("Usage: %s [OPTION]... [FILE]...\n"
> > >  "    --use-strftime      apply the strftime facility to the output file name\n"
> > >  "    --dump-hw-params    dump hw_params of the device\n"
> > >  "    --fatal-errors      treat all errors as fatal\n"
> > > +"    --no-period-wakeup  set no period wakeup flag if necessary\n"
> > >    )
> > >  		, command);
> > >  	printf(_("Recognized sample formats are:")); @@ -429,6 +431,7 @@
> > > enum {
> > >  	OPT_USE_STRFTIME,
> > >  	OPT_DUMP_HWPARAMS,
> > >  	OPT_FATAL_ERRORS,
> > > +	OPT_NO_PERIOD_WAKEUP,
> > >  };
> > >
> > >  /*
> > > @@ -516,6 +519,7 @@ int main(int argc, char *argv[])
> > >  		{"interactive", 0, 0, 'i'},
> > >  		{"dump-hw-params", 0, 0, OPT_DUMP_HWPARAMS},
> > >  		{"fatal-errors", 0, 0, OPT_FATAL_ERRORS},
> > > +		{"no-period-wakeup", 0, 0, OPT_NO_PERIOD_WAKEUP},
> > >  #ifdef CONFIG_SUPPORT_CHMAP
> > >  		{"chmap", 1, 0, 'm'},
> > >  #endif
> > > @@ -799,6 +803,9 @@ int main(int argc, char *argv[])
> > >  		case OPT_FATAL_ERRORS:
> > >  			fatal_errors = 1;
> > >  			break;
> > > +		case OPT_NO_PERIOD_WAKEUP:
> > > +			no_period_wakeup = 1;
> > > +			break;
> > >  #ifdef CONFIG_SUPPORT_CHMAP
> > >  		case 'm':
> > >  			channel_map =
> > snd_pcm_chmap_parse_string(optarg);
> > > @@ -1396,6 +1403,12 @@ static void set_params(void)
> > >  							     &buffer_frames);
> > >  	}
> > >  	assert(err >= 0);
> > > +
> > > +	if (no_period_wakeup) {
> > > +		err = snd_pcm_hw_params_set_period_wakeup(handle,
> > params, 0);
> > > +		assert(err >= 0);
> > > +	}
> > > +
> > >  	monotonic = snd_pcm_hw_params_is_monotonic(params);
> > >  	can_pause = snd_pcm_hw_params_can_pause(params);
> > >  	err = snd_pcm_hw_params(handle, params);
> > > --
> > > 1.9.1
> > 
> > As of v4.21-rc1, runtime of PCM substream can't run no_period_wakeup
> > mode unless two conditions are satisfied[1]:
> >  - driver supports SNDRV_PCM_INFO_NO_PERIOD_WAKEUP
> >  - PCM applications set SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP
> > flag to
> >    hardware parameter structure.
> > 
> > Neither alsa-lib nor the most of existent userspace applications set
> > SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP without explicit call of
> > helper API. In this point, I can get your intention for this patch. If a driver
> > just supports no_period_wakeup mode, such driver can't work well.
> > 
> > However, such driver is problematic because apparently it can not run with
> > many existent userspace applications and alsa-lib. Before applying this
> > patch, we have enough discussion to prevent problems to introduce such
> > problematic drivers into Linux sound subsystem, in my opinion.
> 
> What is helper API?  I found that you have a similar function in axfer/xfer-libasound.c,
> Which is disable_period_wakeup().  Do you think I need to porting this function to
> Aplay? 

I focus on your intention itself instead of change of aplay. Here, I
refer your commit description:

"In the case that alsa driver can't support period wakeup, we need to
set the no period wakeup flag"

In my understanding, your intention is a kind of driver added yet in
mainline. The driver doesn't adopt period wakeup. It just support
no-period-wakeup. Thus the driver works without any periodical
hardware/software IRQs.

At present, the feature, no-period-wakeup is an option of PCM substream.
This means that all of in-kernel drivers _should_ adopt period
wakeup at first. Then, if possible, it can implement no-period-wakeup
with the optional flag.

An addition of driver which support no-period-wakeup only brings
problems to existing applications. For example, execution of ioctl(2)
with SNDRV_PCM_IOCTL_[READ|WRITE][I|N] can bring below message:

"%s write error (DMA or IRQ trouble?)"

This is output in 'wait_for_avail()' of 'sound/core/pcm_lib.c'[1].
This is because nothing awaken up a task of the execution. With normal
drivers, the task is awakened by handlers on any IRQ context.

ALSA drivers should work with such applications, thus it's not good
idea to develop for such drivers. In a case that hardware has no feature
to generate periodical IRQs, kernel timer or hrtimer can be available to
achieve 'pseudo' period wakeup for such drivers.

If you wish for drivers which don't achieve period wakeup, it's better
to have enough discussion to change behaviour of ALSA PCM core at first,
then work for aplay next.

Please inform me if I misread in your description.


[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/pcm_lib.c#n1901


Thanks

Takashi Sakamoto

Re: [PATCH] aplay: support no period wakeup option in argument

[S.j. Wang]

Hi

> -----Original Message-----
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Sent: Thursday, December 27, 2018 12:29 PM
> 
> On Thu, Dec 27, 2018 at 02:13:28AM +0000, S.j. Wang wrote:
> > > -----Original Message-----
> > > From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > Sent: Wednesday, December 26, 2018 9:35 PM
> > >
> > > Hi,
> > >
> > > On Wed, Dec 26, 2018 at 11:28:11AM +0000, S.j. Wang wrote:
> > > > In the case that alsa driver can't support period wakeup, we need
> > > > to set the no period wakeup flag
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > >  aplay/aplay.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/aplay/aplay.c b/aplay/aplay.c index
> > > > efc1eb4cae3a..4f562bfe2884 100644
> > > > --- a/aplay/aplay.c
> > > > +++ b/aplay/aplay.c
> > > > @@ -137,6 +137,7 @@ static int use_strftime = 0;  volatile static
> > > > int recycle_capture_file = 0;  static long term_c_lflag = -1;
> > > > static int dump_hw_params = 0;
> > > > +static int no_period_wakeup = 0;
> > > >
> > > >  static int fd = -1;
> > > >  static off64_t pbrec_count = LLONG_MAX, fdcount; @@ -243,6 +244,7
> > > @@
> > > > _("Usage: %s [OPTION]... [FILE]...\n"
> > > >  "    --use-strftime      apply the strftime facility to the output file
> name\n"
> > > >  "    --dump-hw-params    dump hw_params of the device\n"
> > > >  "    --fatal-errors      treat all errors as fatal\n"
> > > > +"    --no-period-wakeup  set no period wakeup flag if necessary\n"
> > > >    )
> > > >  		, command);
> > > >  	printf(_("Recognized sample formats are:")); @@ -429,6 +431,7 @@
> > > > enum {
> > > >  	OPT_USE_STRFTIME,
> > > >  	OPT_DUMP_HWPARAMS,
> > > >  	OPT_FATAL_ERRORS,
> > > > +	OPT_NO_PERIOD_WAKEUP,
> > > >  };
> > > >
> > > >  /*
> > > > @@ -516,6 +519,7 @@ int main(int argc, char *argv[])
> > > >  		{"interactive", 0, 0, 'i'},
> > > >  		{"dump-hw-params", 0, 0, OPT_DUMP_HWPARAMS},
> > > >  		{"fatal-errors", 0, 0, OPT_FATAL_ERRORS},
> > > > +		{"no-period-wakeup", 0, 0, OPT_NO_PERIOD_WAKEUP},
> > > >  #ifdef CONFIG_SUPPORT_CHMAP
> > > >  		{"chmap", 1, 0, 'm'},
> > > >  #endif
> > > > @@ -799,6 +803,9 @@ int main(int argc, char *argv[])
> > > >  		case OPT_FATAL_ERRORS:
> > > >  			fatal_errors = 1;
> > > >  			break;
> > > > +		case OPT_NO_PERIOD_WAKEUP:
> > > > +			no_period_wakeup = 1;
> > > > +			break;
> > > >  #ifdef CONFIG_SUPPORT_CHMAP
> > > >  		case 'm':
> > > >  			channel_map =
> > > snd_pcm_chmap_parse_string(optarg);
> > > > @@ -1396,6 +1403,12 @@ static void set_params(void)
> > > >  							     &buffer_frames);
> > > >  	}
> > > >  	assert(err >= 0);
> > > > +
> > > > +	if (no_period_wakeup) {
> > > > +		err = snd_pcm_hw_params_set_period_wakeup(handle,
> > > params, 0);
> > > > +		assert(err >= 0);
> > > > +	}
> > > > +
> > > >  	monotonic = snd_pcm_hw_params_is_monotonic(params);
> > > >  	can_pause = snd_pcm_hw_params_can_pause(params);
> > > >  	err = snd_pcm_hw_params(handle, params);
> > > > --
> > > > 1.9.1
> > >
> > > As of v4.21-rc1, runtime of PCM substream can't run no_period_wakeup
> > > mode unless two conditions are satisfied[1]:
> > >  - driver supports SNDRV_PCM_INFO_NO_PERIOD_WAKEUP
> > >  - PCM applications set
> SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP
> > > flag to
> > >    hardware parameter structure.
> > >
> > > Neither alsa-lib nor the most of existent userspace applications set
> > > SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP without explicit call of
> helper
> > > API. In this point, I can get your intention for this patch. If a
> > > driver just supports no_period_wakeup mode, such driver can't work
> well.
> > >
> > > However, such driver is problematic because apparently it can not
> > > run with many existent userspace applications and alsa-lib. Before
> > > applying this patch, we have enough discussion to prevent problems
> > > to introduce such problematic drivers into Linux sound subsystem, in
> my opinion.
> >
> > What is helper API?  I found that you have a similar function in
> > axfer/xfer-libasound.c, Which is disable_period_wakeup().  Do you
> > think I need to porting this function to Aplay?
> 
> I focus on your intention itself instead of change of aplay. Here, I refer your
> commit description:
> 
> "In the case that alsa driver can't support period wakeup, we need to set
> the no period wakeup flag"
> 
> In my understanding, your intention is a kind of driver added yet in
> mainline. The driver doesn't adopt period wakeup. It just support no-
> period-wakeup. Thus the driver works without any periodical
> hardware/software IRQs.
> 
> At present, the feature, no-period-wakeup is an option of PCM substream.
> This means that all of in-kernel drivers _should_ adopt period wakeup at
> first. Then, if possible, it can implement no-period-wakeup with the
> optional flag.
> 
> An addition of driver which support no-period-wakeup only brings
> problems to existing applications. For example, execution of ioctl(2) with
> SNDRV_PCM_IOCTL_[READ|WRITE][I|N] can bring below message:
> 
> "%s write error (DMA or IRQ trouble?)"
> 
> This is output in 'wait_for_avail()' of 'sound/core/pcm_lib.c'[1].
> This is because nothing awaken up a task of the execution. With normal
> drivers, the task is awakened by handlers on any IRQ context.
> 
> ALSA drivers should work with such applications, thus it's not good idea to
> develop for such drivers. In a case that hardware has no feature to generate
> periodical IRQs, kernel timer or hrtimer can be available to achieve 'pseudo'
> period wakeup for such drivers.
> 
> If you wish for drivers which don't achieve period wakeup, it's better to
> have enough discussion to change behaviour of ALSA PCM core at first, then
> work for aplay next.
> 
> Please inform me if I misread in your description.

Thanks for the suggestion to use timer in driver. 

I still have two questions:
1. It seems the no-period-wakeup feature should be dropped for it isn't recommended
   By alsa, right?  I still found some driver in kernel support it, what's the reason?

2. Shall we add this option in aplay for it is feature that alsa support, even it is optional?

Best regards
Wang shengjiu
> 
> 
> [1]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftiwai%2Fsound.git%2
> Ftree%2Fsound%2Fcore%2Fpcm_lib.c%23n1901&amp;data=02%7C01%7Cshe
> ngjiu.wang%40nxp.com%7C333a2e780d2d4dcf000a08d66bb3d352%7C686ea
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636814817430239208&amp;s
> data=ur11k%2F1mx7U45pj4PpNZcz8M4TdHIQiHxqLszjyNTkQ%3D&amp;reser
> ved=0
> 
> 
> Thanks
> 
> Takashi Sakamoto

Re: [PATCH] aplay: support no period wakeup option in argument

[Takashi Sakamoto]

Hi,

On Thu, Dec 27, 2018 at 06:46:57AM +0000, S.j. Wang wrote:
> I still have two questions:
> 1. It seems the no-period-wakeup feature should be dropped for it isn't recommended
>    By alsa, right?  I still found some driver in kernel support it, what's the reason?
 
You can see the driver supports 'period-wakeup' runtime as well as
'no-period-wakeup' runtime. The driver likely includes a condition
statement to check 'struct snd_pcm_runtime.no_period_wakeup' flag,
then it switches own behaviour for both cases.

Please keep it in your mind that 'period-wakeup' runtime is a
default. At present, there's no way for drivers to tell userspace
applications that 'period-wakeup' runtime is not supported.

> 2. Shall we add this option in aplay for it is feature that alsa support, even it is optional?

Initially 'no-period-wakeup' runtime was introduced to achieve
timer-based scheduling model[1]. Current design of aplay is not
suitable to this model in many points (e.g. value of timeout
argument in each call of poll(2) system call). If adopting
aplay to the model, heavy refactoring is required. For this
reason, I'm not positive to your idea. It's not practical.

However, just configuring hardware parameter with 'no-period-wakeup'
flag, it's possible, as you posted. In this case, you misses some
issues. At least:
 - alsa-lib configures the flag just for non-blocking operation.
 - alsa-lib calls poll(2) without timeout in many parts. When
   configuring the flag to PCM runtime, no tasks and IRQ contexts
   awaken the sleep process. Therefore processing of PCM frame is
   blocked forever in some cases.

(A call of 'snd_pcm_sw_params_set_period_event()' expects 'hw' PCM
plugin in alsa-lib to use ALSA timer interface for 'emulated'
periodical wakeup, however an instance of timer is for the PCM
runtime with 'no-period-wakeup' thus it can't awakens the sleep
process. I think this is a bug in alsa-lib.)

[1] http://0pointer.de/blog/projects/pulse-glitch-free.html
[2] https://github.com/alsa-project/alsa-utils/blob/master/axfer/xfer-libasound-timer-mmap.c#L22


Thanks

Takashi Sakamoto

Re: [PATCH] aplay: support no period wakeup option in argument

[Takashi Iwai]

On Thu, 27 Dec 2018 07:46:57 +0100,
S.j. Wang wrote:
> 
> I still have two questions:
> 1. It seems the no-period-wakeup feature should be dropped for it isn't recommended
>    By alsa, right?  I still found some driver in kernel support it, what's the reason?
> 
> 2. Shall we add this option in aplay for it is feature that alsa support, even it is optional?

As Sakamoto-san already mentioned, no-period-wakeup is nothing but an
optional feature for very specific applications and setup, and it's
still valid and useful.

The condition to use this option is:
- If a hardware may perform DMA continuously on a ring buffer (a la 
  free-wheel mode), and
- Application doesn't need the wakeup from kernel at each PCM period,
  but it manages the stream transfer timing by itself.
 
aplay doesn't satisfies the latter, hence adding such a command line
option makes no sense.  But, it doesn't mean that no-period-wakeup is
useless.  It can work well for specific applications like PulseAudio.


Takashi