Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_chamelium: add a test checking modes

["Ser, Simon" <simon.ser@intel.com>]

On Wed, 2019-07-17 at 16:26 +0300, Martin Peres wrote:
> On 04/07/2019 10:25, Ser, Simon wrote:
> > On Wed, 2019-07-03 at 15:14 +0100, Peres, Martin wrote:
> > > On 03/07/2019 10:57, Ser, Simon wrote:
> > > > The idea is to check that the mode is correctly applied. We use the Chamelium's
> > > > GetVideoParams method to get and check mode parameters.
> > > > 
> > > > We need to start a capture of zero frames to trigger the FSM. Without it,
> > > > Chamelium's receiver doesn't get stable video input.
> > > > 
> > > > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > > > ---
> > > >  tests/kms_chamelium.c | 110 ++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 110 insertions(+)
> > > > 
> > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > index b749b5598c1d..b8d6718f5b68 100644
> > > > --- a/tests/kms_chamelium.c
> > > > +++ b/tests/kms_chamelium.c
> > > > @@ -758,6 +758,110 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
> > > >  	drmModeFreeConnector(connector);
> > > >  }
> > > >  
> > > > +#define MODE_CLOCK_ACCURACY 0.05 /* 5% */
> > > > +
> > > > +static void check_mode(struct chamelium *chamelium, struct chamelium_port *port,
> > > > +		       drmModeModeInfo *mode)
> > > > +{
> > > > +	struct chamelium_video_params video_params = {0};
> > > > +	double mode_clock;
> > > > +	int mode_hsync_offset, mode_vsync_offset;
> > > > +	int mode_hsync_width, mode_vsync_width;
> > > > +	int mode_hsync_polarity, mode_vsync_polarity;
> > > > +
> > > > +	chamelium_port_get_video_params(chamelium, port, &video_params);
> > > > +
> > > > +	mode_clock = (double) mode->clock / 1000;
> > > > +	mode_hsync_offset = mode->hsync_start - mode->hdisplay;
> > > > +	mode_vsync_offset = mode->vsync_start - mode->vdisplay;
> > > > +	mode_hsync_width = mode->hsync_end - mode->hsync_start;
> > > > +	mode_vsync_width = mode->vsync_end - mode->vsync_start;
> > > > +	mode_hsync_polarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
> > > > +	mode_vsync_polarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
> > > > +
> > > > +	igt_debug("Checking video mode:\n");
> > > > +	igt_debug("clock: got %f, expected %f ± %f%%\n",
> > > > +		  video_params.clock, mode_clock, MODE_CLOCK_ACCURACY * 100);
> > > > +	igt_debug("hactive: got %d, expected %d\n",
> > > > +		  video_params.hactive, mode->hdisplay);
> > > > +	igt_debug("vactive: got %d, expected %d\n",
> > > > +		  video_params.vactive, mode->vdisplay);
> > > > +	igt_debug("hsync_offset: got %d, expected %d\n",
> > > > +		  video_params.hsync_offset, mode_hsync_offset);
> > > > +	igt_debug("vsync_offset: got %d, expected %d\n",
> > > > +		  video_params.vsync_offset, mode_vsync_offset);
> > > > +	igt_debug("htotal: got %d, expected %d\n",
> > > > +		  video_params.htotal, mode->htotal);
> > > > +	igt_debug("vtotal: got %d, expected %d\n",
> > > > +		  video_params.vtotal, mode->vtotal);
> > > > +	igt_debug("hsync_width: got %d, expected %d\n",
> > > > +		  video_params.hsync_width, mode_hsync_width);
> > > > +	igt_debug("vsync_width: got %d, expected %d\n",
> > > > +		  video_params.vsync_width, mode_vsync_width);
> > > > +	igt_debug("hsync_polarity: got %d, expected %d\n",
> > > > +		  video_params.hsync_polarity, mode_hsync_polarity);
> > > > +	igt_debug("vsync_polarity: got %d, expected %d\n",
> > > > +		  video_params.vsync_polarity, mode_vsync_polarity);
> > > > +
> > > > +	if (!isnan(video_params.clock)) {
> > > > +		igt_assert(video_params.clock >
> > > > +			   mode_clock * (1 - MODE_CLOCK_ACCURACY));
> > > > +		igt_assert(video_params.clock <
> > > > +			   mode_clock * (1 + MODE_CLOCK_ACCURACY));
> > > > +	}
> > > > +	igt_assert(video_params.hactive == mode->hdisplay);
> > > > +	igt_assert(video_params.vactive == mode->vdisplay);
> > > > +	igt_assert(video_params.hsync_offset == mode_hsync_offset);
> > > > +	igt_assert(video_params.vsync_offset == mode_vsync_offset);
> > > > +	igt_assert(video_params.htotal == mode->htotal);
> > > > +	igt_assert(video_params.vtotal == mode->vtotal);
> > > > +	igt_assert(video_params.hsync_width == mode_hsync_width);
> > > > +	igt_assert(video_params.vsync_width == mode_vsync_width);
> > > > +	igt_assert(video_params.hsync_polarity == mode_hsync_polarity);
> > > > +	igt_assert(video_params.vsync_polarity == mode_vsync_polarity);
> > > > +}
> > > > +
> > > > +static void test_modes(data_t *data, struct chamelium_port *port)
> > > > +{
> > > > +	igt_output_t *output;
> > > > +	igt_plane_t *primary;
> > > > +	drmModeConnector *connector;
> > > > +	int fb_id, i;
> > > > +	struct igt_fb fb;
> > > > +
> > > > +	igt_require(chamelium_supports_get_video_params(data->chamelium));
> > > > +
> > > > +	reset_state(data, port);
> > > 
> > > Is that using the default EDID? We want to test all fields, at a few
> > > resolutions. Execution time is really important here since I assume each
> > > round takes around 3s, so we cannot have too many modes being tested.
> > 
> > No: see the prepare_output call below with TEST_EDID_BASE. We use the
> > base IGT EDID for now. It doesn't contain a lot of modes.
> 
> OK! I guess we'll see how long it takes using the drmtip runs, once it
> lands. Please remember to check :)

