[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 08:15:45 UTC 2019
On Tue, 2019-07-30 at 17:42 -0700, Tolakanahalli Pradeep, Madhumitha wrote:
> 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 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).
>
> 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 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.
> >
>
> 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?
Yes. (See Manasi's reply)
> >
> > > +{
> > > + 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?
(See my reply to Manasi)
> > > +
> > > + 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?
drmModeGetProperty allocates on the heap.
<rant>
This is somehow hidden because libdrm had the brilliant idea to
introduce drmModePropertyPtr, which is a type definition for
drmModePropertyRes *. I really don't get what's the point of
drmModePropertyPtr. Its only purpose seems to confuse people.
drmModePropertyRes * is much clearer since we don't have to learn about
a libdrm-specific unidiomatic naming convention to know it's a pointer.
It's clear a function returning thing * allocates something that needs
to be free'd, it's not clear if the function returns otherThing.
Personally I just ignore libdrm weird definitions and just use
drmModePropertyRes *.
</rant>
Anyway, drmModeFreeProperty will free prop.
> > > + 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?
(See the discussion with 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.
>
> 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',
More information about the igt-dev
mailing list