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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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