[igt-dev] [PATCH i-g-t 5/5] tests/kms_chamelium: add an audio EDID test

[Ser, Simon]

On Fri, 2019-05-31 at 16:18 +0300, Arkadiusz Hiler wrote:
> On Fri, May 31, 2019 at 02:22:00PM +0300, Simon Ser wrote:
> > This test enables a Chamelium port with an audio-friendly EDID, and then checks
> > that the EDID-Like Data exposed by ALSA matches our expectations.
> > 
> > Remove some high sampling rates from get_hdmi_audio_edid, since the Chamelium
> > doesn't support them anyway. This makes the SAD check uniform for all ports.
> > 
> > Signed-off-by: Simon Ser <simon.ser at intel.com>
> > ---
> >  tests/kms_chamelium.c | 69 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 64 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 49c3de0b7beb..16d3a773bb2a 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -28,6 +28,7 @@
> >  #include "igt.h"
> >  #include "igt_vc4.h"
> >  #include "igt_edid.h"
> > +#include "igt_eld.h"
> >  
> >  #include <fcntl.h>
> >  #include <pthread.h>
> > @@ -1426,6 +1427,60 @@ test_display_audio(data_t *data, struct chamelium_port *port,
> >  	free(alsa);
> >  }
> >  
> > +static void
> > +test_display_audio_edid(data_t *data, struct chamelium_port *port,
> > +			enum test_edid edid)
> > +{
> > +	igt_output_t *output;
> > +	igt_plane_t *primary;
> > +	struct igt_fb fb;
> > +	drmModeModeInfo *mode;
> > +	drmModeConnector *connector;
> > +	int fb_id;
> > +	struct eld_entry eld;
> > +	const char *monitor_name;
> > +	struct eld_sad *sad;
> > +
> > +	reset_state(data, port);
> > +
> > +	output = prepare_output(data, port, edid);
> > +	connector = chamelium_port_get_connector(data->chamelium, port, false);
> > +	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> > +	igt_assert(primary);
> > +
> > +	/* Enable the output because audio cannot be played on inactive
> > +	 * connectors. */
> > +	igt_assert(connector->count_modes > 0);
> > +	mode = &connector->modes[0];
> > +
> > +	fb_id = igt_create_color_pattern_fb(data->drm_fd,
> > +					    mode->hdisplay, mode->vdisplay,
> > +					    DRM_FORMAT_XRGB8888,
> > +					    LOCAL_DRM_FORMAT_MOD_NONE,
> > +					    0, 0, 0, &fb);
> > +	igt_assert(fb_id > 0);
> > +
> > +	enable_output(data, port, output, mode, &fb);
> > +
> > +	monitor_name = "IGT";
> > +	if (edid == TEST_EDID_CHAMELIUM_DEFAULT)
> > +		monitor_name = "AthenaDP";
> 
> I am starting to grow less and less fond of using the default
> Chamelium's EDID. This will just make us build up on simillar checks
> over time. Let's just define our own copy and then use values from it
> directly.

I agree. I tried to add a DP audio EDID when implementing the HDMI
audio EDID code, however this requires:

1. Comparing our generated EDID with the default Chamelium one
2. Trying to figure out what field in the diff could make audio work
3. Adding support for that field in igt_edid, praying that only this
   part was misisng
4. Goto 1

It took several rounds to get HDMI to work. At some point I just gave
up for DP because we had a simple way to do it with Chamelium's default
EDID.

I still think it would be better to generate all EDIDs. One of my
recent patch series adds support for Speaker Allocation Data blocks
("the other SAD"). I'll try again and see if that works.

> "monitor_name" should probably be named manufacturer_id and be obtained
> by chamelium_get_manufacturer_id(edid_id) or smth.
> 
> And since you were talking about MST in one of the other patches, let's
> talk about MST :-) This would make it more future proof (WIP was
> defining a copy of EDID for each port using manufacturer id in form of
> "I-$CHAM_PORT_ID"). chamelium_get_manufacturer_id() can be extended later
> to also take chamelium_port as parameter.

One other option would be to keep "IGT" for the manufacturer and use
the serial number to tell EDIDs apart.

Note that this mechanism would also be useful for configuration-less
Chamelium setup.

> Other than that the series looks good.

Thanks for the review!

> > +
> > +	igt_assert(eld_from_monitor_name(&eld, monitor_name));
> > +	igt_assert(eld.sads_len == 1);
> > +
> > +	sad = &eld.sads[0];
> > +	igt_assert(sad->coding_type == CEA_SAD_FORMAT_PCM);
> > +	igt_assert(sad->channels == 2);
> > +	igt_assert(sad->rates == (CEA_SAD_SAMPLING_RATE_32KHZ |
> > +		   CEA_SAD_SAMPLING_RATE_44KHZ | CEA_SAD_SAMPLING_RATE_48KHZ));
> > +	igt_assert(sad->bits == (CEA_SAD_SAMPLE_SIZE_16 |
> > +		   CEA_SAD_SAMPLE_SIZE_20 | CEA_SAD_SAMPLE_SIZE_24));
> > +
> > +	igt_remove_fb(data->drm_fd, &fb);
> > +
> > +	drmModeFreeConnector(connector);
> > +}
> >  
> >  static void randomize_plane_stride(data_t *data,
> >  				   uint32_t width, uint32_t height,
> > @@ -2005,11 +2060,7 @@ static unsigned const char *get_hdmi_audio_edid(void)
> >  	channels = 2; /* TODO: speaker alloc blocks for > 2 channels */
> >  	sampling_rates = CEA_SAD_SAMPLING_RATE_32KHZ |
> >  			 CEA_SAD_SAMPLING_RATE_44KHZ |
> > -			 CEA_SAD_SAMPLING_RATE_48KHZ |
> > -			 CEA_SAD_SAMPLING_RATE_88KHZ |
> > -			 CEA_SAD_SAMPLING_RATE_96KHZ |
> > -			 CEA_SAD_SAMPLING_RATE_176KHZ |
> > -			 CEA_SAD_SAMPLING_RATE_192KHZ;
> > +			 CEA_SAD_SAMPLING_RATE_48KHZ;
> >  	sample_sizes = CEA_SAD_SAMPLE_SIZE_16 |
> >  		       CEA_SAD_SAMPLE_SIZE_20 |
> >  		       CEA_SAD_SAMPLE_SIZE_24;
> > @@ -2174,6 +2225,10 @@ igt_main
> >  		connector_subtest("dp-audio", DisplayPort)
> >  			test_display_audio(&data, port, "HDMI",
> >  					   TEST_EDID_CHAMELIUM_DEFAULT);
> > +
> > +		connector_subtest("dp-audio-edid", DisplayPort)
> > +			test_display_audio_edid(&data, port,
> > +						TEST_EDID_CHAMELIUM_DEFAULT);
> >  	}
> >  
> >  	igt_subtest_group {
> > @@ -2325,6 +2380,10 @@ igt_main
> >  		connector_subtest("hdmi-audio", HDMIA)
> >  			test_display_audio(&data, port, "HDMI",
> >  					   TEST_EDID_HDMI_AUDIO);
> > +
> > +		connector_subtest("hdmi-audio-edid", HDMIA)
> > +			test_display_audio_edid(&data, port,
> > +						TEST_EDID_HDMI_AUDIO);
> >  	}
> >  
> >  	igt_subtest_group {
> > -- 
> > 2.21.0
> >