[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
Thu Jul 18 06:43:48 UTC 2019
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).
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?
> +{
> + 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.
> +
> + 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.
> + 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?
> + }
> +
> + 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