[Intel-gfx] [PATCH i-g-t v3 10/11] tests/kms_atomic_transition: add out_fences tests
Robert Foss
robert.foss at collabora.com
Wed Feb 1 00:27:51 UTC 2017
On 2017-01-31 11:52 AM, Brian Starkey wrote:
> On Mon, Jan 30, 2017 at 08:58:46PM -0500, Robert Foss wrote:
>> Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>> Signed-off-by: Robert Foss <robert.foss at collabora.com>
>> ---
>> lib/igt_kms.c | 35 ++++++----
>> tests/kms_atomic_transition.c | 148
>> ++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 169 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index f14496dd..523f3f57 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -53,6 +53,7 @@
>> #include "intel_chipset.h"
>> #include "igt_debugfs.h"
>> #include "igt_sysfs.h"
>> +#include "sw_sync.h"
>>
>> /**
>> * SECTION:igt_kms
>> @@ -2479,6 +2480,21 @@ static int igt_atomic_commit(igt_display_t
>> *display, uint32_t flags, void *user_
>> }
>>
>> ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data);
>> + if (!ret) {
>> +
>> + for_each_pipe(display, pipe) {
>> + igt_pipe_t *pipe_obj = &display->pipes[pipe];
>> +
>> + if (pipe_obj->out_fence != -1)
>> + continue;
>
> Should be "if (pipe_obj->fence == -1)" ? The stuff below seems to be
> assuming the fence is valid.
>
Correct, fixed in v4.
>> +
>> + igt_assert(pipe_obj->out_fence >= 0);
>> + ret = sync_fence_wait(pipe_obj->out_fence, 1000);
>> + igt_assert(ret == 0);
>> + close(pipe_obj->out_fence);
>> + }
>> + }
>> +
>> drmModeAtomicFree(req);
>> return ret;
>>
>> @@ -2972,6 +2988,11 @@ void igt_plane_set_rotation(igt_plane_t *plane,
>> igt_rotation_t rotation)
>> plane->rotation_changed = true;
>> }
>>
>> +void igt_pipe_request_out_fence(igt_pipe_t *pipe)
>
> Oh here it is!
>
>> +{
>> + pipe->out_fence_requested = true;
>> +}
>> +
>> void
>> igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
>> {
>> @@ -2994,20 +3015,6 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void
>> *ptr, size_t length)
>> }
>>
>> /**
>> - * igt_pipe_set_out_fence_ptr:
>> - * @pipe: pipe pointer to which background color to be set
>> - * @fence_ptr: out fence pointer
>> - *
>> - * Sets the out fence pointer that will be passed to the kernel in
>> - * the atomic ioctl. When the kernel returns the out fence pointer
>> - * will contain the fd number of the out fence created by KMS.
>> - */
>> -void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr)
>
> ... and there this one goes. This deletion and the function above
> need to be squashed into the earlier commit I suppose.
>
Ack, fixed in v4.
>> -{
>> - pipe->out_fence_ptr = fence_ptr;
>> -}
>> -
>> -/**
>> * igt_crtc_set_background:
>> * @pipe: pipe pointer to which background color to be set
>> * @background: background color value in BGR 16bpc
>> diff --git a/tests/kms_atomic_transition.c
>> b/tests/kms_atomic_transition.c
>> index 72429759..eebb5d66 100644
>> --- a/tests/kms_atomic_transition.c
>> +++ b/tests/kms_atomic_transition.c
>> @@ -253,6 +253,93 @@ retry:
>> sprite_width, sprite_height, alpha);
>> }
>>
>> +int *timeline;
>> +pthread_t *thread;
>> +int *seqno;
>> +
>> +static void prepare_fencing(igt_display_t *display, enum pipe pipe)
>> +{
>> + igt_plane_t *plane;
>> + int n_planes;
>> +
>> + igt_require_sw_sync();
>> +
>> + n_planes = display->pipes[pipe].n_planes;
>> + timeline = calloc(sizeof(*timeline), n_planes);
>> + igt_assert_f(timeline != NULL, "Failed to allocate memory for
>> timelines\n");
>> + thread = calloc(sizeof(*thread), n_planes);
>> + igt_assert_f(thread != NULL, "Failed to allocate memory for
>> thread\n");
>> + seqno = calloc(sizeof(*seqno), n_planes);
>> + igt_assert_f(seqno != NULL, "Failed to allocate memory for
>> seqno\n");
>
> The 6 lines above are space-indented
>
Ack, fixed in v4.
>> +
>> + for_each_plane_on_pipe(display, pipe, plane)
>> + timeline[plane->index] = sw_sync_timeline_create();
>> +}
>> +
>> +static void unprepare_fencing(igt_display_t *display, enum pipe pipe)
>> +{
>> + igt_plane_t *plane;
>> +
>> + for_each_plane_on_pipe(display, pipe, plane)
>> + close(timeline[plane->index]);
>> +
>> + free(timeline);
>> + free(thread);
>> + free(seqno);
>> +}
>> +
>> +static void *fence_inc_thread(void *arg)
>> +{
>> + int t = *((int *) arg);
>> +
>> + pthread_detach(pthread_self());
>> +
>> + usleep(5000);
>> + sw_sync_timeline_inc(t, 1);
>> + return NULL;
>> +}
>> +
>> +static void configure_fencing(igt_display_t *display, enum pipe pipe)
>> +{
>> + igt_plane_t *plane;
>> + int i, fd, ret;
>> +
>> + for_each_plane_on_pipe(display, pipe, plane) {
>> +
>> + if (!plane->fb)
>> + continue;
>> +
>> + i = plane->index;
>> +
>> + seqno[i]++;
>> + fd = sw_sync_timeline_create_fence(timeline[i], seqno[i]);
>> + igt_plane_set_fence_fd(plane, fd);
>> + ret = pthread_create(&thread[i], NULL, fence_inc_thread,
>> &timeline[i]);
>> + igt_assert_eq(ret, 0);
>> + }
>> +}
>> +
>> +static void clear_fencing(igt_display_t *display, enum pipe pipe)
>> +{
>> + igt_plane_t *plane;
>> +
>> + for_each_plane_on_pipe(display, pipe, plane)
>> + igt_plane_set_fence_fd(plane, -1);
>
> Someone should close the old fence_fd if it's not -1.
>
> I think igt_plane_set_fence_fd() should dup() the passed in fence, and
> igt_display_atomic_commit() should set it to -1 after the commit.
>
> That makes it very obvious, and that way igt_plane_set_fence_fd() can
> always safely close plane->fence_fd if it gets called while the
> current fence_fd is valid, because it always owns that fd. Callers
> have their own copy to take care of.
>
> In any case, I think you're leaking fds here.
>
Fixed in v4:
- Implemented igt_plane_set_fence_fd logic
- Added close() to callers with valid FDs
>> +}
>> +
>> +static void atomic_commit(igt_display_t *display, enum pipe pipe,
>> unsigned int flags, void *data, bool fencing)
>> +{
>> + if (fencing) {
>> + configure_fencing(display, pipe);
>> + igt_pipe_request_out_fence(&display->pipes[pipe]);
>> + }
>> +
>> + igt_display_commit_atomic(display, flags, data);
>> +
>> + if (fencing)
>> + clear_fencing(display, pipe);
>> +}
>> +
>> /*
>> * 1. Set primary plane to a known fb.
>> * 2. Make sure getcrtc returns the correct fb id.
>> @@ -273,6 +360,7 @@ run_transition_test(igt_display_t *display, enum
>> pipe pipe, igt_output_t *output
>> struct plane_parms parms[display->pipes[pipe].n_planes];
>> bool skip_test = false;
>> unsigned flags = DRM_MODE_PAGE_FLIP_EVENT;
>> + int ret;
>>
>> if (nonblocking)
>> flags |= DRM_MODE_ATOMIC_NONBLOCK;
>> @@ -307,11 +395,50 @@ run_transition_test(igt_display_t *display, enum
>> pipe pipe, igt_output_t *output
>>
>> setup_parms(display, pipe, mode, &argb_fb, &sprite_fb, parms);
>>
>> + /*
>> + * In some configurations the tests may not run to completion
>> with all
>> + * sprite planes lit up at 4k resolution, try decreasing
>> width/size of secondary
>> + * planes to fix this
>> + */
>> + while (1) {
>> + wm_setup_plane(display, pipe, iter_max - 1, parms);
>> +
>> + if (fencing)
>> + igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
>
> That function was deleted in this patch.
Fixed in v4.
>
>> + (int64_t *) &out_fence);
>> + ret = igt_display_try_commit_atomic(display,
>> DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>> +
>> + if (ret != -EINVAL || n_planes < 3)
>> + break;
>> +
>> + ret = 0;
>> + for_each_plane_on_pipe(display, pipe, plane) {
>> + i = plane->index;
>> +
>> + if (plane->is_primary || plane->is_cursor)
>> + continue;
>> +
>> + if (parms[i].width <= 512)
>> + continue;
>> +
>> + parms[i].width /= 2;
>> + ret = 1;
>> + igt_info("Reducing sprite %i to %ux%u\n", i - 1,
>> parms[i].width, parms[i].height);
>> + break;
>> + }
>> +
>> + if (!ret)
>> + igt_skip("Cannot run tests without proper size sprite
>> planes\n");
>> + }
>> +
>> for (i = 0; i < iter_max; i++) {
>> igt_output_set_pipe(output, pipe);
>>
>> wm_setup_plane(display, pipe, i, parms);
>>
>> + if (fencing)
>> + igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
>> &out_fence);
>
> Same again.
Fixed in v4.
>
>> +
>> igt_display_commit_atomic(display, flags, (void *)(unsigned
>> long)i);
>> drmHandleEvent(display->drm_fd, &drm_events);
>>
>> @@ -320,6 +447,9 @@ run_transition_test(igt_display_t *display, enum
>> pipe pipe, igt_output_t *output
>>
>> wm_setup_plane(display, pipe, 0, parms);
>>
>> + if (fencing)
>> + igt_pipe_request_out_fence(&display->pipes[pipe]);
>> +
>> igt_display_commit_atomic(display, flags, (void *)0UL);
>>
>> drmHandleEvent(display->drm_fd, &drm_events);
>> @@ -333,6 +463,9 @@ 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);
>
> etc.
Fixed in v4.
>
> -Brian
>
>> +
>> igt_display_commit_atomic(display, flags, (void
>> *)(unsigned long)j);
>> drmHandleEvent(display->drm_fd, &drm_events);
>>
>> @@ -340,6 +473,9 @@ run_transition_test(igt_display_t *display, enum
>> pipe pipe, igt_output_t *output
>> if (type == TRANSITION_MODESET)
>> igt_output_override_mode(output, NULL);
>>
>> + if (fencing)
>> + igt_pipe_set_out_fence_ptr(&display->pipes[pipe],
>> &out_fence);
>> +
>> igt_display_commit_atomic(display, flags, (void
>> *)(unsigned long)i);
>> drmHandleEvent(display->drm_fd, &drm_events);
>> }
>> @@ -676,14 +812,26 @@ igt_main
>> for_each_pipe_with_valid_output(&display, pipe, output)
>> run_transition_test(&display, pipe, output,
>> TRANSITION_PLANES, false, false);
>>
>> + igt_subtest("plane-all-transition-fencing")
>> + for_each_pipe_with_valid_output(&display, pipe, output)
>> + run_transition_test(&display, pipe, output,
>> TRANSITION_PLANES, false, true);
>> +
>> igt_subtest("plane-all-transition-nonblocking")
>> for_each_pipe_with_valid_output(&display, pipe, output)
>> run_transition_test(&display, pipe, output,
>> TRANSITION_PLANES, true, false);
>>
>> + igt_subtest("plane-all-transition-nonblocking-fencing")
>> + for_each_pipe_with_valid_output(&display, pipe, output)
>> + run_transition_test(&display, pipe, output,
>> TRANSITION_PLANES, true, true);
>> +
>> igt_subtest("plane-all-modeset-transition")
>> for_each_pipe_with_valid_output(&display, pipe, output)
>> run_transition_test(&display, pipe, output,
>> TRANSITION_MODESET, false, false);
>>
>> + igt_subtest("plane-all-modeset-transition-fencing")
>> + for_each_pipe_with_valid_output(&display, pipe, output)
>> + run_transition_test(&display, pipe, output,
>> TRANSITION_MODESET, false, true);
>> +
>> igt_subtest("plane-toggle-modeset-transition")
>> for_each_pipe_with_valid_output(&display, pipe, output)
>> run_transition_test(&display, pipe, output,
>> TRANSITION_MODESET_DISABLE, false, false);
>> --
>> 2.11.0.453.g787f75f05
>>
More information about the Intel-gfx
mailing list