Re: [igt-dev] [PATCH i-g-t] tests/kms_multipipe_modeset: Add test to validate max pipe configuration

[Petri Latvala <petri.latvala@intel.com>]

On Thu, Apr 23, 2020 at 03:20:34PM +0530, Karthik B S wrote:
> Thanks a lot for the review.
> 
> On 4/22/2020 1:47 PM, Petri Latvala wrote:
> > On Mon, Apr 13, 2020 at 03:51:44PM +0530, Karthik B S wrote:
> > > Added test to validate max pipe configuration for a platform.
> > > In the test, the reference CRC is collected first by doing modeset
> > > on all the outputs individually.Then a simultaneous modeset is
> > > done on all the outputs and the CRC is collected. This is compared
> > > with the reference CRC.
> > > 
> > > If the number of outputs connected is less than the number of pipes
> > > supported by the platform, then the test skips.
> > > 
> > > This test is added to verify if the max pipe configuration for a
> > > given platform is working fine. Even though there are other tests
> > > which test multipipe configuration, they test some other functionalities
> > > as well together with multipipe. This is a stand alone test that
> > > intends to only verify simultaneous modeset at the max pipe configuration.
> > > 
> > > Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> > > ---
> > >   tests/Makefile.sources        |   1 +
> > >   tests/kms_multipipe_modeset.c | 180 ++++++++++++++++++++++++++++++++++
> > >   tests/meson.build             |   1 +
> > >   3 files changed, 182 insertions(+)
> > >   create mode 100644 tests/kms_multipipe_modeset.c
> > > 
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index 4e44c98c..903c5671 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -60,6 +60,7 @@ TESTS_progs = \
> > >   	kms_lease \
> > >   	kms_legacy_colorkey \
> > >   	kms_mmap_write_crc \
> > > +	kms_multipipe_modeset \
> > >   	kms_panel_fitting \
> > >   	kms_pipe_b_c_ivb \
> > >   	kms_pipe_crc_basic \
> > > diff --git a/tests/kms_multipipe_modeset.c b/tests/kms_multipipe_modeset.c
> > > new file mode 100644
> > > index 00000000..11328165
> > > --- /dev/null
> > > +++ b/tests/kms_multipipe_modeset.c
> > > @@ -0,0 +1,180 @@
> > > +/*
> > > + * Copyright © 2020 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.
> > > + *
> > > + * Author:
> > > + *  Karthik B S <karthik.b.s@intel.com>
> > > + */
> > > +
> > > +#include "igt.h"
> > > +
> > > +IGT_TEST_DESCRIPTION("Test simultaneous modeset on all the supported pipes");
> > > +
> > > +typedef struct {
> > > +	int drm_fd;
> > > +	igt_display_t display;
> > > +	struct igt_fb fb;
> > > +} data_t;
> > > +
> > > +static void run_test(data_t *data, int valid_outputs)
> > > +{
> > > +	igt_output_t *output;
> > > +	igt_pipe_crc_t *pipe_crcs[IGT_MAX_PIPES] = { 0 };
> > > +	igt_crc_t ref_crcs[IGT_MAX_PIPES], new_crcs[IGT_MAX_PIPES];
> > > +	igt_display_t *display = &data->display;
> > > +	int width = 0, height = 0, i, count = 0;
> > > +	int output_to_pipe[valid_outputs];
> > > +	igt_pipe_t *pipe;
> > > +	igt_plane_t *plane;
> > > +	drmModeModeInfo *mode;
> > > +
> > > +	for_each_connected_output(display, output) {
> > > +		mode = igt_output_get_mode(output);
> > > +		igt_assert(mode);
> > > +
> > > +		igt_output_set_pipe(output, PIPE_NONE);
> > > +
> > > +		width = max(width, mode->hdisplay);
> > > +		height = max(height, mode->vdisplay);
> > > +	}
> > > +
> > > +	igt_create_pattern_fb(data->drm_fd, width, height, DRM_FORMAT_XRGB8888,
> > > +			      LOCAL_DRM_FORMAT_MOD_NONE, &data->fb);
> > > +
> > > +	/* Make pipe-ouput mapping which will be used unchanged across the test */
> > 
> > Typo, pipe-output
> > 
> Noted. I'll fix this.
> > 
> > > +	for_each_pipe(display, i) {
> > > +		count = 0;
> > > +		for_each_connected_output(display, output) {
> > > +			if (igt_pipe_connector_valid(i, output) &&
> > > +			    output->pending_pipe == PIPE_NONE) {
> > > +				igt_output_set_pipe(output, i);
> > > +				output_to_pipe[count] = i;
> > > +				break;
> > > +			}
> > > +			count++;
> > > +		}
> > > +	}
> > > +
> > > +	/* Collect reference CRC by Committing individually on all outputs*/
> > > +	for_each_pipe(display, i) {
> > > +		pipe = &display->pipes[i];
> > > +		plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> > > +
> > > +		mode = NULL;
> > > +
> > > +		if (is_i915_device(display->drm_fd))
> > > +			pipe_crcs[i] = igt_pipe_crc_new(display->drm_fd, i,
> > > +							INTEL_PIPE_CRC_SOURCE_AUTO);
> > > +
> > > +		count = 0;
> > > +		for_each_connected_output(display, output) {
> > > +			if (output_to_pipe[count] == pipe->pipe) {
> > > +				igt_output_set_pipe(output, i);
> > > +				mode = igt_output_get_mode(output);
> > > +				igt_assert(mode);
> > > +			} else
> > > +				igt_output_set_pipe(output, PIPE_NONE);
> > > +			count++;
> > > +		}
> > > +
> > > +		igt_plane_set_fb(plane, &data->fb);
> > > +		igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> > > +		igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> > > +
> > > +		igt_display_commit2(display, COMMIT_ATOMIC);
> > > +		igt_pipe_crc_collect_crc(pipe_crcs[i], &ref_crcs[i]);
> > 
> > This will crash on non-i915. igt_pipe_crc_collect_crc needs a non-NULL pipe_crc.
> > 
> Could you please suggest how I can handle this?
> Does it mean I'll have to make the test Intel specific?

No, you just need to make sure i915 and non-i915 take non-crashing paths.

If the test is absolutely useless without crc support, do the crc handling
unconditionally, but call igt_require_pipe_crc() first.

If the test is somewhat useful without crc support, add the
is_i915_device() check for both the creation of the object and the
collecting as well. Although the check should be something like
igt_has_pipe_crc(), which doesn't exist...

I prefer the igt_require_pipe_crc() option myself.


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev