[v2,i-g-t] kms_atomic: Measure speed of some plane ioctls

Commit Message

Tvrtko Ursulin April 27, 2015, 3:34 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Measures DRM_IOCTL_MODE_SETCRTC and DRM_IOCTL_MODE_SETPLANE as proxy for
drm_atomic_helper_update_plane if I got it right.

Discovered some slow cursor updates (1.6ms) so needed something to test
different kernel configs etc.

v2:
   * Move to a test case and fail if ioctl takes more than 1ms. (Chris Wilson)
   * Add some kerneldoc to satisfy the form. (Thomas Wood)

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: Chris Wilson <[e-mail]>
---
 lib/igt_kms.c          |  18 +++--
 lib/igt_kms.h          |   5 ++
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/kms_atomic.c     | 195 +++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 215 insertions(+), 5 deletions(-)
 create mode 100644 tests/kms_atomic.c

Comments

Chris Wilson April 27, 2015, 9:20 p.m. UTC | #1

On Mon, Apr 27, 2015 at 04:34:05PM +0100, Tvrtko Ursulin wrote:
> +static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
> +			 igt_plane_t *plane)
> +{
> +	drmModeModeInfo *mode;
> +	igt_display_t *display = &data->display;
> +	int fb_id, fb_modeset_id;
> +	unsigned int w, h;
> +	uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
> +	uint32_t pixel_format = DRM_FORMAT_XRGB8888;
> +	enum igt_commit_style commit = COMMIT_LEGACY;
> +	igt_plane_t *primary;
> +
> +	igt_output_set_pipe(output, pipe);
> +
> +	mode = igt_output_get_mode(output);
> +
> +	w = mode->hdisplay;
> +	h = mode->vdisplay;
> +
> +	fb_modeset_id = igt_create_fb(data->gfx_fd,
> +				      w, h,
> +				      pixel_format,
> +				      tiling,
> +				      &data->fb_modeset);
> +	igt_assert(fb_modeset_id);
> +
> +	/*
> +	 * With igt_display_commit2 and COMMIT_UNIVERSAL, we call just the
> +	 * setplane without a modeset. So, to be able to call
> +	 * igt_display_commit and ultimately setcrtc to do the first modeset,
> +	 * we create an fb covering the crtc and call commit
> +	 */
> +
> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> +	igt_plane_set_fb(primary, &data->fb_modeset);
> +	igt_display_commit(display);
> +
> +	fb_id = igt_create_fb(data->gfx_fd,
> +			      w, h,
> +			      pixel_format,
> +			      tiling,
> +			      &data->fb);
> +	igt_assert(fb_id);
> +
> +	igt_plane_set_fb(plane, &data->fb);
> +

Please do commit = COMMIT_LEGACY here so I don't have to scan backwads.
> +	if (plane->is_primary || plane->is_cursor) {
> +		igt_require(data->display.has_universal_planes);
> +		commit = COMMIT_UNIVERSAL;
> +	}
> +
> +	igt_display_commit2(display, commit);
> +}
> +
> +static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
> +{
> +	igt_display_t *display = &data->display;
> +
> +	igt_remove_fb(data->gfx_fd, &data->fb);
> +	igt_remove_fb(data->gfx_fd, &data->fb_modeset);
> +
> +	/* XXX: see the note in prepare_crtc() */
> +	if (!plane->is_primary) {
> +		igt_plane_t *primary;
> +
> +		primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> +		igt_plane_set_fb(primary, NULL);
> +	}
> +
> +	igt_plane_set_fb(plane, NULL);
> +	igt_output_set_pipe(output, PIPE_ANY);
> +
> +	igt_display_commit(display);
> +}
> +
> +static double elapsed(const struct timeval *start,
> +		      const struct timeval *end,
> +		      int loop)
> +{
> +	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
> +}
> +
> +static void test_commit_speed(data_t *data, enum igt_plane plane_type)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
> +	enum pipe pipe;
> +	enum igt_commit_style commit = COMMIT_LEGACY;
> +	unsigned int i;
> +	const unsigned int loops = 10000;
> +	struct timeval start, end;
> +	double e;
> +

