[igt-dev] [PATCH i-g-t v4] igt/tests/kms_dp_tiled_display: kms test for display port tiled displays

[Ser, Simon]

Hi,

Thanks for this new version, I like it. The updated page-flip checks
look good to me. Here is my last round of comments, with the non-nit
ones fixed, this is:

Reviewed-by: Simon Ser <simon.ser at intel.com>

On Thu, 2019-09-12 at 16:28 -0700, Manasi Navare wrote:
> From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep at intel.com>
> 
> This test validates the tiled DP displays to display a test pattern
> seamlessly across the two tiles. It validates the transcoder port
> sync feature on i915 to get a tearfree tiled display output.
> 
> Related kernel work patches-
> https://patchwork.freedesktop.org/series/66403/
> 
> This test can eventually be extended to cover tiled display support
> on other connector types.
> 
> v4:
> * Add the logic to check there are
> page flips per connector (Simon)
> * Calloc instead of malloc to initialize (Manasi)
> 
> v3:
> * Fix the pointer leaks (Simon)
> * Code indentation (Manasi)
> * Compare two consecutive flip timestamps (Simon)
> * Use single fb across tiles (Simon)
> * Fix reset mode logic (Manasi)
> 
> v2: Added a check for checking pageflip event timestamps (Simon, Manasi)
>     Minor style changes (Simon)
>    Code clean-up and reordering
> 
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Simon Ser <simon.ser at intel.com>
> Cc: <madhumitha.tp at gmail.com>
> Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep at intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> ---
>  tests/Makefile.sources       |   1 +
>  tests/kms_dp_tiled_display.c | 397 +++++++++++++++++++++++++++++++++++
>  tests/meson.build            |   1 +
>  3 files changed, 399 insertions(+)
>  create mode 100644 tests/kms_dp_tiled_display.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index c02e4d94..7561ab9b 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -41,6 +41,7 @@ TESTS_progs = \
>  	kms_cursor_edge_walk \
>  	kms_cursor_legacy \
>  	kms_dp_dsc \
> +	kms_dp_tiled_display \
>  	kms_draw_crc \
>  	kms_fbcon_fbt \
>  	kms_fence_pin_leak \
> diff --git a/tests/kms_dp_tiled_display.c b/tests/kms_dp_tiled_display.c
> new file mode 100644
> index 00000000..3055c6c3
> --- /dev/null
> +++ b/tests/kms_dp_tiled_display.c
> @@ -0,0 +1,397 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *  Madhumitha Tolakanahalli Pradeep
> + *      <madhumitha.tolakanahalli.pradeep at intel.com>
> + *  Manasi Navare <manasi.d.navare at intel.com>
> + *
> + * Display Port Tiled Display Test
> + * This test parses the tile information of the connectors that have TILE
> + * property set, sets up the framebuffer with correct offsets corresponding to
> + * the tile offsets and does an atomic modeset with two CRTCs for two
> + * connectors. Page flip event timestamp from each CRTC is collected and
> + * compared to make sure that they occurred in a synchronous manner.
> + *
> + * This test currently supports only horizontally tiled displays, in line with
> + * the displays supported by the kernel at the moment.
> + */
> +
> +#include "igt.h"
> +#include "poll.h"
> +#include "drm_mode.h"
> +#include "drm_fourcc.h"
> +
> +IGT_TEST_DESCRIPTION("Test for Transcoder Port Sync for Display Port Tiled Displays");
> +
> +typedef struct {
> +	igt_output_t *output;
> +	igt_tile_info_t tile;
> +	enum pipe pipe;
> +	drmModeConnectorPtr connector;
> +	bool got_page_flip;
> +} data_connector_t;
> +
> +typedef struct {
> +	int drm_fd;
> +	int num_h_tiles;
> +	igt_fb_t fb_test_pattern;
> +	igt_display_t *display;
> +	data_connector_t *conns;
> +	enum igt_commit_style commit;
> +} data_t;
> +
> +static int drm_property_is_tile(drmModePropertyPtr prop)
> +{
> +	return (strcmp(prop->name, "TILE") ? 0 : 1) &&
> +			 drm_property_type_is(prop, DRM_MODE_PROP_BLOB);
> +}
> +
> +static void get_connector_tile_props(data_t *data, drmModeConnectorPtr conn,
> +				     igt_tile_info_t *tile)
> +{
> +	int i = 0;
> +	drmModePropertyPtr prop;
> +	drmModePropertyBlobPtr blob;
> +
> +	for (i = 0; i < conn->count_props; i++) {
> +		prop = drmModeGetProperty(data->drm_fd, conn->props[i]);
> +
> +		igt_assert(prop);
> +
> +		if (!drm_property_is_tile(prop)) {
> +			drmModeFreeProperty(prop);
> +			continue;
> +		}
> +
> +		blob = drmModeGetPropertyBlob(data->drm_fd,
> +				conn->prop_values[i]);
> +
> +		if (!blob)
> +			goto cleanup;
> +
> +		igt_parse_connector_tile_blob(blob, tile);
> +		break;
> +	}
> +
> +cleanup:
> +	drmModeFreeProperty(prop);
> +	drmModeFreePropertyBlob(blob);
> +}
> +
> +static void get_number_of_h_tiles(data_t *data)
> +{
> +	int i;
> +	drmModeResPtr res;
> +	drmModeConnectorPtr connector;
> +	igt_tile_info_t tile = {.num_h_tile = 0};
> +
> +	igt_assert(res = drmModeGetResources(data->drm_fd));
> +
> +	for (i = 0; i < res->count_connectors; i++) {
> +		connector = drmModeGetConnectorCurrent(data->drm_fd,
> +				res->connectors[i]);
> +
> +		igt_assert(connector);
> +
> +		if (connector->connection != DRM_MODE_CONNECTED ||
> +		    connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort) {
> +			drmModeFreeConnector(connector);
> +			continue;
> +		}
> +
> +		get_connector_tile_props(data, connector, &tile);
> +		data->num_h_tiles = tile.num_h_tile;
> +		break;

In case there is a connected DisplayPort screen which isn't tiled, this
will bail out even if there is also a tiled screen connected.

Would be nice to:

	if (tile.num_h_tile == 0) {
		drmModeFreeConnector(connector);
		continue;
	}

> +	}
> +
> +	drmModeFreeResources(res);
> +	drmModeFreeConnector(connector);
> +}
> +
> +static void get_connectors(data_t *data)
> +{
> +	int count = 0;
> +	igt_output_t *output;
> +	data_connector_t *conns = data->conns;
> +
> +	for_each_connected_output(data->display, output) {
> +		conns[count].connector = drmModeGetConnector(data->display->drm_fd,
> +							     output->id);
> +
> +		igt_assert(conns[count].connector);
> +
> +		if (conns[count].connector->connector_type !=
> +		    DRM_MODE_CONNECTOR_DisplayPort) {
> +			drmModeFreeConnector(conns[count].connector);
> +			continue;
> +		}
> +
> +		get_connector_tile_props(data, conns[count].connector,
> +					 &conns[count].tile);

Ditto, would be nice to skip if conns[count].tile.num_h_tile == 0.

> +		/* Check if the connectors belong to the same tile group */
> +		if (count > 0)
> +			igt_assert(conns[count].tile.tile_group_id ==
> +				   conns[count-1].tile.tile_group_id);
> +
> +		count++;
> +	}
> +}
> +
> +static void
> +reset_plane(igt_output_t *output)
> +{
> +	igt_plane_t *primary;
> +
> +	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> +	igt_plane_set_fb(primary, NULL);
> +}
> +
> +static void reset_output(igt_output_t *output)
> +{
> +	igt_output_set_pipe(output, PIPE_NONE);
> +}
> +
> +static void reset_mode(data_t *data)
> +{
> +	int count;
> +	igt_output_t *output;
> +	data_connector_t *conns = data->conns;
> +
> +	for (count = 0; count < data->num_h_tiles; count++) {
> +		output = igt_output_from_connector(data->display,
> +						   conns[count].connector);
> +		igt_output_set_pipe(output, PIPE_NONE);
> +	}
> +	igt_display_commit2(data->display, data->commit);
> +}
> +
> +static void test_cleanup(data_t *data)
> +{
> +	int count;
> +	data_connector_t *conns = data->conns;
> +
> +	for (count = 0; count < data->num_h_tiles; count++) {
> +		if (conns[count].output) {
> +			reset_plane(conns[count].output);
> +			reset_output(conns[count].output);
> +		}
> +	}
> +	igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
> +	igt_display_commit2(data->display, data->commit);
> +	memset(conns, 0, sizeof(data_connector_t) * data->num_h_tiles);
> +}
> +
> +static void setup_mode(data_t *data)
> +{
> +	int count = 0;
> +	enum pipe pipe;
> +	igt_output_t *output;
> +	data_connector_t *conns = data->conns;
> +
> +	/*
> +	 * The output is set to PIPE_NONE and then assigned a pipe.
> +	 * This is done to ensure a complete modeset occures every
> +	 * time the test is run.
> +	 */
> +	reset_mode(data);
> +
> +	for (count = 0; count < data->num_h_tiles; count++) {
> +		output = igt_output_from_connector(data->display,
> +						   conns[count].connector);
> +
> +		for_each_pipe(data->display, pipe) {
> +			if (count > 0 && pipe == conns[count-1].pipe)
> +				continue;

This only checks whether the previous connector picked this pipe. We
need to make sure none of the previous connectors picked this pipe.

For instance, if num_h_tiles == 3, maybe conns[0] has picked pipe A,
and we want to prevent conns[2] from also picking pipe A.

So the easiest way to fix it is to iterate over all previous connectors
(conns[0] till conns[count-1]) and make sure nobody picked the pipe
already. This wouldn't be very efficient, but who cares.

Alternatively, see pick_n_outputs in my kms_atomic_multi patch, which
does it the other way around: iterate over all pipes then iterate over
all connectors. Doing it that way allows output->pending_pipe to be
checked.

> +
> +			if (igt_pipe_connector_valid(pipe, output)) {
> +

Style nit: stray newline here ^

> +				conns[count].pipe = pipe;
> +				conns[count].output = output;
> +
> +				igt_output_set_pipe(conns[count].output,
> +						    conns[count].pipe);
> +				break;
> +			}
> +		}
> +	}
> +	igt_display_commit_atomic(data->display, DRM_MODE_ATOMIC_ALLOW_MODESET,
> +				  NULL);
> +}
> +
> +static void setup_framebuffer(data_t *data)
> +{
> +	int count;
> +	igt_plane_t *primary;
> +	int fb_h_size = 0, fb_v_size = 0;
> +	data_connector_t *conns = data->conns;
> +
> +	for (count = 0; count < data->num_h_tiles; count++) {
> +

Style nit: stray newline here ^

> +		fb_h_size += conns[count].tile.tile_h_size;
> +		/* We support only horizontal tiles, so vertical size is same
> +		 * for all tiles and needs to be assigned only once.
> +		 */
> +		if (!fb_v_size)
> +			fb_v_size = conns[count].tile.tile_v_size;

Nit: maybe igt_assert (or igt_require?) that tile_v_size is the same
for all  tiles?

> +	}
> +
> +	igt_create_pattern_fb(data->drm_fd,
> +			      fb_h_size,
> +			      fb_v_size,
> +			      DRM_FORMAT_XBGR8888,
> +			      LOCAL_DRM_FORMAT_MOD_NONE,
> +			      &data->fb_test_pattern);
> +
> +	for (count = 0; count < data->num_h_tiles; count++) {
> +

Style nit: stray newline here ^

> +		primary = igt_output_get_plane_type(conns[count].output,
> +						    DRM_PLANE_TYPE_PRIMARY);
> +
> +		igt_plane_set_fb(primary, &data->fb_test_pattern);
> +
> +		igt_fb_set_size(&data->fb_test_pattern, primary,
> +				conns[count].tile.tile_h_size,
> +				conns[count].tile.tile_v_size);
> +
> +		igt_fb_set_position(&data->fb_test_pattern, primary,
> +				    (conns[count].tile.tile_h_size *
> +				     conns[count].tile.tile_h_loc),
> +				    (conns[count].tile.tile_v_size *
> +				     conns[count].tile.tile_v_loc));
> +
> +		igt_plane_set_size(primary,
> +				   conns[count].tile.tile_h_size,
> +				   conns[count].tile.tile_v_size);
> +	}
> +}
> +
> +static void page_flip_handler(int fd, unsigned int seq,
> +			      unsigned int tv_sec, unsigned int tv_usec,
> +			      unsigned int crtc_id, void *_data)
> +{
> +	data_t *data = _data;
> +	data_connector_t *conn;
> +	bool is_on_time = false;
> +	static unsigned int _tv_sec, _tv_usec;
> +	int i;
> +
> +	igt_debug("Page Flip Event received from CRTC:%d at %u:%u\n", crtc_id,
> +		  tv_sec, tv_usec);
> +
> +	for (i = 0; i < data->num_h_tiles; i++) {
> +

Style nit: stray newline here ^

> +		conn = &data->conns[i];
> +		if (data->display->pipes[conn->pipe].crtc_id == crtc_id) {
> +			igt_assert_f(!conn->got_page_flip,
> +				     "Got two page-flips for CRTC %u\n",
> +				     crtc_id);
> +			conn->got_page_flip = true;
> +
> +			/* Skip the following checks for the first page flip event */
> +			if (_tv_sec == 0 || _tv_usec == 0) {
> +				_tv_sec = tv_sec;
> +				_tv_usec = tv_usec;
> +				return;
> +			}
> +			/*
> +			 * For seamless tear-free display, the page flip event timestamps
> +			 * from all the tiles should not differ by more than 10us.
> +			 */
> +			is_on_time = tv_sec == _tv_sec && (abs(tv_usec - _tv_usec) < 10);
> +
> +			igt_fail_on_f(!is_on_time, "Delayed page flip event from CRTC:%d at %u:%u\n",
> +				      crtc_id, tv_sec, tv_usec);
> +			return;
> +		}
> +	}
> +
> +	igt_assert_f(false, "Got page-flip event for unexpected CRTC %u\n",
> +		     crtc_id);
> +}
> +
> +static bool got_all_page_flips(data_t *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->num_h_tiles; i++) {
> +		if (!data->conns[i].got_page_flip)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +igt_main
> +{
> +	igt_display_t display;
> +	data_t data = {0};
> +	struct pollfd pfd = {0};
> +	drmEventContext drm_event = {0};
> +	int ret;
> +
> +	igt_fixture {
> +		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> +
> +		kmstest_set_vt_graphics_mode();
> +		igt_display_require(&display, data.drm_fd);
> +		igt_display_reset(&display);
> +
> +		data.display = &display;
> +		pfd.fd = data.drm_fd;
> +		pfd.events = POLLIN;
> +		drm_event.version = 3;
> +		drm_event.page_flip_handler2 = page_flip_handler;
> +		data.commit = data.display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY;
> +		igt_require(data.commit == COMMIT_ATOMIC);
> +
> +		get_number_of_h_tiles(&data);
> +		igt_debug("Number of Horizontal Tiles: %d\n", data.num_h_tiles);
> +		igt_require(data.num_h_tiles > 0);
> +		data.conns = calloc(data.num_h_tiles, sizeof(data_connector_t));
> +	}
> +
> +	igt_subtest("basic-test-pattern") {
> +		igt_assert(data.conns);
> +
> +		get_connectors(&data);
> +		setup_mode(&data);
> +		setup_framebuffer(&data);
> +		igt_display_commit_atomic(data.display, DRM_MODE_ATOMIC_NONBLOCK |
> +					  DRM_MODE_PAGE_FLIP_EVENT, &data);
> +		while (!got_all_page_flips(&data)) {
> +			ret = poll(&pfd, 1, 1000);
> +			igt_assert(ret == 1);
> +			drmHandleEvent(data.drm_fd, &drm_event);
> +		}
> +
> +		test_cleanup(&data);
> +	}
> +
> +	igt_fixture {
> +		free(data.conns);
> +		close(data.drm_fd);
> +		kmstest_restore_vt_mode();
> +		igt_display_fini(data.display);
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index a7b2b322..50292df8 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -26,6 +26,7 @@ test_progs = [
>  	'kms_cursor_edge_walk',
>  	'kms_cursor_legacy',
>  	'kms_dp_dsc',
> +	'kms_dp_tiled_display',
>  	'kms_draw_crc',
>  	'kms_fbcon_fbt',
>  	'kms_fence_pin_leak',