[igt-dev] [PATCH] tests/kms_vblank: Turn on hardware before testing invalid vblank.

[Mark Yacoub]

friendly ping.

On Tue, Jun 1, 2021 at 1:33 PM Mark Yacoub <markyacoub at chromium.org> wrote:
>
> On Mon, May 31, 2021 at 2:57 PM Rodrigo Siqueira Jordao
> <Rodrigo.Siqueira at amd.com> wrote:
> >
> > Hi Mark,
> >
> > See my comments inline.
> >
> > On 2021-05-20 3:34 p.m., Mark Yacoub wrote:
> > > From: Mark Yacoub <markyacoub at google.com>
> > >
> > > [Why]
> > > Before any hardware is on, the vblank is off and its ref counter is in
> > > an initialized state as each driver handles toggling it differently.
> > > Ioctl DRM_IOCTL_WAIT_VBLANK could return 0 or invalid depending on the
> > > hardware, without necessarily meaning anything.
> >
> > Maybe you can add some hardware examples here.
> Updated the commit msg in v2.
> >
> > > [How]
> > > For invalid_subtest(), active the CRTCs to turn the hardware on so
> > > DRM_IOCTL_WAIT_VBLANK returns something meaningful.
> >
> > You forgot the `Signed-off-by`.
> >
> > > ---
> > >   tests/kms_vblank.c | 11 ++++++++++-
> > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> > > index 93b01eba..cb68a2f8 100644
> > > --- a/tests/kms_vblank.c
> > > +++ b/tests/kms_vblank.c
> > > @@ -475,8 +475,14 @@ static void invalid_subtest(data_t *data, int fd)
> > >   {
> > >       union drm_wait_vblank vbl;
> > >       unsigned long valid_flags;
> > > +     igt_display_t* display = &data->display;
> >
> > Why add this variable instead of just use `&data->display`?
> it's used more than once so thought it would be nice to have it in a var
> >
> > > +     enum pipe p = 0;
> >
> > Why did you add this variable? Improve the readability? If so, I'm not
> > sure that p is better than have 0 here. Or did I miss something?
> not much for readability but cause it's used like 4 times, including
> at the test end so i encapsulated the value in a variable
> >
> > > +     igt_output_t* output;
> > >
> > > -     igt_display_require_output_on_pipe(&data->display, 0);
> > > +     igt_display_require_output_on_pipe(display, p);
> > > +     data->pipe = p;
> > > +     for_each_valid_output_on_pipe(display, p, output)
> > > +             prepare_crtc(data, fd, output);
> > >
> > >       /* First check all is well with a simple query */
> > >       memset(&vbl, 0, sizeof(vbl));
> > > @@ -511,6 +517,9 @@ static void invalid_subtest(data_t *data, int fd)
> > >       vbl.request.type |= _DRM_VBLANK_SECONDARY;
> > >       vbl.request.type |= _DRM_VBLANK_FLAGS_MASK;
> > >       igt_assert_eq(wait_vblank(fd, &vbl), -EINVAL);
> > > +
> > > +     for_each_valid_output_on_pipe(display, p, output)
> > > +             cleanup_crtc(data, fd, output);
> > >   }
> >
> > This patch makes sense to me. I tested it in a Raven system (DCN1.0),
> > and everything looks fine to me; however, even without your patch, I see
> > the same result.
> >
> > Btw, thanks a lot for your patch.
> >
> > >   igt_main
> > >
> >