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

[Martin Peres <martin.peres@linux.intel.com>]

On 20/05/2019 16:10, Ser, Simon wrote:
> 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@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.

Fair-enough :)

> 
> 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.

Sounds great! So we'll want to either use the inject test for this, or
use a chamelium test for this... or both since chamelium is conceptually
better :)

Martin

> 
>> Regardless, this patch looks good with the above TODO removed:
>> Reviewed-by: Martin Peres <martin.peres@linux.intel.com>
>>
>>
>>>  	/* Make sure all runs were successful. */
>>>  	igt_assert(success);
>>>
>>
>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev