Re: [PATCH i-g-t 2/8] kms_frontbuffer_tracking: Skip on unreliable CRC.

[Daniel Vetter <daniel@ffwll.ch>]

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@intel.com>:
> > 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@intel.com>
> > ---
> >  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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx