[igt-dev] [PATCH] tests/kms_flip: Discard any stale event at each retry

[Arkadiusz Hiler]

On Wed, Jul 28, 2021 at 01:16:16PM -0400, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub at google.com>
> 
> [Why]
> On slower devices, non blocking do_page_flip followed by wait_for_events does not finish fast enough to send an event to be consumed by wait_for_events.

This sentence should be wrapped like the rest of the commit message.

> Instead, wait_for_events ends up finding an unconsumed event from a
> previous do_page_flip (i.e. a skipped preceding subtest) and signals
> that the userspace is ready to do another do_page_flip while the first
> one hasn't finished yet, resulting in an EBUSY.
> 
> [How]
> At the beginning of every test running on crtc set, check for any stale
> events and consume them so the kernel has no pending events.

I am not opposed to this approach, it's nice to know that we can trust
the state as the test start, but I wonder where and how do we leak those
pending flips? Have you looked into where those are coming from?

> Fixes: flip-vs-suspend and flip-vs-suspend-interruptible.
> Tested on ChromeOS Volteer(i915-TGL) and Trogdor(msm)
> 
> Signed-off-by: Mark Yacoub <markyacoub at chromium.org>
> Change-Id: I254a84a202140c332312aba1d9edf15ea78b7a61

It would be nice to not include your internal gerrit's Change-Id
(or discard them upon commit). Those does not mean anything in the
upstream.

> ---
>  tests/kms_flip.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> index f2fce8d2..0a2041c5 100755
> --- a/tests/kms_flip.c
> +++ b/tests/kms_flip.c
> @@ -1261,6 +1261,26 @@ static bool needs_retry_after_link_reset(struct udev_monitor *mon)
>  	return hotplug_detected;
>  }
>  
> +static void discard_any_stale_events() {
> +	fd_set fds;
> +	FD_ZERO(&fds);
> +	FD_SET(drm_fd, &fds);
> +	struct timeval timeout = { .tv_sec = 3, .tv_usec = 0 };

Is the 3s timeout necessary? I can imagine that in a lot of cases this
will end up being sleep(3), as we don't expect any stale events in a
normal case. That's a significant time increase if you count all the
invocations.

> +	int ret = select(drm_fd + 1, &fds, NULL, NULL, &timeout);
> +
> +	if (ret > 0) {
> +		drmEventContext evctx;
> +		memset(&evctx, 0, sizeof evctx);
> +		evctx.version = 2;
> +		igt_info("Stale Event found - Discarding now\n");
> +		drmHandleEvent(drm_fd, &evctx);
> +	}
> +	else {
> +		igt_debug("No stale Event found\n");
> +	}
> +}

I think it should plural (and lowercase) "events" as it will go through
all the pending events on that fd.

> +
> +
>  static void __run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  				   int crtc_count, int duration_ms)
>  {
> @@ -1311,6 +1331,9 @@ static void __run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  		kmstest_dump_mode(&o->kmode[i]);
>  
>  retry:
> +	// Discard any pending event that hasn't been consumed from a previous retry or subtest.

Generally C-style comments (/* bla */) are preferred, but I don't think
IGT or the kernel coding style explicitly mentions that. Also there are
occurrences of C++-style comments in the code already. Oh well...

All the cosmetic nitpicks can be fixed by me or another committer when
merging the patch (assuming that you are ok with them), so there's not
need to resend unless you want to make changes to the actual code.

Cheers,
Arek

> +	discard_any_stale_events();
> +
>  	memset(&o->vblank_state, 0, sizeof(o->vblank_state));
>  	memset(&o->flip_state, 0, sizeof(o->flip_state));
>  	o->flip_state.name = "flip";
> -- 
> 2.32.0.432.gabb21c7263-goog