[i-g-t] kms_atomic: Measure speed of some plane ioctls
diff mbox

Message ID 1430225183-3606-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New, archived
Headers show

Commit Message

Tvrtko Ursulin April 28, 2015, 12:46 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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)

v3:
   * Simplified a bit for better clarity. (Chris Wilson)
   * Removed internal use comment. (Thomas Wood)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/igt_kms.c          |  19 +++--
 lib/igt_kms.h          |   5 ++
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/kms_atomic.c     | 193 +++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 214 insertions(+), 5 deletions(-)
 create mode 100644 tests/kms_atomic.c

Comments

Chris Wilson April 28, 2015, 12:56 p.m. UTC | #1
On Tue, Apr 28, 2015 at 01:46:23PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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)
> 
> v3:
>    * Simplified a bit for better clarity. (Chris Wilson)
>    * Removed internal use comment. (Thomas Wood)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  lib/igt_kms.c          |  19 +++--
>  lib/igt_kms.h          |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile.sources |   1 +
>  tests/kms_atomic.c     | 193 +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 214 insertions(+), 5 deletions(-)
>  create mode 100644 tests/kms_atomic.c
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 0665d70..308db1e 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1537,14 +1537,23 @@ 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
> + *
>   * Commit position and fb changes to a plane.  The value of @s will determine
>   * which API is used to do the programming.
> + *
> + * This is a low-level function and you are recommened to rather use something
> + * from the igt_display_commit family.
>   */
> -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..5510c9f
> --- /dev/null
> +++ b/tests/kms_atomic.c
> @@ -0,0 +1,193 @@
> +/*
> + * 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, enum igt_commit_style commit)
> +{
> +	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;
> +	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);
> +
> +	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_commit_style commit)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
> +	enum pipe pipe;
> +	unsigned int i;
> +	const unsigned int loops = 20000;
> +	struct timeval start, end;
> +	double e;
> +
> +	if (commit == COMMIT_UNIVERSAL)
> +		igt_require(data->display.has_universal_planes);
> +
> +	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, IGT_PLANE_PRIMARY);
> +
> +			prepare_crtc(data, output, pipe, plane, commit);
> +
> +			gettimeofday(&start, NULL);
> +			for (i = loops; i > 0; i--) {
> +				/*
> +				 * Trick igt_plane_commit into calling either
> +				 * DRM_IOCTL_MODE_SETCRTC or
> +				 * DRM_IOCTL_MODE_SETPLANE, depending on the
> +				 * commit mode.
> +				 *
> +				 * igt_plane_commit is a low-level API which
> +				 * basically just calls one of the two commit
> +				 * paths.
> +				 */
> +				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. */
> +
> +			cleanup_crtc(data, output, plane);
> +
> +			break;

Test each pipe, otherwise people will only fix bugs on pipe 0! From
experience, switching CRTCs tends to ellicit new ang exciting bugs.

Otherwise, lgtm.
-Chris

Patch
diff mbox

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 0665d70..308db1e 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1537,14 +1537,23 @@  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
+ *
  * Commit position and fb changes to a plane.  The value of @s will determine
  * which API is used to do the programming.
+ *
+ * This is a low-level function and you are recommened to rather use something
+ * from the igt_display_commit family.
  */
-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..5510c9f
--- /dev/null
+++ b/tests/kms_atomic.c
@@ -0,0 +1,193 @@ 
+/*
+ * 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, enum igt_commit_style commit)
+{
+	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;
+	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);
+
+	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_commit_style commit)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe pipe;
+	unsigned int i;
+	const unsigned int loops = 20000;
+	struct timeval start, end;
+	double e;
+
+	if (commit == COMMIT_UNIVERSAL)
+		igt_require(data->display.has_universal_planes);
+
+	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, IGT_PLANE_PRIMARY);
+
+			prepare_crtc(data, output, pipe, plane, commit);
+
+			gettimeofday(&start, NULL);
+			for (i = loops; i > 0; i--) {
+				/*
+				 * Trick igt_plane_commit into calling either
+				 * DRM_IOCTL_MODE_SETCRTC or
+				 * DRM_IOCTL_MODE_SETPLANE, depending on the
+				 * commit mode.
+				 *
+				 * igt_plane_commit is a low-level API which
+				 * basically just calls one of the two commit
+				 * paths.
+				 */
+				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. */
+
+			cleanup_crtc(data, output, plane);
+
+			break;
+		}
+	}
+}
+
+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, COMMIT_LEGACY);
+	}
+
+	igt_subtest_f("setplane-speed") {
+		test_commit_speed(&data, COMMIT_UNIVERSAL);
+	}
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}