<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Sep 4, 2014 at 2:04 AM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Wed, Sep 03, 2014 at 09:30:03PM -0400, Rodrigo Vivi wrote: > 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 <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>> </div>Ok, everything changed ;-) So a bit hard to review just as a patch and so just a few comments on top: </blockquote><div> </div><div>I'm sorry about that.... I got surprised I could split all in 9 patches, but I know this one staid ugly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - 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. </blockquote><div> </div><div>It is already catching something, what is good! :)</div><div>and not broken anymore!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - 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. </blockquote><div> </div><div>yeah, but residency check is bad for vlv/chv.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - 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. </blockquote><div> </div><div>cool. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Aside: A suspend/resume testcase might be useful, if it can reliably reproduce the issue you're working on. But again, follow-up. </blockquote><div> </div><div>For sure. I just saw we have igt_system_suspend_autoresume. I'll definetely add another test using it.</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -Daniel <div class="HOEnZb"><div class="h5"> > --- > 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 > <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </div></div>-- Daniel Vetter Software Engineer, Intel Corporation <a href="tel:%2B41%20%280%29%2079%20365%2057%2048" value="+41793655748">+41 (0) 79 365 57 48</a> - <a href="http://blog.ffwll.ch" target="_blank">http://blog.ffwll.ch</a> <div class="HOEnZb"><div class="h5">_______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </div></div></blockquote></div> <div> </div>-- <div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div> </div></div>