[igt-dev] [PATCH i-g-t] igt/tests/kms_dp_tiled_display.c : kms test file for display port tiled displays
Manasi Navare
manasi.d.navare at intel.com
Thu Aug 1 17:38:13 UTC 2019
Thank Simon for all your inputs. Please find some of my questions below:
On Thu, Aug 01, 2019 at 12:41:37AM -0700, Ser, Simon wrote:
> On Wed, 2019-07-31 at 10:13 -0700, Manasi Navare wrote:
> > On Wed, Jul 31, 2019 at 12:25:09AM -0700, Ser, Simon wrote:
> > > On Tue, 2019-07-30 at 12:21 -0700, Manasi Navare wrote:
> > > > On Wed, Jul 17, 2019 at 11:43:48PM -0700, Ser, Simon wrote:
> > > > > Thanks for your patch! Here are some comments.
> > > > >
> > > > > Style: we use tabs for indentation, not spaces.
> > > > >
> > > > > On Wed, 2019-07-17 at 11:02 -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.
> > > > > >
> > > > > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > > > > > Cc: Petri Latvala <petri.latvala at intel.com>
> > > > > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep at intel.com>
> > > > >
> > > > > Next time, can you send both patches in one go? It's not possible to
> > > > > check whether this test actually passes CI, since build fails (see the
> > > > > two automated failure replies).
> > > >
> > > > Yes please send this as a series with 1st patch that defines the tile parser
> > > > and second with the actual test that uses it
> > > >
> > > > > To learn how to use git-send-email, you can read:
> > > > > https://git-send-email.io/
> > > > >
> > > > > > ---
> > > > > > tests/Makefile.sources | 1 +
> > > > > > tests/kms_dp_tiled_display.c | 239 +++++++++++++++++++++++++++++++++++
> > > > > > tests/meson.build | 1 +
> > > > > > 3 files changed, 241 insertions(+)
> > > > > > create mode 100644 tests/kms_dp_tiled_display.c
> > > > > >
> > > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > > > > index 250dbd33..e9373941 100644
> > > > > > --- a/tests/Makefile.sources
> > > > > > +++ b/tests/Makefile.sources
> > > > > > @@ -40,6 +40,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..af644573
> > > > > > --- /dev/null
> > > > > > +++ b/tests/kms_dp_tiled_display.c
> > > > > > @@ -0,0 +1,239 @@
> > > > > > +/*
> > > > > > + * 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.
> > > > > > + *
> > > > > > + * This test currently supports only horizontally tiled displays, in line with
> > > > > > + * the displays supported by the kernel at the moment.
> > > > > > + */
> > > > > > +
> > > > > > +#include "drm_mode.h"
> > > > > > +#include "drm_fourcc.h"
> > > > > > +#include "igt.h"
> > > > > > +
> > > > > > +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;
> > > > > > + drmModeConnectorPtr connector;
> > > > > > + enum pipe pipe;
> > > > > > + enum igt_commit_style commit;
> > > > > > +} data_connector_t;
> > > > > > +
> > > > > > +static inline int drm_property_is_tile(drmModePropertyPtr prop)
> > > > >
> > > > > I wouldn't personally use `inline`. The compiler is smarter than humans
> > > > > to decide whether or not a function should be inlined.
> > > > >
> > > > > > +{
> > > > > > + return(strcmp(prop->name ? prop->name : "", "TILE")? 0 : 1);
> > > > >
> > > > > Style: unnecessary parentheses, missing space before `?`.
> > > > >
> > > > > > +}
> > > > > > +
> > > > > > +static void get_number_of_h_tiles(data_t *data)
> > > > >
> > > > > What if multiple tiles are connected? Shouldn't we use tile_group_id?
> > > >
> > > > Yes there could be multiple monitors with tiles so use tile_group_id to check
> > > > that the tile belongs to the same group and then use num_h_tiles
> >
> > But actually the current hardware like Madhumitha mentioned only supports one tile group,
> > if we have to support multiple tile groups then we would need to malloc the array of structures
> > for the connectors associated with separate tile groups and then have a separate subtest for
> > tile group 1 tile group 2 etc. This logic will not get tested right now due to HW constraint.
> >
> > So the idea is for the IGT to check the tile group id but only support 1 tile group so far
> > and leave the room for further extending it to multiple tiles instead of over complicating
> > the logic now.
> >
> > Does that make sense or do you think we should already add the subtests for multiple tile groups?
>
> That makes perfect sense to me, support for multiple tile groups can be
> left as a TODO for later.
>
> Incremental patches are way easier to review than one-patch-that-
> implements-all-features-at-once. :)
Great so we will keep the implemnetation to 1 tile group id for now
>
> > > > > > +{
> > > > > > + int i;
> > > > > > + drmModeRes *res;
> > > > > > + drmModeConnector *connector;
> > > > > > + drmModePropertyPtr prop;
> > > > > > + drmModePropertyBlobPtr blob;
> > > > > > + igt_tile_info_t tile;
> > > > > > +
> > > > > > + igt_require(res = drmModeGetResources(data->drm_fd));
> > > > >
> > > > > This should probably be igt_assert.
> > > > >
> > > > > > + for(i = 0; i < res->count_connectors; i++)
> > > > > > + {
> > > > >
> > > > > Style: no newline before `{` (only for functions, applies to the whole
> > > > > patch).
> > > > >
> > > > > > + connector = drmModeGetConnectorCurrent(data->drm_fd, \
> > > > >
> > > > > Style: unnecessary `\`.
> > > > >
> > > > > We could igt_assert(connector).
> > > > >
> > > > > > + res->connectors[i]);
> > > > > > +
> > > > > > + if(!((connector->connection == DRM_MODE_CONNECTED) &&
> > > > > > + (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)))
> > > > > > + continue;
> > > > >
> > > > > Style: we can invert this condition to make clearer.
> > > >
> > > > But wouldnt that add additional nesting?
> > >
> > > I mean, this could be re-written as:
> > >
> > > if (connector->connection != DRM_MODE_CONNECTED ||
> > > connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> >
> > Oh yes that makes perfect sense!
> >
> > > > > > +
> > > > > > + for(i = 0; i < connector->count_props; i++)
> > > > > > + {
> > > > > > + prop = drmModeGetProperty(data->drm_fd, connector->props[i]);
> > > > >
> > > > > We could igt_assert(prop)
> > > > >
> > > > > We need to free it at some point.
> > > > >
> > > > > > + if(!(drm_property_is_tile(prop) &&
> > > > >
> > > > > Style: missing space after if keyword (applies to the whole patch).
> > > > >
> > > > > > + drm_property_type_is(prop, DRM_MODE_PROP_BLOB)))
> > > > >
> > > > > I'd move this check into drm_property_is_tile.
> > > > >
> > > > > > + continue;
> > > > > > +
> > > > > > + blob = drmModeGetPropertyBlob(data->drm_fd, \
> > > > > > + connector->prop_values[i]);
> > > > > > +
> > > > > > + if(!blob)
> > > > > > + break;
> > > > > > + igt_parse_connector_tile_blob(blob, &tile);
> > > > > > + data->num_h_tiles = tile.num_h_tile;
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +static int set_test_pattern_fb(data_t *data, data_connector_t *conn_data)
> > > > > > +{
> > > > > > + int i;
> > > > > > + int count = 0;
> > > > > > + bool is_tile = false;
> > > > > > + enum pipe pipe;
> > > > > > + igt_tile_info_t tile;
> > > > > > + igt_output_t *output;
> > > > > > + igt_plane_t *primary;
> > > > > > + drmModePropertyPtr prop;
> > > > > > + drmModePropertyBlobPtr blob;
> > > > > > +
> > > > > > + for_each_connected_output(data->display, output)
> > > > > > + {
> > > > > > + is_tile = false;
> > > > > > +
> > > > > > + conn_data[count].connector = drmModeGetConnector(data->display->drm_fd, \
> > > > > > + output->id);
> > > > > > +
> > > > > > + if(!(conn_data[count].connector->connector_type ==
> > > > > > + DRM_MODE_CONNECTOR_DisplayPort))
> > > > > > + continue;
> > > > > > +
> > > > > > + /* Parse through each connector property */
> > > > > > + for(i = 0; i < conn_data[count].connector->count_props; i++)
> > > > > > + {
> > > > > > + prop = drmModeGetProperty(data->display->drm_fd, \
> > > > > > + conn_data[count].connector->props[i]);
> > > > > > +
> > > > > > + if(!((drm_property_is_tile(prop) &&
> > > > > > + drm_property_type_is(prop, DRM_MODE_PROP_BLOB))))
> > > > > > + continue;
> > > > > > +
> > > > > > + blob = drmModeGetPropertyBlob(data->display->drm_fd, \
> > > > > > + conn_data[count].connector->prop_values[i]);
> > > > > > +
> > > > > > + if(!blob)
> > > > > > + break;
> > > > > > +
> > > > > > + igt_parse_connector_tile_blob(blob, &tile);
> > > > > > + conn_data[count].tile = &tile;
> > > > > > + is_tile = true;
> > > > > > + break;
> > > > > > + }
> > > > >
> > > > > I feel like this whole block above is duplicated. Could be extracted
> > > > > into a function, e.g.
> > > > >
> > > > > bool connector_get_tile(drmModeConnectorRes *conn, igt_tile_info_t *tile_info)
> > > > >
> > > > > > + if(!is_tile)
> > > > > > + continue;
> > > > > > +
> > > > > > + output = igt_output_from_connector(data->display, \
> > > > > > + conn_data[count].connector);
> > > > >
> > > > > Style: unnecessary `\`. Applies to the whole patch.
> > > > >
> > > > > > +
> > > > > > + for_each_pipe(data->display, pipe)
> > > > > > + {
> > > > > > + igt_debug("Current PIPE is %d\n", pipe);
> > > > > > +
> > > > > > + if((count > 0) && (pipe == conn_data[count-1].pipe))
> > > > > > + continue;
> > > > >
> > > > > Style: unnecessary parentheses.
> > > > >
> > > > > > + 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);
> > > > > > +
> > > > > > + igt_create_pattern_fb(data->drm_fd,\
> > > > > > + (conn_data[count].tile->tile_h_size *\
> > > > > > + data->num_h_tiles),\
> > > > > > + conn_data[count].tile->tile_v_size,\
> > > > > > + DRM_FORMAT_XBGR8888,\
> > > > > > + LOCAL_DRM_FORMAT_MOD_NONE,\
> > > > > > + &conn_data[count].fb_test_pattern);
> > > > > > +
> > > > > > + primary = igt_output_get_plane_type(conn_data[count].output,\
> > > > > > + DRM_PLANE_TYPE_PRIMARY);
> > > > > > +
> > > > > > + igt_plane_set_fb(primary, &conn_data[count].fb_test_pattern);
> > > > > > +
> > > > > > + igt_fb_set_size(&conn_data[count].fb_test_pattern, primary,\
> > > > > > + conn_data[count].tile->tile_h_size,\
> > > > > > + conn_data[count].tile->tile_v_size);
> > > > > > +
> > > > > > + igt_fb_set_position(&conn_data[count].fb_test_pattern, \
> > > > > > + primary,\
> > > > > > + (conn_data[count].tile->tile_h_size *\
> > > > > > + conn_data[count].tile->tile_h_loc),
> > > > > > + (conn_data[count].tile->tile_v_size *\
> > > > > > + conn_data[count].tile->tile_v_loc));
> > > > > > +
> > > > > > + igt_plane_set_size(primary,\
> > > > > > + conn_data[count].tile->tile_h_size,\
> > > > > > + conn_data[count].tile->tile_v_size);
> > > > > > +
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > + count++;
> > > > > > + }
> > > > > > +
> > > > > > + return count;
> > > > > > +}
> > > > > > +
> > > > > > +igt_main
> > > > > > +{
> > > > > > + igt_display_t display;
> > > > > > + data_t data = {};
> > > > > > + data_connector_t *conn_data;
> > > > > > +
> > > > > > + igt_fixture {
> > > > > > + data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> > > > > > + kmstest_set_vt_graphics_mode();
> > > > > > + igt_display_require(&display, data.drm_fd);
> > > > > > + 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;
> > > > > > + }
> > > > > > +
> > > > > > + if(data.num_h_tiles > 0)
> > > > > > + conn_data = (data_connector_t *)malloc(data.num_h_tiles * \
> > > > > > + sizeof(data_connector_t));
> > > > >
> > > > > No need for this cast, unnecessary `\`.
> > > > >
> > > > > > + igt_assert(conn_data);
> > > > >
> > > > > This check reads garbage from the stack if data.num_h_tiles == 0.
> > > >
> > > > This check should possibly be inside the same if
> > > >
> > > > > > + igt_subtest_f("basic-test-pattern") {
> > > > >
> > > > > No need for the _f suffix.
> > > > >
> > > > > This subtest is missing a description. Could you add one? For more
> > > > > information, see:
> > > > > https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe
> > > > >
> > > > > > + igt_skip_on(!data.num_h_tiles);
> > > > > > + set_test_pattern_fb(&data, conn_data);
> > > > > > + igt_display_commit2(data.display, data.commit);
> > > > >
> > > > > I'm not sure I understand what this test checks. It does a commit on
> > > > > all tiled connectors, but doesn't check anything afterwards. Maybe it
> > > > > should check the pageflip serial and timings?
> > > >
> > > > This is so far a manual test where we just look at the pattern and make sure no artifacts
> > > > and auto testing just make sure the commit did not fail, any better idea to check if the
> > > > page_flip timeouts didnt occur or not sure if we can do crc checks to ensure no artifacts?
> > > > Any ideas/suggestions?
> > >
> > > To be honest I'm not a fan of manual testing.
> > >
> > > As I understand it, tiled displays must be updated in sync so that you
> > > don't get tearing. If one display is page-flipped before the other,
> > > there will be tearing in the middle of the tiled screen. Am I reading
> > > the commit message correctly?
> >
> > Yes there would be tearing if one tile page flipped before the other
> > and the HW prevents this from happening by synchronizing the two vblanks
> > from the two ports.
> >
> > If there an example of igt where we can check the vblank timing or page flip timings
> > to add this check?
>
> I believe tests in kms_flip do a similar check. The page_flip_handler
> function handles the page flip event (which is given the flip timestamp
> and sequence number). check_state verifies that the timestamp is close
> enough to what we expect (in our atomic check, maybe we can compare
> with strict equality).
>
> Since we're dealing with multiple CRTCs, it may be a good idea to use
> drmEventContext.version = 3 with page_flip_handler2 so that we also get
> the CRTC in the page flip handler. Version 3 is a core DRM feature, all
> drivers support it given a recent enough kernel.
>
> Rough pseudo-code:
>
> /* When committing buffer, request a page flip */
> drmModeAtomicCommit(fd, req, DRM_MODE_ATOMIC_NONBLOCK |
> DRM_MODE_PAGE_FLIP_EVENT, data);
Here we do the modese with igt_display_commit2 call that then calls drmModeAtomicCommit but
doesnt call it with Page flip, do we have another igt helper that does a atomic commit with
page flip event?
Can we not just do atomic commit and get vblank events so for two crtcs we get two events and compare
the timestamp and make sure they are close enough?
Manasi
>
> static void page_flip_handler(int fd, unsigned seq,
> unsigned tv_sec,
> unsigned tv_usec,
> unsigned crtc_id, void *data)
> {
> /* Compare params with the other page flip */
> }
>
> /* When we get POLLIN in the DRM FD */
> drmEventContext event = {
> .version = 3,
> .page_flip_handler2 = page_flip_handler,
> };
> drmHandleEvent(fd, &event);
>
> > Manasi
> >
> > > If so, maybe we can check the page-flip timings and make sure they are
> > > equal?
> > >
> > > A CRC check might be a good idea too, to make sure bandwidth isn't an
> > > issue.
> > >
> > > CC'ing Martin in case he has better ideas.
> > >
> > > > Manasi
> > > >
> > > > > > + }
> > > > > > +
> > > > > > + igt_fixture {
> > > > > > + if(data.num_h_tiles > 0)
> > > > > > + free(conn_data);
> > > > >
> > > > > If conn_data was initialized to NULL, no need for the check, because
> > > > > free(NULL) is a no-op.
> > > > >
> > > > > > + kmstest_restore_vt_mode();
> > > > > > + igt_display_fini(data.display);
> > > > > > + }
> > > > > > +}
> > > > > > diff --git a/tests/meson.build b/tests/meson.build
> > > > > > index 34a74025..6a6e9ce9 100644
> > > > > > --- a/tests/meson.build
> > > > > > +++ b/tests/meson.build
> > > > > > @@ -25,6 +25,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