tests/kms_chamelium: add dp-audio test

Before this patch, the audio test first sends an audio signal for 2s, and then checks whether the captured signal matches.

This patch makes it so we send and check the signal in parallel. Thus we can stop the test as soon as we receive the correct signal. This saves ~2s per audio test.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_alsa.c        | 15 +++++++-------
 tests/kms_chamelium.c | 47 +++++++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 19 deletions(-)

lib/igt_alsa.c +7 -8

index 22089881..fc6d336b 100644
@@ -497,7 +497,8 @@ void alsa_register_input_callback(struct alsa *alsa,
 /**
  * alsa_run:
  * @alsa: The target alsa structure
- * @duration_ms: The maximum duration of the run in milliseconds
+ * @duration_ms: The maximum duration of the run in milliseconds, or -1 for an
+ * infinite duration.
  *
  * Run ALSA playback and capture on the input and output devices for at
  * most @duration_ms milliseconds, calling the registered callbacks when needed.

@@ -545,7 +546,7 @@ int alsa_run(struct alsa *alsa, int duration_ms)
 	do {
 		reached = true;
 
-		if (output_total < output_limit) {
+		if (output_limit < 0 || output_total < output_limit) {
 			reached = false;
 
 			if (!output_ready) {

@@ -607,7 +608,8 @@ int alsa_run(struct alsa *alsa, int duration_ms)
 
 		}
 
-		if (alsa->input_callback && input_total < input_limit) {
+		if (alsa->input_callback &&
+		    (input_limit < 0 || input_total < input_limit)) {
 			reached = false;
 
 			if (input_count == input_trigger) {

@@ -660,11 +662,8 @@ int alsa_run(struct alsa *alsa, int duration_ms)
 	ret = 0;
 
 complete:
-	if (output_buffer)
-		free(output_buffer);
-
-	if (input_buffer)
-		free(input_buffer);
+	free(output_buffer);
+	free(input_buffer);
 
 	return ret;
 }

tests/kms_chamelium.c +29 -11

index a712250a..014a22b3 100644
@@ -29,7 +29,9 @@
 #include "igt_vc4.h"
 
 #include <fcntl.h>
+#include <pthread.h>
 #include <string.h>
+#include <stdatomic.h>
 
 typedef struct {
 	struct chamelium *chamelium;

@@ -719,7 +721,7 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
 /* Capture paremeters control the audio signal we receive */
 #define CAPTURE_SAMPLES 2048
 
-#define AUDIO_DURATION 2000 /* ms */
+#define AUDIO_TIMEOUT 2000 /* ms */
 /* A streak of 3 gives confidence that the signal is good. */
 #define MIN_STREAK 3
 

@@ -746,14 +748,28 @@ static int test_frequencies[] = {
 
 static int test_frequencies_count = sizeof(test_frequencies) / sizeof(int);
 
+struct audio_state {
+	struct audio_signal *signal;
+	atomic_bool run;
+};
+
 static int
-output_callback(void *data, short *buffer, int frames)
+audio_output_callback(void *data, short *buffer, int frames)
 {
-	struct audio_signal *signal = (struct audio_signal *) data;
+	struct audio_state *state = data;
+
+	audio_signal_fill(state->signal, buffer, frames);
 
-	audio_signal_fill(signal, buffer, frames);
+	return state->run ? 0 : -1;
+}
+
+static void *
+run_audio_thread(void *data)
+{
+	struct alsa *alsa = data;
 
-	return 0;
+	alsa_run(alsa, -1);
+	return NULL;
 }
 
 static bool

@@ -774,6 +790,8 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	char dump_suffix[64];
 	char *dump_path = NULL;
 	int dump_fd = -1;
+	pthread_t thread;
+	struct audio_state state = {};
 
 	if (!alsa_test_output_configuration(alsa, playback_channels,
 					    playback_rate))

@@ -811,15 +829,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		audio_signal_add_frequency(signal, test_frequencies[i]);
 	audio_signal_synthesize(signal);
 
-	alsa_register_output_callback(alsa, output_callback, signal,
+	state.signal = signal;
+	state.run = true;
+	alsa_register_output_callback(alsa, audio_output_callback, &state,
 				      PLAYBACK_SAMPLES);
 
-	/* TODO: detect signal in real-time */
-	ret = alsa_run(alsa, AUDIO_DURATION);
+	/* Start playing audio */
+	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
 	igt_assert(ret == 0);
 
-	alsa_close_output(alsa);
-
 	/* Needs to be a multiple of 128, because that's the number of samples
 	 * we get per channel each time we receive an audio page from the
 	 * Chamelium device. */

@@ -836,7 +854,7 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	streak = 0;
 	msec = 0;
 	i = 0;
-	while (streak < MIN_STREAK && msec < AUDIO_DURATION) {
+	while (streak < MIN_STREAK && msec < AUDIO_TIMEOUT) {
 		ok = chamelium_stream_receive_realtime_audio(stream,
 							     &page_count,
 							     &recv, &recv_len);

@@ -871,6 +889,13 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		i++;
 	}
 

This commit updates the audio test to make sure we receive a signal from both audio channels. However this commit doesn't check that left and right channels are not swapped. Such a check requires some more work (because the Chamelium device does swap left and right channels) and will be implemented in a future commit.

This commit adds a new channel argument to audio_signal_add_frequency, to add a frequency to a single channel only.

Some light refactoring has been performed (a proper audio_signal_deinit function has been introduced) and logging has been improved.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_alsa.c        |  26 +++++++--
 lib/igt_audio.c       | 119 +++++++++++++++++++++++++-----------------
 lib/igt_audio.h       |  11 ++--
 tests/kms_chamelium.c |  56 ++++++++++++--------
 4 files changed, 137 insertions(+), 75 deletions(-)

lib/igt_alsa.c +0 -0

index fc6d336b..a478686a 100644

lib/igt_alsa.c +3 -0

 				snprintf(name, sizeof(name), "hw:%d,%d", card,
 					 dev);
 

@@ -329,6 +331,9 @@ static bool alsa_test_configuration(snd_pcm_t *handle, int channels,
 {
 	snd_pcm_hw_params_t *params;
 	int ret;
+	unsigned int min_channels, max_channels;
+	unsigned int min_rate, max_rate;
+	int min_rate_dir, max_rate_dir;
 
 	snd_pcm_hw_params_alloca(&params);
 

@@ -337,12 +342,24 @@ static bool alsa_test_configuration(snd_pcm_t *handle, int channels,
 		return false;
 

lib/igt_alsa.c +8 -1

 	ret = snd_pcm_hw_params_test_channels(handle, params, channels);
-	if (ret < 0)
+	if (ret < 0) {
+		snd_pcm_hw_params_get_channels_min(params, &min_channels);
+		snd_pcm_hw_params_get_channels_max(params, &max_channels);
+		igt_debug("Output device supports between %u and "
+			  "%u channels, requested %d\n",
+			  min_channels, max_channels, channels);
 		return false;
+	}
 
 	return true;
 }

@@ -409,13 +426,16 @@ void alsa_configure_output(struct alsa *alsa, int channels,
 	snd_pcm_t *handle;
 	int ret;
 	int i;
+	int soft_resample = 0; /* Don't allow ALSA to resample */

lib/igt_alsa.c +0 -0

 		igt_assert(ret >= 0);
 	}
 

lib/igt_audio.c +5 -4

index 7624f565..a2a5c594 100644
@@ -35,7 +35,7 @@
 #include "igt_audio.h"
 #include "igt_core.h"
 
-#define FREQS_MAX	8
+#define FREQS_MAX 64
 
 /**
  * SECTION:igt_audio

@@ -49,9 +49,10 @@
 
 struct audio_signal_freq {
 	int freq;
+	int channel;
 
-	short *period;
-	int frames;
+	int16_t *period;
+	size_t period_len;
 	int offset;
 };
 

@@ -60,7 +61,7 @@ struct audio_signal {
 	int sampling_rate;
 
 	struct audio_signal_freq freqs[FREQS_MAX];
-	int freqs_count;
+	size_t freqs_count;
 };
 
 /**

@@ -89,21 +90,28 @@ struct audio_signal *audio_signal_init(int channels, int sampling_rate)
  * audio_signal_add_frequency:
  * @signal: The target signal structure
  * @frequency: The frequency to add to the signal

lib/igt_audio.c +2 -3

 {
-	int index = signal->freqs_count;
+	size_t index = signal->freqs_count;
+	struct audio_signal_freq *freq;
 
-	if (index == FREQS_MAX)
-		return -1;

lib/igt_audio.c +12 -12

 	/* Clip the frequency to an integer multiple of the sampling rate.
 	 * This to be able to store a full period of it and use that for

@@ -111,11 +119,14 @@ int audio_signal_add_frequency(struct audio_signal *signal, int frequency)
 	 */
 	frequency = signal->sampling_rate / (signal->sampling_rate / frequency);
 
-	igt_debug("Adding test frequency %d\n", frequency);
+	igt_debug("Adding test frequency %d to channel %d\n",
+		  frequency, channel);
+
+	freq = &signal->freqs[index];
+	memset(freq, 0, sizeof(*freq));
+	freq->freq = frequency;
+	freq->channel = channel;
 
-	signal->freqs[index].freq = frequency;
-	signal->freqs[index].frames = 0;
-	signal->freqs[index].offset = 0;
 	signal->freqs_count++;
 
 	return 0;

@@ -133,20 +144,17 @@ void audio_signal_synthesize(struct audio_signal *signal)
 {
 	int16_t *period;
 	double value;
-	int frames;
+	size_t period_len;
 	int freq;
 	int i, j;
 
-	if (signal->freqs_count == 0)
-		return;
-
 	for (i = 0; i < signal->freqs_count; i++) {
 		freq = signal->freqs[i].freq;
-		frames = signal->sampling_rate / freq;
+		period_len = signal->sampling_rate / freq;
 
-		period = calloc(1, frames * sizeof(short));
+		period = calloc(1, period_len * sizeof(int16_t));
 
-		for (j = 0; j < frames; j++) {
+		for (j = 0; j < period_len; j++) {
 			value = 2.0 * M_PI * freq / signal->sampling_rate * j;
 			value = sin(value) * INT16_MAX / signal->freqs_count;
 

@@ -154,26 +162,34 @@ void audio_signal_synthesize(struct audio_signal *signal)
 		}
 
 		signal->freqs[i].period = period;
-		signal->freqs[i].frames = frames;
+		signal->freqs[i].period_len = period_len;

lib/igt_audio.c +38 -25

+{
+	audio_signal_reset(signal);
+	free(signal);
+}
+
+/**
+ * audio_signal_reset:
  * @signal: The target signal structure
  *
  * Free the resources allocated by audio_signal_synthesize and remove
  * the previously-added frequencies.
  */
-void audio_signal_clean(struct audio_signal *signal)
+void audio_signal_reset(struct audio_signal *signal)
 {
-	int i;
+	size_t i;
 
 	for (i = 0; i < signal->freqs_count; i++) {
-		if (signal->freqs[i].period)
-			free(signal->freqs[i].period);
-
-		memset(&signal->freqs[i], 0, sizeof(struct audio_signal_freq));
+		free(signal->freqs[i].period);
 	}
 
 	signal->freqs_count = 0;

@@ -183,44 +199,45 @@ void audio_signal_clean(struct audio_signal *signal)
  * audio_signal_fill:
  * @signal: The target signal structure
  * @buffer: The target buffer to fill
- * @frames: The number of frames to fill
+ * @samples: The number of samples to fill
  *
- * Fill the requested number of frames to the target buffer with the audio
+ * Fill the requested number of samples to the target buffer with the audio
  * signal data (in interleaved S16_LE format), at the requested sampling rate
  * and number of channels.
  */
-void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames)
+void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
+		       size_t buffer_len)
 {
 	int16_t *destination, *source;
+	struct audio_signal_freq *freq;
 	int total;
-	int freq_frames;
-	int freq_offset;
 	int count;
 	int i, j, k;
 
-	memset(buffer, 0, sizeof(int16_t) * signal->channels * frames);
+	memset(buffer, 0, sizeof(int16_t) * signal->channels * buffer_len);
 
 	for (i = 0; i < signal->freqs_count; i++) {
+		freq = &signal->freqs[i];
 		total = 0;
 
-		while (total < frames) {
-			freq_frames = signal->freqs[i].frames;
-			freq_offset = signal->freqs[i].offset;
+		igt_assert(freq->period);
 
-			source = signal->freqs[i].period + freq_offset;
+		while (total < buffer_len) {
+			source = freq->period + freq->offset;
 			destination = buffer + total * signal->channels;
 
-			count = freq_frames - freq_offset;
-			if (count > (frames - total))
-				count = frames - total;
+			count = freq->period_len - freq->offset;
+			if (count > buffer_len - total)
+				count = buffer_len - total;
 
-			freq_offset += count;
-			freq_offset %= freq_frames;
-
-			signal->freqs[i].offset = freq_offset;
+			freq->offset += count;
+			freq->offset %= freq->period_len;
 
 			for (j = 0; j < count; j++) {
 				for (k = 0; k < signal->channels; k++) {
+					if (freq->channel >= 0 &&
+					    freq->channel != k)
+						continue;
 					destination[j * signal->channels + k] += source[j];
 				}
 			}

@@ -237,11 +254,11 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames)
  * sampling_rate is given in Hz. data_len is the number of elements in data.
  */
 bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
-			 double *data, size_t data_len)
+			 int channel, double *data, size_t data_len)
 {
 	size_t bin_power_len = data_len / 2 + 1;
 	double bin_power[bin_power_len];
-	bool detected[signal->freqs_count];
+	bool detected[FREQS_MAX];
 	int ret, freq_accuracy, freq, local_max_freq;
 	double max, local_max, threshold;
 	size_t i, j;

@@ -308,6 +325,10 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
 		 * invalid. */
 		if (bin_power[i] < threshold) {
 			for (j = 0; j < signal->freqs_count; j++) {
+				if (signal->freqs[j].channel >= 0 &&
+				    signal->freqs[j].channel != channel)
+					continue;
+
 				if (signal->freqs[j].freq >
 				    local_max_freq - freq_accuracy &&
 				    signal->freqs[j].freq <

@@ -340,6 +361,10 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
 
 	/* Check that all frequencies we generated have been detected. */
 	for (i = 0; i < signal->freqs_count; i++) {
+		if (signal->freqs[i].channel >= 0 &&
+		    signal->freqs[i].channel != channel)
+			continue;
+
 		if (!detected[i]) {
 			igt_debug("Missing frequency: %d\n",
 				  signal->freqs[i].freq);

lib/igt_audio.h +7 -4

index 4aa43e69..fe26bb57 100644
@@ -35,12 +35,15 @@
 struct audio_signal;
 
 struct audio_signal *audio_signal_init(int channels, int sampling_rate);
-int audio_signal_add_frequency(struct audio_signal *signal, int frequency);
+void audio_signal_deinit(struct audio_signal *signal);
+int audio_signal_add_frequency(struct audio_signal *signal, int frequency,
+			       int channel);
 void audio_signal_synthesize(struct audio_signal *signal);
-void audio_signal_clean(struct audio_signal *signal);
-void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames);
+void audio_signal_reset(struct audio_signal *signal);
+void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
+		       size_t buffer_len);
 bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
-			 double *data, size_t data_len);
+			 int channel, double *data, size_t data_len);
 size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
 				    int32_t *src, size_t src_len,
 				    int n_channels, int channel);

tests/kms_chamelium.c +33 -18

index 014a22b3..d336612f 100644
@@ -777,16 +777,16 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		      struct alsa *alsa, int playback_channels,
 		      int playback_rate)
 {
-	int ret, capture_rate, capture_channels, msec;
+	int ret, capture_rate, capture_channels, msec, freq;
 	struct chamelium_audio_file *audio_file;
 	struct chamelium_stream *stream;
 	enum chamelium_stream_realtime_mode stream_mode;
 	struct audio_signal *signal;
 	int32_t *recv, *buf;
 	double *channel;
-	size_t i, streak, page_count;
+	size_t i, j, streak, page_count;
 	size_t recv_len, buf_len, buf_cap, buf_size, channel_len;
-	bool ok;
+	bool ok, success;
 	char dump_suffix[64];
 	char *dump_path = NULL;
 	int dump_fd = -1;

@@ -794,10 +794,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	struct audio_state state = {};
 
 	if (!alsa_test_output_configuration(alsa, playback_channels,
-					    playback_rate))
+					    playback_rate)) {
+		igt_debug("Skipping test with sample rate %d and %d channels "
+			  "because selected output devices don't support this "
+			  "configuration\n", playback_rate, playback_channels);
 		return false;
+	}
 
-	igt_debug("Testing with playback sampling rate %d\n", playback_rate);
+	igt_debug("Testing with playback sampling rate %d and %d channels\n",
+		  playback_rate, playback_channels);
 	alsa_configure_output(alsa, playback_channels, playback_rate);
 
 	chamelium_start_capturing_audio(data->chamelium, port, false);

@@ -825,8 +830,12 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	signal = audio_signal_init(playback_channels, playback_rate);
 	igt_assert(signal);
 
-	for (i = 0; i < test_frequencies_count; i++)
-		audio_signal_add_frequency(signal, test_frequencies[i]);
+	for (i = 0; i < test_frequencies_count; i++) {
+		for (j = 0; j < playback_channels; j++) {
+			freq = test_frequencies[i];
+			audio_signal_add_frequency(signal, freq, j);
+		}
+	}
 	audio_signal_synthesize(signal);
 
 	state.signal = signal;

@@ -851,10 +860,11 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	recv = NULL;
 	recv_len = 0;
 
+	success = false;
 	streak = 0;
 	msec = 0;
 	i = 0;
-	while (streak < MIN_STREAK && msec < AUDIO_TIMEOUT) {
+	while (!success && msec < AUDIO_TIMEOUT) {
 		ok = chamelium_stream_receive_realtime_audio(stream,
 							     &page_count,
 							     &recv, &recv_len);

@@ -872,21 +882,27 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 			igt_assert(write(dump_fd, buf, buf_size) == buf_size);
 		}
 
-		/* TODO: check other channels too, not just the first one */
-		audio_extract_channel_s32_le(channel, channel_len, buf, buf_len,
-					     capture_channels, 0);
-
 		msec = i * channel_len / (double) capture_rate * 1000;
 		igt_debug("Detecting audio signal, t=%d msec\n", msec);
 
-		if (audio_signal_detect(signal, capture_rate, channel,
-					channel_len))
-			streak++;
-		else
-			streak = 0;
+		for (j = 0; j < playback_channels; j++) {
+			igt_debug("Processing channel %zu\n", j);
+
+			audio_extract_channel_s32_le(channel, channel_len,
+						     buf, buf_len,
+						     capture_channels, j);
+
+			if (audio_signal_detect(signal, capture_rate, j,
+						channel, channel_len))
+				streak++;
+			else
+				streak = 0;
+		}
 
 		buf_len = 0;
 		i++;
+
+		success = streak == MIN_STREAK * playback_channels;
 	}
 
 	igt_debug("Stopping audio playback\n");

@@ -921,12 +937,10 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		chamelium_destroy_audio_file(audio_file);
 	}
 
-	audio_signal_clean(signal);

Quoted 28 lines · Simon Ser

This commit updates the audio test to make sure we receive a signal from both audio channels. However this commit doesn't check that left and right channels are not swapped. Such a check requires some more work (because the Chamelium device does swap left and right channels) and will be implemented in a future commit.

This commit adds a new channel argument to audio_signal_add_frequency, to add a frequency to a single channel only.

Some light refactoring has been performed (a proper audio_signal_deinit function has been introduced) and logging has been improved.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_alsa.c        |  26 +++++++--
 lib/igt_audio.c       | 119 +++++++++++++++++++++++++-----------------
 lib/igt_audio.h       |  11 ++--
 tests/kms_chamelium.c |  56 ++++++++++++--------
 4 files changed, 137 insertions(+), 75 deletions(-)

lib/igt_alsa.c +0 -0

index fc6d336b..a478686a 100644
@@ -182,6 +182,8 @@ static char *alsa_resolve_indentifier(const char *device_name, int skip)
 					continue;
 				}
 

lib/igt_alsa.c static char *alsa_resolve_indentifier(const char *device_name, int skip) · line 186

Quoted 2 lines

lib/igt_alsa.c +2 -0

+				igt_debug("Matched device \"%s\"\n", pcm_name);
+

Should have been a separate commit.

Quoted 16 lines

lib/igt_alsa.c +3 -0

 				snprintf(name, sizeof(name), "hw:%d,%d", card,
 					 dev);
 

@@ -329,6 +331,9 @@ static bool alsa_test_configuration(snd_pcm_t *handle, int channels,
 {
 	snd_pcm_hw_params_t *params;
 	int ret;
+	unsigned int min_channels, max_channels;
+	unsigned int min_rate, max_rate;
+	int min_rate_dir, max_rate_dir;
 
 	snd_pcm_hw_params_alloca(&params);
 

@@ -337,12 +342,24 @@ static bool alsa_test_configuration(snd_pcm_t *handle, int channels,
 		return false;
 

lib/igt_alsa.c static bool alsa_test_configuration(snd_pcm_t *handle, int channels, · line 352

Quoted 10 lines

lib/igt_alsa.c +7 -1

 	ret = snd_pcm_hw_params_test_rate(handle, params, sampling_rate, 0);
-	if (ret < 0)
+	if (ret < 0) {
+		snd_pcm_hw_params_get_rate_min(params, &min_rate, &min_rate_dir);
+		snd_pcm_hw_params_get_rate_max(params, &max_rate, &max_rate_dir);
+		igt_debug("Output device supports rates between %u and %u, "
+			  "requested %d\n",
+			  min_rate, max_rate, sampling_rate);
 		return false;
+	}

This likely should be in patch 5/7.

Quoted 24 lines

lib/igt_alsa.c +9 -1

 	ret = snd_pcm_hw_params_test_channels(handle, params, channels);
-	if (ret < 0)
+	if (ret < 0) {
+		snd_pcm_hw_params_get_channels_min(params, &min_channels);
+		snd_pcm_hw_params_get_channels_max(params, &max_channels);
+		igt_debug("Output device supports between %u and "
+			  "%u channels, requested %d\n",
+			  min_channels, max_channels, channels);
 		return false;
+	}
 
 	return true;
 }

@@ -409,13 +426,16 @@ void alsa_configure_output(struct alsa *alsa, int channels,
 	snd_pcm_t *handle;
 	int ret;
 	int i;
+	int soft_resample = 0; /* Don't allow ALSA to resample */
+	unsigned int latency = 0;
 
 	for (i = 0; i < alsa->output_handles_count; i++) {
 		handle = alsa->output_handles[i];
 

lib/igt_alsa.c void alsa_configure_output(struct alsa *alsa, int channels, · line 438

Quoted 5 lines

lib/igt_alsa.c +2 -1

 		ret = snd_pcm_set_params(handle, SND_PCM_FORMAT_S16_LE,
 					 SND_PCM_ACCESS_RW_INTERLEAVED,
-					 channels, sampling_rate, 0, 0);
+					 channels, sampling_rate,
+					 soft_resample, latency);

Probably should have been a separate commit again ;)

Quoted 46 lines

lib/igt_alsa.c +0 -0

 		igt_assert(ret >= 0);
 	}
 

lib/igt_audio.c +7 -4

index 7624f565..a2a5c594 100644
@@ -35,7 +35,7 @@
 #include "igt_audio.h"
 #include "igt_core.h"
 
-#define FREQS_MAX	8
+#define FREQS_MAX 64
 
 /**
  * SECTION:igt_audio

@@ -49,9 +49,10 @@
 
 struct audio_signal_freq {
 	int freq;
+	int channel;
 
-	short *period;
-	int frames;
+	int16_t *period;
+	size_t period_len;
 	int offset;
 };
 

@@ -60,7 +61,7 @@ struct audio_signal {
 	int sampling_rate;
 
 	struct audio_signal_freq freqs[FREQS_MAX];
-	int freqs_count;
+	size_t freqs_count;
 };
 
 /**

@@ -89,21 +90,28 @@ struct audio_signal *audio_signal_init(int channels, int sampling_rate)
  * audio_signal_add_frequency:
  * @signal: The target signal structure
  * @frequency: The frequency to add to the signal
+ * @channel: The channel to add this frequency to, or -1 to add it to all
+ * channels
  *
  * Add a frequency to the signal.

lib/igt_audio.c struct audio_signal *audio_signal_init(int channels, int sampling_rate) · line 101

Quoted 6 lines

lib/igt_audio.c +2 -1

  *
  * Returns: An integer equal to zero for success and negative for failure
  */
-int audio_signal_add_frequency(struct audio_signal *signal, int frequency)
+int audio_signal_add_frequency(struct audio_signal *signal, int frequency,
+			       int channel)

Is the alignment correct here? Doesn't look like it!

Quoted 10 lines

lib/igt_audio.c +4 -3

 {
-	int index = signal->freqs_count;
+	size_t index = signal->freqs_count;
+	struct audio_signal_freq *freq;
 
-	if (index == FREQS_MAX)
-		return -1;
+	igt_assert(index < FREQS_MAX);
+	igt_assert(channel < signal->channels);
 

lib/igt_audio.c struct audio_signal *audio_signal_init(int channels, int sampling_rate) · line 114

Quoted 7 lines

lib/igt_audio.c +4 -1

 	/* Stay within the Nyquist–Shannon sampling theorem. */
-	if (frequency > signal->sampling_rate / 2)
+	if (frequency > signal->sampling_rate / 2) {
+		igt_debug("Skipping frequency %d: too high for a %d Hz "
+			  "sampling rate\n", frequency, signal->sampling_rate);
 		return -1;
+	}

Should have been a separate commit.

Quoted 56 lines

lib/igt_audio.c +12 -12

 	/* Clip the frequency to an integer multiple of the sampling rate.
 	 * This to be able to store a full period of it and use that for

@@ -111,11 +119,14 @@ int audio_signal_add_frequency(struct audio_signal *signal, int frequency)
 	 */
 	frequency = signal->sampling_rate / (signal->sampling_rate / frequency);
 
-	igt_debug("Adding test frequency %d\n", frequency);
+	igt_debug("Adding test frequency %d to channel %d\n",
+		  frequency, channel);
+
+	freq = &signal->freqs[index];
+	memset(freq, 0, sizeof(*freq));
+	freq->freq = frequency;
+	freq->channel = channel;
 
-	signal->freqs[index].freq = frequency;
-	signal->freqs[index].frames = 0;
-	signal->freqs[index].offset = 0;
 	signal->freqs_count++;
 
 	return 0;

@@ -133,20 +144,17 @@ void audio_signal_synthesize(struct audio_signal *signal)
 {
 	int16_t *period;
 	double value;
-	int frames;
+	size_t period_len;
 	int freq;
 	int i, j;
 
-	if (signal->freqs_count == 0)
-		return;
-
 	for (i = 0; i < signal->freqs_count; i++) {
 		freq = signal->freqs[i].freq;
-		frames = signal->sampling_rate / freq;
+		period_len = signal->sampling_rate / freq;
 
-		period = calloc(1, frames * sizeof(short));
+		period = calloc(1, period_len * sizeof(int16_t));
 
-		for (j = 0; j < frames; j++) {
+		for (j = 0; j < period_len; j++) {
 			value = 2.0 * M_PI * freq / signal->sampling_rate * j;
 			value = sin(value) * INT16_MAX / signal->freqs_count;
 

@@ -154,26 +162,34 @@ void audio_signal_synthesize(struct audio_signal *signal)
 		}
 
 		signal->freqs[i].period = period;
-		signal->freqs[i].frames = frames;
+		signal->freqs[i].period_len = period_len;
 	}
 }
 

lib/igt_audio.c void audio_signal_synthesize(struct audio_signal *signal) · line 174

Quoted 7 lines

lib/igt_audio.c +5 -1

 /**
- * audio_signal_synthesize:
+ * audio_signal_deinit:
+ *
+ * Release the signal.
+ */
+void audio_signal_deinit(struct audio_signal *signal)

Is the coding style to use deinit or fini?

Quoted 262 lines

lib/igt_audio.c +38 -25

+{
+	audio_signal_reset(signal);
+	free(signal);
+}
+
+/**
+ * audio_signal_reset:
  * @signal: The target signal structure
  *
  * Free the resources allocated by audio_signal_synthesize and remove
  * the previously-added frequencies.
  */
-void audio_signal_clean(struct audio_signal *signal)
+void audio_signal_reset(struct audio_signal *signal)
 {
-	int i;
+	size_t i;
 
 	for (i = 0; i < signal->freqs_count; i++) {
-		if (signal->freqs[i].period)
-			free(signal->freqs[i].period);
-
-		memset(&signal->freqs[i], 0, sizeof(struct audio_signal_freq));
+		free(signal->freqs[i].period);
 	}
 
 	signal->freqs_count = 0;

@@ -183,44 +199,45 @@ void audio_signal_clean(struct audio_signal *signal)
  * audio_signal_fill:
  * @signal: The target signal structure
  * @buffer: The target buffer to fill
- * @frames: The number of frames to fill
+ * @samples: The number of samples to fill
  *
- * Fill the requested number of frames to the target buffer with the audio
+ * Fill the requested number of samples to the target buffer with the audio
  * signal data (in interleaved S16_LE format), at the requested sampling rate
  * and number of channels.
  */
-void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames)
+void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
+		       size_t buffer_len)
 {
 	int16_t *destination, *source;
+	struct audio_signal_freq *freq;
 	int total;
-	int freq_frames;
-	int freq_offset;
 	int count;
 	int i, j, k;
 
-	memset(buffer, 0, sizeof(int16_t) * signal->channels * frames);
+	memset(buffer, 0, sizeof(int16_t) * signal->channels * buffer_len);
 
 	for (i = 0; i < signal->freqs_count; i++) {
+		freq = &signal->freqs[i];
 		total = 0;
 
-		while (total < frames) {
-			freq_frames = signal->freqs[i].frames;
-			freq_offset = signal->freqs[i].offset;
+		igt_assert(freq->period);
 
-			source = signal->freqs[i].period + freq_offset;
+		while (total < buffer_len) {
+			source = freq->period + freq->offset;
 			destination = buffer + total * signal->channels;
 
-			count = freq_frames - freq_offset;
-			if (count > (frames - total))
-				count = frames - total;
+			count = freq->period_len - freq->offset;
+			if (count > buffer_len - total)
+				count = buffer_len - total;
 
-			freq_offset += count;
-			freq_offset %= freq_frames;
-
-			signal->freqs[i].offset = freq_offset;
+			freq->offset += count;
+			freq->offset %= freq->period_len;
 
 			for (j = 0; j < count; j++) {
 				for (k = 0; k < signal->channels; k++) {
+					if (freq->channel >= 0 &&
+					    freq->channel != k)
+						continue;
 					destination[j * signal->channels + k] += source[j];
 				}
 			}

@@ -237,11 +254,11 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames)
  * sampling_rate is given in Hz. data_len is the number of elements in data.
  */
 bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
-			 double *data, size_t data_len)
+			 int channel, double *data, size_t data_len)
 {
 	size_t bin_power_len = data_len / 2 + 1;
 	double bin_power[bin_power_len];
-	bool detected[signal->freqs_count];
+	bool detected[FREQS_MAX];
 	int ret, freq_accuracy, freq, local_max_freq;
 	double max, local_max, threshold;
 	size_t i, j;

@@ -308,6 +325,10 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
 		 * invalid. */
 		if (bin_power[i] < threshold) {
 			for (j = 0; j < signal->freqs_count; j++) {
+				if (signal->freqs[j].channel >= 0 &&
+				    signal->freqs[j].channel != channel)
+					continue;
+
 				if (signal->freqs[j].freq >
 				    local_max_freq - freq_accuracy &&
 				    signal->freqs[j].freq <

@@ -340,6 +361,10 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
 
 	/* Check that all frequencies we generated have been detected. */
 	for (i = 0; i < signal->freqs_count; i++) {
+		if (signal->freqs[i].channel >= 0 &&
+		    signal->freqs[i].channel != channel)
+			continue;
+
 		if (!detected[i]) {
 			igt_debug("Missing frequency: %d\n",
 				  signal->freqs[i].freq);

lib/igt_audio.h +7 -4

index 4aa43e69..fe26bb57 100644
@@ -35,12 +35,15 @@
 struct audio_signal;
 
 struct audio_signal *audio_signal_init(int channels, int sampling_rate);
-int audio_signal_add_frequency(struct audio_signal *signal, int frequency);
+void audio_signal_deinit(struct audio_signal *signal);
+int audio_signal_add_frequency(struct audio_signal *signal, int frequency,
+			       int channel);
 void audio_signal_synthesize(struct audio_signal *signal);
-void audio_signal_clean(struct audio_signal *signal);
-void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames);
+void audio_signal_reset(struct audio_signal *signal);
+void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
+		       size_t buffer_len);
 bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
-			 double *data, size_t data_len);
+			 int channel, double *data, size_t data_len);
 size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
 				    int32_t *src, size_t src_len,
 				    int n_channels, int channel);

tests/kms_chamelium.c +33 -18

index 014a22b3..d336612f 100644
@@ -777,16 +777,16 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		      struct alsa *alsa, int playback_channels,
 		      int playback_rate)
 {
-	int ret, capture_rate, capture_channels, msec;
+	int ret, capture_rate, capture_channels, msec, freq;
 	struct chamelium_audio_file *audio_file;
 	struct chamelium_stream *stream;
 	enum chamelium_stream_realtime_mode stream_mode;
 	struct audio_signal *signal;
 	int32_t *recv, *buf;
 	double *channel;
-	size_t i, streak, page_count;
+	size_t i, j, streak, page_count;
 	size_t recv_len, buf_len, buf_cap, buf_size, channel_len;
-	bool ok;
+	bool ok, success;
 	char dump_suffix[64];
 	char *dump_path = NULL;
 	int dump_fd = -1;

@@ -794,10 +794,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	struct audio_state state = {};
 
 	if (!alsa_test_output_configuration(alsa, playback_channels,
-					    playback_rate))
+					    playback_rate)) {
+		igt_debug("Skipping test with sample rate %d and %d channels "
+			  "because selected output devices don't support this "
+			  "configuration\n", playback_rate, playback_channels);
 		return false;
+	}
 
-	igt_debug("Testing with playback sampling rate %d\n", playback_rate);
+	igt_debug("Testing with playback sampling rate %d and %d channels\n",
+		  playback_rate, playback_channels);
 	alsa_configure_output(alsa, playback_channels, playback_rate);
 
 	chamelium_start_capturing_audio(data->chamelium, port, false);

@@ -825,8 +830,12 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	signal = audio_signal_init(playback_channels, playback_rate);
 	igt_assert(signal);
 
-	for (i = 0; i < test_frequencies_count; i++)
-		audio_signal_add_frequency(signal, test_frequencies[i]);
+	for (i = 0; i < test_frequencies_count; i++) {
+		for (j = 0; j < playback_channels; j++) {
+			freq = test_frequencies[i];
+			audio_signal_add_frequency(signal, freq, j);
+		}
+	}
 	audio_signal_synthesize(signal);
 
 	state.signal = signal;

@@ -851,10 +860,11 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	recv = NULL;
 	recv_len = 0;
 
+	success = false;
 	streak = 0;
 	msec = 0;
 	i = 0;
-	while (streak < MIN_STREAK && msec < AUDIO_TIMEOUT) {
+	while (!success && msec < AUDIO_TIMEOUT) {
 		ok = chamelium_stream_receive_realtime_audio(stream,
 							     &page_count,
 							     &recv, &recv_len);

@@ -872,21 +882,27 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 			igt_assert(write(dump_fd, buf, buf_size) == buf_size);
 		}
 
-		/* TODO: check other channels too, not just the first one */
-		audio_extract_channel_s32_le(channel, channel_len, buf, buf_len,
-					     capture_channels, 0);
-
 		msec = i * channel_len / (double) capture_rate * 1000;
 		igt_debug("Detecting audio signal, t=%d msec\n", msec);
 
-		if (audio_signal_detect(signal, capture_rate, channel,
-					channel_len))
-			streak++;
-		else
-			streak = 0;
+		for (j = 0; j < playback_channels; j++) {
+			igt_debug("Processing channel %zu\n", j);
+
+			audio_extract_channel_s32_le(channel, channel_len,
+						     buf, buf_len,
+						     capture_channels, j);
+
+			if (audio_signal_detect(signal, capture_rate, j,
+						channel, channel_len))
+				streak++;
+			else
+				streak = 0;
+		}
 
 		buf_len = 0;
 		i++;
+
+		success = streak == MIN_STREAK * playback_channels;
 	}
 
 	igt_debug("Stopping audio playback\n");

@@ -921,12 +937,10 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		chamelium_destroy_audio_file(audio_file);
 	}
 
-	audio_signal_clean(signal);

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 946

Quoted 10 lines

tests/kms_chamelium.c +2 -3

-	free(signal);
-
+	audio_signal_deinit(signal);
 	chamelium_stream_deinit(stream);
 
-	igt_assert(streak == MIN_STREAK);
+	igt_assert(success);
 	return true;
 }
 

With the coding style issues fixed:

Reviewed-by Martin Peres <[email protected]>

Send a different signal to each channel and make sure captured audio channels are not swapped or mixed up.

The Chamelium device has a bug and already swaps the left and right channels. For this reason, clients need to retrieve the Chamelium channel mapping and accomodate for this. See https://crbug.com/950922 for a discussion about this.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c   | 34 ++++++++++++++++++++++++++++++++++
 lib/igt_chamelium.h   |  3 +++
 tests/kms_chamelium.c | 37 +++++++++++++++++++++++++++++++------
 3 files changed, 68 insertions(+), 6 deletions(-)

lib/igt_chamelium.c +34 -0

index 7c9030d1..6ac7b722 100644
@@ -930,6 +930,40 @@ int chamelium_get_captured_frame_count(struct chamelium *chamelium)
 	return ret;
 }
 
+/**
+ * chamelium_get_audio_channel_mapping:
+ * @chamelium: the Chamelium instance
+ * @port: the audio port
+ * @mapping: will be filled with the channel mapping
+ *
+ * Obtains the channel mapping for an audio port.
+ *
+ * Audio channels are not guaranteed not to be swapped. Users can use the
+ * channel mapping to match an input channel to a capture channel.
+ *
+ * The mapping contains one element per capture channel. Each element indicates
+ * which input channel the capture channel is mapped to. As a special case, -1
+ * means that the channel isn't mapped.
+ */
+void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
+					 struct chamelium_port *port,
+					 int mapping[static 8])
+{
+	xmlrpc_value *res, *res_channel;
+	int res_len, i;
+
+	res = chamelium_rpc(chamelium, port, "GetAudioChannelMapping", "(i)",
+			    port->id);
+	res_len = xmlrpc_array_size(&chamelium->env, res);
+	igt_assert(res_len == 8);
+	for (i = 0; i < res_len; i++) {
+		xmlrpc_array_read_item(&chamelium->env, res, i, &res_channel);
+		xmlrpc_read_int(&chamelium->env, res_channel, &mapping[i]);
+		xmlrpc_DECREF(res_channel);
+	}
+	xmlrpc_DECREF(res);
+}
+
 /**
  * chamelium_start_capturing_audio:
  * @chamelium: the Chamelium instance

lib/igt_chamelium.h +3 -0

index 047f8c5d..728d16ea 100644
@@ -106,6 +106,9 @@ void chamelium_start_capture(struct chamelium *chamelium,
 void chamelium_stop_capture(struct chamelium *chamelium, int frame_count);
 void chamelium_capture(struct chamelium *chamelium, struct chamelium_port *port,
 		       int x, int y, int w, int h, int frame_count);
+void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
+					 struct chamelium_port *port,
+					 int mapping[static 8]);
 void chamelium_start_capturing_audio(struct chamelium *chamelium,
 				    struct chamelium_port *port, bool save_to_file);
 struct chamelium_audio_file *chamelium_stop_capturing_audio(struct chamelium *chamelium,

tests/kms_chamelium.c +1 -1

index d336612f..8d6cc092 100644
@@ -777,7 +777,7 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		      struct alsa *alsa, int playback_channels,
 		      int playback_rate)
 {
-	int ret, capture_rate, capture_channels, msec, freq;
+	int ret, capture_rate, capture_channels, msec, freq, step;
 	struct chamelium_audio_file *audio_file;
 	struct chamelium_stream *stream;
 	enum chamelium_stream_realtime_mode stream_mode;

tests/kms_chamelium.c +1 -1

 			  "configuration\n", playback_rate, playback_channels);
 		return false;
 	}
 
-	igt_debug("Testing with playback sampling rate %d and %d channels\n",
+	igt_debug("Testing with playback sampling rate %d Hz and %d channels\n",
 		  playback_rate, playback_channels);
 	alsa_configure_output(alsa, playback_channels, playback_rate);
 

tests/kms_chamelium.c +18 -2

 	for (i = 0; i < test_frequencies_count; i++) {
 		for (j = 0; j < playback_channels; j++) {
-			freq = test_frequencies[i];
+			freq = test_frequencies[i] + j * step;
 			audio_signal_add_frequency(signal, freq, j);
 		}
 	}

@@ -847,6 +854,20 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
 	igt_assert(ret == 0);
 
+	chamelium_get_audio_channel_mapping(data->chamelium, port,
+					    channel_mapping);
+	/* Make sure we can capture all channels we send. */
+	for (i = 0; i < playback_channels; i++) {
+		ok = false;
+		for (j = 0; j < capture_channels; j++) {
+			if (channel_mapping[j] == i) {
+				ok = true;
+				break;
+			}
+		}
+		igt_assert(ok);
+	}
+
 	/* Needs to be a multiple of 128, because that's the number of samples
 	 * we get per channel each time we receive an audio page from the
 	 * Chamelium device. */

@@ -886,11 +907,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		igt_debug("Detecting audio signal, t=%d msec\n", msec);
 
 		for (j = 0; j < playback_channels; j++) {
-			igt_debug("Processing channel %zu\n", j);
+			capture_chan = channel_mapping[j];
+			igt_assert(capture_chan >= 0);
+			igt_debug("Processing channel %zu (captured as "

Quoted 87 lines · Simon Ser

Send a different signal to each channel and make sure captured audio channels are not swapped or mixed up.

The Chamelium device has a bug and already swaps the left and right channels. For this reason, clients need to retrieve the Chamelium channel mapping and accomodate for this. See https://crbug.com/950922 for a discussion about this.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c   | 34 ++++++++++++++++++++++++++++++++++
 lib/igt_chamelium.h   |  3 +++
 tests/kms_chamelium.c | 37 +++++++++++++++++++++++++++++++------
 3 files changed, 68 insertions(+), 6 deletions(-)

lib/igt_chamelium.c +34 -0

index 7c9030d1..6ac7b722 100644
@@ -930,6 +930,40 @@ int chamelium_get_captured_frame_count(struct chamelium *chamelium)
 	return ret;
 }
 
+/**
+ * chamelium_get_audio_channel_mapping:
+ * @chamelium: the Chamelium instance
+ * @port: the audio port
+ * @mapping: will be filled with the channel mapping
+ *
+ * Obtains the channel mapping for an audio port.
+ *
+ * Audio channels are not guaranteed not to be swapped. Users can use the
+ * channel mapping to match an input channel to a capture channel.
+ *
+ * The mapping contains one element per capture channel. Each element indicates
+ * which input channel the capture channel is mapped to. As a special case, -1
+ * means that the channel isn't mapped.
+ */
+void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
+					 struct chamelium_port *port,
+					 int mapping[static 8])
+{
+	xmlrpc_value *res, *res_channel;
+	int res_len, i;
+
+	res = chamelium_rpc(chamelium, port, "GetAudioChannelMapping", "(i)",
+			    port->id);
+	res_len = xmlrpc_array_size(&chamelium->env, res);
+	igt_assert(res_len == 8);
+	for (i = 0; i < res_len; i++) {
+		xmlrpc_array_read_item(&chamelium->env, res, i, &res_channel);
+		xmlrpc_read_int(&chamelium->env, res_channel, &mapping[i]);
+		xmlrpc_DECREF(res_channel);
+	}
+	xmlrpc_DECREF(res);
+}
+
 /**
  * chamelium_start_capturing_audio:
  * @chamelium: the Chamelium instance

lib/igt_chamelium.h +3 -0

index 047f8c5d..728d16ea 100644
@@ -106,6 +106,9 @@ void chamelium_start_capture(struct chamelium *chamelium,
 void chamelium_stop_capture(struct chamelium *chamelium, int frame_count);
 void chamelium_capture(struct chamelium *chamelium, struct chamelium_port *port,
 		       int x, int y, int w, int h, int frame_count);
+void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
+					 struct chamelium_port *port,
+					 int mapping[static 8]);
 void chamelium_start_capturing_audio(struct chamelium *chamelium,
 				    struct chamelium_port *port, bool save_to_file);
 struct chamelium_audio_file *chamelium_stop_capturing_audio(struct chamelium *chamelium,

tests/kms_chamelium.c +1 -1

index d336612f..8d6cc092 100644
@@ -777,7 +777,7 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		      struct alsa *alsa, int playback_channels,
 		      int playback_rate)
 {
-	int ret, capture_rate, capture_channels, msec, freq;
+	int ret, capture_rate, capture_channels, msec, freq, step;
 	struct chamelium_audio_file *audio_file;
 	struct chamelium_stream *stream;
 	enum chamelium_stream_realtime_mode stream_mode;

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 800

Quoted 10 lines

tests/kms_chamelium.c +2 -1

@@ -792,16 +792,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	int dump_fd = -1;
 	pthread_t thread;
 	struct audio_state state = {};
+	int channel_mapping[8], capture_chan;
 
 	if (!alsa_test_output_configuration(alsa, playback_channels,
 					    playback_rate)) {
-		igt_debug("Skipping test with sample rate %d and %d channels "
+		igt_debug("Skipping test with sample rate %d Hz and %d channels "
 			  "because selected output devices don't support this "

How about "because at least some output devices do not support this"?

Quoted 13 lines

tests/kms_chamelium.c +1 -1

 			  "configuration\n", playback_rate, playback_channels);
 		return false;
 	}
 
-	igt_debug("Testing with playback sampling rate %d and %d channels\n",
+	igt_debug("Testing with playback sampling rate %d Hz and %d channels\n",
 		  playback_rate, playback_channels);
 	alsa_configure_output(alsa, playback_channels, playback_rate);
 

@@ -830,9 +831,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	signal = audio_signal_init(playback_channels, playback_rate);
 	igt_assert(signal);
 

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 839

Quoted 6 lines

tests/kms_chamelium.c +6 -0

+	/* We'll choose different frequencies per channel to make sure they are
+	 * independent from each other. To do so, we'll add a different offset
+	 * to the base frequencies for each channel. We need to choose a big
+	 * enough offset so that we're sure to detect mixed up channels.
+	 */
+	step = 2 * capture_rate / CAPTURE_SAMPLES;

Please say that this formula adds an offset of 2 bins in the final FFT, thus enforcing a clear difference (please make sure of that).

Quoted 44 lines

tests/kms_chamelium.c +21 -3

 	for (i = 0; i < test_frequencies_count; i++) {
 		for (j = 0; j < playback_channels; j++) {
-			freq = test_frequencies[i];
+			freq = test_frequencies[i] + j * step;
 			audio_signal_add_frequency(signal, freq, j);
 		}
 	}

@@ -847,6 +854,20 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
 	igt_assert(ret == 0);
 
+	chamelium_get_audio_channel_mapping(data->chamelium, port,
+					    channel_mapping);
+	/* Make sure we can capture all channels we send. */
+	for (i = 0; i < playback_channels; i++) {
+		ok = false;
+		for (j = 0; j < capture_channels; j++) {
+			if (channel_mapping[j] == i) {
+				ok = true;
+				break;
+			}
+		}
+		igt_assert(ok);
+	}
+
 	/* Needs to be a multiple of 128, because that's the number of samples
 	 * we get per channel each time we receive an audio page from the
 	 * Chamelium device. */

@@ -886,11 +907,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		igt_debug("Detecting audio signal, t=%d msec\n", msec);
 
 		for (j = 0; j < playback_channels; j++) {
-			igt_debug("Processing channel %zu\n", j);
+			capture_chan = channel_mapping[j];
+			igt_assert(capture_chan >= 0);
+			igt_debug("Processing channel %zu (captured as "
+				  "channel %d)\n", j, capture_chan);
 
 			audio_extract_channel_s32_le(channel, channel_len,
 						     buf, buf_len,
-						     capture_channels, j);
+						     capture_channels,
+						     capture_chan);
 

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 921

Quoted 3 lines

tests/kms_chamelium.c +0 -0

 			if (audio_signal_detect(signal, capture_rate, j,
 						channel, channel_len))

Looks great!

Reviewed-by Martin Peres <[email protected]>

The audio test is now run multiple times with a variety of playback sampling rates.

We now query the capture audio format from the Chamelium XML-RPC API instead of hardcoding it.

One limitation is that we need to start sendting an audio signal before being

able to query the capture audio format. However we need the capture sample rate to decide which frequencies we generate. For now we use the playback rate and check that it's the same as the capture rate.

Another limitation is that the DP receiver reports an unknown sampling rate during the 41.1KHz test. In this case we assume the capture rate is the same as the playback rate. We'll fail later anyway if this assumption is incorrect since we check the signal we receive.

Chameleon bug: https://crbug.com/950913

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c        | 80 +++++++++++++++++++++++++-------------
 lib/igt_chamelium.h        |  3 ++
 lib/igt_chamelium_stream.c | 17 --------
 lib/igt_chamelium_stream.h |  2 -
 tests/kms_chamelium.c      | 56 +++++++++++++++-----------
 5 files changed, 90 insertions(+), 68 deletions(-)

lib/igt_chamelium.c +53 -27

index 6ac7b722..ffc68f35 100644
@@ -964,31 +964,6 @@ void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
 	xmlrpc_DECREF(res);
 }
 
-/**
- * chamelium_start_capturing_audio:
- * @chamelium: the Chamelium instance
- * @port: the port to capture audio from (it must support audio)
- * @save_to_file: whether the captured audio data should be saved to a file on
- * the Chamelium device
- *
- * Starts capturing audio from a Chamelium port. To stop the capture, use
- * #chamelium_stop_capturing_audio. To retrieve the audio data, either use the
- * stream server or enable @save_to_file (the latter is mainly useful for
- * debugging purposes).
- *
- * It isn't possible to capture audio from multiple ports at the same time.
- */
-void chamelium_start_capturing_audio(struct chamelium *chamelium,
-				    struct chamelium_port *port,
-				    bool save_to_file)
-{
-	xmlrpc_value *res;
-
-	res = chamelium_rpc(chamelium, port, "StartCapturingAudio", "(ib)",
-			    port->id, save_to_file);
-	xmlrpc_DECREF(res);
-}
-
 static void audio_format_from_xml(struct chamelium *chamelium,
 				  xmlrpc_value *res, int *rate, int *channels)
 {

@@ -1008,8 +983,10 @@ static void audio_format_from_xml(struct chamelium *chamelium,
 	igt_assert(strcmp(sample_format, "S32_LE") == 0);
 	free(sample_format);
 
-	xmlrpc_read_int(&chamelium->env, res_rate, rate);
-	xmlrpc_read_int(&chamelium->env, res_channel, channels);
+	if (rate)
+		xmlrpc_read_int(&chamelium->env, res_rate, rate);
+	if (channels)
+		xmlrpc_read_int(&chamelium->env, res_channel, channels);
 
 	xmlrpc_DECREF(res_channel);
 	xmlrpc_DECREF(res_sample_format);

@@ -1017,6 +994,55 @@ static void audio_format_from_xml(struct chamelium *chamelium,
 	xmlrpc_DECREF(res_type);
 }
 
+/**
+ * chamelium_get_audio_format:
+ * @chamelium: the Chamelium instance
+ * @port: the audio port
+ * @rate: if non-NULL, will be set to the sample rate in Hz
+ * @channels: if non-NULL, will be set to the number of channels
+ *
+ * Obtains the audio format of the captured data. Users should start sending an
+ * audio signal to the Chamelium device prior to calling this function.
+ *
+ * The captured data is guaranteed to be in the S32_LE format.
+ */
+void chamelium_get_audio_format(struct chamelium *chamelium,
+				struct chamelium_port *port,
+				int *rate, int *channels)
+{
+	xmlrpc_value *res;
+
+	res = chamelium_rpc(chamelium, port, "GetAudioFormat", "(i)",
+			    port->id);
+	audio_format_from_xml(chamelium, res, rate, channels);
+	xmlrpc_DECREF(res);
+}
+
+/**
+ * chamelium_start_capturing_audio:
+ * @chamelium: the Chamelium instance
+ * @port: the port to capture audio from (it must support audio)
+ * @save_to_file: whether the captured audio data should be saved to a file on
+ * the Chamelium device
+ *
+ * Starts capturing audio from a Chamelium port. To stop the capture, use
+ * #chamelium_stop_capturing_audio. To retrieve the audio data, either use the
+ * stream server or enable @save_to_file (the latter is mainly useful for
+ * debugging purposes).
+ *
+ * It isn't possible to capture audio from multiple ports at the same time.
+ */
+void chamelium_start_capturing_audio(struct chamelium *chamelium,
+				    struct chamelium_port *port,
+				    bool save_to_file)
+{
+	xmlrpc_value *res;
+
+	res = chamelium_rpc(chamelium, port, "StartCapturingAudio", "(ib)",
+			    port->id, save_to_file);
+	xmlrpc_DECREF(res);
+}
+
 /**
  * chamelium_stop_capturing_audio:
  * @chamelium: the Chamelium instance

lib/igt_chamelium.h +3 -0

index 728d16ea..f47b84cb 100644
@@ -109,6 +109,9 @@ void chamelium_capture(struct chamelium *chamelium, struct chamelium_port *port,
 void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
 					 struct chamelium_port *port,
 					 int mapping[static 8]);
+void chamelium_get_audio_format(struct chamelium *chamelium,
+				struct chamelium_port *port,
+				int *rate, int *channels);
 void chamelium_start_capturing_audio(struct chamelium *chamelium,
 				    struct chamelium_port *port, bool save_to_file);
 struct chamelium_audio_file *chamelium_stop_capturing_audio(struct chamelium *chamelium,

lib/igt_chamelium_stream.c +0 -17

index 68ddb217..a8cd19e5 100644
@@ -537,23 +537,6 @@ bool chamelium_stream_stop_realtime_audio(struct chamelium_stream *client)
 	return true;
 }
 
-/**
- * chamelium_stream_audio_format:
- *
- * Gets the format used for audio pages.
- *
- * Data will always be captured in raw pages of S32_LE elements. This function
- * exposes the sampling rate and the number of channels.
- */
-void chamelium_stream_audio_format(struct chamelium_stream *stream,
-				   int *rate, int *channels)
-{
-	/* TODO: the Chamelium streaming server doesn't expose those yet.
-	 * Just hardcode the values for now. */
-	*rate = 48000;
-	*channels = 8;
-}
-
 /**
  * chamelium_stream_init:
  *

lib/igt_chamelium_stream.h +0 -2

index de4e9931..3e1c5d14 100644
@@ -42,8 +42,6 @@ struct chamelium_stream *chamelium_stream_init(void);
 void chamelium_stream_deinit(struct chamelium_stream *client);
 bool chamelium_stream_dump_realtime_audio(struct chamelium_stream *client,
 					  enum chamelium_stream_realtime_mode mode);
-void chamelium_stream_audio_format(struct chamelium_stream *stream,
-				   int *rate, int *channels);
 bool chamelium_stream_receive_realtime_audio(struct chamelium_stream *client,
 					     size_t *page_count,
 					     int32_t **buf, size_t *buf_len);

tests/kms_chamelium.c +23 -21

index 8d6cc092..f535ca52 100644
@@ -725,15 +725,14 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
 /* A streak of 3 gives confidence that the signal is good. */
 #define MIN_STREAK 3
 
-/* TODO: Chamelium only supports 48KHz for now */
 static int sampling_rates[] = {
-/*	32000, */
-/*	44100, */
+	32000,
+	44100,
 	48000,
-/*	88200, */
-/*	96000, */
-/*	176400, */
-/*	192000, */
+	88200,
+	96000,
+	176400,
+	192000,
 };
 
 static int sampling_rates_count = sizeof(sampling_rates) / sizeof(int);

@@ -815,19 +814,6 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ok = chamelium_stream_dump_realtime_audio(stream, stream_mode);
 	igt_assert(ok);
 
-	chamelium_stream_audio_format(stream, &capture_rate, &capture_channels);
-
-	if (igt_frame_dump_is_enabled()) {
-		snprintf(dump_suffix, sizeof(dump_suffix), "capture-%dch-%d",
-			 playback_channels, playback_rate);
-
-		dump_fd = audio_create_wav_file_s32_le(dump_suffix,
-						       capture_rate,
-						       capture_channels,
-						       &dump_path);
-		igt_assert(dump_fd >= 0);
-	}
-
 	signal = audio_signal_init(playback_channels, playback_rate);
 	igt_assert(signal);
 

@@ -835,8 +821,12 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	 * independent from each other. To do so, we'll add a different offset
 	 * to the base frequencies for each channel. We need to choose a big
 	 * enough offset so that we're sure to detect mixed up channels.
+	 *
+	 * Note that we assume capture_rate == playback_rate. We'll assert this
+	 * later on. We cannot retrieve the capture rate before starting
+	 * playing audio, so we don't really have the choice.
 	 */
-	step = 2 * capture_rate / CAPTURE_SAMPLES;
+	step = 2 * playback_rate / CAPTURE_SAMPLES;
 	for (i = 0; i < test_frequencies_count; i++) {
 		for (j = 0; j < playback_channels; j++) {
 			freq = test_frequencies[i] + j * step;

@@ -854,6 +844,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
 	igt_assert(ret == 0);
 
+	/* Only after we've started playing audio, we can retrieve the capture
+	 * format used by the Chamelium device. */
+	chamelium_get_audio_format(data->chamelium, port,
+				   &capture_rate, &capture_channels);
+	if (capture_rate == 0) {
+		igt_debug("Audio receiver doesn't indicate the capture "
+			 "sampling rate, assuming it's %d Hz\n", playback_rate);
+		capture_rate = playback_rate;
+	} else
+		igt_assert(capture_rate == playback_rate);
+
 	chamelium_get_audio_channel_mapping(data->chamelium, port,
 					    channel_mapping);
 	/* Make sure we can capture all channels we send. */

@@ -868,6 +869,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		igt_assert(ok);
 	}
 
+	if (igt_frame_dump_is_enabled()) {

Quoted 7 lines · Simon Ser

The audio test is now run multiple times with a variety of playback sampling rates.

We now query the capture audio format from the Chamelium XML-RPC API instead of hardcoding it.

One limitation is that we need to start sendting an audio signal before being

Typo on sending.

Quoted 262 lines

able to query the capture audio format. However we need the capture sample rate to decide which frequencies we generate. For now we use the playback rate and check that it's the same as the capture rate.

Another limitation is that the DP receiver reports an unknown sampling rate during the 41.1KHz test. In this case we assume the capture rate is the same as the playback rate. We'll fail later anyway if this assumption is incorrect since we check the signal we receive.

Chameleon bug: https://crbug.com/950913

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c        | 80 +++++++++++++++++++++++++-------------
 lib/igt_chamelium.h        |  3 ++
 lib/igt_chamelium_stream.c | 17 --------
 lib/igt_chamelium_stream.h |  2 -
 tests/kms_chamelium.c      | 56 +++++++++++++++-----------
 5 files changed, 90 insertions(+), 68 deletions(-)

lib/igt_chamelium.c +53 -27

index 6ac7b722..ffc68f35 100644
@@ -964,31 +964,6 @@ void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
 	xmlrpc_DECREF(res);
 }
 
-/**
- * chamelium_start_capturing_audio:
- * @chamelium: the Chamelium instance
- * @port: the port to capture audio from (it must support audio)
- * @save_to_file: whether the captured audio data should be saved to a file on
- * the Chamelium device
- *
- * Starts capturing audio from a Chamelium port. To stop the capture, use
- * #chamelium_stop_capturing_audio. To retrieve the audio data, either use the
- * stream server or enable @save_to_file (the latter is mainly useful for
- * debugging purposes).
- *
- * It isn't possible to capture audio from multiple ports at the same time.
- */
-void chamelium_start_capturing_audio(struct chamelium *chamelium,
-				    struct chamelium_port *port,
-				    bool save_to_file)
-{
-	xmlrpc_value *res;
-
-	res = chamelium_rpc(chamelium, port, "StartCapturingAudio", "(ib)",
-			    port->id, save_to_file);
-	xmlrpc_DECREF(res);
-}
-
 static void audio_format_from_xml(struct chamelium *chamelium,
 				  xmlrpc_value *res, int *rate, int *channels)
 {

@@ -1008,8 +983,10 @@ static void audio_format_from_xml(struct chamelium *chamelium,
 	igt_assert(strcmp(sample_format, "S32_LE") == 0);
 	free(sample_format);
 
-	xmlrpc_read_int(&chamelium->env, res_rate, rate);
-	xmlrpc_read_int(&chamelium->env, res_channel, channels);
+	if (rate)
+		xmlrpc_read_int(&chamelium->env, res_rate, rate);
+	if (channels)
+		xmlrpc_read_int(&chamelium->env, res_channel, channels);
 
 	xmlrpc_DECREF(res_channel);
 	xmlrpc_DECREF(res_sample_format);

@@ -1017,6 +994,55 @@ static void audio_format_from_xml(struct chamelium *chamelium,
 	xmlrpc_DECREF(res_type);
 }
 
+/**
+ * chamelium_get_audio_format:
+ * @chamelium: the Chamelium instance
+ * @port: the audio port
+ * @rate: if non-NULL, will be set to the sample rate in Hz
+ * @channels: if non-NULL, will be set to the number of channels
+ *
+ * Obtains the audio format of the captured data. Users should start sending an
+ * audio signal to the Chamelium device prior to calling this function.
+ *
+ * The captured data is guaranteed to be in the S32_LE format.
+ */
+void chamelium_get_audio_format(struct chamelium *chamelium,
+				struct chamelium_port *port,
+				int *rate, int *channels)
+{
+	xmlrpc_value *res;
+
+	res = chamelium_rpc(chamelium, port, "GetAudioFormat", "(i)",
+			    port->id);
+	audio_format_from_xml(chamelium, res, rate, channels);
+	xmlrpc_DECREF(res);
+}
+
+/**
+ * chamelium_start_capturing_audio:
+ * @chamelium: the Chamelium instance
+ * @port: the port to capture audio from (it must support audio)
+ * @save_to_file: whether the captured audio data should be saved to a file on
+ * the Chamelium device
+ *
+ * Starts capturing audio from a Chamelium port. To stop the capture, use
+ * #chamelium_stop_capturing_audio. To retrieve the audio data, either use the
+ * stream server or enable @save_to_file (the latter is mainly useful for
+ * debugging purposes).
+ *
+ * It isn't possible to capture audio from multiple ports at the same time.
+ */
+void chamelium_start_capturing_audio(struct chamelium *chamelium,
+				    struct chamelium_port *port,
+				    bool save_to_file)
+{
+	xmlrpc_value *res;
+
+	res = chamelium_rpc(chamelium, port, "StartCapturingAudio", "(ib)",
+			    port->id, save_to_file);
+	xmlrpc_DECREF(res);
+}
+
 /**
  * chamelium_stop_capturing_audio:
  * @chamelium: the Chamelium instance

lib/igt_chamelium.h +3 -0

index 728d16ea..f47b84cb 100644
@@ -109,6 +109,9 @@ void chamelium_capture(struct chamelium *chamelium, struct chamelium_port *port,
 void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
 					 struct chamelium_port *port,
 					 int mapping[static 8]);
+void chamelium_get_audio_format(struct chamelium *chamelium,
+				struct chamelium_port *port,
+				int *rate, int *channels);
 void chamelium_start_capturing_audio(struct chamelium *chamelium,
 				    struct chamelium_port *port, bool save_to_file);
 struct chamelium_audio_file *chamelium_stop_capturing_audio(struct chamelium *chamelium,

lib/igt_chamelium_stream.c +0 -17

index 68ddb217..a8cd19e5 100644
@@ -537,23 +537,6 @@ bool chamelium_stream_stop_realtime_audio(struct chamelium_stream *client)
 	return true;
 }
 
-/**
- * chamelium_stream_audio_format:
- *
- * Gets the format used for audio pages.
- *
- * Data will always be captured in raw pages of S32_LE elements. This function
- * exposes the sampling rate and the number of channels.
- */
-void chamelium_stream_audio_format(struct chamelium_stream *stream,
-				   int *rate, int *channels)
-{
-	/* TODO: the Chamelium streaming server doesn't expose those yet.
-	 * Just hardcode the values for now. */
-	*rate = 48000;
-	*channels = 8;
-}
-
 /**
  * chamelium_stream_init:
  *

lib/igt_chamelium_stream.h +0 -2

index de4e9931..3e1c5d14 100644
@@ -42,8 +42,6 @@ struct chamelium_stream *chamelium_stream_init(void);
 void chamelium_stream_deinit(struct chamelium_stream *client);
 bool chamelium_stream_dump_realtime_audio(struct chamelium_stream *client,
 					  enum chamelium_stream_realtime_mode mode);
-void chamelium_stream_audio_format(struct chamelium_stream *stream,
-				   int *rate, int *channels);
 bool chamelium_stream_receive_realtime_audio(struct chamelium_stream *client,
 					     size_t *page_count,
 					     int32_t **buf, size_t *buf_len);

tests/kms_chamelium.c +22 -21

index 8d6cc092..f535ca52 100644
@@ -725,15 +725,14 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
 /* A streak of 3 gives confidence that the signal is good. */
 #define MIN_STREAK 3
 
-/* TODO: Chamelium only supports 48KHz for now */
 static int sampling_rates[] = {
-/*	32000, */
-/*	44100, */
+	32000,
+	44100,
 	48000,
-/*	88200, */
-/*	96000, */
-/*	176400, */
-/*	192000, */
+	88200,
+	96000,
+	176400,
+	192000,
 };
 
 static int sampling_rates_count = sizeof(sampling_rates) / sizeof(int);

@@ -815,19 +814,6 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ok = chamelium_stream_dump_realtime_audio(stream, stream_mode);
 	igt_assert(ok);
 
-	chamelium_stream_audio_format(stream, &capture_rate, &capture_channels);
-
-	if (igt_frame_dump_is_enabled()) {
-		snprintf(dump_suffix, sizeof(dump_suffix), "capture-%dch-%d",
-			 playback_channels, playback_rate);
-
-		dump_fd = audio_create_wav_file_s32_le(dump_suffix,
-						       capture_rate,
-						       capture_channels,
-						       &dump_path);
-		igt_assert(dump_fd >= 0);
-	}
-
 	signal = audio_signal_init(playback_channels, playback_rate);
 	igt_assert(signal);
 

@@ -835,8 +821,12 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	 * independent from each other. To do so, we'll add a different offset
 	 * to the base frequencies for each channel. We need to choose a big
 	 * enough offset so that we're sure to detect mixed up channels.
+	 *
+	 * Note that we assume capture_rate == playback_rate. We'll assert this
+	 * later on. We cannot retrieve the capture rate before starting
+	 * playing audio, so we don't really have the choice.
 	 */
-	step = 2 * capture_rate / CAPTURE_SAMPLES;
+	step = 2 * playback_rate / CAPTURE_SAMPLES;
 	for (i = 0; i < test_frequencies_count; i++) {
 		for (j = 0; j < playback_channels; j++) {
 			freq = test_frequencies[i] + j * step;

@@ -854,6 +844,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
 	igt_assert(ret == 0);
 
+	/* Only after we've started playing audio, we can retrieve the capture
+	 * format used by the Chamelium device. */
+	chamelium_get_audio_format(data->chamelium, port,
+				   &capture_rate, &capture_channels);
+	if (capture_rate == 0) {
+		igt_debug("Audio receiver doesn't indicate the capture "
+			 "sampling rate, assuming it's %d Hz\n", playback_rate);
+		capture_rate = playback_rate;
+	} else
+		igt_assert(capture_rate == playback_rate);
+
 	chamelium_get_audio_channel_mapping(data->chamelium, port,
 					    channel_mapping);
 	/* Make sure we can capture all channels we send. */

@@ -868,6 +869,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		igt_assert(ok);
 	}
 

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 882

Quoted 11 lines

tests/kms_chamelium.c +11 -0

+	if (igt_frame_dump_is_enabled()) {
+		snprintf(dump_suffix, sizeof(dump_suffix), "capture-%dch-%d",
+			 playback_channels, playback_rate);
+
+		dump_fd = audio_create_wav_file_s32_le(dump_suffix,
+						       capture_rate,
+						       capture_channels,
+						       &dump_path);
+		igt_assert(dump_fd >= 0);
+	}
+

This belongs to another commit.

Quoted 5 lines

tests/kms_chamelium.c +0 -0

 	/* Needs to be a multiple of 128, because that's the number of samples
 	 * we get per channel each time we receive an audio page from the
 	 * Chamelium device. */

@@ -939,7 +951,7 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 957

Quoted 8 lines

tests/kms_chamelium.c +1 -1

 	if (dump_fd >= 0) {
 		close(dump_fd);
-		if (streak == MIN_STREAK) {
+		if (success) {
 			/* Test succeeded, no need to keep the captured data */
 			unlink(dump_path);
 		} else

Dito.

Otherwise, the code looks OK:

Reviewed-by Martin Peres <[email protected]>

Quoted 273 lines · Martin Peres

Quoted 7 lines · Simon Ser

The audio test is now run multiple times with a variety of playback sampling rates.

We now query the capture audio format from the Chamelium XML-RPC API instead of hardcoding it.

One limitation is that we need to start sendting an audio signal before being

Typo on sending.

Quoted 262 lines

able to query the capture audio format. However we need the capture sample rate to decide which frequencies we generate. For now we use the playback rate and check that it's the same as the capture rate.

Another limitation is that the DP receiver reports an unknown sampling rate during the 41.1KHz test. In this case we assume the capture rate is the same as the playback rate. We'll fail later anyway if this assumption is incorrect since we check the signal we receive.

Chameleon bug: https://crbug.com/950913

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c        | 80 +++++++++++++++++++++++++-------------
 lib/igt_chamelium.h        |  3 ++
 lib/igt_chamelium_stream.c | 17 --------
 lib/igt_chamelium_stream.h |  2 -
 tests/kms_chamelium.c      | 56 +++++++++++++++-----------
 5 files changed, 90 insertions(+), 68 deletions(-)

lib/igt_chamelium.c +53 -27

index 6ac7b722..ffc68f35 100644
@@ -964,31 +964,6 @@ void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
 	xmlrpc_DECREF(res);
 }
 
-/**
- * chamelium_start_capturing_audio:
- * @chamelium: the Chamelium instance
- * @port: the port to capture audio from (it must support audio)
- * @save_to_file: whether the captured audio data should be saved to a file on
- * the Chamelium device
- *
- * Starts capturing audio from a Chamelium port. To stop the capture, use
- * #chamelium_stop_capturing_audio. To retrieve the audio data, either use the
- * stream server or enable @save_to_file (the latter is mainly useful for
- * debugging purposes).
- *
- * It isn't possible to capture audio from multiple ports at the same time.
- */
-void chamelium_start_capturing_audio(struct chamelium *chamelium,
-				    struct chamelium_port *port,
-				    bool save_to_file)
-{
-	xmlrpc_value *res;
-
-	res = chamelium_rpc(chamelium, port, "StartCapturingAudio", "(ib)",
-			    port->id, save_to_file);
-	xmlrpc_DECREF(res);
-}
-
 static void audio_format_from_xml(struct chamelium *chamelium,
 				  xmlrpc_value *res, int *rate, int *channels)
 {

@@ -1008,8 +983,10 @@ static void audio_format_from_xml(struct chamelium *chamelium,
 	igt_assert(strcmp(sample_format, "S32_LE") == 0);
 	free(sample_format);
 
-	xmlrpc_read_int(&chamelium->env, res_rate, rate);
-	xmlrpc_read_int(&chamelium->env, res_channel, channels);
+	if (rate)
+		xmlrpc_read_int(&chamelium->env, res_rate, rate);
+	if (channels)
+		xmlrpc_read_int(&chamelium->env, res_channel, channels);
 
 	xmlrpc_DECREF(res_channel);
 	xmlrpc_DECREF(res_sample_format);

@@ -1017,6 +994,55 @@ static void audio_format_from_xml(struct chamelium *chamelium,
 	xmlrpc_DECREF(res_type);
 }
 
+/**
+ * chamelium_get_audio_format:
+ * @chamelium: the Chamelium instance
+ * @port: the audio port
+ * @rate: if non-NULL, will be set to the sample rate in Hz
+ * @channels: if non-NULL, will be set to the number of channels
+ *
+ * Obtains the audio format of the captured data. Users should start sending an
+ * audio signal to the Chamelium device prior to calling this function.
+ *
+ * The captured data is guaranteed to be in the S32_LE format.
+ */
+void chamelium_get_audio_format(struct chamelium *chamelium,
+				struct chamelium_port *port,
+				int *rate, int *channels)
+{
+	xmlrpc_value *res;
+
+	res = chamelium_rpc(chamelium, port, "GetAudioFormat", "(i)",
+			    port->id);
+	audio_format_from_xml(chamelium, res, rate, channels);
+	xmlrpc_DECREF(res);
+}
+
+/**
+ * chamelium_start_capturing_audio:
+ * @chamelium: the Chamelium instance
+ * @port: the port to capture audio from (it must support audio)
+ * @save_to_file: whether the captured audio data should be saved to a file on
+ * the Chamelium device
+ *
+ * Starts capturing audio from a Chamelium port. To stop the capture, use
+ * #chamelium_stop_capturing_audio. To retrieve the audio data, either use the
+ * stream server or enable @save_to_file (the latter is mainly useful for
+ * debugging purposes).
+ *
+ * It isn't possible to capture audio from multiple ports at the same time.
+ */
+void chamelium_start_capturing_audio(struct chamelium *chamelium,
+				    struct chamelium_port *port,
+				    bool save_to_file)
+{
+	xmlrpc_value *res;
+
+	res = chamelium_rpc(chamelium, port, "StartCapturingAudio", "(ib)",
+			    port->id, save_to_file);
+	xmlrpc_DECREF(res);
+}
+
 /**
  * chamelium_stop_capturing_audio:
  * @chamelium: the Chamelium instance

lib/igt_chamelium.h +3 -0

index 728d16ea..f47b84cb 100644
@@ -109,6 +109,9 @@ void chamelium_capture(struct chamelium *chamelium, struct chamelium_port *port,
 void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
 					 struct chamelium_port *port,
 					 int mapping[static 8]);
+void chamelium_get_audio_format(struct chamelium *chamelium,
+				struct chamelium_port *port,
+				int *rate, int *channels);
 void chamelium_start_capturing_audio(struct chamelium *chamelium,
 				    struct chamelium_port *port, bool save_to_file);
 struct chamelium_audio_file *chamelium_stop_capturing_audio(struct chamelium *chamelium,

lib/igt_chamelium_stream.c +0 -17

index 68ddb217..a8cd19e5 100644
@@ -537,23 +537,6 @@ bool chamelium_stream_stop_realtime_audio(struct chamelium_stream *client)
 	return true;
 }
 
-/**
- * chamelium_stream_audio_format:
- *
- * Gets the format used for audio pages.
- *
- * Data will always be captured in raw pages of S32_LE elements. This function
- * exposes the sampling rate and the number of channels.
- */
-void chamelium_stream_audio_format(struct chamelium_stream *stream,
-				   int *rate, int *channels)
-{
-	/* TODO: the Chamelium streaming server doesn't expose those yet.
-	 * Just hardcode the values for now. */
-	*rate = 48000;
-	*channels = 8;
-}
-
 /**
  * chamelium_stream_init:
  *

lib/igt_chamelium_stream.h +0 -2

index de4e9931..3e1c5d14 100644
@@ -42,8 +42,6 @@ struct chamelium_stream *chamelium_stream_init(void);
 void chamelium_stream_deinit(struct chamelium_stream *client);
 bool chamelium_stream_dump_realtime_audio(struct chamelium_stream *client,
 					  enum chamelium_stream_realtime_mode mode);
-void chamelium_stream_audio_format(struct chamelium_stream *stream,
-				   int *rate, int *channels);
 bool chamelium_stream_receive_realtime_audio(struct chamelium_stream *client,
 					     size_t *page_count,
 					     int32_t **buf, size_t *buf_len);

tests/kms_chamelium.c +22 -21

index 8d6cc092..f535ca52 100644
@@ -725,15 +725,14 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
 /* A streak of 3 gives confidence that the signal is good. */
 #define MIN_STREAK 3
 
-/* TODO: Chamelium only supports 48KHz for now */
 static int sampling_rates[] = {
-/*	32000, */
-/*	44100, */
+	32000,
+	44100,
 	48000,
-/*	88200, */
-/*	96000, */
-/*	176400, */
-/*	192000, */
+	88200,
+	96000,
+	176400,
+	192000,
 };
 
 static int sampling_rates_count = sizeof(sampling_rates) / sizeof(int);

@@ -815,19 +814,6 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ok = chamelium_stream_dump_realtime_audio(stream, stream_mode);
 	igt_assert(ok);
 
-	chamelium_stream_audio_format(stream, &capture_rate, &capture_channels);
-
-	if (igt_frame_dump_is_enabled()) {
-		snprintf(dump_suffix, sizeof(dump_suffix), "capture-%dch-%d",
-			 playback_channels, playback_rate);
-
-		dump_fd = audio_create_wav_file_s32_le(dump_suffix,
-						       capture_rate,
-						       capture_channels,
-						       &dump_path);
-		igt_assert(dump_fd >= 0);
-	}
-
 	signal = audio_signal_init(playback_channels, playback_rate);
 	igt_assert(signal);
 

@@ -835,8 +821,12 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	 * independent from each other. To do so, we'll add a different offset
 	 * to the base frequencies for each channel. We need to choose a big
 	 * enough offset so that we're sure to detect mixed up channels.
+	 *
+	 * Note that we assume capture_rate == playback_rate. We'll assert this
+	 * later on. We cannot retrieve the capture rate before starting
+	 * playing audio, so we don't really have the choice.
 	 */
-	step = 2 * capture_rate / CAPTURE_SAMPLES;
+	step = 2 * playback_rate / CAPTURE_SAMPLES;
 	for (i = 0; i < test_frequencies_count; i++) {
 		for (j = 0; j < playback_channels; j++) {
 			freq = test_frequencies[i] + j * step;

@@ -854,6 +844,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
 	igt_assert(ret == 0);
 
+	/* Only after we've started playing audio, we can retrieve the capture
+	 * format used by the Chamelium device. */
+	chamelium_get_audio_format(data->chamelium, port,
+				   &capture_rate, &capture_channels);
+	if (capture_rate == 0) {
+		igt_debug("Audio receiver doesn't indicate the capture "
+			 "sampling rate, assuming it's %d Hz\n", playback_rate);
+		capture_rate = playback_rate;
+	} else
+		igt_assert(capture_rate == playback_rate);
+
 	chamelium_get_audio_channel_mapping(data->chamelium, port,
 					    channel_mapping);
 	/* Make sure we can capture all channels we send. */

@@ -868,6 +869,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 		igt_assert(ok);
 	}
 

Quoted 13 lines

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 882

Quoted 11 lines

tests/kms_chamelium.c +11 -0

+	if (igt_frame_dump_is_enabled()) {
+		snprintf(dump_suffix, sizeof(dump_suffix), "capture-%dch-%d",
+			 playback_channels, playback_rate);
+
+		dump_fd = audio_create_wav_file_s32_le(dump_suffix,
+						       capture_rate,
+						       capture_channels,
+						       &dump_path);
+		igt_assert(dump_fd >= 0);
+	}
+

This belongs to another commit.

Nack. This has just been moved around, see line 815 above. This is necessary because of the new chamelium_get_audio_format call.

Quoted 6 lines

Quoted 6 lines

tests/kms_chamelium.c +0 -0

 	/* Needs to be a multiple of 128, because that's the number of samples
 	 * we get per channel each time we receive an audio page from the
 	 * Chamelium device. */

@@ -939,7 +951,7 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
 

Quoted 10 lines

tests/kms_chamelium.c do_test_display_audio(data_t *data, struct chamelium_port *port, · line 957

Quoted 8 lines

tests/kms_chamelium.c +1 -1

 	if (dump_fd >= 0) {
 		close(dump_fd);
-		if (streak == MIN_STREAK) {
+		if (success) {
 			/* Test succeeded, no need to keep the captured data */
 			unlink(dump_path);
 		} else

Dito.

Ack. Moved to patch 3 "tests/kms_chamelium: test we receive a signal from both audio channels" which introduced this variable.

Quoted 4 lines

Otherwise, the code looks OK:

Reviewed-by Martin Peres <[email protected]>

Waiting for the whole series to be complete to decide whether or not we want this test in fast-feedback.

Signed-off-by Simon Ser <[email protected]>

---
 tests/intel-ci/fast-feedback.testlist | 1 +
 1 file changed, 1 insertion(+)

tests/intel-ci/fast-feedback.testlist +0 -0

index 40475b1a..28f5fa55 100644