[igt-dev] [PATCH i-g-t] igt/tests/kms_dp_tiled_display.c : kms test file for display port tiled displays
Ser, Simon
simon.ser at intel.com
Wed Jul 31 07:25:09 UTC 2019
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
>
> > > +{
> > > + 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)
> > > +
> > > + 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?
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