[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
Thu Sep 12 12:51:53 UTC 2019
On Wed, 2019-09-11 at 17:47 -0700, Manasi Navare wrote:
> On Fri, Aug 30, 2019 at 04:39:53AM -0700, Ser, Simon wrote:
> > On Tue, 2019-08-27 at 14:29 -0700, Manasi Navare wrote:
> > > 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?
> >
> > Yeah, that would fix it.
> >
> > > > > + 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
> >
> > Oh, I forgot that igt_output_set_pipe also sets the mode.
> >
> > > > > +{
> > > > > + 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.
> >
> > Hmm, why do we need to do a new modeset? If the mode is already set to
> > what we want, no need for this?
> >
> > (Sorry, I'm unfamiliar with the kernel side of things!)
> >
> > > > > + 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
> >
> > As a userspace dev, I'd say a single FB would be a little bit more
> > realistic, but yeah I guess userspace could use two FBs as well. No big
> > deal, I'm fine with both.
> >
> > > > 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
> >
> > Yeah. We could for instance add a "bool got_page_flip" field in our
> > data_connector_t struct and set it to true here (making sure it's not
> > set to true already).
> >
> > A pointer can be passed to page_flip_handler via the data argument.
>
> I didnt understand what you meant by passing a pointer to page_flip_handler
> via the data argument?
> I looked through the drmHandleEvent definition and it populates the void *data
> in page_flip_handler with vblank->user_data
> So its not clear to me how we can pass our data_connector_t -> got_page_flip_bool to page_flip_handler
> and set that to true here.
Hi!
The user_data field is set to the last argument given to
drmModeAtomicCommit (in IGT, to igt_display_commit_atomic). Currently
we set it to NULL, we could set it to the data_connector_t array.
If it helps, here [1] is a very similar test that just atomically page-
flips on multiple CRTCs at once. Note how the page_flip_handler sets
got_page_flip (and makes sure there are no double page-flips). Also
note that the last argument to igt_display_commit_atomic is not NULL
when page-flipping.
[1]: https://patchwork.freedesktop.org/patch/329336/
> I have all the other changes done, so in my revision that I am about to send all the changes except for
> this check to double check number of page flip events == num_h_tiles is missing.
>
> Also i observed that since the events are queued, it calls the page flip handler twice in the first wait_for_page_flip() call
> so even if i loop it the number of times = number of connectors, it calls page flip handler twice for the first loop
> and then doesnt call page flip handler teh second time.
>
> So I am skipping that looping part as well.
Instead of relying on the events being queued and all arriving in just
one poll() call, we can just loop until we got all page-flips. For
instance, this is done in the test I linked before [1]:
while (!got_all_page_flips(&data)) {
ret = poll(&pfd, 1, 1000);
igt_assert(ret == 1);
drmHandleEvent(data.drm_fd, &drm_event);
}
Let me know if something's not clear, or if you think this isn't the
right approach!
> Please let me know your thoughts and I look forward to your review on teh new revision I am sending today.
Thanks!
> Manasi
>
> > > > > + /* 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?
> >
> > Correct! So we can just drop these 4 lines, that should do it.
> >
> > > > > +}
> > > > > +
> > > > > +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?
> >
> > Yes, just in case.
> >
> > (As a side note, poll(2) simply means "the poll system call" and refers
> > to poll's syscall manpage section.)
> >
> > > > > + 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?
> >
> > Yes!
> >
> > > > > +}
> > > > > +
> > > > > +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?
> >
> > Oh, I see. So it seems two events are queued. Then we should probably
> > loop until we have a page-flip for all CRTCs, since there's no
> > guarantee all events will be sent at once.
> >
> > > 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