Re: [igt-dev] ✓ Fi.CI.BAT: success for Skip VBlank tests in modules without VBlank (rev2)

[Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>]

On 01/14, Daniel Vetter wrote:
> On Sun, Jan 13, 2019 at 06:31:07PM -0200, Rodrigo Siqueira wrote:
> > Hi Arkadiusz,
> > 
> > On 08/27, Arkadiusz Hiler wrote:
> > > On Thu, Aug 23, 2018 at 05:47:41PM +0000, Patchwork wrote:
> > > > == Series Details ==
> > > > 
> > > > Series: Skip VBlank tests in modules without VBlank (rev2)
> > > > URL   : https://patchwork.freedesktop.org/series/48468/
> > > > State : success
> > > > 
> > > > == Summary ==
> > > > 
> > > > = CI Bug Log - changes from CI_DRM_4702 -> IGTPW_1741 =
> > > > 
> > > > == Summary - WARNING ==
> > > > 
> > > >   Minor unknown changes coming with IGTPW_1741 need to be verified
> > > >   manually.
> > > >   
> > > >   If you think the reported changes have nothing to do with the changes
> > > >   introduced in IGTPW_1741, please notify your bug team to allow them
> > > >   to document this new failure mode, which will reduce false positives in CI.
> > > > 
> > > >   External URL: https://patchwork.freedesktop.org/api/1.0/series/48468/revisions/2/mbox/
> > > > 
> > > > == Possible new issues ==
> > > > 
> > > >   Here are the unknown changes that may have been introduced in IGTPW_1741:
> > > > 
> > > >   === IGT changes ===
> > > > 
> > > >     ==== Warnings ====
> > > > 
> > > >     igt@kms_flip@basic-flip-vs-wf_vblank:
> > > >       fi-hsw-peppy:       PASS -> SKIP
> > > >       fi-snb-2600:        PASS -> SKIP
> > > >       fi-skl-6260u:       PASS -> SKIP
> > > >       fi-hsw-4770r:       PASS -> SKIP
> > > >       {fi-bdw-samus}:     PASS -> SKIP
> > > >       {fi-cfl-8109u}:     PASS -> SKIP
> > > >       fi-blb-e6850:       PASS -> SKIP
> > > >       fi-kbl-r:           PASS -> SKIP
> > > >       fi-bwr-2160:        PASS -> SKIP
> > > >       fi-bdw-5557u:       PASS -> SKIP
> > > >       fi-skl-6600u:       PASS -> SKIP
> > > >       fi-kbl-7560u:       PASS -> SKIP
> > > >       fi-hsw-4770:        PASS -> SKIP
> > > >       fi-byt-j1900:       PASS -> SKIP
> > > >       fi-skl-6700k2:      PASS -> SKIP
> > > >       fi-ivb-3770:        PASS -> SKIP
> > > >       fi-cnl-psr:         PASS -> SKIP
> > > >       {fi-bsw-kefka}:     PASS -> SKIP
> > > >       fi-skl-6700hq:      PASS -> SKIP
> > > >       fi-bxt-j4205:       PASS -> SKIP
> > > >       fi-bsw-n3050:       PASS -> SKIP
> > > >       fi-skl-gvtdvm:      PASS -> SKIP
> > > >       {fi-byt-clapper}:   PASS -> SKIP
> > > >       fi-gdg-551:         PASS -> SKIP
> > > >       fi-glk-dsi:         PASS -> SKIP
> > > >       fi-glk-j4005:       PASS -> SKIP
> > > >       fi-skl-guc:         PASS -> SKIP
> > > >       fi-kbl-7567u:       PASS -> SKIP
> > > >       fi-bdw-gvtdvm:      PASS -> SKIP
> > > >       fi-ivb-3520m:       PASS -> SKIP
> > > >       fi-kbl-7500u:       PASS -> SKIP
> > > >       fi-cfl-8700k:       PASS -> SKIP
> > > >       fi-whl-u:           PASS -> SKIP
> > > >       fi-pnv-d510:        PASS -> SKIP
> > > >       fi-snb-2520m:       PASS -> SKIP
> > > >       fi-cfl-s3:          PASS -> SKIP
> > > >       {fi-skl-iommu}:     PASS -> SKIP
> > > >       fi-cfl-guc:         PASS -> SKIP
> > > >       fi-byt-n2820:       PASS -> SKIP
> > > >       fi-skl-6770hq:      PASS -> SKIP
> > > >       fi-elk-e7500:       PASS -> SKIP
> > > >       fi-ilk-650:         PASS -> SKIP
> > > 
> > > 
> > > Hey,
> > > 
> > > The patch makes overall sense and thanks for sending it.
> > > 
> > > The test result changes seen above suggest that something is off with
> > > the logic in the patch or flags for the tests.
> > > 
> > > There are more of those in the IGT run (NOTE: if you see +43, that means
> > > that 43 changes like that were observerd).
> > > 
> > > They were passing before now they are skipping. Please investigate that.
> > 
> > First of all, sorry for the long time to take a look at this.
> > 
> > I tried to understand the skipping problem by investigating the test
> > “basic-flip-vs-wf_vblank()” in “kms_flip.c”, and I replicated the issue
> > in my local machine running i915 driver. Without my patch, the
> > "flip-vs-wf_vblank" worked like a charm; however, after applied my patch
> > I got the following log:
> > 
> > 	IGT-Version: 1.23-g8d81c2c2 (x86_64) (Linux: 4.20.0-arch1-1-ARCH x86_64)
> > 	Using monotonic timestamps
> > 	Starting subtest: basic-flip-vs-wf_vblank
> > 	Beginning basic-flip-vs-wf_vblank on pipe A, connector eDP-1
> > 		1920x1080 60 1920 2028 2076 2100 1080 1090 1100 1126 0xa 0x48 142000
> > 	Expected frametime: 16652us; measured 16652.4us +- 5.726us accuracy 0.10%
> > 
> > 	basic-flip-vs-wf_vblank on pipe A, connector eDP-1: PASSED
> > 
> > 	Beginning basic-flip-vs-wf_vblank on pipe B, connector eDP-1
> > 		1920x1080 60 1920 2028 2076 2100 1080 1090 1100 1126 0xa 0x48 142000
> > 	Test requirement not met in function run_test_on_crtc_set, file ../tests/kms_flip.c:1317:
> > 	Test requirement: vblank
> > 	There is no Vblank
> > 	Last errno: 22, Invalid argument
> > 	Subtest basic-flip-vs-wf_vblank: SKIP (4.441s)
> > 
> > The test was skipped because we get EINVAL (-22) which means many things
> > in this case (I'll detail this ahead). To better understand the problem,
> > I highlighted the function that I used in the patch for checking if a
> > driver has or not vblank support:
> > 
> > +bool kms_has_vblank(int fd)
> > +{
> > +    drmVBlank dummy_vbl;
> > +
> > +    memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > +    dummy_vbl.request.type = DRM_VBLANK_ABSOLUTE;
> > +
> > +    return drmWaitVBlank(fd, &dummy_vbl) == 0;
> > +}
> > 
> > As you can see in the return line, we just check if “drmWaitVBlank()”
> > returns 0 and this is the problem. If you take a look at
> > “drivers/gpu/drm/drm_vblank.c” (kernel), the function
> > “drm_wait_vblank_ioctl()”, you'll notice that the only error returned to
> > the user space is “EINVAL”; which includes the check if a driver support
> > vblank or not (“dev->irq_enabled”).
> > 
> > IMHO the correct solution is changing the “drm_wait_vblank_ioctl()” to
> > return “EOPNOTSUPP” in the case that a driver does not support vblank. I
> > I already sent a patch to change it, and you can see the discussion here:
> > 
> > 	https://lkml.org/lkml/2018/10/15/609
> > 
> > So... for checking my point, I applied the above patch and made a change
> > in “kms_has_vblank” as follows:
> > 
> > +bool kms_has_vblank(int fd)
> > +{
> > +    drmVBlank dummy_vbl;
> > +
> > +    memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > +    dummy_vbl.request.type = DRM_VBLANK_ABSOLUTE;
> > +
> > +    drmWaitVBlank(fd, &dummy_vbl);
> > +    return (errno != EOPNOTSUPP);
> > +}
> > 
> > After that, the “basic-flip-vs-wf_vblank()” worked as expected. I tested
> > in i915, Bosch (no vblank), and VKMS. Everything looks right now.
> > 
> > So... make sense? Do you see another alternative to solve this problem?
> 
> Yup. Even with your patch we can still get -EINVAL (when the crtc is off),
> so merging the EOPNOTSUPP patches first, then this here, and only skipping
> if EOPNOTSUPP is the only way to go I think.
> 
> The above is probably good in a comment in kms_has_vblank().

Hi, thanks for the review.

I will prepare a V3 with this fix.
 
> Btw I thought the EOPNOTSUPP was merged already, why is it stuck? Or
> should retesting this fix things?

About the patch, we have some discussion about it because two tests failed
in the CI. Probably I have to send another patch for making the tests
correctly handling EOPNOTSUPP.

Thanks.

> -Daniel
> > 
> > Additionally, it is weird for me that “drm_wait_vblank_ioctl()” returns
> > EINVAL in some case related to “basic-flip-vs-wf_vblank()”.
> > 
> > Best Regards
> > 
> > > -- 
> > > Cheers,
> > > Arek
> > > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev