[igt-dev] [PATCH i-g-t v2 2/2] igt/tests/kms_dp_tiled_display: kms test for display port tiled displays
Ser, Simon
simon.ser at intel.com
Tue Aug 27 10:49:38 UTC 2019
Thanks for the new version! Here are some more comments. Some of them
are nits, some of them are more serious questions. Feel free to let me
know if I'm mistaken or if you don't understand one of them.
On Fri, 2019-08-23 at 11:23 -0700, Madhumitha Tolakanahalli Pradeep wrote:
> 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/59837/#rev4.
>
> This test can eventually be extended to cover tiled display support
> on other connector types.
>
> 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>
> ---
> tests/Makefile.sources | 1 +
> tests/kms_dp_tiled_display.c | 346 +++++++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 3 files changed, 348 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..162fbdd9
> --- /dev/null
> +++ b/tests/kms_dp_tiled_display.c
> @@ -0,0 +1,346 @@
> +/*
> + * 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>
> + *
> + * 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 Display Port Tiled Displays");
> +
> +typedef struct {
> + int drm_fd;
> + int num_h_tiles;
> + igt_display_t *display;
> + enum igt_commit_style commit;
> +} data_t;
> +
> +typedef struct {
> + igt_output_t *output;
> + igt_tile_info_t tile;
> + igt_fb_t fb_test_pattern;
> + enum pipe pipe;
> + enum igt_commit_style commit;
> + drmModeConnectorPtr connector;
> +} data_connector_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))
> + continue;
This branch leaks prop.
> + blob = drmModeGetPropertyBlob(data->drm_fd,
> + conn->prop_values[i]);
> +
> + if (!blob)
> + return;
This branch leaks prop.
> + igt_parse_connector_tile_blob(blob, tile);
> + break;
> + }
> +
> + 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))
> + continue;
This can be simplified to
connection != DRM_MODE_CONNECTED ||
connector_type != DRM_MODE_CONNECTOR_DisplayPort
Additionally, this branch leaks the connector.
> + get_connector_tile_props(data, connector, &tile);
> + data->num_h_tiles = tile.num_h_tile;
> + break;
> + }
> +
> + drmModeFreeResources(res);
> + drmModeFreeConnector(connector);
> +}
> +
> +static void get_connector(data_t *data, data_connector_t *conn)
Nit: since this fills all connectors, maybe it should be named
get_connectors (with an s). Also it's not clear that conn refers to an
array of connectors, maybe it should be renamed to conns.
(Applies to the whole patch)
> +{
> + int count = 0;
> + igt_output_t *output;
> +
> + for_each_connected_output(data->display, output) {
> + conn[count].connector = drmModeGetConnector(data->display->drm_fd,
> + output->id);
> +
> + igt_assert(conn[count].connector);
> +
> + if (!(conn[count].connector->connector_type ==
> + DRM_MODE_CONNECTOR_DisplayPort))
> + continue;
This can be simplified to
if (connector_type != DRM_MODE_CONNECTOR_DisplayPort)
Additionally, this branch leaks the connector.
> + get_connector_tile_props(data, conn[count].connector,
> + &conn[count].tile);
> +
> + /* Check if the connectors belong to the same tile group */
> + if (count > 0)
> + igt_assert(conn[count].tile.tile_group_id ==
> + conn[count-1].tile.tile_group_id);
> +
> + count++;
> + }
> +}
> +
> +static void
> +reset_framebuffer(int drm_fd, igt_output_t *output, igt_fb_t *fb)
> +{
> + igt_plane_t *primary;
> +
> + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> + igt_plane_set_fb(primary, NULL);
> + igt_remove_fb(drm_fd, fb);
> +}
> +
> +static void reset_output(igt_output_t *output)
> +{
> + igt_output_set_pipe(output, PIPE_NONE);
> +}
> +
> +static void test_cleanup(data_t *data, data_connector_t *conn)
> +{
> + int count;
> +
> + for (count = 0; count < data->num_h_tiles; count++) {
> + if (conn[count].output) {
> + reset_framebuffer(data->drm_fd, conn[count].output,
> + &conn[count].fb_test_pattern);
> + reset_output(conn[count].output);
> + }
> + }
> + igt_display_commit2(data->display, data->commit);
> +}
> +
> +static void setup_mode(data_t *data, data_connector_t *conn_data)
This function doesn't actually setup the output mode, it only sets up
the pipe. Is this an overlook?
> +{
> + int count = 0;
> + enum pipe pipe;
> + igt_output_t *output;
> +
> + for (count = 0; count < data->num_h_tiles; count++) {
> + output = igt_output_from_connector(data->display,
> + conn_data[count].connector);
> +
> + /*
> + * 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_output(output);
Is this necessary?
> + for_each_pipe(data->display, pipe) {
> + if (count > 0 && pipe == conn_data[count-1].pipe)
> + continue;
> +
> + if (igt_pipe_connector_valid(pipe, output)) {
> +
> + conn_data[count].pipe = pipe;
> + conn_data[count].output = output;
> +
> + igt_output_set_pipe(conn_data[count].output,
> + conn_data[count].pipe);
> + break;
> + }
> + }
> + }
> + igt_display_commit_atomic(data->display, DRM_MODE_ATOMIC_ALLOW_MODESET,
> + NULL);
> +}
> +
> +static void setup_framebuffer(data_t *data, data_connector_t *conn)
> +{
> + int count;
> + igt_plane_t *primary;
> +
> + for (count = 0; count < data->num_h_tiles; count++) {
> +
> + igt_create_pattern_fb(data->drm_fd,
> + (conn[count].tile.tile_h_size *
> + data->num_h_tiles),
Do we need to multiply by the number of horizontal tiles here? It seems
we only use the first tile_h_size pixels of the buffer. Am I missing
something?
But maybe a test that would better reflect reality would create a
single framebuffer for all tiled displays, to display a single image
across all of them?
> + conn[count].tile.tile_v_size,
> + DRM_FORMAT_XBGR8888,
> + LOCAL_DRM_FORMAT_MOD_NONE,
> + &conn[count].fb_test_pattern);
> +
> + primary = igt_output_get_plane_type(conn[count].output,
> + DRM_PLANE_TYPE_PRIMARY);
> +
> + igt_plane_set_fb(primary, &conn[count].fb_test_pattern);
> +
> + igt_fb_set_size(&conn[count].fb_test_pattern, primary,
> + conn[count].tile.tile_h_size,
> + conn[count].tile.tile_v_size);
> +
> + igt_fb_set_position(&conn[count].fb_test_pattern, primary,
> + (conn[count].tile.tile_h_size *
> + conn[count].tile.tile_h_loc),
> + (conn[count].tile.tile_v_size *
> + conn[count].tile.tile_v_loc));
> +
> + igt_plane_set_size(primary,
> + conn[count].tile.tile_h_size,
> + conn[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)
> +{
> + bool expr = false;
> + static unsigned int _tv_sec, _tv_usec;
> +
> + igt_debug("Page Flip Event received from CRTC:%d at %u:%u\n", crtc_id,
> + tv_sec, tv_usec);
We should also make sure we receive exactly one page-flip per CRTC.
Currently we don't use crtc_id (apart from logging purposes).
> + /* 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.
> + */
> + expr = tv_sec == _tv_sec && (abs(tv_usec - _tv_usec) < 10);
Nit: this could be renamed to e.g. is_on_time, expr is a pretty vague
name.
> + igt_fail_on_f(!expr, "Delayed page flip event from CRTC:%d at %u:%u\n",
> + crtc_id, tv_sec, tv_usec);
> +
> + if (tv_sec < _tv_sec)
> + _tv_sec = tv_sec;
> + if (tv_usec < _tv_usec)
> + _tv_usec = tv_usec;
This updates the timestamp on each page-flip. This means that the first
and last page-flips could be delayed by more than 10µs if there are
intermediate page-flips. For instance, if the first page-flip happens
at t=0µs and the second one happens at t=9µs, the last one could happen
at t=18µs without making this test fails. Is this something we want to
allow?
> +}
> +
> +static void wait_for_pageflip(int drm_fd)
> +{
> + struct pollfd pfd;
> + drmEventContext drm_event;
Nit: to make sure garbage isn't read from this struct (there are other
function pointers), it's probably safer to zero-fill it (= {0}).
> + drm_event.version = 3;
> + drm_event.page_flip_handler2 = page_flip_handler;
> +
> + pfd.fd = drm_fd;
> + pfd.events = POLLIN;
> + pfd.revents = 0;
> +
> + poll(&pfd, 1, 1000);
Maybe we should check poll(2)'s return value?
> + if (pfd.revents & POLLIN)
> + drmHandleEvent(drm_fd, &drm_event);
If we don't get POLLIN (e.g. if we timeout), wait_for_pageflip will
silently do nothing, and the test will pass. Instead, we probably want
to fail in this case.
> +}
> +
> +igt_main
> +{
> + igt_display_t display;
> + data_connector_t *conn_data = NULL;
> + data_t data = {.drm_fd = 0, .num_h_tiles = 0,
> + .display = NULL, .commit = COMMIT_LEGACY};
Nit: one can just do assign to {0} to zero-fill the struct.
> + 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;
> +
> + get_number_of_h_tiles(&data);
> + igt_debug("Number of Horizontal Tiles: %d\n", data.num_h_tiles);
> +
> + data.commit = data.display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY;
Should we try to run this test at all on drivers that don't support
atomic?
If the driver doesn't support atomic, user-space will submit two page-
flip requests. However it's possible that the hardware executes a page-
flip between both requests: user-space calls the first drmModePageFlip,
the vblank for this particular CRTC triggers, then user-space calls the
second drmModePageFlip. Page-flips will be out-of-sync by one frame.
> + if (data.num_h_tiles > 0)
> + conn_data = malloc(data.num_h_tiles * sizeof(data_connector_t));
> + }
> +
> + igt_subtest("basic-test-pattern") {
> + igt_skip_on(data.num_h_tiles == 0);
> + igt_assert(conn_data);
> +
> + get_connector(&data, conn_data);
> + setup_mode(&data, conn_data);
> + setup_framebuffer(&data, conn_data);
> + igt_display_commit_atomic(data.display, DRM_MODE_ATOMIC_NONBLOCK |
> + DRM_MODE_PAGE_FLIP_EVENT, NULL);
> + wait_for_pageflip(data.drm_fd);
Here we only wait for a single page-flip. This will only stash the
timestamp of the frst page-flip without checking the time-stamp of
subsequent page-flips. We should wait for as many page-flips as enabled
tiled connectors.
> + test_cleanup(&data, conn_data);
> + }
> +
> + igt_fixture {
> + free(conn_data);
> + 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',
More information about the igt-dev
mailing list