[igt-dev] [PATCH i-g-t v8] tests/kms_async_flips: Add test to validate asynchronous flips
Karthik B S
karthik.b.s at intel.com
Wed Sep 23 14:12:56 UTC 2020
On 9/23/2020 3:25 PM, Ville Syrjälä wrote:
> On Wed, Sep 23, 2020 at 08:23:44AM +0530, Karthik B S wrote:
>>
>>
>> On 9/18/2020 5:02 PM, Ville Syrjälä wrote:
>>> On Fri, Sep 18, 2020 at 12:13:18PM +0530, Karthik B S wrote:
>>>> Asynchronous flips are issued using the page flip IOCTL.
>>>> The test consists of two subtests. The first subtest waits for
>>>> the page flip event to be received before giving the next flip,
>>>> and the second subtest doesn't wait for page flip events.
>>>>
>>>> The test passes if the IOCTL is successful.
>>>>
>>>> v2: -Add authors in the test file. (Paulo)
>>>> -Reduce the run time and timeouts to suit IGT needs. (Paulo)
>>>> -Replace igt_debug's with igt_assert's to catch slow flips. (Paulo)
>>>> -Follow IGT coding style regarding spaces. (Paulo)
>>>> -Make set up code part of igt_fixture. (Paulo)
>>>> -Skip the test if async flips are not supported. (Paulo)
>>>> -Replace suggested-by. (Paulo)
>>>> -Added description for test and subtests.
>>>>
>>>> v3: -Rename the test to kms_async_flips. (Paulo)
>>>> -Modify the TODO comment. (Paulo)
>>>> -Remove igt_debug in flip_handler. (Paulo)
>>>> -Use drmIoctl() in has_async function. (Paulo)
>>>> -Add more details in igt_assert in flip_handler. (Paulo)
>>>> -Remove flag variable in flip_handler. (Paulo)
>>>> -Call igt_assert in flip_handler after the warm up time.
>>>>
>>>> v4: -Calculate the time stamp in flip_handler from userspace, as the
>>>> kernel will return vbl timestamps and this cannot be used
>>>> for async flips.
>>>> -Add a new subtest to verify that the async flip time stamp
>>>> lies in between the previous and next vblank time stamp. (Daniel)
>>>>
>>>> v5: -Add test that alternates between sync and async flips. (Ville)
>>>> -Add test to verify invalid async flips. (Ville)
>>>> -Remove the subtest async-flip-without-page-flip-events. (Michel)
>>>> -Remove the intel gpu restriction and make the test generic. (Michel)
>>>>
>>>> v6: -Change the THRESHOLD from 10 to 8 as failures are seen on CI
>>>> on platforms <= gen10.
>>>> -In older platforms(<= gen10), async address update bit in plane ctl
>>>> is double buffered. Made changes in subtests to accomodate this.
>>>> -Moved the igt_assert from flip_handler to individual subtest as we
>>>> now have four subtests and adding conditions for the assert in
>>>> flip handler is messy.
>>>>
>>>> v7: -Change flip_interval from int to float for more precision.
>>>> -Remove the fb height change case in 'invalid' subtest as per the
>>>> feedback received on the kernel patches.
>>>> -Add subtest to verify legacy cursor IOCTL. (Ville)
>>>>
>>>> v8: -Add a cursor flip before async flip in cursor test. (Ville)
>>>> -Make flip_interval double for more precision as failures are seen
>>>> on older platforms on CI.
>>>>
>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
>>>> ---
>>>> tests/Makefile.sources | 1 +
>>>> tests/kms_async_flips.c | 428 ++++++++++++++++++++++++++++++++++++++++
>>>> tests/meson.build | 1 +
>>>> 3 files changed, 430 insertions(+)
>>>> create mode 100644 tests/kms_async_flips.c
>>>>
>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>>> index 6ae95155..f32ea9cf 100644
>>>> --- a/tests/Makefile.sources
>>>> +++ b/tests/Makefile.sources
>>>> @@ -32,6 +32,7 @@ TESTS_progs = \
>>>> feature_discovery \
>>>> kms_3d \
>>>> kms_addfb_basic \
>>>> + kms_async_flips \
>>>> kms_atomic \
>>>> kms_atomic_interruptible \
>>>> kms_atomic_transition \
>>>> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
>>>> new file mode 100644
>>>> index 00000000..ef63ec45
>>>> --- /dev/null
>>>> +++ b/tests/kms_async_flips.c
>>>> @@ -0,0 +1,428 @@
>>>> +/*
>>>> + * Copyright © 2020 Intel Corporation
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>>> + * copy of this software and associated documentation files (the "Software"),
>>>> + * to deal in the Software without restriction, including without limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>>> + * Software is furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the next
>>>> + * paragraph) shall be included in all copies or substantial portions of the
>>>> + * Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>>> + * IN THE SOFTWARE.
>>>> + *
>>>> + * Authors:
>>>> + * Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>> + * Karthik B S <karthik.b.s at intel.com>
>>>> + */
>>>> +
>>>> +#include "igt.h"
>>>> +#include "igt_aux.h"
>>>> +#include <sys/ioctl.h>
>>>> +#include <sys/time.h>
>>>> +#include <poll.h>
>>>> +
>>>> +#define BUFS 4
>>>
>>> I'd just truct igt_fb bufs[4], and replace all the other
>>> "BUFS" usages with ARRAY_SIZE(bufs).
>>
>> Thanks for the review.
>> Sure, I'll change this.
>>>
>>>> +#define SYNC_FLIP 0
>>>> +#define ASYNC_FLIP 1
>>>
>>> enum
>>>
>>
>> Will update this.
>>
>>>> +#define CURSOR_RES 64
>>>
>>> Should query that from the kernel so that test is driver agnostic.
>>>
>>
>> Sure. Will change this.
>>>> +#define CURSOR_POS 128
>>>> +
>>>> +/*
>>>> + * These constants can be tuned in case we start getting unexpected
>>>> + * results in CI.
>>>> + */
>>>> +
>>>> +#define WARM_UP_TIME 1
>>>> +#define RUN_TIME 2
>>>> +#define THRESHOLD 8
>>>
>>> A rather non-descriptive name. I guess this is "min flips per frame" or
>>> something along those lines?
>>>
>>
>> Sure. I will change this to MIN_FLIPS_PER_FRAME.
>>>> +
>>>> +IGT_TEST_DESCRIPTION("Test asynchrous page flips.");
>>>
>>> Check the spelling
>>>
>>
>> Will fix this.
>>>> +
>>>> +typedef struct {
>>>> + int drm_fd;
>>>> + uint32_t crtc_id;
>>>> + struct igt_fb bufs[BUFS];
>>>> + igt_display_t display;
>>>> +} data_t;
>>>> +
>>>> +uint32_t refresh_rate;
>>>> +unsigned long flip_timestamp_us;
>>>> +double flip_interval;
>>>
>>> Hmm. I wonder why these are outside the data_t?
>>>
>>
>> Will move the refresh_rate variable inside data_t.
>> The other two are getting updated in flip_handler. Hence kept them
>> global. Would you suggest making data_t global. Or any other way I can
>> handle this better?
>
> It should be possible to pass arbitrary data to the event handler.
>
> <snip>
Thanks for the inputs.
I will update this.
>> Sure. Will add a TODO here and add these changes as a follow up.
>>>> + LOCAL_I915_FORMAT_MOD_Y_TILED, &fb2);
>>>> +
>>>> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
>>>> + fb1.fb_id, flags, NULL);
>>>> +
>>>> + igt_assert(ret == 0);
>>>
>>> Not quite sure why we have this first page flip here?
>>>
>>
>> First to have an async flip with xtiled buffer and then follow it up
>> with an async flip with a y-tiled buffer, so the second one is expected
>> to fail. Could this be done in one flip? Am I missing something here?
>
> What does the fist async flip achieve for us? Nothing AFAICS. We must
> already be scanning out a similar fb or else the first async flip would
> also fail.
>
> If it's possible that we are not already scanning out a similar fb to
> the first one then we should do a full modeset/atomic commit to make
> it so. Otherwise the second async flip is going to be testing random
> things.
>
Got it. I misunderstood this part earlier.
Will remove the first flip in the next revision.
Thanks,
Karthik.B.S
>>>> +
>>>> + wait_flip_event(data);
>>>> +
>>>> + /* Flip with a different fb modifier which is expected to be rejected */
>>>> + ret = drmModePageFlip(data->drm_fd, data->crtc_id,
>>>> + fb2.fb_id, flags, NULL);
>>>> +
>
More information about the igt-dev
mailing list