[Intel-gfx] [PATCH i-g-t 1/3] lib/igt_kms: Drop all stale events on first commit.

[Chris Wilson]

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 at 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 at chris-wilson.co.uk>
-Chris