[Intel-gfx] [PATCH i-g-t v3 4/4] kms_vblank: Switch from using crtc0 statically to explicitly setting mode.
Tomeu Vizoso
tomeu.vizoso at collabora.com
Tue Apr 26 12:32:10 UTC 2016
On 25 April 2016 at 17:05, <robert.foss at collabora.com> wrote:
> From: Robert Foss <robert.foss at collabora.com>
>
> Previously crtc0 was statically used for VBLANK tests, but
> that assumption is not valid for the VC4 platform.
> Instead we're now explicitly setting the mode.
>
> Also add support for testing all connected connectors during
> the same test.
Is there any reason why this cannot be done in a subsequent patch?
> Signed-off-by: Robert Foss <robert.foss at collabora.com>
> ---
> tests/kms_vblank.c | 179 +++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 140 insertions(+), 39 deletions(-)
>
> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> index 40ab6fd..970fefe 100644
> --- a/tests/kms_vblank.c
> +++ b/tests/kms_vblank.c
> @@ -44,6 +44,14 @@
>
> IGT_TEST_DESCRIPTION("Test speed of WaitVblank.");
>
> +typedef struct {
> + int drm_fd;
> + igt_display_t display;
> + struct igt_fb primary_fb;
> + igt_output_t *output;
> + enum pipe pipe;
> +} data_t;
> +
> static double elapsed(const struct timespec *start,
> const struct timespec *end,
> int loop)
> @@ -51,75 +59,164 @@ static double elapsed(const struct timespec *start,
> return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_nsec - start->tv_nsec)/1000)/loop;
> }
>
> -static bool crtc0_active(int fd)
> +static uint32_t crtc_id_to_flag(uint32_t crtc_id)
This seems to be pipe id, not the crtc id. Maybe this belongs to the library.
> {
> - union drm_wait_vblank vbl;
> + if (crtc_id == 0)
> + return 0;
> + else if (crtc_id == 1)
> + return 1 | _DRM_VBLANK_SECONDARY;
> + else
> + return crtc_id << 1;
> +}
>
> - memset(&vbl, 0, sizeof(vbl));
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> - return drmIoctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl) == 0;
> +static bool prepare_crtc(data_t *data, igt_output_t *output)
> +{
> + drmModeModeInfo *mode;
> + igt_display_t *display = &data->display;
> + igt_plane_t *primary;
> +
> + /* select the pipe we want to use */
> + igt_output_set_pipe(output, data->pipe);
> + igt_display_commit(display);
> +
> + if (!output->valid) {
> + igt_output_set_pipe(output, PIPE_ANY);
> + igt_display_commit(display);
> + return false;
> + }
> +
> + /* create and set the primary plane fb */
> + mode = igt_output_get_mode(output);
I thought the plan was to set a mode, instead of relying on one being
already set (eg. by fbcon)? Otherwise, I don't see why this cannot be
in the library?
> + igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> + DRM_FORMAT_XRGB8888,
> + LOCAL_DRM_FORMAT_MOD_NONE,
> + 0.0, 0.0, 0.0,
> + &data->primary_fb);
> +
> + primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> + igt_plane_set_fb(primary, &data->primary_fb);
> +
> + igt_display_commit(display);
> +
> + igt_wait_for_vblank(data->drm_fd, data->pipe);
> +
> + return true;
> +}
> +
> +static void cleanup_crtc(data_t *data, igt_output_t *output)
> +{
> + igt_display_t *display = &data->display;
> + igt_plane_t *primary;
> +
> + igt_remove_fb(data->drm_fd, &data->primary_fb);
> +
> + primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> + igt_plane_set_fb(primary, NULL);
> +
> + igt_output_set_pipe(output, PIPE_ANY);
> + igt_display_commit(display);
> }
>
> -static void accuracy(int fd)
> +static void run_test(data_t *data, void (*testfunc)(data_t *, bool), bool boolean)
Probably not a big deal, but I think it's more customary to have a
bitfield in data_t that tells what behavior the test should have,
rather than passing a callback.
> +{
> + igt_display_t *display = &data->display;
> + igt_output_t *output;
> + enum pipe p;
> + int valid_tests = 0;
unsigned
> +
> + for_each_connected_output(display, output) {
> + for_each_pipe(display, p) {
> + data->pipe = p;
> +
> + if (!prepare_crtc(data, output))
> + continue;
> +
> + valid_tests++;
> +
> + igt_info("Beginning %s on pipe %s, connector %s\n",
> + igt_subtest_name(),
> + kmstest_pipe_name(data->pipe),
> + igt_output_name(output));
> +
> + testfunc(data, boolean);
> +
> + igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
> + igt_subtest_name(),
> + kmstest_pipe_name(data->pipe),
> + igt_output_name(output));
> +
> + /* cleanup what prepare_crtc() has done */
> + cleanup_crtc(data, output);
> + }
> + }
> +
> + igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
> +}
> +
> +static void accuracy(data_t *data, bool busy)
> {
> union drm_wait_vblank vbl;
> unsigned long target;
> + uint32_t crtc_id_flag;
> int n;
>
> memset(&vbl, 0, sizeof(vbl));
> + crtc_id_flag = crtc_id_to_flag(data->pipe);
>
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 1;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
Don't see much point in moving the drm_fd into the data struct, I
think I would just make fd a global and avoid these changes.
> target = vbl.reply.sequence + 60;
> for (n = 0; n < 60; n++) {
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 1;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>
> - vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
> + vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT | crtc_id_flag;
> vbl.request.sequence = target;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> }
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 0;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> igt_assert_eq(vbl.reply.sequence, target);
>
> for (n = 0; n < 60; n++) {
> struct drm_event_vblank ev;
> - igt_assert_eq(read(fd, &ev, sizeof(ev)), sizeof(ev));
> + igt_assert_eq(read(data->drm_fd, &ev, sizeof(ev)), sizeof(ev));
> igt_assert_eq(ev.sequence, target);
> }
> }
>
> -static void vblank_query(int fd, bool busy)
> +static void vblank_query(data_t *data, bool busy)
> {
> union drm_wait_vblank vbl;
> struct timespec start, end;
> unsigned long sq, count = 0;
> struct drm_event_vblank buf;
> + uint32_t crtc_id_flag;
>
> memset(&vbl, 0, sizeof(vbl));
> + crtc_id_flag = crtc_id_to_flag(data->pipe);
>
> if (busy) {
> - vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> + vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT | crtc_id_flag;
> vbl.request.sequence = 72;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> }
>
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 0;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>
> sq = vbl.reply.sequence;
>
> clock_gettime(CLOCK_MONOTONIC, &start);
> do {
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 0;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> count++;
> } while ((vbl.reply.sequence - sq) <= 60);
> clock_gettime(CLOCK_MONOTONIC, &end);
> @@ -128,35 +225,37 @@ static void vblank_query(int fd, bool busy)
> busy ? "busy" : "idle", elapsed(&start, &end, count));
>
> if (busy)
> - igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> + igt_assert_eq(read(data->drm_fd, &buf, sizeof(buf)), sizeof(buf));
> }
>
> -static void vblank_wait(int fd, bool busy)
> +static void vblank_wait(data_t *data, bool busy)
> {
> union drm_wait_vblank vbl;
> struct timespec start, end;
> unsigned long sq, count = 0;
> struct drm_event_vblank buf;
> + uint32_t crtc_id_flag;
>
> memset(&vbl, 0, sizeof(vbl));
> + crtc_id_flag = crtc_id_to_flag(data->pipe);
>
> if (busy) {
> - vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> + vbl.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT | crtc_id_flag;
I would personally just add such a line afterwards, find it more
readable (here and elsewhere):
vbl.request.type |= crtc_id_flag;
> vbl.request.sequence = 72;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> }
>
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type |= DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 0;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
>
> sq = vbl.reply.sequence;
>
> clock_gettime(CLOCK_MONOTONIC, &start);
> do {
> - vbl.request.type = DRM_VBLANK_RELATIVE;
> + vbl.request.type = DRM_VBLANK_RELATIVE | crtc_id_flag;
> vbl.request.sequence = 1;
> - do_ioctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> + do_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl);
> count++;
> } while ((vbl.reply.sequence - sq) <= 60);
> clock_gettime(CLOCK_MONOTONIC, &end);
> @@ -167,32 +266,34 @@ static void vblank_wait(int fd, bool busy)
> elapsed(&start, &end, count));
>
> if (busy)
> - igt_assert_eq(read(fd, &buf, sizeof(buf)), sizeof(buf));
> + igt_assert_eq(read(data->drm_fd, &buf, sizeof(buf)), sizeof(buf));
> }
>
> igt_main
> {
> - int fd;
> + data_t data;
>
> igt_skip_on_simulation();
>
> igt_fixture {
> - fd = drm_open_driver(DRIVER_ANY);
> - igt_require(crtc0_active(fd));
> + data.drm_fd = drm_open_driver(DRIVER_ANY);
> + kmstest_set_vt_graphics_mode();
> + igt_display_init(&data.display, data.drm_fd);
> }
>
> igt_subtest("accuracy")
> - accuracy(fd);
> + run_test(&data, accuracy, false);
>
> igt_subtest("query-idle")
> - vblank_query(fd, false);
> + run_test(&data, vblank_query, false);
>
> igt_subtest("query-busy")
> - vblank_query(fd, true);
> + run_test(&data, vblank_query, true);
>
> igt_subtest("wait-idle")
> - vblank_wait(fd, false);
> + run_test(&data, vblank_wait, false);
>
> igt_subtest("wait-busy")
> - vblank_wait(fd, true);
> + run_test(&data, vblank_wait, true);
> }
> +
Thanks!
Tomeu
More information about the Intel-gfx
mailing list