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

Wed Jul 31 00:42:43 UTC 2019

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?

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