Re: [PATCH i-g-t v3 11/11] tests/kms_atomic_transition: add in_fences tests

[Robert Foss <robert.foss@xxxxxxxxxxxxx>]

On 2017-01-31 11:52 AM, Brian Starkey wrote:
> On Mon, Jan 30, 2017 at 08:58:47PM -0500, Robert Foss wrote:
> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> > ---
> > lib/igt_kms.c                 |  3 +++
> > tests/kms_atomic_transition.c | 48
> > ++++++++++++++++++++++++++-----------------
> > 2 files changed, 32 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 523f3f57..bc815363 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -2009,6 +2009,9 @@ igt_atomic_prepare_plane_commit(igt_plane_t
> > *plane, igt_pipe_t *pipe,
> >     if (plane->fence_fd >= 0) {
> >         uint64_t fence_fd = (int64_t) plane->fence_fd;
> >         igt_atomic_populate_plane_req(req, plane,
> > IGT_PLANE_IN_FENCE_FD, fence_fd);
> > +
> > +        /* reset fence_fd to prevent it from being set for the next
> > commit */
> > +        plane->fence_fd = -1;
>
> Who closes it?

Fixed in v4.

> >     }
> >
> >     if (plane->fb_changed) {
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index eebb5d66..0876bbb3 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -23,7 +23,9 @@
> >
> > #include "igt.h"
> > #include "drmtest.h"
> > +#include "sw_sync.h"
> > #include <errno.h>
> > +#include <pthread.h>
> > #include <stdbool.h>
> > #include <stdio.h>
> > #include <string.h>
> > @@ -362,6 +364,9 @@ run_transition_test(igt_display_t *display, enum
> > pipe pipe, igt_output_t *output
> >     unsigned flags = DRM_MODE_PAGE_FLIP_EVENT;
> >     int ret;
> >
> > +    if (fencing)
> > +        prepare_fencing(display, pipe);
> > +
> >     if (nonblocking)
> >         flags |= DRM_MODE_ATOMIC_NONBLOCK;
> >
> > @@ -404,18 +409,19 @@ run_transition_test(igt_display_t *display, enum
> > pipe pipe, igt_output_t *output
> >         wm_setup_plane(display, pipe, iter_max - 1, parms);
> >
> >         if (fencing)
> > -            igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
> > -                (int64_t *) &out_fence);
> > +            igt_pipe_request_out_fence(&display->pipes[pipe]);
> > +
>
> Hopefully this can get rebased away? I'm getting confused about
> what's actually being added/changed in this commit.

Fixed in v4.
The igt_pipe_set_out_fence_ptr calls were really spread about.

> >         ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> >
> > -        if (ret != -EINVAL || n_planes < 3)
> > +        if (ret != -EINVAL || display->pipes[pipe].n_planes < 3)
> >             break;
> >
> >         ret = 0;
> >         for_each_plane_on_pipe(display, pipe, plane) {
> >             i = plane->index;
> >
> > -            if (plane->is_primary || plane->is_cursor)
> > +            if (plane->type == DRM_PLANE_TYPE_PRIMARY ||
> > +                plane->type == DRM_PLANE_TYPE_CURSOR)
> >                 continue;
>
> This seems spurious?
>
> ...
>
> A bit more add/remove churn below which can hopefully go away with a
> rebase.

Ack, removing churn in v4.

> Cheers,
> -Brian
> >             if (parms[i].width <= 512)
> > @@ -436,10 +442,8 @@ run_transition_test(igt_display_t *display, enum
> > pipe pipe, igt_output_t *output
> >
> >         wm_setup_plane(display, pipe, i, parms);
> >
> > -        if (fencing)
> > -            igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
> > &out_fence);
> > +        atomic_commit(display, pipe, flags, (void *)(unsigned long)i,
> > fencing);
> >
> > -        igt_display_commit_atomic(display, flags, (void *)(unsigned
> > long)i);
> >         drmHandleEvent(display->drm_fd, &drm_events);
> >
> >         if (type == TRANSITION_MODESET_DISABLE) {
> > @@ -463,19 +467,14 @@ run_transition_test(igt_display_t *display, enum
> > pipe pipe, igt_output_t *output
> >                 if (type == TRANSITION_MODESET)
> >                     igt_output_override_mode(output, &override_mode);
> >
> > -                if (fencing)
> > -                    igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
> > &out_fence);
> > -
> > -                igt_display_commit_atomic(display, flags, (void
> > *)(unsigned long)j);
> > +                atomic_commit(display, pipe, flags, (void *)(unsigned
> > long)i, fencing);
> >                 drmHandleEvent(display->drm_fd, &drm_events);
> >
> >                 wm_setup_plane(display, pipe, i, parms);
> >                 if (type == TRANSITION_MODESET)
> >                     igt_output_override_mode(output, NULL);
> >
> > -                if (fencing)
> > -                    igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
> > &out_fence);
> > -
> > +                atomic_commit(display, pipe, flags, (void *)(unsigned
> > long)i, fencing);
> >                 igt_display_commit_atomic(display, flags, (void
> > *)(unsigned long)i);
> >                 drmHandleEvent(display->drm_fd, &drm_events);
> >             }
> > @@ -483,6 +482,8 @@ run_transition_test(igt_display_t *display, enum
> > pipe pipe, igt_output_t *output
> >     }
> >
> > cleanup:
> > +    unprepare_fencing(display, pipe);
> > +
> >     igt_output_set_pipe(output, PIPE_NONE);
> >
> >     for_each_plane_on_pipe(display, pipe, plane)
> > @@ -617,7 +618,7 @@ static void collect_crcs_mask(igt_pipe_crc_t
> > **pipe_crcs, unsigned mask, igt_crc
> >     }
> > }
> >
> > -static void run_modeset_tests(igt_display_t *display, int howmany,
> > bool nonblocking)
> > +static void run_modeset_tests(igt_display_t *display, int howmany,
> > bool nonblocking, bool fencing)
> > {
> >     struct igt_fb fbs[2];
> >     int i, j;
> > @@ -664,6 +665,9 @@ static void run_modeset_tests(igt_display_t
> > *display, int howmany, bool nonblock
> >             igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> >         } else
> >             igt_plane_set_fb(plane, NULL);
> > +
> > +        if(fencing)
> > +            igt_pipe_request_out_fence(&display->pipes[i]);
> >     }
> >
> >     /*
> > @@ -751,7 +755,7 @@ cleanup:
> >
> > }
> >
> > -static void run_modeset_transition(igt_display_t *display, int
> > requested_outputs, bool nonblocking)
> > +static void run_modeset_transition(igt_display_t *display, int
> > requested_outputs, bool nonblocking, bool fencing)
> > {
> >     igt_output_t *outputs[I915_MAX_PIPES] = {};
> >     int num_outputs = 0;
> > @@ -779,7 +783,7 @@ static void run_modeset_transition(igt_display_t
> > *display, int requested_outputs
> >               "Should have at least %i outputs, found %i\n",
> >               requested_outputs, num_outputs);
> >
> > -    run_modeset_tests(display, requested_outputs, nonblocking);
> > +    run_modeset_tests(display, requested_outputs, nonblocking, fencing);
> > }
> >
> > igt_main
> > @@ -838,10 +842,16 @@ igt_main
> >
> >     for (i = 1; i <= I915_MAX_PIPES; i++) {
> >         igt_subtest_f("%ix-modeset-transitions", i)
> > -            run_modeset_transition(&display, i, false);
> > +            run_modeset_transition(&display, i, false, false);
> >
> >         igt_subtest_f("%ix-modeset-transitions-nonblocking", i)
> > -            run_modeset_transition(&display, i, true);
> > +            run_modeset_transition(&display, i, true, false);
> > +
> > +        igt_subtest_f("%ix-modeset-transitions-fencing", i)
> > +            run_modeset_transition(&display, i, false, true);
> > +
> > +        igt_subtest_f("%ix-modeset-transitions-nonblocking-fencing", i)
> > +            run_modeset_transition(&display, i, true, true);
> >     }
> >
> >     igt_fixture {
> > --
> > 2.11.0.453.g787f75f05
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx