Re: [igt-dev] [PATCH RFC i-g-t] tests/kms_atomic_multi: new test

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 06, 2019 at 02:15:10PM +0000, Ser, Simon wrote:
> Thanks for the review!
> 
> On Thu, 2019-09-05 at 17:51 +0300, Ville Syrjälä wrote:
> > On Thu, Sep 05, 2019 at 05:35:30PM +0300, Simon Ser wrote:
> > > This test performs a single atomic commit on multiple CRTCs at once. The goal
> > > is to check that a page-flip event for each CRTC is received.
> > > 
> > > I chose not to integrate this to an existing test, because kms_atomic is
> > > basically a single function with lots of options. Integrating this test to
> > > kms_atomic would mix up these two and make it difficult to understand the test.
> > > 
> > > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > > ---
> > > 
> > > This is basically the test I wrote while reviewing the kms_dp_tiled_display
> > > series [1]. I'm sending it as an RFC to know if there's any interest in having
> > > it merged.
> > > 
> > > [1]: https://lists.freedesktop.org/archives/igt-dev/2019-August/015724.html
> > > 
> > >  tests/Makefile.sources   |   1 +
> > >  tests/kms_atomic_multi.c | 149 +++++++++++++++++++++++++++++++++++++++
> > >  tests/meson.build        |   1 +
> > >  3 files changed, 151 insertions(+)
> > >  create mode 100644 tests/kms_atomic_multi.c
> > > 
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index c02e4d9489f2..e08968d2b0fc 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -29,6 +29,7 @@ TESTS_progs = \
> > >  	kms_atomic \
> > >  	kms_atomic_interruptible \
> > >  	kms_atomic_transition \
> > > +	kms_atomic_multi \
> > >  	kms_available_modes_crc \
> > >  	kms_big_fb \
> > >  	kms_busy \
> > > diff --git a/tests/kms_atomic_multi.c b/tests/kms_atomic_multi.c
> > > new file mode 100644
> > > index 000000000000..96212ab9a1e6
> > > --- /dev/null
> > > +++ b/tests/kms_atomic_multi.c
> > > @@ -0,0 +1,149 @@
> > > +/*
> > > + * 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.
> > > + */
> > > +
> > > +#include <poll.h>
> > > +#include <drm_mode.h>
> > > +#include <drm_fourcc.h>
> > > +#include "igt.h"
> > > +
> > > +struct test_connector {
> > > +	igt_output_t *output;
> > > +	struct igt_fb fb;
> > > +	drmModeConnectorPtr connector;
> > > +	enum pipe pipe;
> > > +	bool page_flip;
> > > +};
> > > +
> > > +struct test_data {
> > > +	int drm_fd;
> > > +	igt_display_t *display;
> > > +	struct test_connector conns[2];
> > > +};
> > > +
> > > +static void create_fb(struct test_data *data, struct test_connector *conn)
> > > +{
> > > +	igt_plane_t *primary;
> > > +	drmModeModeInfo *mode;
> > > +
> > > +	mode = igt_output_get_mode(conn->output);
> > > +	igt_create_pattern_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > > +			      DRM_FORMAT_XBGR8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > +			      &conn->fb);
> > > +
> > > +	primary = igt_output_get_plane_type(conn->output, DRM_PLANE_TYPE_PRIMARY);
> > > +	igt_assert(primary);
> > > +
> > > +	igt_plane_set_fb(primary, &conn->fb);
> > > +}
> > > +
> > > +static void pick_2_outputs(struct test_data *data)
> > > +{
> > > +	igt_output_t *output;
> > > +	struct test_connector *conn;
> > > +	size_t i;
> > > +
> > > +	i = 0;
> > > +	for_each_connected_output(data->display, output) {
> > > +		if (i >= 2)
> > > +			break;
> > > +
> > > +		conn = &data->conns[i];
> > > +		conn->output = output;
> > > +		conn->pipe = i;
> > 
> > That assumes every output can be driven by any pipe.
> > Using for_each_valid_output_on_pipe() should avoid that
> > assumption.
> > 
> > > +		igt_output_set_pipe(output, conn->pipe);
> > > +		i++;
> > > +	}
> > > +
> > > +	igt_skip_on(i < 2);
> > > +	/* TODO: test_only commit, skip on failure */
> > > +	igt_display_commit_atomic(data->display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > > +}
> > > +
> > > +static void page_flip_handler(int fd, unsigned seq, unsigned tv_sec,
> > > +			      unsigned tv_usec, unsigned crtc_id, void *_data)
> > > +{
> > > +	struct test_data *data = _data;
> > > +	struct test_connector *conn;
> > > +	size_t i;
> > > +
> > > +	for (i = 0; i < sizeof(data->conns) / sizeof(data->conns[0]); i++) {
> > 
> > ARRAY_SIZE()
> > 
> > > +		conn = &data->conns[i];
> > > +		if (data->display->pipes[conn->pipe].crtc_id == crtc_id) {
> > > +			igt_assert_f(!conn->page_flip,
> > > +				     "Got two page-flips for CRTC %u\n",
> > > +				     crtc_id);
> > > +			conn->page_flip = true;
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > > +	igt_assert_f(0, "Got page-flip event for unexpected CRTC %u\n", crtc_id);
> > > +}
> > > +
> > > +igt_main
> > > +{
> > > +	int ret;
> > > +	struct test_data data = {0};
> > > +	igt_display_t display;
> > > +	struct pollfd pfd = {0};
> > > +	drmEventContext drm_event = {0};
> > > +	int i;
> > > +
> > > +	igt_fixture {
> > > +		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> > > +		kmstest_set_vt_graphics_mode();
> > > +		igt_display_require(&display, data.drm_fd);
> > > +		igt_display_reset(&display);
> > > +		igt_require(display.is_atomic);
> > > +		data.display = &display;
> > > +
> > > +		pfd.fd = data.drm_fd;
> > > +		pfd.events = POLLIN;
> > > +
> > > +		drm_event.version = 3;
> > > +		drm_event.page_flip_handler2 = page_flip_handler;
> > > +	}
> > > +
> > > +	igt_subtest("2x-flip") {
> > 
> > Are we planning more subtests? I guess 1x,2x,3x,... might be nice?
> 
> Indeed
> 
> > > +		pick_2_outputs(&data);
> > > +
> > > +		create_fb(&data, &data.conns[0]);
> > > +		create_fb(&data, &data.conns[1]);
> > > +
> > > +		igt_display_commit_atomic(data.display, DRM_MODE_ATOMIC_NONBLOCK |
> > > +					  DRM_MODE_PAGE_FLIP_EVENT, &data);
> > > +
> > > +		/* TODO: handle two events sent at once */
> > > +		for (i = 0; i < 2; i++) {
> > > +			ret = poll(&pfd, 1, 1000);
> > > +			igt_assert(ret == 1);
> > > +			drmHandleEvent(data.drm_fd, &drm_event);
> > 
> > Not verifying that we didn't get too many events?
> 
> This is done in page_flip_handler
> 
> > I guess it might also be nice to have different subtests
> > for page flips and full modesets.
> 
> Yeah, I plan on expanding this test with more subtests
> 
> > I guess there is no generic way to force the kernel to modeset
> > a crtc we didn't explicitly include in the request, so can't
> > really test that scenario :(
> 
> Hmm, not sure I get this scenario. How could we modeset a CRTC not
> included in the atomic request? Can you elaborate?

Current scenarios are:
- cdclk frequency needs to be changed
- ran out of FDI bandwidth on IVB and need to reduce the
  other pipe's contribution

Future scenarios I want to get fixed:
- ran out of MST link bandwidth and need to reduce other pipes'
  contributions

> 
> > > +		}
> > > +	}
> > > +
> > > +	igt_fixture {
> > > +		close(data.drm_fd);
> > > +		kmstest_restore_vt_mode();
> > > +		igt_display_fini(data.display);
> > > +	}
> > > +}
> > > diff --git a/tests/meson.build b/tests/meson.build
> > > index a7b2b3221304..df00ab4a2f81 100644
> > > --- a/tests/meson.build
> > > +++ b/tests/meson.build
> > > @@ -14,6 +14,7 @@ test_progs = [
> > >  	'kms_atomic',
> > >  	'kms_atomic_interruptible',
> > >  	'kms_atomic_transition',
> > > +	'kms_atomic_multi',
> > >  	'kms_available_modes_crc',
> > >  	'kms_big_fb',
> > >  	'kms_busy',
> > > --
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev