| Message ID | 20170919113113.7591-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 19, 2017 at 01:31:13PM +0200, Maarten Lankhorst wrote: > Collecting CRC may force a modeset, which is a bad idea after we just > forced a hang. The hang is intended to make sure the page flip doesn't > complete before the cursor, making sure that works. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102259 > Cc: Marta Lofstedt <marta.lofstedt@intel.com> > --- > tests/kms_cursor_legacy.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c > index 2d32d3a91570..c3149535b838 100644 > --- a/tests/kms_cursor_legacy.c > +++ b/tests/kms_cursor_legacy.c > @@ -1334,7 +1334,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) > igt_pipe_crc_t *pipe_crc; > igt_pipe_t *pipe_connected = &display->pipes[pipe]; > igt_plane_t *plane_primary = igt_pipe_get_plane_type(pipe_connected, DRM_PLANE_TYPE_PRIMARY); > - igt_crc_t crcs[3]; > + igt_crc_t crcs[2]; > > if (atomic) > igt_require(display->is_atomic); > @@ -1348,7 +1348,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) > > igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); > > - pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); > + pipe_crc = igt_pipe_crc_new_nonblock(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); > > set_cursor_on_pipe(display, pipe, &cursor_fb); > igt_display_commit2(display, COMMIT_UNIVERSAL); > @@ -1371,9 +1371,17 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) > igt_plane_set_fb(plane_primary, &fb_info[0]); > igt_display_commit2(display, COMMIT_UNIVERSAL); > > + /* > + * We must enable CRC collecting here since this may force > + * a modeset, and this loop is timing sensitive. > + */ > + igt_pipe_crc_start(pipe_crc); > + > /* Disable cursor, and immediately queue a flip. Check if resulting crc is correct. */ > for (int i = 1; i >= 0; i--) { > uint32_t *busy; > + igt_crc_t *received_crcs = NULL; > + int ncrcs; > > busy = make_fb_busy(display->drm_fd, &fb_info[1]); > > @@ -1384,7 +1392,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) > > igt_assert_eq(get_vblank(display->drm_fd, pipe, 0), vblank_start); > > - igt_pipe_crc_collect_crc(pipe_crc, &crcs[2]); > + ncrcs = igt_pipe_crc_get_crcs(pipe_crc, 8, &received_crcs); > > finish_fb_busy(busy); > > @@ -1397,13 +1405,22 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) > igt_plane_set_fb(plane_primary, &fb_info[0]); > igt_display_commit2(display, COMMIT_UNIVERSAL); > > - igt_assert_crc_equal(&crcs[i], &crcs[2]); > + igt_assert(ncrcs > 0); Should we perhaps assign some arbitrary upper limit on the crcs we get? If the kernel already buffered 8 or more crcs, we may get a crc mismatch even though the problem is that the test blocked for too long and we're now looking at a stale crc rather than the current crc being wrong. Might make it slightly easier to diagnose the failure correctly. Anyways Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > + igt_assert_crc_equal(&crcs[i], &received_crcs[ncrcs - 1]); > + free(received_crcs); > } > > do_cleanup_display(display); > igt_remove_fb(display->drm_fd, &fb_info[1]); > igt_remove_fb(display->drm_fd, &fb_info[0]); > igt_remove_fb(display->drm_fd, &cursor_fb); > + > + /* > + * igt_pipe_crc_stop() may force a modeset for workarounds, call > + * it after do_cleanup_display since we disable the display anyway. > + */ > + igt_pipe_crc_stop(pipe_crc); > igt_pipe_crc_free(pipe_crc); > } > > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 19-09-17 om 13:49 schreef Ville Syrjälä: > On Tue, Sep 19, 2017 at 01:31:13PM +0200, Maarten Lankhorst wrote: >> Collecting CRC may force a modeset, which is a bad idea after we just >> forced a hang. The hang is intended to make sure the page flip doesn't >> complete before the cursor, making sure that works. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102259 >> Cc: Marta Lofstedt <marta.lofstedt@intel.com> >> --- >> tests/kms_cursor_legacy.c | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c >> index 2d32d3a91570..c3149535b838 100644 >> --- a/tests/kms_cursor_legacy.c >> +++ b/tests/kms_cursor_legacy.c >> @@ -1334,7 +1334,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> igt_pipe_crc_t *pipe_crc; >> igt_pipe_t *pipe_connected = &display->pipes[pipe]; >> igt_plane_t *plane_primary = igt_pipe_get_plane_type(pipe_connected, DRM_PLANE_TYPE_PRIMARY); >> - igt_crc_t crcs[3]; >> + igt_crc_t crcs[2]; >> >> if (atomic) >> igt_require(display->is_atomic); >> @@ -1348,7 +1348,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> >> igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); >> >> - pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); >> + pipe_crc = igt_pipe_crc_new_nonblock(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); >> >> set_cursor_on_pipe(display, pipe, &cursor_fb); >> igt_display_commit2(display, COMMIT_UNIVERSAL); >> @@ -1371,9 +1371,17 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> igt_plane_set_fb(plane_primary, &fb_info[0]); >> igt_display_commit2(display, COMMIT_UNIVERSAL); >> >> + /* >> + * We must enable CRC collecting here since this may force >> + * a modeset, and this loop is timing sensitive. >> + */ >> + igt_pipe_crc_start(pipe_crc); >> + >> /* Disable cursor, and immediately queue a flip. Check if resulting crc is correct. */ >> for (int i = 1; i >= 0; i--) { >> uint32_t *busy; >> + igt_crc_t *received_crcs = NULL; >> + int ncrcs; >> >> busy = make_fb_busy(display->drm_fd, &fb_info[1]); >> >> @@ -1384,7 +1392,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> >> igt_assert_eq(get_vblank(display->drm_fd, pipe, 0), vblank_start); >> >> - igt_pipe_crc_collect_crc(pipe_crc, &crcs[2]); >> + ncrcs = igt_pipe_crc_get_crcs(pipe_crc, 8, &received_crcs); >> >> finish_fb_busy(busy); >> >> @@ -1397,13 +1405,22 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> igt_plane_set_fb(plane_primary, &fb_info[0]); >> igt_display_commit2(display, COMMIT_UNIVERSAL); >> >> - igt_assert_crc_equal(&crcs[i], &crcs[2]); >> + igt_assert(ncrcs > 0); > Should we perhaps assign some arbitrary upper limit on the crcs we get? > If the kernel already buffered 8 or more crcs, we may get a crc mismatch > even though the problem is that the test blocked for too long and we're > now looking at a stale crc rather than the current crc being wrong. > Might make it slightly easier to diagnose the failure correctly. > > Anyways > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > I figured 8 is enough, ack if I change it to igt_assert(ncrcs > 0 && ncrcs < 8), so we know we don't overflow?
Op 19-09-17 om 13:49 schreef Ville Syrjälä: > On Tue, Sep 19, 2017 at 01:31:13PM +0200, Maarten Lankhorst wrote: >> Collecting CRC may force a modeset, which is a bad idea after we just >> forced a hang. The hang is intended to make sure the page flip doesn't >> complete before the cursor, making sure that works. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102259 >> Cc: Marta Lofstedt <marta.lofstedt@intel.com> >> --- >> tests/kms_cursor_legacy.c | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c >> index 2d32d3a91570..c3149535b838 100644 >> --- a/tests/kms_cursor_legacy.c >> +++ b/tests/kms_cursor_legacy.c >> @@ -1334,7 +1334,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> igt_pipe_crc_t *pipe_crc; >> igt_pipe_t *pipe_connected = &display->pipes[pipe]; >> igt_plane_t *plane_primary = igt_pipe_get_plane_type(pipe_connected, DRM_PLANE_TYPE_PRIMARY); >> - igt_crc_t crcs[3]; >> + igt_crc_t crcs[2]; >> >> if (atomic) >> igt_require(display->is_atomic); >> @@ -1348,7 +1348,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> >> igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); >> >> - pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); >> + pipe_crc = igt_pipe_crc_new_nonblock(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); >> >> set_cursor_on_pipe(display, pipe, &cursor_fb); >> igt_display_commit2(display, COMMIT_UNIVERSAL); >> @@ -1371,9 +1371,17 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> igt_plane_set_fb(plane_primary, &fb_info[0]); >> igt_display_commit2(display, COMMIT_UNIVERSAL); >> >> + /* >> + * We must enable CRC collecting here since this may force >> + * a modeset, and this loop is timing sensitive. >> + */ >> + igt_pipe_crc_start(pipe_crc); >> + >> /* Disable cursor, and immediately queue a flip. Check if resulting crc is correct. */ >> for (int i = 1; i >= 0; i--) { >> uint32_t *busy; >> + igt_crc_t *received_crcs = NULL; >> + int ncrcs; >> >> busy = make_fb_busy(display->drm_fd, &fb_info[1]); >> >> @@ -1384,7 +1392,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> >> igt_assert_eq(get_vblank(display->drm_fd, pipe, 0), vblank_start); >> >> - igt_pipe_crc_collect_crc(pipe_crc, &crcs[2]); >> + ncrcs = igt_pipe_crc_get_crcs(pipe_crc, 8, &received_crcs); >> >> finish_fb_busy(busy); >> >> @@ -1397,13 +1405,22 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) >> igt_plane_set_fb(plane_primary, &fb_info[0]); >> igt_display_commit2(display, COMMIT_UNIVERSAL); >> >> - igt_assert_crc_equal(&crcs[i], &crcs[2]); >> + igt_assert(ncrcs > 0); > Should we perhaps assign some arbitrary upper limit on the crcs we get? > If the kernel already buffered 8 or more crcs, we may get a crc mismatch > even though the problem is that the test blocked for too long and we're > now looking at a stale crc rather than the current crc being wrong. > Might make it slightly easier to diagnose the failure correctly. > > Anyways > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> + >> + igt_assert_crc_equal(&crcs[i], &received_crcs[ncrcs - 1]); >> + free(received_crcs); >> } >> >> do_cleanup_display(display); >> igt_remove_fb(display->drm_fd, &fb_info[1]); >> igt_remove_fb(display->drm_fd, &fb_info[0]); >> igt_remove_fb(display->drm_fd, &cursor_fb); >> + >> + /* >> + * igt_pipe_crc_stop() may force a modeset for workarounds, call >> + * it after do_cleanup_display since we disable the display anyway. >> + */ >> + igt_pipe_crc_stop(pipe_crc); >> igt_pipe_crc_free(pipe_crc); >> } >> >> -- >> 2.14.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx Thanks, pushed with a limit in place to prevent overflow. :)
diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c index 2d32d3a91570..c3149535b838 100644 --- a/tests/kms_cursor_legacy.c +++ b/tests/kms_cursor_legacy.c @@ -1334,7 +1334,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) igt_pipe_crc_t *pipe_crc; igt_pipe_t *pipe_connected = &display->pipes[pipe]; igt_plane_t *plane_primary = igt_pipe_get_plane_type(pipe_connected, DRM_PLANE_TYPE_PRIMARY); - igt_crc_t crcs[3]; + igt_crc_t crcs[2]; if (atomic) igt_require(display->is_atomic); @@ -1348,7 +1348,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); - pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); + pipe_crc = igt_pipe_crc_new_nonblock(display->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO); set_cursor_on_pipe(display, pipe, &cursor_fb); igt_display_commit2(display, COMMIT_UNIVERSAL); @@ -1371,9 +1371,17 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) igt_plane_set_fb(plane_primary, &fb_info[0]); igt_display_commit2(display, COMMIT_UNIVERSAL); + /* + * We must enable CRC collecting here since this may force + * a modeset, and this loop is timing sensitive. + */ + igt_pipe_crc_start(pipe_crc); + /* Disable cursor, and immediately queue a flip. Check if resulting crc is correct. */ for (int i = 1; i >= 0; i--) { uint32_t *busy; + igt_crc_t *received_crcs = NULL; + int ncrcs; busy = make_fb_busy(display->drm_fd, &fb_info[1]); @@ -1384,7 +1392,7 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) igt_assert_eq(get_vblank(display->drm_fd, pipe, 0), vblank_start); - igt_pipe_crc_collect_crc(pipe_crc, &crcs[2]); + ncrcs = igt_pipe_crc_get_crcs(pipe_crc, 8, &received_crcs); finish_fb_busy(busy); @@ -1397,13 +1405,22 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic) igt_plane_set_fb(plane_primary, &fb_info[0]); igt_display_commit2(display, COMMIT_UNIVERSAL); - igt_assert_crc_equal(&crcs[i], &crcs[2]); + igt_assert(ncrcs > 0); + + igt_assert_crc_equal(&crcs[i], &received_crcs[ncrcs - 1]); + free(received_crcs); } do_cleanup_display(display); igt_remove_fb(display->drm_fd, &fb_info[1]); igt_remove_fb(display->drm_fd, &fb_info[0]); igt_remove_fb(display->drm_fd, &cursor_fb); + + /* + * igt_pipe_crc_stop() may force a modeset for workarounds, call + * it after do_cleanup_display since we disable the display anyway. + */ + igt_pipe_crc_stop(pipe_crc); igt_pipe_crc_free(pipe_crc); }
Collecting CRC may force a modeset, which is a bad idea after we just forced a hang. The hang is intended to make sure the page flip doesn't complete before the cursor, making sure that works. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102259 Cc: Marta Lofstedt <marta.lofstedt@intel.com> --- tests/kms_cursor_legacy.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)