| Message ID | 20171207134027.61108-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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);
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(-)