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

Robert Foss
2017-02-01T01:11:21Z

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@collabora.co.uk>
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>> 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
>>