Same comment for commit = COMMIT_LEGACY here, mainly now for consistency
with before.
> +	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
> +		igt_require(data->display.has_universal_planes);
> +		commit = COMMIT_UNIVERSAL;
> +	}
> +
> +	for_each_connected_output(display, output) {
> +		for_each_pipe(display, pipe) {
> +			igt_plane_t *plane;
> +
> +			igt_output_set_pipe(output, pipe);
> +			plane = igt_output_get_plane(output, plane_type);
> +
> +			prepare_crtc(data, output, pipe, plane);
> +
> +			igt_display_commit2(display, commit);
> +
> +			gettimeofday(&start, NULL);
> +			for (i = loops; i > 0; i--) {
> +				plane->position_changed = true;
> +				igt_plane_commit(plane, output, commit, true);
> +			}
> +			gettimeofday(&end, NULL);
> +			e = elapsed(&start, &end, loops);
> +			igt_info("Pipe: %u Plane: %u Time: %7.3fµs\n",
> +				 pipe, plane->index, e);
> +			igt_require(e < 1000); /* 1ms, just so. */
> +
> +			kmstest_restore_vt_mode();
> +			kmstest_set_vt_graphics_mode();

Do you really mean to switch VT between text/gfx on every display/pipe?
Why?

> +
> +			cleanup_crtc(data, output, plane);
> +		}
> +	}
> +}

Looks like magic to me. Is this enough variation to flush out future
bugs?
-Chris

Tvrtko Ursulin April 28, 2015, 9:18 a.m. UTC | #2

On 04/27/2015 10:20 PM, Chris Wilson wrote:
> On Mon, Apr 27, 2015 at 04:34:05PM +0100, Tvrtko Ursulin wrote:
>> +static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
>> +			 igt_plane_t *plane)
>> +{
>> +	drmModeModeInfo *mode;
>> +	igt_display_t *display = &data->display;
>> +	int fb_id, fb_modeset_id;
>> +	unsigned int w, h;
>> +	uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
>> +	uint32_t pixel_format = DRM_FORMAT_XRGB8888;
>> +	enum igt_commit_style commit = COMMIT_LEGACY;
>> +	igt_plane_t *primary;
>> +
>> +	igt_output_set_pipe(output, pipe);
>> +
>> +	mode = igt_output_get_mode(output);
>> +
>> +	w = mode->hdisplay;
>> +	h = mode->vdisplay;
>> +
>> +	fb_modeset_id = igt_create_fb(data->gfx_fd,
>> +				      w, h,
>> +				      pixel_format,
>> +				      tiling,
>> +				      &data->fb_modeset);
>> +	igt_assert(fb_modeset_id);
>> +
>> +	/*
>> +	 * With igt_display_commit2 and COMMIT_UNIVERSAL, we call just the
>> +	 * setplane without a modeset. So, to be able to call
>> +	 * igt_display_commit and ultimately setcrtc to do the first modeset,
>> +	 * we create an fb covering the crtc and call commit
>> +	 */
>> +
>> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
>> +	igt_plane_set_fb(primary, &data->fb_modeset);
>> +	igt_display_commit(display);
>> +
>> +	fb_id = igt_create_fb(data->gfx_fd,
>> +			      w, h,
>> +			      pixel_format,
>> +			      tiling,
>> +			      &data->fb);
>> +	igt_assert(fb_id);
>> +
>> +	igt_plane_set_fb(plane, &data->fb);
>> +
>
> Please do commit = COMMIT_LEGACY here so I don't have to scan backwads.

Yeah makes sense, too much copy&paste.

>> +	if (plane->is_primary || plane->is_cursor) {
>> +		igt_require(data->display.has_universal_planes);
>> +		commit = COMMIT_UNIVERSAL;
>> +	}
>> +
>> +	igt_display_commit2(display, commit);
>> +}
>> +
>> +static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
>> +{
>> +	igt_display_t *display = &data->display;
>> +
>> +	igt_remove_fb(data->gfx_fd, &data->fb);
>> +	igt_remove_fb(data->gfx_fd, &data->fb_modeset);
>> +
>> +	/* XXX: see the note in prepare_crtc() */
>> +	if (!plane->is_primary) {
>> +		igt_plane_t *primary;
>> +
>> +		primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
>> +		igt_plane_set_fb(primary, NULL);
>> +	}
>> +
>> +	igt_plane_set_fb(plane, NULL);
>> +	igt_output_set_pipe(output, PIPE_ANY);
>> +
>> +	igt_display_commit(display);
>> +}
>> +
>> +static double elapsed(const struct timeval *start,
>> +		      const struct timeval *end,
>> +		      int loop)
>> +{
>> +	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
>> +}
>> +
>> +static void test_commit_speed(data_t *data, enum igt_plane plane_type)
>> +{
>> +	igt_display_t *display = &data->display;
>> +	igt_output_t *output;
>> +	enum pipe pipe;
>> +	enum igt_commit_style commit = COMMIT_LEGACY;
>> +	unsigned int i;
>> +	const unsigned int loops = 10000;
>> +	struct timeval start, end;
>> +	double e;
>> +
>
> Same comment for commit = COMMIT_LEGACY here, mainly now for consistency
> with before.

Here we want to be able to choose between commit types if we want to 
keep two subtests.

>> +	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
>> +		igt_require(data->display.has_universal_planes);
>> +		commit = COMMIT_UNIVERSAL;
>> +	}
>> +
>> +	for_each_connected_output(display, output) {
>> +		for_each_pipe(display, pipe) {
>> +			igt_plane_t *plane;
>> +
>> +			igt_output_set_pipe(output, pipe);
>> +			plane = igt_output_get_plane(output, plane_type);
>> +
>> +			prepare_crtc(data, output, pipe, plane);
>> +
>> +			igt_display_commit2(display, commit);
>> +
>> +			gettimeofday(&start, NULL);
>> +			for (i = loops; i > 0; i--) {
>> +				plane->position_changed = true;
>> +				igt_plane_commit(plane, output, commit, true);
>> +			}
>> +			gettimeofday(&end, NULL);
>> +			e = elapsed(&start, &end, loops);
>> +			igt_info("Pipe: %u Plane: %u Time: %7.3fµs\n",
>> +				 pipe, plane->index, e);
>> +			igt_require(e < 1000); /* 1ms, just so. */
>> +
>> +			kmstest_restore_vt_mode();
>> +			kmstest_set_vt_graphics_mode();
>
> Do you really mean to switch VT between text/gfx on every display/pipe?
> Why?

Because copy&paste is so easy. :)

>> +
>> +			cleanup_crtc(data, output, plane);
>> +		}
>> +	}
>> +}
>
> Looks like magic to me. Is this enough variation to flush out future
> bugs?

