Make kms_fbc_crc useful again
cover Make kms_fbc_crc useful again
From: Paulo Zanoni <[email protected]>
After the recent IGT regressions and the FBC conversion to the frontbuffer tracking infrastructure, kms_fbc_crc was not being that useful anymore. So this series makes it useful again.
The first 2 patches are alternate forms of patches that I already sent to the mailing list a few times. They are needed due to the Kernel FBC changes. In the previous versions I was adding gem_sync() calls, but now I'm just increasing the timeouts. I'm aware we want to have test cases for both situations, but for now let's fix the ones we already have with the least intrusive solution.
The other 6 patches are just for the CRC regression fix. Notice that kms_fbc_crc is completely useless without this fix. Daniel has already sent an earlier for this problem, but I believe my proposed solution is more complete.
Of course, to make kms_fbc_crc actually useful we need to enable FBC by default again, but we're not there yet.
Also notice that there's still no conversion to use lib/igt_draw, but some of the reworks required by it are already on this series. I want to land any possible conversion only after we fix the current known regressions.
Thanks,
Paulo
Paulo Zanoni (8):
tests/kms_fbc_crc: exec_nop() can also invalidate FBC tests/kms_fbc_crc: increase the FBC wait timeout tests/kms_fbc_crc: unify flip handling tests/kms_fbc_crc: refactor context handling code tests/kms_fbc_crc: extract check_crc() tests/kms_fbc_crc: make the blt test draw the same pattern tests/kms_fbc_crc: extract fill_mmap_{cpu,gtt}() tests/kms_fb_crc: fix the reference CRC checking
tests/kms_fbc_crc.c | 193 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 129 insertions(+), 64 deletions(-)
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/8 tests/kms_fbc_crc: exec_nop() can also invalidate FBC
From: Paulo Zanoni <[email protected]>
So make sure that, at prepare_test(), we wait for FBC to be enabled again after we run the exec_nop() call. Since after this happens, we just assert fbc_enabled() at test_crc() instead of waiting for it to be enabled.
This is now needed because we moved to software frontbuffer tracking, so it can take some considerable time for FBC to be reenabled after it is disabled.
A previous version of this patch was just calling gem_sync() after exec_nop().
--- tests/kms_fbc_crc.c | 1 + 1 file changed, 1 insertion(+)
tests/kms_fbc_crc.c +1 -0
@@ -383,6 +383,7 @@ static bool prepare_test(data_t *data, enum test_mode test_mode)* FBC RT address is left as disabled. */ exec_nop (data, data->fb[0 ].gem_handle, data->ctx[0 ]); +igt_assert (wait_for_fbc_enabled (data)); }igt_wait_for_vblank (data->drm_fd, data->pipe);
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/8 tests/kms_fbc_crc: increase the FBC wait timeout
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Now that we moved to the frontbuffer tracking scheme, it may take a long time for FBC to be updated after it is invalidated: 300ms is not enough anymore.
The problem starts when i915_gem_execbuffer2() indirectly calls intel_fb_obj_invalidate(), which disables FBC. After this, FBC only gets reenabled when i915_gem_retire_work_handler() happens and indirectly calls intel_frontbuffer_flush(). Notice that while FBC is not yet enabled, the screen contents are correct, so this shouldn't really be a bug.
Previous versions of this patch were replacing the gem_bo_busy() calls with gem_sync(), but after some discussion we concluded this was not the correct way to handle the problem, so let's just increase the timeout so we can kill those subtest failures.
--- tests/kms_fbc_crc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tests/kms_fbc_crc.c +1 -1
@@ -204,7 +204,7 @@ static bool fbc_enabled(data_t *data)static bool wait_for_fbc_enabled (data_t *data) { -return igt_wait (fbc_enabled (data),300 ,30 ); +return igt_wait (fbc_enabled (data),3000 ,30 ); }static void test_crc (data_t *data,enum test_mode mode)
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 3/8 tests/kms_fbc_crc: unify flip handling
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Just a small simplification to make the code a little easier to understand, and to help us when we further split drawing vs flipping later.
--- tests/kms_fbc_crc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
tests/kms_fbc_crc.c +3 -4
@@ -216,19 +216,18 @@ static void test_crc(data_t *data, enum test_mode mode)igt_assert (fbc_enabled (data)); -if (mode >=TEST_PAGE_FLIP_AND_MMAP_CPU ) { +if (mode ==TEST_PAGE_FLIP || mode >=TEST_PAGE_FLIP_AND_MMAP_CPU ) { handle = data->fb[1 ].gem_handle;igt_assert (drmModePageFlip (data->drm_fd, crtc_id, data->fb[1 ].fb_id,0 ,NULL ) ==0 ); -igt_assert (wait_for_fbc_enabled (data)); +if (mode !=TEST_PAGE_FLIP ) +igt_assert (wait_for_fbc_enabled (data)); }switch (mode) {void *ptr;case TEST_PAGE_FLIP : -igt_assert (drmModePageFlip (data->drm_fd, crtc_id, - data->fb[1 ].fb_id,0 ,NULL ) ==0 );break ;case TEST_MMAP_CPU :case TEST_PAGE_FLIP_AND_MMAP_CPU :
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 4/8 tests/kms_fbc_crc: refactor context handling code
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Just a small modification to make the code a little easier to understand, IMHO.
--- tests/kms_fbc_crc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
tests/kms_fbc_crc.c +5 -5
@@ -227,6 +227,7 @@ static void test_crc(data_t *data, enum test_mode mode)switch (mode) {void *ptr; +drm_intel_context *context =NULL ;case TEST_PAGE_FLIP :break ;case TEST_MMAP_CPU :
@@ -248,13 +249,12 @@ static void test_crc(data_t *data, enum test_mode mode)case TEST_PAGE_FLIP_AND_BLT :fill_blt (data, handle, data->fb, ~0 );break ; -case TEST_RENDER :case TEST_CONTEXT : -case TEST_PAGE_FLIP_AND_RENDER :case TEST_PAGE_FLIP_AND_CONTEXT : -fill_render (data, handle, - (mode ==TEST_CONTEXT || mode ==TEST_PAGE_FLIP_AND_CONTEXT ) ? - data->ctx[1 ] :NULL ,0xff ); + context = data->ctx[1 ]; +case TEST_RENDER : +case TEST_PAGE_FLIP_AND_RENDER : +fill_render (data, handle, context,0xff );break ; }
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 5/8 tests/kms_fbc_crc: extract check_crc()
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
To remove some duplicated code. When we finally fix that FIXME, the code will get a little bigger too.
--- tests/kms_fbc_crc.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
tests/kms_fbc_crc.c +14 -12
@@ -207,11 +207,21 @@ static bool wait_for_fbc_enabled(data_t *data)return igt_wait (fbc_enabled (data),3000 ,30 ); } -static void test_crc (data_t *data,enum test_mode mode) +static void check_crc (data_t *data,enum test_mode mode) { -uint32_t crtc_id = data->output->config.crtc->crtc_id;igt_pipe_crc_t *pipe_crc = data->pipe_crc;igt_crc_t crc; + +igt_pipe_crc_collect_crc (pipe_crc, &crc); +if (mode ==TEST_PAGE_FLIP ) +igt_assert_crc_equal (&crc, &data->ref_crc[1 ]); +else + ;/* FIXME: missing reference CRCs */ +} + +static void test_crc (data_t *data,enum test_mode mode) +{ +uint32_t crtc_id = data->output->config.crtc->crtc_id;uint32_t handle = data->fb[0 ].gem_handle;igt_assert (fbc_enabled (data));
@@ -266,11 +276,7 @@ static void test_crc(data_t *data, enum test_mode mode)igt_wait_for_vblank (data->drm_fd, data->pipe);igt_wait_for_vblank (data->drm_fd, data->pipe); -igt_pipe_crc_collect_crc (pipe_crc, &crc); -if (mode ==TEST_PAGE_FLIP ) -igt_assert_crc_equal (&crc, &data->ref_crc[1 ]); -else - ;/* FIXME: missing reference CRCs */ +check_crc (data, mode);/* * Allow time for FBC to kick in again if it
@@ -278,11 +284,7 @@ static void test_crc(data_t *data, enum test_mode mode)*/ igt_assert (wait_for_fbc_enabled (data)); -igt_pipe_crc_collect_crc (pipe_crc, &crc); -if (mode ==TEST_PAGE_FLIP ) -igt_assert_crc_equal (&crc, &data->ref_crc[1 ]); -else - ;/* FIXME: missing reference CRCs */ +check_crc (data, mode); }static bool prepare_crtc (data_t *data)
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 6/8 tests/kms_fbc_crc: make the blt test draw the same pattern
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
... as the other drawing tests: single white pixel at top/left of the screen, instead of painting the whole screen blue.
This will make it much easier to fix the CRC checking code.
--- tests/kms_fbc_crc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
tests/kms_fbc_crc.c +3 -2
@@ -98,6 +98,7 @@ static void fill_blt(data_t *data,struct intel_batchbuffer *batch;unsigned flags;int pitch; +uint32_t pixel = color | (color <<8 ) | (color <<16 ) | (color <<24 ); batch =intel_batchbuffer_alloc (data->bufmgr, data->devid);igt_assert (batch);
@@ -113,9 +114,9 @@ static void fill_blt(data_t *data,COLOR_BLIT_COPY_BATCH_START (flags);OUT_BATCH (3 <<24 |0xf0 <<16 | pitch);OUT_BATCH (0 ); -OUT_BATCH (fb->height <<16 | fb->width); +OUT_BATCH (1 <<16 |1 );OUT_RELOC_FENCED (dst,I915_GEM_DOMAIN_RENDER ,I915_GEM_DOMAIN_RENDER ,0 ); -OUT_BATCH (color); +OUT_BATCH (pixel);ADVANCE_BATCH ();intel_batchbuffer_flush (batch);
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 7/8 tests/kms_fbc_crc: extract fill_mmap_{cpu, gtt}()
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Just like we have fill_render() and fill_blt(). I'm also going to use fill_mmap_gtt() for the code that generates the reference CRCs.
--- tests/kms_fbc_crc.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)
tests/kms_fbc_crc.c +25 -10
@@ -190,6 +190,29 @@ static void fill_render(data_t *data, uint32_t handle,gem_bo_busy (data->drm_fd, handle); } +static void fill_mmap_cpu (data_t *data,uint32_t handle,unsigned char color) +{ +void *ptr; + + ptr =gem_mmap__cpu (data->drm_fd, handle,0 ,4096 ,PROT_WRITE ); +gem_set_domain (data->drm_fd, handle,I915_GEM_DOMAIN_CPU , +I915_GEM_DOMAIN_CPU ); +memset (ptr, color,4 ); +munmap (ptr,4096 ); +gem_sw_finish (data->drm_fd, handle); +} + +static void fill_mmap_gtt (data_t *data,uint32_t handle,unsigned char color) +{ +void *ptr; + + 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 ); +memset (ptr, color,4 ); +munmap (ptr,4096 ); +} +static bool fbc_enabled (data_t *data) {FILE *status;
@@ -237,24 +260,16 @@ static void test_crc(data_t *data, enum test_mode mode) }switch (mode) { -void *ptr;drm_intel_context *context =NULL ;case TEST_PAGE_FLIP :break ;case TEST_MMAP_CPU :case TEST_PAGE_FLIP_AND_MMAP_CPU : - ptr =gem_mmap__cpu (data->drm_fd, handle,0 ,4096 ,PROT_WRITE ); -gem_set_domain (data->drm_fd, handle,I915_GEM_DOMAIN_CPU ,I915_GEM_DOMAIN_CPU ); -memset (ptr,0xff ,4 ); -munmap (ptr,4096 ); -gem_sw_finish (data->drm_fd, handle); +fill_mmap_cpu (data, handle,0xff );break ;case TEST_MMAP_GTT :case TEST_PAGE_FLIP_AND_MMAP_GTT : - 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 ); -memset (ptr,0xff ,4 ); -munmap (ptr,4096 ); +fill_mmap_gtt (data, handle,0xff );break ;case TEST_BLT :case TEST_PAGE_FLIP_AND_BLT :
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 8/8 tests/kms_fb_crc: fix the reference CRC checking
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Now we get the reference CRCs on separate untiled FBs just to make sure FBC is not there to mess the CRC computation. We also get the reference CRCs for buffers that were drawn in the same way that we draw them during the tests, so we can finally get rid of that FIXME comment we have!
With this, kms_fbc_crc now actually tests something instead of just wasting CPU cycles...
Fixes regression introduced by:
commit 562bbe12f6fb75811a746c742a28d32a17b26aa9
Author: Daniel Vetter <[email protected]> Date: Fri Feb 27 22:04:18 2015 +0100
tests: Remove usage of igt_crc_equal and _non_null
--- tests/kms_fbc_crc.c | 117 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 82 insertions(+), 35 deletions(-)
tests/kms_fbc_crc.c +82 -35
@@ -56,7 +56,7 @@ enum test_mode { typedef struct { int drm_fd; - igt_crc_t ref_crc[2]; + igt_crc_t ref_crc[4]; igt_pipe_crc_t *pipe_crc; drm_intel_bufmgr *bufmgr; drm_intel_context *ctx[2];
@@ -234,13 +234,32 @@ static bool wait_for_fbc_enabled(data_t *data)static void check_crc (data_t *data,enum test_mode mode) {igt_pipe_crc_t *pipe_crc = data->pipe_crc; -igt_crc_t crc; +igt_crc_t crc, *ref_crc; + +switch (mode) { +case TEST_PAGE_FLIP : + ref_crc = &data->ref_crc[1 ]; +break ; +case TEST_MMAP_CPU : +case TEST_MMAP_GTT : +case TEST_BLT : +case TEST_RENDER : +case TEST_CONTEXT : + ref_crc = &data->ref_crc[2 ]; +break ; +case TEST_PAGE_FLIP_AND_MMAP_CPU : +case TEST_PAGE_FLIP_AND_MMAP_GTT : +case TEST_PAGE_FLIP_AND_BLT : +case TEST_PAGE_FLIP_AND_RENDER : +case TEST_PAGE_FLIP_AND_CONTEXT : + ref_crc = &data->ref_crc[3 ]; +break ; +default : +igt_assert (false); + }igt_pipe_crc_collect_crc (pipe_crc, &crc); -if (mode ==TEST_PAGE_FLIP ) -igt_assert_crc_equal (&crc, &data->ref_crc[1 ]); -else - ;/* FIXME: missing reference CRCs */ +igt_assert_crc_equal (&crc, ref_crc); }static void test_crc (data_t *data,enum test_mode mode)
@@ -321,27 +340,71 @@ static bool prepare_crtc(data_t *data)return true; } +static void create_fbs (data_t *data,bool tiled,struct igt_fb *fbs) +{ +int rc; +drmModeModeInfo *mode =igt_output_get_mode (data->output); +uint64_t tiling = tiled ?LOCAL_I915_FORMAT_MOD_X_TILED : +LOCAL_DRM_FORMAT_MOD_NONE ; + + rc =igt_create_color_fb (data->drm_fd, mode->hdisplay, mode->vdisplay, +DRM_FORMAT_XRGB8888 , tiling, +0.0 ,0.0 ,0.0 , &fbs[0 ]); +igt_assert (rc); + rc =igt_create_color_fb (data->drm_fd, mode->hdisplay, mode->vdisplay, +DRM_FORMAT_XRGB8888 , tiling, +0.1 ,0.1 ,0.1 , &fbs[1 ]); +igt_assert (rc); +} + +/* Since we want to be really safe that the CRCs are actually what we really +* want, use untiled FBs, so FBC won't happen to disrupt things. Also do the +* drawing before setting the modes, just to be sure. */ +static void get_ref_crcs (data_t *data) +{ +igt_display_t *display = &data->display; +struct igt_fb fbs[4 ]; +int i; + +create_fbs (data, false, &fbs[0 ]); +create_fbs (data, false, &fbs[2 ]); + +fill_mmap_gtt (data, fbs[2 ].gem_handle,0xff ); +fill_mmap_gtt (data, fbs[3 ].gem_handle,0xff ); + +for (i =0 ; i <4 ; i++) { +igt_plane_set_fb (data->primary, &fbs[i]); +igt_display_commit (display); +igt_wait_for_vblank (data->drm_fd, data->pipe); +igt_assert (!fbc_enabled (data)); +igt_pipe_crc_collect_crc (data->pipe_crc, &data->ref_crc[i]); +igt_assert (!fbc_enabled (data)); + } + +igt_plane_set_fb (data->primary, &data->fb[1 ]); +igt_display_commit (display); + +for (i =0 ; i <4 ; i++) +igt_remove_fb (data->drm_fd, &fbs[i]); +} +static bool prepare_test (data_t *data,enum test_mode test_mode) {igt_display_t *display = &data->display;igt_output_t *output = data->output; -drmModeModeInfo *mode;igt_pipe_crc_t *pipe_crc; -int rc; data->primary =igt_output_get_plane (data->output,IGT_PLANE_PRIMARY ); - mode =igt_output_get_mode (data->output); - rc =igt_create_color_fb (data->drm_fd, mode->hdisplay, mode->vdisplay, -DRM_FORMAT_XRGB8888 , -LOCAL_I915_FORMAT_MOD_X_TILED , -0.0 ,0.0 ,0.0 , &data->fb[0 ]); -igt_assert (rc); - rc =igt_create_color_fb (data->drm_fd, mode->hdisplay, mode->vdisplay, -DRM_FORMAT_XRGB8888 , -LOCAL_I915_FORMAT_MOD_X_TILED , -0.1 ,0.1 ,0.1 , &data->fb[1 ]); -igt_assert (rc); +create_fbs (data, true, data->fb); + +igt_pipe_crc_free (data->pipe_crc); + data->pipe_crc =NULL ; + pipe_crc =igt_pipe_crc_new (data->pipe, +INTEL_PIPE_CRC_SOURCE_AUTO ); + data->pipe_crc = pipe_crc; + +get_ref_crcs (data);/* scanout = fb[1] */ igt_plane_set_fb (data->primary, &data->fb[1 ]);
@@ -359,19 +422,6 @@ static bool prepare_test(data_t *data, enum test_mode test_mode)return false; } -igt_pipe_crc_free (data->pipe_crc); - data->pipe_crc =NULL ; - - pipe_crc =igt_pipe_crc_new (data->pipe, -INTEL_PIPE_CRC_SOURCE_AUTO ); - - data->pipe_crc = pipe_crc; - -igt_wait_for_vblank (data->drm_fd, data->pipe); - -/* get reference crc for fb[1] */ -igt_pipe_crc_collect_crc (pipe_crc, &data->ref_crc[1 ]); -if (test_mode ==TEST_CONTEXT || test_mode ==TEST_PAGE_FLIP_AND_CONTEXT ) { data->ctx[0 ] =drm_intel_gem_context_create (data->bufmgr);igt_assert (data->ctx[0 ]);
@@ -405,9 +455,6 @@ static bool prepare_test(data_t *data, enum test_mode test_mode)igt_wait_for_vblank (data->drm_fd, data->pipe); -/* get reference crc for fb[0] */ -igt_pipe_crc_collect_crc (pipe_crc, &data->ref_crc[0 ]); -return true; }
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx