[igt-dev] [PATCH i-g-t 4/5] tests/kms_chamelium: add S24_LE and S32_LE audio tests

[Peres, Martin]

On 17/05/2019 19:03, Ser, Simon wrote:
> This adds a new test_formats array, which is a list of PCM formats we want to
> run the tests with. We try to run tests for all combinations of sampling rates
> and formats.
> 
> The ALSA callback now needs to fill the playback buffer differently depending
> on the format.
> 
> Signed-off-by: Simon Ser <simon.ser at intel.com>
> ---
>  tests/kms_chamelium.c | 86 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 60 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index af9f54d1f4c7..88356dd232b6 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -795,8 +795,17 @@ static int test_frequencies[] = {
>  
>  static int test_frequencies_count = sizeof(test_frequencies) / sizeof(int);
>  
> +static const snd_pcm_format_t test_formats[] = {
> +	SND_PCM_FORMAT_S16_LE,
> +	SND_PCM_FORMAT_S24_LE,
> +	SND_PCM_FORMAT_S32_LE,
> +};
> +
> +static const size_t test_formats_count = sizeof(test_formats) / sizeof(test_formats[0]);
> +
>  struct audio_state {
>  	struct audio_signal *signal;
> +	snd_pcm_format_t format;
>  	atomic_bool run;
>  };
>  
> @@ -805,7 +814,19 @@ audio_output_callback(void *data, void *buffer, int samples)
>  {
>  	struct audio_state *state = data;
>  
> -	audio_signal_fill_s16_le(state->signal, buffer, samples);
> +	switch (state->format) {
> +	case SND_PCM_FORMAT_S16_LE:
> +		audio_signal_fill_s16_le(state->signal, buffer, samples);
> +		break;
> +	case SND_PCM_FORMAT_S24_LE:
> +		audio_signal_fill_s24_le(state->signal, buffer, samples);
> +		break;
> +	case SND_PCM_FORMAT_S32_LE:
> +		audio_signal_fill_s32_le(state->signal, buffer, samples);
> +		break;
> +	default:
> +		assert(false); /* unreachable */
> +	}
>  
>  	return state->run ? 0 : -1;
>  }
> @@ -881,6 +902,7 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>  	audio_signal_synthesize(signal);
>  
>  	state.signal = signal;
> +	state.format = playback_format;
>  	state.run = true;
>  	alsa_register_output_callback(alsa, audio_output_callback, &state,
>  				      PLAYBACK_SAMPLES);
> @@ -1027,6 +1049,22 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>  	return success;
>  }
>  
> +static bool test_audio_configuration(struct alsa *alsa, snd_pcm_format_t format,
> +				     int channels, int sampling_rate)
> +{
> +	if (!alsa_test_output_configuration(alsa, format, channels,
> +					    sampling_rate)) {
> +		igt_debug("Skipping test with format %s, sampling rate %d Hz "
> +			  "and %d channels because at least one of the "
> +			  "selected output devices doesn't support this "
> +			  "configuration\n",
> +			  snd_pcm_format_name(format),
> +			  sampling_rate, channels);
> +		return false;
> +	}
> +	return true;
> +}
> +
>  static void
>  test_display_audio(data_t *data, struct chamelium_port *port,
>  		   const char *audio_device, enum test_edid edid)
> @@ -1039,7 +1077,7 @@ test_display_audio(data_t *data, struct chamelium_port *port,
>  	struct igt_fb fb;
>  	drmModeModeInfo *mode;
>  	drmModeConnector *connector;
> -	int fb_id, i;
> +	int fb_id, i, j;
>  	int channels, sampling_rate;
>  	snd_pcm_format_t format;
>  
> @@ -1076,35 +1114,31 @@ test_display_audio(data_t *data, struct chamelium_port *port,
>  	run = false;
>  	success = true;
>  	for (i = 0; i < test_sampling_rates_count; i++) {
> -		ret = alsa_open_output(alsa, audio_device);
> -		igt_assert(ret >= 0);
> -
> -		/* TODO: playback with different formats */
> -		/* TODO: playback on all 8 available channels */
> -		format = SND_PCM_FORMAT_S16_LE;
> -		channels = PLAYBACK_CHANNELS;
> -		sampling_rate = test_sampling_rates[i];
> -
> -		if (!alsa_test_output_configuration(alsa, format, channels,
> -						    sampling_rate)) {
> -			igt_debug("Skipping test with format %s, sample rate "
> -				  "%d Hz and %d channels because at least one "
> -				  "of the selected output devices doesn't "
> -				  "support this configuration\n",
> -				  snd_pcm_format_name(format),
> -				  channels, sampling_rate);
> -			continue;
> -		}
> +		for (j = 0; j < test_formats_count; j++) {
> +			ret = alsa_open_output(alsa, audio_device);
> +			igt_assert(ret >= 0);
> +
> +			/* TODO: playback with different formats */

You forgot to remove the TODO.

> +			/* TODO: playback on all 8 available channels */
> +			format = test_formats[j];
> +			channels = PLAYBACK_CHANNELS;
> +			sampling_rate = test_sampling_rates[i];
>  
> -		run = true;
> +			if (!test_audio_configuration(alsa, format, channels,
> +						      sampling_rate))
> +				continue;
>  
> -		success &= do_test_display_audio(data, port, alsa, format,
> -						 channels, sampling_rate);
> +			run = true;
>  
> -		alsa_close_output(alsa);
> +			success &= do_test_display_audio(data, port, alsa,
> +							 format, channels,
> +							 sampling_rate);
> +
> +			alsa_close_output(alsa);
> +		}
>  	}
>  
> -	/* Make sure we tested at least one frequency. */
> +	/* Make sure we tested at least one frequency and format. */
>  	igt_assert(run);

Do we really want to fail if alsa did not export any format?

I mean, maybe we should just skip in this condition and have another
test with EDID that would test that the audio driver parses the EDIDs
correctly and exposes the right formats.

Regardless, this patch looks good with the above TODO removed:

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>


>  	/* Make sure all runs were successful. */
>  	igt_assert(success);
> 

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.