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

Ser, Simon simon.ser at intel.com
Mon May 20 13:10:58 UTC 2019


On Mon, 2019-05-20 at 13:20 +0100, Peres, Martin wrote:
> 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.

Derp

> > +			/* 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.

Hmm. I'd rather keep it until we have this test.

However checking whether ALSA exposes the correct formats isn't going
to work: the Chamelium device only exposes 16 and 24-bit formats but
ALSA reports a 32-bit format…

However /proc/asound parses and reports correct values. Maybe we could
have a test that uses this instead.

> 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);
> > 
> 
> 


More information about the igt-dev mailing list