Yeah!

> > > > +
> > > > +	output = prepare_output(data, port, TEST_EDID_BASE);
> > > > +	connector = chamelium_port_get_connector(data->chamelium, port, false);
> > > > +	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> > > > +	igt_assert(primary);
> > > > +
> > > > +	igt_assert(connector->count_modes > 0);
> > > > +	for (i = 0; i < connector->count_modes; i++) {
> > > > +		drmModeModeInfo *mode = &connector->modes[i];
> > > > +
> > > > +		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);
> > > 
> > > Creating a new FB might be a little slow on some machines. Why not reuse
> > > the same FB and scale it up and down? Or even better, why set an FB at all?
> > 
> > I don't think you can do a modeset without a FB.
> > 
> > I'm also not sure scaling is always supported. Maybe we could create
> > multiple FBs with different sizes from the same dumb buffer.
> 
> Fair-enough! Maybe we could add debug messages in
> igt_create_color_pattern_fb to tell how long this takes?

Indeed :)

/me adds to todo-list

> > > > +		igt_assert(fb_id > 0);
> > > > +
> > > > +		enable_output(data, port, output, mode, &fb);
> > > > +
> > > > +		/* Trigger the FSM */
> > > > +		chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 0);
> > > > +
> > > > +		check_mode(data->chamelium, port, mode);
> > > > +
> > > > +		igt_remove_fb(data->drm_fd, &fb);
> > > > +	}
> > > > +
> > > > +	drmModeFreeConnector(connector);
> > > > +}
> > > > +
> > > >  
> > > >  /* Playback parameters control the audio signal we synthesize and send */
> > > >  #define PLAYBACK_CHANNELS 2
> > > > @@ -2160,6 +2264,9 @@ igt_main
> > > >  		connector_subtest("dp-frame-dump", DisplayPort)
> > > >  			test_display_frame_dump(&data, port);
> > > >  
> > > > +		connector_subtest("dp-modes", DisplayPort)
> > > > +			test_modes(&data, port);
> > > 
> > > dp-modes-check?
> > 
> > I'm not a fan of adding "check" in test names, because it seems
> > redundant. It's a test, so yes it's going to check something…
> 
> That is a fair point :D How about dp-mode-timings?

Works for me!

> > > The series is:
> > > 
> > > Reviewed-by: Martin Peres <martin.peres@linux.intel.com>
> > > 
> > > However, I would really like to see if we can run this test under 30s
> > > (unlike testdisplay).
> > 
> > Yeah, that's really not happening right now. It takes 38s on my
> > machine.
> > 
> > The modesets are taking a long time. I'm not sure we can do anything
> > about it.
> 
> Well, we can: Reduce the amount of them by crafting a better EDID that
> would check what we want to check.
> 
> I am still OK with merging it for now, but more work is needed to make
> it better!

Oh right. I suppose we could even expose just one or two modes each
time, picked at random from a list.

> Martin
> > > Martin
> > > 
> > > > +
> > > >  		connector_subtest("dp-audio", DisplayPort)
> > > >  			test_display_audio(&data, port, "HDMI",
> > > >  					   TEST_EDID_DP_AUDIO);
> > > > @@ -2315,6 +2422,9 @@ igt_main
> > > >  		connector_subtest("hdmi-frame-dump", HDMIA)
> > > >  			test_display_frame_dump(&data, port);
> > > >  
> > > > +		connector_subtest("hdmi-modes", HDMIA)
> > > > +			test_modes(&data, port);
> > > > +
> > > >  		connector_subtest("hdmi-audio", HDMIA)
> > > >  			test_display_audio(&data, port, "HDMI",
> > > >  					   TEST_EDID_HDMI_AUDIO);
> > > > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev