[igt-dev] [PATCH i-g-t v2 2/2] igt/tests/kms_dp_tiled_display: kms test for display port tiled displays
Manasi Navare
manasi.d.navare at intel.com
Tue Aug 27 21:29:49 UTC 2019
On Tue, Aug 27, 2019 at 03:49:38AM -0700, Ser, Simon wrote:
> 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.
Thanks for the review comments Simon. I will be working on addressing the following
review comments and will take over this patch since Madhumitha's internship has ended.
I really appreciate all your help and feedback on this.
Please find my answers below:
>
> 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.
Freeing prop here before continuing shd fix this, will fix this in the next rev
>
> > + blob = drmModeGetPropertyBlob(data->drm_fd,
> > + conn->prop_values[i]);
> > +
> > + if (!blob)
> > + return;
>
> This branch leaks prop.
Agree, will add drmModeFreePropertyBlob(blob); before return
>
> > + 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
Yes will simplify in the next rev
>
> Additionally, this branch leaks the connector.
Hmm, so call drmModeFreeConnector(connector); before continuing shd be followed in general throughout
the code right?
>
> > + 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.
Yes I agree that will make it more readable, I will fix it in next rev
>
> (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?
yes we just set the pipe/CRTC here and the atomic commit call does a complete
modeset with check and commit without any output since we didnt setup fb yet.
But I agree that calling it just setup_pipe robably makes it more intuitive
>
> > +{
> > + 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?
Yes this is necessary to force a complete modeset for each test since if the pipe
assignments or the mode has not changed it will not set needs_modeset flag in side kernel
and will not do a full atomic check and commit.
>
> > + 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?
>
Yes we only use the tile_h_size of the buffer at the proper offset of tile_h_loc
So this below logic shd be modified to only create fb of the size tile_h_size if its in the for loop
or just create one fb outside the loop of the size tile_h_size * num_tiles and then just use that with dfferent
offsets for two CRTCs
When I discussed this on dri-devel, people said that real userspace can decide to do either one single fb for all tiles
or separate ones where each is of the size equal to tile size.
Let me know if there is a preference
> 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).
I see so somehow have a static count of events and make sure per crtc id its the same count or
some similar logic
>
> > + /* 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.
yes agree will rename to make it more readable
>
> > + 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?
Nope all of them should happen within the same vblank so the diff between all
should be <10us, will have to modify the logic to do that.
so basically instead of making <10us condition for the consecutive ones, it should be between the first one
and all other events, correct?
>
> > +}
> > +
> > +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}).
Got it
>
> > + 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?
poll(2)? Or just check return value of the function poll() just for error handling?
>
> > + 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.
Yes thats correct, just add else case with igt_assert?
>
> > +}
> > +
> > +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.
Got it
>
> > + 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.
Hmm, yea should probbaly have igt_require on atomic
>
> > + 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.
That was our initial understanding as well, but with just one wait_for_pageflip call we
were geting two page flip events for two tiles
Do we need to call wait for page flip explicitly the number of times = number of tiles?
Regards
Manasi
>
> > + 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