[igt-dev] [PATCH i-g-t v3 09/10] tests/kms_chamelium: add a flatline audio test

[Tvrtko Ursulin]

On 05/06/2019 14:04, Ser, Simon wrote:
> On Wed, 2019-06-05 at 11:51 +0100, Tvrtko Ursulin wrote:
>> On 04/06/2019 15:06, Ser, Simon wrote:
>>> On Tue, 2019-06-04 at 13:59 +0100, Tvrtko Ursulin wrote:
>>>> On 04/06/2019 13:26, Peres, Martin wrote:
>>>>> On 04/06/2019 14:22, Ser, Simon wrote:
>>>>>> On Tue, 2019-06-04 at 09:38 +0100, Tvrtko Ursulin wrote:
>>>>>>> On 27/05/2019 15:34, Simon Ser wrote:
>>>>>>>> This commit adds a flatline test alongside the existing frequencies test.
>>>>>>>>
>>>>>>>> The test sends a constant value and checks that the amplitude is correct. A
>>>>>>>> window is used to check that each sample is within acceptable bounds. The test
>>>>>>>> is stopped as soon as 3 audio pages pass the test.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Ser <simon.ser at intel.com>
>>>>>>>> Reviewed-by: Martin Peres <martin.peres at linux.intel.com>
>>>>>>>> ---
>>>>>>>>      tests/kms_chamelium.c | 101 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 101 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
>>>>>>>> index 40ca93687c20..451a616f1a2e 100644
>>>>>>>> --- a/tests/kms_chamelium.c
>>>>>>>> +++ b/tests/kms_chamelium.c
>>>>>>>> @@ -772,6 +772,9 @@ 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
>>>>>>>>      
>>>>>>>> +#define FLATLINE_AMPLITUDE 0.9 /* normalized, ie. in [0, 1] */
>>>>>>>
>>>>>>> I assume the test is making triple sure it only ever outputs this signal
>>>>>>> to connectors connected to Chamelium, in all possible scenarios? (I am
>>>>>>> thinking it could be dangerous to some amps/speakers if by some kind of
>>>>>>> accident.)
>>>>>>
>>>>>> Not at all. The signal is sent to all HDMI/DP ports.
>>>>>>
>>>>>> I have to check whether it's easy to match ALSA outputs to monitor
>>>>>> names.
>>>>>>
>>>>>> Martin, is this a concern?
>>>>>
>>>>> This is true that a non-zero constant voltage could be damaging for
>>>>> speakers as it can make them overheat without us hearing anything
>>>>> (constant position == no sound heard, but Ohm's law still applies). It
>>>>> would take longer than 1s though... On top of this, all speakers (except
>>>>> subwoofers) have high-pass filters that should remove the DC-offset so
>>>>> all we should be left with is a nice pop which might or might not be
>>>>> loud depending on how powerful the speakers are and how loud their
>>>>> settings are. Multi-kW systems definitely don't like them, but how
>>>>> likely is it that people would run IGT on it? :D
>>>>
>>>> Why would all speakers have high-pass filters? I would be surprised if
>>>> full range ones do, and N-way definitely do not in their totality.
>>>> Considering the range of laptop speakers, monitor speakers, TVs, etc,
>>>> all of varying quality, I'd be quite conservative and cautious before
>>>> outputting even one second of +90% DC signal. I know it is just IGT and
>>>> so extremely unlikely that if anything fries it would be anyone apart
>>>> from us frying our own stuff, but, still, do we have to use DC as test
>>>> signal?
>>>
>>> DC is just so simple. It also allows us to check that all samples are
>>> in a very precise range.
>>
>> Why is this test important? Is there some path in i915 which can mess up
>> with individual sample levels? Like, it will lower 127 to 100, but won't
>> affect 126? (In signed 8-bit for simplicity.)
> 
> Quoting Martin Peres:
> 
>     Cross-component validation is every component's responsibility: each
>     component needs to check its integration with the latest released
>     version, and the last LTS of each dependency/user (at least at a
>     weekly cadence). It is also their job to report bugs (I certainly
>     do)! i915 CI already runs Piglit (GL testsuite) both in post- and
>     pre-merge, and we'll run Media and Compute tests (we need to test
>     more versions of mesa though).
> 
> Yes, we want to make sure the audio is perfect. The approach used with
> the Chamelium is black-box: I don't know how i915 works, how ALSA
> works, I don't know whether i915 touches the samples or not. I just
> want to make sure actual users get proper sound.
> 
> If the test fails because of an ALSA bug, I'll be more than happy to
> report the bug to ALSA and to make sure it gets fixed so that our users
> don't run into it.
> 
> If nobody wants to have end-to-end tests, then nothing will get
> properly tested. I can imagine someone working with ALSA saying that
> the DRM path won't be tested because it involves i915. In the end this
> means bad coverage.
> 
> What do you think?

Note that Piglit tests that we run, and media and compute tests that we 
plan to (I hope so at least for the latter), will not be written by i915 
developers but respective component owners. From this follows that it 
may very well make sense to run some ALSA test suite (or cross-cooperate 
to write or adapt one, depending on what is available), but less so to 
write our own.

When you get to the point where simplest way of figuring out whether 
sound works is not enough is where I would draw a line. And as you 
already seem to be past this point my recommendation would be to 
reconsider, especially the DC output test. Or at least reach out to ALSA 
folks to get their opinion on how safe or unsafe that might be against a 
wide palette of random consumer class junk hardware.

Regards,

Tvrtko

> 
>>> Would it be fine to use a 25% DC?
>>
>> Must be better by definition but it would be best to ask ALSA folks.
>>
>>>>> That being said, if we can associate the alsa output to a certain
>>>>> connector (the one we are reading the sound from), then it would
>>>>> actually be a good thing to test the sound on this connector only, since
>>>>> it would allow us to verify that the mapping is indeed correct!
>>>>
>>>> Yeah.. and why do we even test this in such detail? I mean outputting
>>>> different signals and stuff.
>>>
>>> Because audio could be broken in a lot of different ways.
>> I don't understand if this "broken in a lot of different ways" is
>> something i915 can affect or it more belongs in ALSA testing?
> 
> (See above)
> 
>>>> Why not just send a sine wave beep or
>>>> something and check it was captured? I did not figure out from the
>>>> commit message why DC.
>>>
>>> We already have sine wave tests, but sine waves make it complicated to
>>> check that amplitude is correct for all samples.
>>
>> Yeah, I don't get it as said above.
>>
>> Regards,
>>
>> Tvrtko