adding suspend_autoresume on primary tests like this:
@ -470,6 +472,8 @@ igt_main
data.test_plane = PRIMARY;
data.op = op;
run_test(&data);
+ igt_system_suspend_autoresume();
+ run_test(&data);
on BDW I got these results:
vivijim rdvivi-seattle tests$ sudo ./kms_psr_sink_crc
IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64)
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:44:03 2014
Subtest primary_page_flip: SUCCESS
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:44:40 2014
Subtest primary_mmap_gtt: SUCCESS
Waiting 10s...
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:45:27 2014
Waiting 10s...
Subtest primary_mmap_gtt_waiting: SUCCESS
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:46:13 2014
Subtest primary_mmap_cpu: SUCCESS
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:46:50 2014
Subtest primary_blt: SUCCESS
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:47:27 2014
Subtest primary_render: SUCCESS
on HSW I couldn't test because suspend/resume breaks even with psr disabled.
I'm going to check more tomorrow..
But regarding the suspend resume test, how do you suggest to organize it?
Extra loops for all current cases?
suspend_{primary, sprite, cursor}_{page_flip, mmap_gtt, etc}? I believe the test will take so long to finish on this case what is bad for qa alghouth it is the complete one. What do you think?
On Thu, Sep 4, 2014 at 1:24 PM, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
On Thu, Sep 4, 2014 at 2:04 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
On Wed, Sep 03, 2014 at 09:30:03PM -0400, Rodrigo Vivi wrote:Ok, everything changed ;-) So a bit hard to review just as a patch and so
> In order to get all test cases fixed and the matrix planes-operations working
> it was needed to use the common new igt kms functions for all cases.
> Previously only sprite testcase was using it.
>
> Fixed the fb colors in a way to make tests more clear and be impossible to see
> black screen during the tests.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
just a few comments on top:I'm sorry about that.... I got surprised I could split all in 9 patches, but I know this one staid ugly.- Looks good overall, but the gold standard is whether it'll catch bugs.
So if you remove some of the frontbuffer tracking (as little as possible
in each test) and the new testcase catches it all, then I think we're
good.It is already catching something, what is good! :)and not broken anymore!
- I think we should have a residency check before we grab a new crc, to
make sure that we're really again (or if the kernel is buggy, still) in
psr mode.yeah, but residency check is bad for vlv/chv.
- Checking for mismatching crc is risky, see the comment in a different
reply in this thread.
But overall looks good so probably best to just push these patches and
then fixup anything missing afterwards. I'll read through the entire test
again once it all landed to double-check we haven't missed anything really
important.cool. Thanks!
Aside: A suspend/resume testcase might be useful, if it can reliably
reproduce the issue you're working on. But again, follow-up.For sure. I just saw we have igt_system_suspend_autoresume. I'll definetely add another test using it.
-Daniel
--
> ---
> tests/kms_psr_sink_crc.c | 269 ++++++++++++++++++-----------------------------
> 1 file changed, 101 insertions(+), 168 deletions(-)
>
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index 4358889..27f3df9 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_sink_crc.c
> @@ -27,8 +27,6 @@
> #include <stdio.h>
> #include <string.h>
>
> -#include "drm_fourcc.h"
> -
> #include "ioctl_wrappers.h"
> #include "drmtest.h"
> #include "intel_bufmgr.h"
> @@ -79,88 +77,37 @@ typedef struct {
> int drm_fd;
> enum planes test_plane;
> enum operations op;
> - drmModeRes *resources;
> - drm_intel_bufmgr *bufmgr;
> uint32_t devid;
> - uint32_t handle[2];
> uint32_t crtc_id;
> - uint32_t crtc_idx;
> - uint32_t fb_id[3];
> - struct kmstest_connector_config config;
> igt_display_t display;
> - struct igt_fb fb[2];
> - igt_plane_t *plane[2];
> + drm_intel_bufmgr *bufmgr;
> + struct igt_fb fb_green, fb_white;
> + igt_plane_t *primary, *sprite, *cursor;
> } data_t;
>
> -static uint32_t create_fb(data_t *data,
> - int w, int h,
> - double r, double g, double b,
> - struct igt_fb *fb)
> +static void create_cursor_fb(data_t *data)
> {
> - uint32_t fb_id;
> cairo_t *cr;
> + uint32_t fb_id;
>
> - fb_id = igt_create_fb(data->drm_fd, w, h,
> - DRM_FORMAT_XRGB8888, I915_TILING_X, fb);
> + fb_id = igt_create_fb(data->drm_fd, 64, 64,
> + DRM_FORMAT_ARGB8888, I915_TILING_NONE,
> + &data->fb_white);
> igt_assert(fb_id);
>
> - cr = igt_get_cairo_ctx(data->drm_fd, fb);
> - igt_paint_color(cr, 0, 0, w, h, r, g, b);
> - igt_assert(cairo_status(cr) == 0);
> - cairo_destroy(cr);
> -
> - return fb_id;
> -}
> -
> -static void create_cursor_fb(data_t *data, struct igt_fb *fb)
> -{
> - cairo_t *cr;
> -
> - data->fb_id[2] = igt_create_fb(data->drm_fd, 64, 64,
> - DRM_FORMAT_ARGB8888, I915_TILING_NONE,
> - fb);
> - igt_assert(data->fb_id[2]);
> -
> - cr = igt_get_cairo_ctx(data->drm_fd, fb);
> + cr = igt_get_cairo_ctx(data->drm_fd, &data->fb_white);
> igt_paint_color_alpha(cr, 0, 0, 64, 64, 1.0, 1.0, 1.0, 1.0);
> igt_assert(cairo_status(cr) == 0);
> }
>
> -static bool
> -connector_set_mode(data_t *data, drmModeModeInfo *mode, uint32_t fb_id)
> -{
> - struct kmstest_connector_config *config = &data->config;
> - int ret;
> -
> -#if 0
> - fprintf(stdout, "Using pipe %s, %dx%d\n", kmstest_pipe_name(config->pipe),
> - mode->hdisplay, mode->vdisplay);
> -#endif
> -
> - ret = drmModeSetCrtc(data->drm_fd,
> - config->crtc->crtc_id,
> - fb_id,
> - 0, 0, /* x, y */
> - &config->connector->connector_id,
> - 1,
> - mode);
> - igt_assert(ret == 0);
> -
> - return 0;
> -}
> -
> static void display_init(data_t *data)
> {
> igt_display_init(&data->display, data->drm_fd);
> - data->resources = drmModeGetResources(data->drm_fd);
> - igt_assert(data->resources);
> }
>
> static void display_fini(data_t *data)
> {
> igt_display_fini(&data->display);
> - drmModeSetCursor(data->drm_fd, data->crtc_id, 0, 0, 0);
> - drmModeFreeResources(data->resources);
> }
>
> static void fill_blt(data_t *data, uint32_t handle, unsigned char color)
> @@ -316,112 +263,105 @@ static void get_sink_crc(data_t *data, char *crc) {
>
> static void test_crc(data_t *data)
> {
> - uint32_t handle = data->handle[0];
> + uint32_t handle = data->fb_white.gem_handle;
> + igt_plane_t *test_plane;
> + void *ptr;
> char ref_crc[12];
> char crc[12];
>
> - if (data->op == PLANE_MOVE) {
> - igt_assert(drmModeSetCursor(data->drm_fd, data->crtc_id,
> - handle, 64, 64) == 0);
> - igt_assert(drmModeMoveCursor(data->drm_fd, data->crtc_id,
> - 1, 1) == 0);
> + igt_plane_set_fb(data->primary, &data->fb_green);
> + igt_display_commit(&data->display);
> +
> + /* Setting a secondary fb/plane */
> + switch (data->test_plane) {
> + case PRIMARY: default: test_plane = data->primary; break;
> + case SPRITE: test_plane = data->sprite; break;
> + case CURSOR: test_plane = data->cursor; break;
> }
> + igt_plane_set_fb(test_plane, &data->fb_white);
> + igt_display_commit(&data->display);
>
> igt_assert(wait_psr_entry(data, 10));
> get_sink_crc(data, ref_crc);
>
> switch (data->op) {
> - void *ptr;
> case PAGE_FLIP:
> + /* Only in use when testing primary plane */
> igt_assert(drmModePageFlip(data->drm_fd, data->crtc_id,
> - data->fb_id[1], 0, NULL) == 0);
> + data->fb_green.fb_id, 0, NULL) == 0);
> break;
> - case MMAP_CPU:
> - ptr = gem_mmap__cpu(data->drm_fd, handle, 4096, PROT_WRITE);
> + case MMAP_GTT:
> + ptr = gem_mmap__gtt(data->drm_fd, handle, 4096, PROT_WRITE);
> gem_set_domain(data->drm_fd, handle,
> - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> - sleep(1);
> + I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> memset(ptr, 0, 4);
> munmap(ptr, 4096);
> - sleep(1);
> - gem_sw_finish(data->drm_fd, handle);
> break;
> - case MMAP_GTT:
> case MMAP_GTT_WAITING:
> ptr = gem_mmap__gtt(data->drm_fd, handle, 4096, PROT_WRITE);
> gem_set_domain(data->drm_fd, handle,
> I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +
> + /* Printing white on white so the screen shouldn't change */
> memset(ptr, 0xff, 4);
> + get_sink_crc(data, crc);
> + igt_assert(strcmp(ref_crc, crc) == 0);
> +
> + fprintf(stdout, "Waiting 10s...\n");
> + sleep(10);
> +
> + /* Now lets print black to change the screen */
> + memset(ptr, 0, 4);
> munmap(ptr, 4096);
> - gem_bo_busy(data->drm_fd, handle);
> + break;
> + case MMAP_CPU:
> + ptr = gem_mmap__cpu(data->drm_fd, handle, 4096, PROT_WRITE);
> + gem_set_domain(data->drm_fd, handle,
> + I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> + memset(ptr, 0, 4);
> + munmap(ptr, 4096);
> + gem_sw_finish(data->drm_fd, handle);
> break;
> case BLT:
> - fill_blt(data, handle, 0xff);
> + fill_blt(data, handle, 0);
> break;
> case RENDER:
> - fill_render(data, handle, 0xff);
> + fill_render(data, handle, 0);
> break;
> case PLANE_MOVE:
> - igt_assert(drmModeMoveCursor(data->drm_fd, data->crtc_id, 1, 2) == 0);
> + /* Only in use when testing Sprite and Cursor */
> + igt_plane_set_position(test_plane, 1, 1);
> + igt_display_commit(&data->display);
> break;
> case PLANE_ONOFF:
> - igt_plane_set_fb(data->plane[0], &data->fb[0]);
> - igt_display_commit(&data->display);
> - igt_plane_set_fb(data->plane[1], &data->fb[1]);
> + /* Only in use when testing Sprite and Cursor */
> + igt_plane_set_fb(test_plane, NULL);
> igt_display_commit(&data->display);
> break;
> }
> -
> - igt_wait_for_vblank(data->drm_fd, data->crtc_idx);
> -
> get_sink_crc(data, crc);
> igt_assert(strcmp(ref_crc, crc) != 0);
> }
>
> -static bool prepare_crtc(data_t *data, uint32_t connector_id)
> -{
> - if (!kmstest_get_connector_config(data->drm_fd,
> - connector_id,
> - 1 << data->crtc_idx,
> - &data->config))
> - return false;
> -
> - data->fb_id[0] = create_fb(data,
> - data->config.default_mode.hdisplay,
> - data->config.default_mode.vdisplay,
> - 0.0, 1.0, 0.0, &data->fb[0]);
> - igt_assert(data->fb_id[0]);
> -
> - if (data->op == PLANE_MOVE)
> - create_cursor_fb(data, &data->fb[0]);
> -
> - data->fb_id[1] = create_fb(data,
> - data->config.default_mode.hdisplay,
> - data->config.default_mode.vdisplay,
> - 1.0, 0.0, 0.0, &data->fb[1]);
> - igt_assert(data->fb_id[1]);
> -
> - data->handle[0] = data->fb[0].gem_handle;
> - data->handle[1] = data->fb[1].gem_handle;
> -
> - /* scanout = fb[1] */
> - connector_set_mode(data, &data->config.default_mode,
> - data->fb_id[1]);
> +static void test_cleanup(data_t *data) {
> + igt_plane_set_fb(data->primary, NULL);
> + if (data->test_plane == SPRITE)
> + igt_plane_set_fb(data->sprite, NULL);
> + if (data->test_plane == CURSOR)
> + igt_plane_set_fb(data->cursor, NULL);
>
> - /* scanout = fb[0] */
> - connector_set_mode(data, &data->config.default_mode,
> - data->fb_id[0]);
> + igt_display_commit(&data->display);
>
> - kmstest_free_connector_config(&data->config);
> -
> - return true;
> + igt_remove_fb(data->drm_fd, &data->fb_green);
> + igt_remove_fb(data->drm_fd, &data->fb_white);
> }
>
> -static void test_sprite(data_t *data)
> +static void run_test(data_t *data)
> {
> igt_display_t *display = &data->display;
> igt_output_t *output;
> drmModeModeInfo *mode;
> + uint32_t white_h, white_v;
>
> for_each_connected_output(display, output) {
> drmModeConnectorPtr c = output->config.connector;
> @@ -431,6 +371,7 @@ static void test_sprite(data_t *data)
> continue;
>
> igt_output_set_pipe(output, PIPE_ANY);
> + data->crtc_id = output->config.crtc->crtc_id;
>
> mode = igt_output_get_mode(output);
>
> @@ -438,51 +379,45 @@ static void test_sprite(data_t *data)
> mode->hdisplay, mode->vdisplay,
> DRM_FORMAT_XRGB8888, I915_TILING_X,
> 0.0, 1.0, 0.0,
> - &data->fb[0]);
> -
> - igt_create_color_fb(data->drm_fd,
> - mode->hdisplay/2, mode->vdisplay/2,
> - DRM_FORMAT_XRGB8888, I915_TILING_X,
> - 1.0, 0.0, 0.0,
> - &data->fb[1]);
> + &data->fb_green);
> +
> + data->primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> + igt_plane_set_fb(data->primary, NULL);
> +
> + white_h = mode->hdisplay;
> + white_v = mode->vdisplay;
> +
> + switch (data->test_plane) {
> + case SPRITE:
> + data->sprite = igt_output_get_plane(output,
> + IGT_PLANE_2);
> + igt_plane_set_fb(data->sprite, NULL);
> + /* To make it different for human eyes let's make
> + * sprite visible in only one quarter of the primary
> + */
> + white_h = white_h/2;
> + white_v = white_v/2;
> + case PRIMARY:
> + igt_create_color_fb(data->drm_fd,
> + white_h, white_v,
> + DRM_FORMAT_XRGB8888, I915_TILING_X,
> + 1.0, 1.0, 1.0,
> + &data->fb_white);
> + break;
> + case CURSOR:
> + data->cursor = igt_output_get_plane(output,
> + IGT_PLANE_CURSOR);
> + igt_plane_set_fb(data->cursor, NULL);
> + create_cursor_fb(data);
> + igt_plane_set_position(data->cursor, 0, 0);
> + break;
> + }
>
> - data->plane[0] = igt_output_get_plane(output, 0);
> - data->plane[1] = igt_output_get_plane(output, 1);
> + igt_display_commit(&data->display);
>
> test_crc(data);
> - }
> -}
> -
> -static void run_test(data_t *data)
> -{
> - int i, n;
> - drmModeConnectorPtr c;
> - /* Baytrail supports per-pipe PSR configuration, however PSR on
> - * PIPE_B isn't working properly. So let's keep it disabled for now.
> - * crtcs = IS_VALLEYVIEW(data->devid)? 2 : 1; */
> - int crtcs = 1;
> -
> - if (data->op == PLANE_ONOFF) {
> - test_sprite(data);
> - return;
> - }
>
> - for (i = 0; i < data->resources->count_connectors; i++) {
> - uint32_t connector_id = data->resources->connectors[i];
> - c = drmModeGetConnector(data->drm_fd, connector_id);
> -
> - if (c->connector_type != DRM_MODE_CONNECTOR_eDP ||
> - c->connection != DRM_MODE_CONNECTED)
> - continue;
> - for (n = 0; n < crtcs; n++) {
> - data->crtc_idx = n;
> - data->crtc_id = data->resources->crtcs[n];
> -
> - if (!prepare_crtc(data, connector_id))
> - continue;
> -
> - test_crc(data);
> - }
> + test_cleanup(data);
> }
> }
>
> @@ -496,7 +431,6 @@ igt_main
> igt_fixture {
> data.drm_fd = drm_open_any();
> kmstest_set_vt_graphics_mode();
> -
> data.devid = intel_get_drm_devid(data.drm_fd);
>
> igt_skip_on(!psr_enabled(&data));
> @@ -508,7 +442,6 @@ igt_main
> display_init(&data);
> }
>
> -
> for (op = PAGE_FLIP; op <= RENDER; op++) {
> igt_subtest_f("primary_%s", op_str(op)) {
> data.test_plane = PRIMARY;
> @@ -517,7 +450,7 @@ igt_main
> }
> }
>
> - for (op = PLANE_ONOFF; op <= PLANE_ONOFF; op++) {
> + for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> igt_subtest_f("sprite_%s", op_str(op)) {
> data.test_plane = SPRITE;
> data.op = op;
> @@ -525,7 +458,7 @@ igt_main
> }
> }
>
> - for (op = PLANE_MOVE; op <= PLANE_MOVE; op++) {
> + for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> igt_subtest_f("cursor_%s", op_str(op)) {
> data.test_plane = CURSOR;
> data.op = op;
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--Rodrigo ViviBlog: http://blog.vivi.eng.br
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