On which part does this apply?

Regards,

Tvrtko

Chris Wilson April 28, 2015, 9:27 a.m. UTC | #3

On Tue, Apr 28, 2015 at 10:18:53AM +0100, Tvrtko Ursulin wrote:
> On 04/27/2015 10:20 PM, Chris Wilson wrote:
> >Same comment for commit = COMMIT_LEGACY here, mainly now for consistency
> >with before.
> 
> Here we want to be able to choose between commit types if we want to
> keep two subtests.

I just meant to have the assignment closer to where we choose between
LEGACY/UNIVERSAL.

> >Looks like magic to me. Is this enough variation to flush out future
> >bugs?
> 
> On which part does this apply?

The igt/kms interface is like magic and seems to me hides a lot of
details, so I am not really sure what the test does.

The second part was just wondering if coverage is wide enough to catch
bugs like panning being synchronous, or cursor images, or cursor size,
or sprite position/size, etc.
-Chris

Patch

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index b7d1e90..12948f9 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1537,14 +1537,22 @@  static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 }
 
 
-/*
+/**
+ * igt_plane_commit:
+ * @plane: plane to commit
+ * @output: output
+ * @s: commit style, legacy or univeral
+ * @fail_on_error: fail or just return an error code
+ *
+ * ******* INTERNAL USE ONLY - DO NOT  USE *******
+ *
  * Commit position and fb changes to a plane.  The value of @s will determine
  * which API is used to do the programming.
  */
