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

[Ser, Simon]

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?

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