Re: [igt-dev] [PATCH i-g-t] igt/tests/kms_dp_tiled_display.c : kms test file for display port tiled displays

Tolakanahalli Pradeep, Madhumitha
2019-07-31T00:42:44Z

Thank you very much for your comments, Simon.

Thanks for pointing out the style realted issues, I have them
fixed (tabs, `\` etc).

On Thu, 2019-07-18 at 07:43 +0100, 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@intel.com>
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > Signed-off-by: Madhumitha Tolakanahalli Pradeep <
> > madhumitha.tolakanahalli.pradeep@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).

My apologies, I shall keep that in mind for the next time.

> 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@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.
>

Makes sense, I shall remove 'inline'.

> > +{
> > + 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?

Currently, the hardware does not support displays that are connected to
multiple tiles. Only connectors with the same tile_group_id are
supported for tiled monitors.
I was thinking of adding a check for making sure both the connected DP
connectors belong to the same tile group as that make the test more
robust. Any suggestions for the same?

>
>
> > +{
> > + 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.

Yes, makes sense. I shall update it for rev2.

>
> > + 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).

Yes, makes sense. I shall update it for rev2.

>
> > + 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.

This was done to reduce nesting. Would you suggest we do otherwise?

>
> > +
> > + for(i = 0; i < connector->count_props; i++)
> > + {
> > + prop = drmModeGetProperty(data->drm_fd, connector-
> > >props[i]);
>
> We could igt_assert(prop)

Yes, makes sense. I shall update it for rev2.

> We need to free it at some point.

'prop' is a local pointer that isnt dynamically allocated. Not too sure
why that would need free-ing. Could you please provide more clarity on
the above comment?

>
> > + 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.

Yes, makes sense. I shall update it for rev2.

>
> > + 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.

I was contemplating on that too, shall do it for rev2.

> 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].connec
> > tor);
>
> 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_pat
> > tern);
> > +
> > + 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_patt
> > ern, \
> > + 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_connec
> > tor_t));
>
> No need for this cast, unnecessary `\`.
>
> > + igt_assert(conn_data);
>
> This check reads garbage from the stack if data.num_h_tiles == 0.

I moved the assert to igt_subtest block after igt_skip_on so that if
data.num_h_tiles == 0, assert would not be performed.

>
> > + igt_subtest_f("basic-test-pattern") {
>
> No need for the _f suffix.

Yes, makes sense. I shall update it for rev2.

>
> 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

I shall add a description.

>
> > + 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 test was meant to make sure the transcoder port sync feature was
working without any offsets in the two tiles of the display. Any
suggestions/resources for how to test pageflips?

>
> > + }
> > +
> > + 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.

Yes, makes sense. I shall update it for rev2.

>
> > + 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',