-static int igt_plane_commit(igt_plane_t *plane,
-			    igt_output_t *output,
-			    enum igt_commit_style s,
-			    bool fail_on_error)
+int igt_plane_commit(igt_plane_t *plane,
+		     igt_output_t *output,
+		     enum igt_commit_style s,
+		     bool fail_on_error)
 {
 	if (plane->is_cursor && s == COMMIT_LEGACY) {
 		return igt_cursor_commit_legacy(plane, output, fail_on_error);
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 09c08aa..a4dd30b 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -267,6 +267,11 @@  void igt_fb_set_size(struct igt_fb *fb, igt_plane_t *plane,
 
 void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
 
+int igt_plane_commit(igt_plane_t *plane,
+		     igt_output_t *output,
+		     enum igt_commit_style s,
+		     bool fail_on_error);
+
 #define for_each_connected_output(display, output)		\
 	for (int i__ = 0;  i__ < (display)->n_outputs; i__++)	\
 		if ((output = &(display)->outputs[i__]), output->valid)
diff --git a/tests/.gitignore b/tests/.gitignore
index 796e330..7b479d3 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -124,6 +124,7 @@  gen3_render_tiledy_blits
 gen7_forcewake_mt
 kms_3d
 kms_addfb
+kms_atomic
 kms_cursor_crc
 kms_fbc_crc
 kms_fence_pin_leak
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 4cbc50d..feb84eb 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -59,6 +59,7 @@  TESTS_progs_M = \
 	gem_userptr_blits \
 	gem_write_read_ring_switch \
 	kms_addfb \
+	kms_atomic \
 	kms_cursor_crc \
 	kms_fbc_crc \
 	kms_flip \
diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
new file mode 100644
index 0000000..f807ab4
--- /dev/null
+++ b/tests/kms_atomic.c
@@ -0,0 +1,195 @@ 
+/*
+ * Copyright © 2015 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 <math.h>
+#include <sys/time.h>
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "igt_core.h"
+#include "intel_chipset.h"
+
+typedef struct {
+	int gfx_fd;
+	igt_display_t display;
+	struct igt_fb fb;
+	struct igt_fb fb_modeset;
+} data_t;
+
+
+static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
+			 igt_plane_t *plane)
+{
+	drmModeModeInfo *mode;
+	igt_display_t *display = &data->display;
+	int fb_id, fb_modeset_id;
+	unsigned int w, h;
+	uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
+	uint32_t pixel_format = DRM_FORMAT_XRGB8888;
+	enum igt_commit_style commit = COMMIT_LEGACY;
+	igt_plane_t *primary;
+
+	igt_output_set_pipe(output, pipe);
+
+	mode = igt_output_get_mode(output);
+
+	w = mode->hdisplay;
+	h = mode->vdisplay;
+
+	fb_modeset_id = igt_create_fb(data->gfx_fd,
+				      w, h,
+				      pixel_format,
+				      tiling,
+				      &data->fb_modeset);
+	igt_assert(fb_modeset_id);
+
+	/*
+	 * With igt_display_commit2 and COMMIT_UNIVERSAL, we call just the
+	 * setplane without a modeset. So, to be able to call
+	 * igt_display_commit and ultimately setcrtc to do the first modeset,
+	 * we create an fb covering the crtc and call commit
+	 */
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+	igt_plane_set_fb(primary, &data->fb_modeset);
+	igt_display_commit(display);
+
+	fb_id = igt_create_fb(data->gfx_fd,
+			      w, h,
+			      pixel_format,
+			      tiling,
+			      &data->fb);
+	igt_assert(fb_id);
+
+	igt_plane_set_fb(plane, &data->fb);
+
+	if (plane->is_primary || plane->is_cursor) {
+		igt_require(data->display.has_universal_planes);
+		commit = COMMIT_UNIVERSAL;
+	}
+
+	igt_display_commit2(display, commit);
+}
+
+static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
+{
+	igt_display_t *display = &data->display;
+
+	igt_remove_fb(data->gfx_fd, &data->fb);
+	igt_remove_fb(data->gfx_fd, &data->fb_modeset);
+
+	/* XXX: see the note in prepare_crtc() */
+	if (!plane->is_primary) {
+		igt_plane_t *primary;
+
+		primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+		igt_plane_set_fb(primary, NULL);
+	}
+
+	igt_plane_set_fb(plane, NULL);
+	igt_output_set_pipe(output, PIPE_ANY);
+
+	igt_display_commit(display);
+}
+
+static double elapsed(const struct timeval *start,
+		      const struct timeval *end,
+		      int loop)
+{
+	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
+}
+
+static void test_commit_speed(data_t *data, enum igt_plane plane_type)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe pipe;
+	enum igt_commit_style commit = COMMIT_LEGACY;
+	unsigned int i;
+	const unsigned int loops = 10000;
+	struct timeval start, end;
+	double e;
+
+	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
+		igt_require(data->display.has_universal_planes);
+		commit = COMMIT_UNIVERSAL;
+	}
+
+	for_each_connected_output(display, output) {
+		for_each_pipe(display, pipe) {
+			igt_plane_t *plane;
+
+			igt_output_set_pipe(output, pipe);
+			plane = igt_output_get_plane(output, plane_type);
+
+			prepare_crtc(data, output, pipe, plane);
+
+			igt_display_commit2(display, commit);
+
+			gettimeofday(&start, NULL);
+			for (i = loops; i > 0; i--) {
+				plane->position_changed = true;
+				igt_plane_commit(plane, output, commit, true);
+			}
+			gettimeofday(&end, NULL);
+			e = elapsed(&start, &end, loops);
+			igt_info("Pipe: %u Plane: %u Time: %7.3fµs\n",
+				 pipe, plane->index, e);
+			igt_require(e < 1000); /* 1ms, just so. */
+
+			kmstest_restore_vt_mode();
+			kmstest_set_vt_graphics_mode();
+
+			cleanup_crtc(data, output, plane);
+		}
+	}
+}
+
+igt_main
+{
+	data_t data = {};
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.gfx_fd = drm_open_any_master();
+
+		kmstest_set_vt_graphics_mode();
+
+		igt_display_init(&data.display, data.gfx_fd);
+	}
+
+	igt_subtest_f("setcrtc-speed") {
+		test_commit_speed(&data, IGT_PLANE_PRIMARY);
+	}
+
+	igt_subtest_f("setplane-speed") {
+		test_commit_speed(&data, IGT_PLANE_2);
+	}
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}