On Thu, Nov 05, 2015 at 06:30:30PM -0200, Paulo Zanoni wrote: > 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>: > > Even with all sink crc re-works we still have platforms > > where after 6 vblanks it is unable to calculate the > > sink crc. But if we don't get the sink crc it isn't true > > that test failed, but that we have no ways to say test > > passed or failed. > > > > So let's print a message and move forward in case sink crc > > cannot help us to know if the screen has been updated. > > As much as I understand your reasoning here, "Try running this test > again" will be ignored by our future bots. > > Instead of just skipping, isn't there something else we could do, such > as trying again 10 times? 60 frames doesn't seem expensive. If it > works at least sometimes, I'd say it's worth the try. > > Besides, did we try the AUX_MUTEX register that was suggested here: > http://patchwork.freedesktop.org/patch/57693/ ? Maybe it would solve > all our sink CRCs problem. > > Another comment: FBC doesn't really need sink CRC, but it's currently > checking sink CRC, so it may get SKIPs. Maybe instead of a SKIP for > failed sink CRC on FBC we could just ignore and move on? Maybe we > could pass some flags to collect_crcs() so it can know if sink CRCs > are ignorable. Yeah, at least we should make FBC tests not depend upon sink crcs. Otherwise test coverage might artificially go down. -Daniel > > Another problem is: what if we fail while getting the reference CRC? > We will leave garbage inside crc->data, and the other tests will > compare themselves against the garbage in case reading sink CRCs end > up working for them, so we'll have test failures that are not real > failures. Maybe we should pass some flag to collect_crtcs() signaling > that we're trying a reference CRC, so it can write something to > crtc->data, just like we have the "unsupported!" string. Then we'd > have to check this special string later. > > You also probably need to fix setup_sink_crc(), because it currently > doesn't check for ETIMETDOUT. > > I'm not blocking the patch, just starting the discussion :) > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > --- > > tests/kms_frontbuffer_tracking.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > > index cd2879d..606d0a9 100644 > > --- a/tests/kms_frontbuffer_tracking.c > > +++ b/tests/kms_frontbuffer_tracking.c > > @@ -858,10 +858,17 @@ static bool psr_wait_until_enabled(void) > > > > static void get_sink_crc(sink_crc_t *crc) > > { > > + int rc, errno_; > > + > > lseek(sink_crc.fd, 0, SEEK_SET); > > > > - igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) == > > - SINK_CRC_SIZE); > > + rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE); > > + errno_ = errno; > > + > > + if (rc == -1 && errno_ == ETIMEDOUT) > > + igt_skip("Sink CRC is unreliable on this machine. Try running this test again individually\n"); > > + > > + igt_assert(rc == SINK_CRC_SIZE); > > } > > > > static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b) > > -- > > 2.4.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
