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

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

[Tvrtko Ursulin]

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/.gitignore       |   1 +
 benchmarks/Makefile.sources |   3 +-
 benchmarks/kms_atomic.c     | 193 ++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_kms.c               |   8 +-
 lib/igt_kms.h               |   5 ++
 5 files changed, 205 insertions(+), 5 deletions(-)
 create mode 100644 benchmarks/kms_atomic.c

diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore
index 09e5bd8..9a0b8af 100644
--- a/benchmarks/.gitignore
+++ b/benchmarks/.gitignore
@@ -3,4 +3,5 @@ intel_upload_blit_large
 intel_upload_blit_large_gtt
 intel_upload_blit_large_map
 intel_upload_blit_small
+kms_atomic
 # Please keep sorted alphabetically
diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
index 60bdae2..25557d7 100644
--- a/benchmarks/Makefile.sources
+++ b/benchmarks/Makefile.sources
@@ -3,4 +3,5 @@ bin_PROGRAMS =                          \
 	intel_upload_blit_large_gtt     \
 	intel_upload_blit_large_map     \
 	intel_upload_blit_small		\
-	gem_userptr_benchmark
+	gem_userptr_benchmark		\
+	kms_atomic
diff --git a/benchmarks/kms_atomic.c b/benchmarks/kms_atomic.c
new file mode 100644
index 0000000..c758f66
--- /dev/null
+++ b/benchmarks/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)
+{
+	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 = 1000;
+	struct timeval start, end;
+
+	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, false);
+			}
+			gettimeofday(&end, NULL);
+			igt_info("Pipe: %u Plane: %u Time: %7.3fµs\n",
+				 pipe, plane->index,
+				 elapsed(&start, &end, loops));
+
+			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);
+	}
+}
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 14abae8..151b157 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1488,10 +1488,10 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
  * 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 565df14..58f7a32 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -255,6 +255,11 @@ void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation);
 
 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)
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] kms_atomic: Measure speed of some plane ioctls

[Tvrtko Ursulin]

On 04/24/2015 04:27 PM, 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.

Disabling all kernel debugging options brings this down to ~40us.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] kms_atomic: Measure speed of some plane ioctls

[Chris Wilson]

On Fri, Apr 24, 2015 at 04:27:58PM +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.

Looks good. Can you cook this up into a real test to fail if these
suddenly take a vblank? As well as invent a foolproot regression
benchmark infrastucture :) 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] kms_atomic: Measure speed of some plane ioctls

[Thomas Wood]

On 24 April 2015 at 16:27, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> 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.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  benchmarks/.gitignore       |   1 +
>  benchmarks/Makefile.sources |   3 +-
>  benchmarks/kms_atomic.c     | 193 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_kms.c               |   8 +-
>  lib/igt_kms.h               |   5 ++
>  5 files changed, 205 insertions(+), 5 deletions(-)
>  create mode 100644 benchmarks/kms_atomic.c
>
> diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore
> index 09e5bd8..9a0b8af 100644
> --- a/benchmarks/.gitignore
> +++ b/benchmarks/.gitignore
> @@ -3,4 +3,5 @@ intel_upload_blit_large
>  intel_upload_blit_large_gtt
>  intel_upload_blit_large_map
>  intel_upload_blit_small
> +kms_atomic
>  # Please keep sorted alphabetically
> diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
> index 60bdae2..25557d7 100644
> --- a/benchmarks/Makefile.sources
> +++ b/benchmarks/Makefile.sources
> @@ -3,4 +3,5 @@ bin_PROGRAMS =                          \
>         intel_upload_blit_large_gtt     \
>         intel_upload_blit_large_map     \
>         intel_upload_blit_small         \
> -       gem_userptr_benchmark
> +       gem_userptr_benchmark           \
> +       kms_atomic
> diff --git a/benchmarks/kms_atomic.c b/benchmarks/kms_atomic.c
> new file mode 100644
> index 0000000..c758f66
> --- /dev/null
> +++ b/benchmarks/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"

Although this isn't a test case, it might still be nice to add a short
description for it using the IGT_TEST_DESCRIPTION macro.


> +
> +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 = 1000;
> +       struct timeval start, end;
> +
> +       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, false);
> +                       }
> +                       gettimeofday(&end, NULL);
> +                       igt_info("Pipe: %u Plane: %u Time: %7.3fµs\n",
> +                                pipe, plane->index,
> +                                elapsed(&start, &end, loops));
> +
> +                       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);
> +       }
> +}
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 14abae8..151b157 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1488,10 +1488,10 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>   * 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)

Since igt_plane_commit is now public, could you add some API
documentation for it here?


> +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 565df14..58f7a32 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -255,6 +255,11 @@ void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation);
>
>  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)
> --
> 2.3.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 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

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);
+	}
+}
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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;
+		}
+	}
+}
+
+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);
+	}
+}
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] kms_atomic: Measure speed of some plane ioctls

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx