[i-g-t,1/3] lib/igt_kms: Drop all stale events on first commit.
diff mbox

Message ID 20171207134027.61108-1-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Dec. 7, 2017, 1:40 p.m. UTC
I've been trying to make kms_cursor_legacy work when subtests fail.
Other subtests will start failing too because of expired events or
stale pipe crc. The latter can be resolved in the test, but the former
could affect other tests

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
 lib/igt_kms.h |  1 +
 2 files changed, 39 insertions(+), 1 deletion(-)

Comments

Chris Wilson Dec. 7, 2017, 3:03 p.m. UTC | #1
Quoting Maarten Lankhorst (2017-12-07 13:40:25)
> I've been trying to make kms_cursor_legacy work when subtests fail.
> Other subtests will start failing too because of expired events or
> stale pipe crc. The latter can be resolved in the test, but the former
> could affect other tests
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  lib/igt_kms.h |  1 +
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 223dbe4ca565..9e14f071ea57 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2943,7 +2943,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>                         output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
>         }
>  
> -       display->first_commit = false;
> +       if (display->first_commit) {
> +               igt_display_drop_events(display);
> +               display->first_commit = false;
> +       }

So I spent quite a bit of time debating whether there is merit in "do
something; set-first-mode" that this would then clobber. After some
thought, no that doesn't seem like a wise test construction. I would
however suggest that we igt_debug() if we drop any events here.
-Chris
Maarten Lankhorst Dec. 7, 2017, 3:42 p.m. UTC | #2
Op 07-12-17 om 16:03 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2017-12-07 13:40:25)
>> I've been trying to make kms_cursor_legacy work when subtests fail.
>> Other subtests will start failing too because of expired events or
>> stale pipe crc. The latter can be resolved in the test, but the former
>> could affect other tests
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>  lib/igt_kms.h |  1 +
>>  2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 223dbe4ca565..9e14f071ea57 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -2943,7 +2943,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>>                         output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
>>         }
>>  
>> -       display->first_commit = false;
>> +       if (display->first_commit) {
>> +               igt_display_drop_events(display);
>> +               display->first_commit = false;
>> +       }
> So I spent quite a bit of time debating whether there is merit in "do
> something; set-first-mode" that this would then clobber. After some
> thought, no that doesn't seem like a wise test construction. I would
> however suggest that we igt_debug() if we drop any events here.
> -Chris

Exactly. I'm using igt_info since it's supposed to be uncommon, is it ok if I upgrade it to a igt_warn() since nothing in CI will trigger it?

~Maarten
Chris Wilson Dec. 7, 2017, 3:50 p.m. UTC | #3
Quoting Maarten Lankhorst (2017-12-07 15:42:54)
> Op 07-12-17 om 16:03 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2017-12-07 13:40:25)
> >> I've been trying to make kms_cursor_legacy work when subtests fail.
> >> Other subtests will start failing too because of expired events or
> >> stale pipe crc. The latter can be resolved in the test, but the former
> >> could affect other tests
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >>  lib/igt_kms.h |  1 +
> >>  2 files changed, 39 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >> index 223dbe4ca565..9e14f071ea57 100644
> >> --- a/lib/igt_kms.c
> >> +++ b/lib/igt_kms.c
> >> @@ -2943,7 +2943,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
> >>                         output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
> >>         }
> >>  
> >> -       display->first_commit = false;
> >> +       if (display->first_commit) {
> >> +               igt_display_drop_events(display);
> >> +               display->first_commit = false;
> >> +       }
> > So I spent quite a bit of time debating whether there is merit in "do
> > something; set-first-mode" that this would then clobber. After some
> > thought, no that doesn't seem like a wise test construction. I would
> > however suggest that we igt_debug() if we drop any events here.
> > -Chris
> 
> Exactly. I'm using igt_info since it's supposed to be uncommon, is it ok if I upgrade it to a igt_warn() since nothing in CI will trigger it?

Oh, I wouldn't have put it inside drop_events itself, since that is just
doing its job -- whether or not it is relevant depends on the caller. So
I would suggest reducing it to igt_debug, or just reporting the number
dropped and making the judgement in the caller.

No, I don't this should be an igt_warn, as then CI blames the following
test for environmental errors left over from previous runs.
-Chris
Maarten Lankhorst Dec. 7, 2017, 3:57 p.m. UTC | #4
Op 07-12-17 om 16:50 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2017-12-07 15:42:54)
>> Op 07-12-17 om 16:03 schreef Chris Wilson:
>>> Quoting Maarten Lankhorst (2017-12-07 13:40:25)
>>>> I've been trying to make kms_cursor_legacy work when subtests fail.
>>>> Other subtests will start failing too because of expired events or
>>>> stale pipe crc. The latter can be resolved in the test, but the former
>>>> could affect other tests
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>>>  lib/igt_kms.h |  1 +
>>>>  2 files changed, 39 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>>> index 223dbe4ca565..9e14f071ea57 100644
>>>> --- a/lib/igt_kms.c
>>>> +++ b/lib/igt_kms.c
>>>> @@ -2943,7 +2943,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>>>>                         output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
>>>>         }
>>>>  
>>>> -       display->first_commit = false;
>>>> +       if (display->first_commit) {
>>>> +               igt_display_drop_events(display);
>>>> +               display->first_commit = false;
>>>> +       }
>>> So I spent quite a bit of time debating whether there is merit in "do
>>> something; set-first-mode" that this would then clobber. After some
>>> thought, no that doesn't seem like a wise test construction. I would
>>> however suggest that we igt_debug() if we drop any events here.
>>> -Chris
>> Exactly. I'm using igt_info since it's supposed to be uncommon, is it ok if I upgrade it to a igt_warn() since nothing in CI will trigger it?
> Oh, I wouldn't have put it inside drop_events itself, since that is just
> doing its job -- whether or not it is relevant depends on the caller. So
> I would suggest reducing it to igt_debug, or just reporting the number
> dropped and making the judgement in the caller.
>
> No, I don't this should be an igt_warn, as then CI blames the following
> test for environmental errors left over from previous runs.
> -Chris

It's not even possible, CI runs each test separately. When you open a
new copy of the drm fd you don't get events for the previous fd.

Only when subtests fail you should get a dropped event, because of this
it will never happen in CI and it's safe to change to a warn.

~Maarten
Chris Wilson Dec. 7, 2017, 4:12 p.m. UTC | #5
Quoting Maarten Lankhorst (2017-12-07 15:57:09)
> Op 07-12-17 om 16:50 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2017-12-07 15:42:54)
> >> Op 07-12-17 om 16:03 schreef Chris Wilson:
> >>> Quoting Maarten Lankhorst (2017-12-07 13:40:25)
> >>>> I've been trying to make kms_cursor_legacy work when subtests fail.
> >>>> Other subtests will start failing too because of expired events or
> >>>> stale pipe crc. The latter can be resolved in the test, but the former
> >>>> could affect other tests
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >>>>  lib/igt_kms.h |  1 +
> >>>>  2 files changed, 39 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>>> index 223dbe4ca565..9e14f071ea57 100644
> >>>> --- a/lib/igt_kms.c
> >>>> +++ b/lib/igt_kms.c
> >>>> @@ -2943,7 +2943,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
> >>>>                         output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
> >>>>         }
> >>>>  
> >>>> -       display->first_commit = false;
> >>>> +       if (display->first_commit) {
> >>>> +               igt_display_drop_events(display);
> >>>> +               display->first_commit = false;
> >>>> +       }
> >>> So I spent quite a bit of time debating whether there is merit in "do
> >>> something; set-first-mode" that this would then clobber. After some
> >>> thought, no that doesn't seem like a wise test construction. I would
> >>> however suggest that we igt_debug() if we drop any events here.
> >>> -Chris
> >> Exactly. I'm using igt_info since it's supposed to be uncommon, is it ok if I upgrade it to a igt_warn() since nothing in CI will trigger it?
> > Oh, I wouldn't have put it inside drop_events itself, since that is just
> > doing its job -- whether or not it is relevant depends on the caller. So
> > I would suggest reducing it to igt_debug, or just reporting the number
> > dropped and making the judgement in the caller.
> >
> > No, I don't this should be an igt_warn, as then CI blames the following
> > test for environmental errors left over from previous runs.
> > -Chris
> 
> It's not even possible, CI runs each test separately. When you open a
> new copy of the drm fd you don't get events for the previous fd.
> 
> Only when subtests fail you should get a dropped event, because of this
> it will never happen in CI and it's safe to change to a warn.

But others just run all the test as a whole, so the next subtest will
generate warns because of earlier subtests. There's also the concept of
a fork-server so that even different igt may end up in the same process,
continuing on from the same fd. (Depending on how fast we want to strive
for; certainly for fuzzing we want to retain as much state as safely can
be.)
-Chris
Chris Wilson Dec. 19, 2017, 1:18 p.m. UTC | #6
Quoting Chris Wilson (2017-12-07 16:12:34)
> Quoting Maarten Lankhorst (2017-12-07 15:57:09)
> > Op 07-12-17 om 16:50 schreef Chris Wilson:
> > > Quoting Maarten Lankhorst (2017-12-07 15:42:54)
> > >> Op 07-12-17 om 16:03 schreef Chris Wilson:
> > >>> Quoting Maarten Lankhorst (2017-12-07 13:40:25)
> > >>>> I've been trying to make kms_cursor_legacy work when subtests fail.
> > >>>> Other subtests will start failing too because of expired events or
> > >>>> stale pipe crc. The latter can be resolved in the test, but the former
> > >>>> could affect other tests
> > >>>>
> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>>> ---
> > >>>>  lib/igt_kms.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > >>>>  lib/igt_kms.h |  1 +
> > >>>>  2 files changed, 39 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > >>>> index 223dbe4ca565..9e14f071ea57 100644
> > >>>> --- a/lib/igt_kms.c
> > >>>> +++ b/lib/igt_kms.c
> > >>>> @@ -2943,7 +2943,10 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
> > >>>>                         output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
> > >>>>         }
> > >>>>  
> > >>>> -       display->first_commit = false;
> > >>>> +       if (display->first_commit) {
> > >>>> +               igt_display_drop_events(display);
> > >>>> +               display->first_commit = false;
> > >>>> +       }
> > >>> So I spent quite a bit of time debating whether there is merit in "do
> > >>> something; set-first-mode" that this would then clobber. After some
> > >>> thought, no that doesn't seem like a wise test construction. I would
> > >>> however suggest that we igt_debug() if we drop any events here.
> > >>> -Chris
> > >> Exactly. I'm using igt_info since it's supposed to be uncommon, is it ok if I upgrade it to a igt_warn() since nothing in CI will trigger it?
> > > Oh, I wouldn't have put it inside drop_events itself, since that is just
> > > doing its job -- whether or not it is relevant depends on the caller. So
> > > I would suggest reducing it to igt_debug, or just reporting the number
> > > dropped and making the judgement in the caller.
> > >
> > > No, I don't this should be an igt_warn, as then CI blames the following
> > > test for environmental errors left over from previous runs.
> > > -Chris
> > 
> > It's not even possible, CI runs each test separately. When you open a
> > new copy of the drm fd you don't get events for the previous fd.
> > 
> > Only when subtests fail you should get a dropped event, because of this
> > it will never happen in CI and it's safe to change to a warn.
> 
> But others just run all the test as a whole, so the next subtest will
> generate warns because of earlier subtests. There's also the concept of
> a fork-server so that even different igt may end up in the same process,
> continuing on from the same fd. (Depending on how fast we want to strive
> for; certainly for fuzzing we want to retain as much state as safely can
> be.)

So I don't really mind what happens with dropping the stale events. If
you made tests cleanup after themselves, then it would appropriate for a
warn. If other tests encounter events as they start up, that to me
suggests debug since it is noise for this particular test. I would still
just have a debug here, and a warn/assert higher up for dropped events
where appropriate.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Patch
diff mbox

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 223dbe4ca565..9e14f071ea57 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -2943,7 +2943,10 @@  display_commit_changed(igt_display_t *display, enum igt_commit_style s)
 			output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
 	}
 
-	display->first_commit = false;
+	if (display->first_commit) {
+		igt_display_drop_events(display);
+		display->first_commit = false;
+	}
 }
 
 /*
@@ -3024,6 +3027,10 @@  int igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *
 	if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY))
 		return ret;
 
+	if (display->first_commit)
+		igt_fail_on_f(flags & (DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK),
+			      "First commit has to drop all stale events\n");
+
 	display_commit_changed(display, COMMIT_ATOMIC);
 
 	igt_debug_wait_for_keypress("modeset");
@@ -3121,6 +3128,36 @@  int igt_display_commit(igt_display_t *display)
 	return igt_display_commit2(display, COMMIT_LEGACY);
 }
 
+/**
+ * igt_display_drop_events:
+ * @display: DRM device handle
+ *
+ * Nonblockingly reads all current events and drops them, for highest
+ * reliablility, call igt_display_commit2() first to flush all outstanding
+ * events.
+ *
+ * This will be called on the first commit after igt_display_reset() too,
+ * to make sure any stale events are flushed.
+ */
+void igt_display_drop_events(igt_display_t *display)
+{
+	/* Clear all events from drm fd. */
+	struct pollfd pfd = {
+		.fd = display->drm_fd,
+		.events = POLLIN
+	};
+
+	while (poll(&pfd, 1, 0) > 0) {
+		struct drm_event ev;
+		char buf[128];
+
+		read(display->drm_fd, &ev, sizeof(ev));
+		igt_info("Dropping event type %u length %u\n", ev.type, ev.length);
+		igt_assert(ev.length <= sizeof(buf));
+		read(display->drm_fd, buf, ev.length);
+	}
+}
+
 const char *igt_output_name(igt_output_t *output)
 {
 	return output->name;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 2a480bf39956..342881a69177 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -360,6 +360,7 @@  int  igt_display_commit(igt_display_t *display);
 int  igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *user_data);
 void igt_display_commit_atomic(igt_display_t *display, uint32_t flags, void *user_data);
 int  igt_display_try_commit2(igt_display_t *display, enum igt_commit_style s);
+void igt_display_drop_events(igt_display_t *display);
 int  igt_display_get_n_pipes(igt_display_t *display);
 void igt_display_require_output(igt_display_t *display);
 void igt_display_require_output_on_pipe(igt_display_t *display, enum pipe pipe);