diff mbox series

[V5,i-g-t] tests/kms_flip: Skip VBlank tests in modules without VBlank

Message ID 20190316140023.b5gn44gtojoy6iqi@smtp.gmail.com (mailing list archive)
State New, archived
Headers show
Series [V5,i-g-t] tests/kms_flip: Skip VBlank tests in modules without VBlank | expand

Commit Message

Rodrigo Siqueira March 16, 2019, 2 p.m. UTC
The kms_flip test relies on VBlank support, and this situation may
exclude some virtual drivers to take advantage of this set of tests.
This commit adds a mechanism that checks if a module has VBlank. If the
target module has VBlank support, kms_flip will run all the VBlank
tests; otherwise, the VBlank tests will be skipped. Additionally, this
commit improves the test coverage by checks if the function
drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).

V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
DRM_VBLANK_NEXTONMISS

V3: Add documentation (Daniel Vetter)

V2: Add new branch coverage to check if VBlank is enabled or not and
update commit message

V1: Chris Wilson
  - Change function name from igt_there_is_vblank to kms_has_vblank
  - Move vblank function check from igt_aux to igt_kms
  - Utilizes memset in dummy_vbl variable
  - Directly return the result of drmWaitVBlank()

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 lib/igt_kms.c    | 20 ++++++++++++++++++++
 lib/igt_kms.h    |  2 ++
 tests/kms_flip.c | 22 ++++++++++++++++++++++
 3 files changed, 44 insertions(+)

Comments

Ville Syrjälä March 18, 2019, 2:01 p.m. UTC | #1
On Sat, Mar 16, 2019 at 11:00:23AM -0300, Rodrigo Siqueira wrote:
> The kms_flip test relies on VBlank support, and this situation may
> exclude some virtual drivers to take advantage of this set of tests.
> This commit adds a mechanism that checks if a module has VBlank. If the
> target module has VBlank support, kms_flip will run all the VBlank
> tests; otherwise, the VBlank tests will be skipped. Additionally, this
> commit improves the test coverage by checks if the function
> drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).
> 
> V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
> DRM_VBLANK_NEXTONMISS
> 
> V3: Add documentation (Daniel Vetter)
> 
> V2: Add new branch coverage to check if VBlank is enabled or not and
> update commit message
> 
> V1: Chris Wilson
>   - Change function name from igt_there_is_vblank to kms_has_vblank
>   - Move vblank function check from igt_aux to igt_kms
>   - Utilizes memset in dummy_vbl variable
>   - Directly return the result of drmWaitVBlank()
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  lib/igt_kms.c    | 20 ++++++++++++++++++++
>  lib/igt_kms.h    |  2 ++
>  tests/kms_flip.c | 22 ++++++++++++++++++++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e1eacc1e..1d2d7188 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1655,6 +1655,26 @@ void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
>  	igt_assert_eq(visible, visibility);
>  }
>  
> +/**
> + * kms_has_vblank:
> + * @fd: DRM fd
> + *
> + * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
> + * function is useful for checking if a driver has support or not for VBlank.
> + *
> + * Returns: true if target driver has VBlank support, otherwise return false.
> + */
> +bool kms_has_vblank(int fd)
> +{
> +	drmVBlank dummy_vbl;
> +
> +	memset(&dummy_vbl, 0, sizeof(drmVBlank));
> +	dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;

Why the NEXTONMISS?

> +
> +	drmWaitVBlank(fd, &dummy_vbl);
> +	return (errno != EOPNOTSUPP);
> +}
> +
>  /*
>   * A small modeset API
>   */
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 407f3d64..1cc15eea 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -230,6 +230,8 @@ void kmstest_wait_for_pageflip(int fd);
>  unsigned int kmstest_get_vblank(int fd, int pipe, unsigned int flags);
>  void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility);
>  
> +bool kms_has_vblank(int fd);
> +
>  /*
>   * A small modeset API
>   */
> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> index dfa5a69e..614efc5b 100755
> --- a/tests/kms_flip.c
> +++ b/tests/kms_flip.c
> @@ -71,6 +71,7 @@
>  #define TEST_SUSPEND		(1 << 26)
>  #define TEST_BO_TOOBIG		(1 << 28)
>  
> +#define TEST_NO_VBLANK		(1 << 29)
>  #define TEST_BASIC		(1 << 30)
>  
>  #define EVENT_FLIP		(1 << 0)
> @@ -126,6 +127,18 @@ struct event_state {
>  	int seq_step;
>  };
>  
> +static bool vblank_dependence(int flags)
> +{
> +	int vblank_flags = TEST_VBLANK | TEST_VBLANK_BLOCK |
> +			   TEST_VBLANK_ABSOLUTE | TEST_VBLANK_EXPIRED_SEQ |
> +			   TEST_CHECK_TS | TEST_VBLANK_RACE;
> +
> +	if (flags & vblank_flags)
> +		return true;
> +
> +	return false;
> +}
> +
>  static float timeval_float(const struct timeval *tv)
>  {
>  	return tv->tv_sec + tv->tv_usec / 1000000.0f;
> @@ -1177,6 +1190,7 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  	unsigned bo_size = 0;
>  	uint64_t tiling;
>  	int i;
> +	int vblank = true;
>  
>  	switch (crtc_count) {
>  	case RUN_TEST:
> @@ -1260,6 +1274,14 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
>  	}
>  	igt_assert(fb_is_bound(o, o->fb_ids[0]));
>  
> +	vblank = kms_has_vblank(drm_fd);
> +	if (!vblank) {
> +		if (vblank_dependence(o->flags))
> +			igt_require_f(vblank, "There is no VBlank\n");
> +		else
> +			o->flags |= TEST_NO_VBLANK;
> +	}
> +
>  	/* quiescent the hw a bit so ensure we don't miss a single frame */
>  	if (o->flags & TEST_CHECK_TS)
>  		calibrate_ts(o, crtc_idxs[0]);
> -- 
> 2.21.0
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
Rodrigo Siqueira March 18, 2019, 9:35 p.m. UTC | #2
On 03/18, Ville Syrjälä wrote:
> On Sat, Mar 16, 2019 at 11:00:23AM -0300, Rodrigo Siqueira wrote:
> > The kms_flip test relies on VBlank support, and this situation may
> > exclude some virtual drivers to take advantage of this set of tests.
> > This commit adds a mechanism that checks if a module has VBlank. If the
> > target module has VBlank support, kms_flip will run all the VBlank
> > tests; otherwise, the VBlank tests will be skipped. Additionally, this
> > commit improves the test coverage by checks if the function
> > drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).
> > 
> > V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
> > DRM_VBLANK_NEXTONMISS
> > 
> > V3: Add documentation (Daniel Vetter)
> > 
> > V2: Add new branch coverage to check if VBlank is enabled or not and
> > update commit message
> > 
> > V1: Chris Wilson
> >   - Change function name from igt_there_is_vblank to kms_has_vblank
> >   - Move vblank function check from igt_aux to igt_kms
> >   - Utilizes memset in dummy_vbl variable
> >   - Directly return the result of drmWaitVBlank()
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  lib/igt_kms.c    | 20 ++++++++++++++++++++
> >  lib/igt_kms.h    |  2 ++
> >  tests/kms_flip.c | 22 ++++++++++++++++++++++
> >  3 files changed, 44 insertions(+)
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index e1eacc1e..1d2d7188 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1655,6 +1655,26 @@ void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
> >  	igt_assert_eq(visible, visibility);
> >  }
> >  
> > +/**
> > + * kms_has_vblank:
> > + * @fd: DRM fd
> > + *
> > + * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
> > + * function is useful for checking if a driver has support or not for VBlank.
> > + *
> > + * Returns: true if target driver has VBlank support, otherwise return false.
> > + */
> > +bool kms_has_vblank(int fd)
> > +{
> > +	drmVBlank dummy_vbl;
> > +
> > +	memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > +	dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
> 
> Why the NEXTONMISS?

I added this flag because I was suspecting that in case of any problem
during the kms_has_vblank() execution the flag NEXTONMISS will wait for
the next VBlank and avoid to generate problems in the subsequent tests.
However, I was not 100% sure about this decision since this code will
only test the first if condition inside drm_wait_vblank_ioctl().
Additionally, I could not find a good way to test the drmWaitVBlank()
with and without this flag.

Please, correct if I'm wrong. I'm fighting to understand the vblank API.

Thanks for you review :)

> > +
> > +	drmWaitVBlank(fd, &dummy_vbl);
> > +	return (errno != EOPNOTSUPP);
> > +}
> > +
> >  /*
> >   * A small modeset API
> >   */
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 407f3d64..1cc15eea 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -230,6 +230,8 @@ void kmstest_wait_for_pageflip(int fd);
> >  unsigned int kmstest_get_vblank(int fd, int pipe, unsigned int flags);
> >  void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility);
> >  
> > +bool kms_has_vblank(int fd);
> > +
> >  /*
> >   * A small modeset API
> >   */
> > diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> > index dfa5a69e..614efc5b 100755
> > --- a/tests/kms_flip.c
> > +++ b/tests/kms_flip.c
> > @@ -71,6 +71,7 @@
> >  #define TEST_SUSPEND		(1 << 26)
> >  #define TEST_BO_TOOBIG		(1 << 28)
> >  
> > +#define TEST_NO_VBLANK		(1 << 29)
> >  #define TEST_BASIC		(1 << 30)
> >  
> >  #define EVENT_FLIP		(1 << 0)
> > @@ -126,6 +127,18 @@ struct event_state {
> >  	int seq_step;
> >  };
> >  
> > +static bool vblank_dependence(int flags)
> > +{
> > +	int vblank_flags = TEST_VBLANK | TEST_VBLANK_BLOCK |
> > +			   TEST_VBLANK_ABSOLUTE | TEST_VBLANK_EXPIRED_SEQ |
> > +			   TEST_CHECK_TS | TEST_VBLANK_RACE;
> > +
> > +	if (flags & vblank_flags)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  static float timeval_float(const struct timeval *tv)
> >  {
> >  	return tv->tv_sec + tv->tv_usec / 1000000.0f;
> > @@ -1177,6 +1190,7 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
> >  	unsigned bo_size = 0;
> >  	uint64_t tiling;
> >  	int i;
> > +	int vblank = true;
> >  
> >  	switch (crtc_count) {
> >  	case RUN_TEST:
> > @@ -1260,6 +1274,14 @@ static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
> >  	}
> >  	igt_assert(fb_is_bound(o, o->fb_ids[0]));
> >  
> > +	vblank = kms_has_vblank(drm_fd);
> > +	if (!vblank) {
> > +		if (vblank_dependence(o->flags))
> > +			igt_require_f(vblank, "There is no VBlank\n");
> > +		else
> > +			o->flags |= TEST_NO_VBLANK;
> > +	}
> > +
> >  	/* quiescent the hw a bit so ensure we don't miss a single frame */
> >  	if (o->flags & TEST_CHECK_TS)
> >  		calibrate_ts(o, crtc_idxs[0]);
> > -- 
> > 2.21.0
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Ville Syrjälä
> Intel
Chris Wilson March 18, 2019, 9:38 p.m. UTC | #3
Quoting Rodrigo Siqueira (2019-03-18 21:35:44)
> On 03/18, Ville Syrjälä wrote:
> > On Sat, Mar 16, 2019 at 11:00:23AM -0300, Rodrigo Siqueira wrote:
> > > The kms_flip test relies on VBlank support, and this situation may
> > > exclude some virtual drivers to take advantage of this set of tests.
> > > This commit adds a mechanism that checks if a module has VBlank. If the
> > > target module has VBlank support, kms_flip will run all the VBlank
> > > tests; otherwise, the VBlank tests will be skipped. Additionally, this
> > > commit improves the test coverage by checks if the function
> > > drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).
> > > 
> > > V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
> > > DRM_VBLANK_NEXTONMISS
> > > 
> > > V3: Add documentation (Daniel Vetter)
> > > 
> > > V2: Add new branch coverage to check if VBlank is enabled or not and
> > > update commit message
> > > 
> > > V1: Chris Wilson
> > >   - Change function name from igt_there_is_vblank to kms_has_vblank
> > >   - Move vblank function check from igt_aux to igt_kms
> > >   - Utilizes memset in dummy_vbl variable
> > >   - Directly return the result of drmWaitVBlank()
> > > 
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > ---
> > >  lib/igt_kms.c    | 20 ++++++++++++++++++++
> > >  lib/igt_kms.h    |  2 ++
> > >  tests/kms_flip.c | 22 ++++++++++++++++++++++
> > >  3 files changed, 44 insertions(+)
> > > 
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index e1eacc1e..1d2d7188 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -1655,6 +1655,26 @@ void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
> > >     igt_assert_eq(visible, visibility);
> > >  }
> > >  
> > > +/**
> > > + * kms_has_vblank:
> > > + * @fd: DRM fd
> > > + *
> > > + * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
> > > + * function is useful for checking if a driver has support or not for VBlank.
> > > + *
> > > + * Returns: true if target driver has VBlank support, otherwise return false.
> > > + */
> > > +bool kms_has_vblank(int fd)
> > > +{
> > > +   drmVBlank dummy_vbl;
> > > +
> > > +   memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > > +   dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
> > 
> > Why the NEXTONMISS?
> 
> I added this flag because I was suspecting that in case of any problem
> during the kms_has_vblank() execution the flag NEXTONMISS will wait for
> the next VBlank and avoid to generate problems in the subsequent tests.

That is true, and a valid use for using NEXTONMISS if you want to align
your code to the start of a vblank to avoid overrunning into the next
vblank.

However, that is not the purpose of kms_has_vblank()! Whose only purpose
is answer the question of whether the device has vblank support, and so
should be as quick and simple as possible, so the trivial query of the
current vblank counter.
-Chris
Rodrigo Siqueira March 18, 2019, 9:53 p.m. UTC | #4
On 03/18, Chris Wilson wrote:
> Quoting Rodrigo Siqueira (2019-03-18 21:35:44)
> > On 03/18, Ville Syrjälä wrote:
> > > On Sat, Mar 16, 2019 at 11:00:23AM -0300, Rodrigo Siqueira wrote:
> > > > The kms_flip test relies on VBlank support, and this situation may
> > > > exclude some virtual drivers to take advantage of this set of tests.
> > > > This commit adds a mechanism that checks if a module has VBlank. If the
> > > > target module has VBlank support, kms_flip will run all the VBlank
> > > > tests; otherwise, the VBlank tests will be skipped. Additionally, this
> > > > commit improves the test coverage by checks if the function
> > > > drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).
> > > > 
> > > > V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
> > > > DRM_VBLANK_NEXTONMISS
> > > > 
> > > > V3: Add documentation (Daniel Vetter)
> > > > 
> > > > V2: Add new branch coverage to check if VBlank is enabled or not and
> > > > update commit message
> > > > 
> > > > V1: Chris Wilson
> > > >   - Change function name from igt_there_is_vblank to kms_has_vblank
> > > >   - Move vblank function check from igt_aux to igt_kms
> > > >   - Utilizes memset in dummy_vbl variable
> > > >   - Directly return the result of drmWaitVBlank()
> > > > 
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > ---
> > > >  lib/igt_kms.c    | 20 ++++++++++++++++++++
> > > >  lib/igt_kms.h    |  2 ++
> > > >  tests/kms_flip.c | 22 ++++++++++++++++++++++
> > > >  3 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > > index e1eacc1e..1d2d7188 100644
> > > > --- a/lib/igt_kms.c
> > > > +++ b/lib/igt_kms.c
> > > > @@ -1655,6 +1655,26 @@ void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
> > > >     igt_assert_eq(visible, visibility);
> > > >  }
> > > >  
> > > > +/**
> > > > + * kms_has_vblank:
> > > > + * @fd: DRM fd
> > > > + *
> > > > + * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
> > > > + * function is useful for checking if a driver has support or not for VBlank.
> > > > + *
> > > > + * Returns: true if target driver has VBlank support, otherwise return false.
> > > > + */
> > > > +bool kms_has_vblank(int fd)
> > > > +{
> > > > +   drmVBlank dummy_vbl;
> > > > +
> > > > +   memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > > > +   dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
> > > 
> > > Why the NEXTONMISS?
> > 
> > I added this flag because I was suspecting that in case of any problem
> > during the kms_has_vblank() execution the flag NEXTONMISS will wait for
> > the next VBlank and avoid to generate problems in the subsequent tests.
> 
> That is true, and a valid use for using NEXTONMISS if you want to align
> your code to the start of a vblank to avoid overrunning into the next
> vblank.
> 
> However, that is not the purpose of kms_has_vblank()! Whose only purpose
> is answer the question of whether the device has vblank support, and so
> should be as quick and simple as possible, so the trivial query of the
> current vblank counter.

Nice! Thanks for your answer.

So, I suppose that use DRM_VBLANK_RELATIVE is enough for this case,
right?

> -Chris
Chris Wilson March 18, 2019, 9:56 p.m. UTC | #5
Quoting Rodrigo Siqueira (2019-03-18 21:53:42)
> On 03/18, Chris Wilson wrote:
> > Quoting Rodrigo Siqueira (2019-03-18 21:35:44)
> > > On 03/18, Ville Syrjälä wrote:
> > > > On Sat, Mar 16, 2019 at 11:00:23AM -0300, Rodrigo Siqueira wrote:
> > > > > The kms_flip test relies on VBlank support, and this situation may
> > > > > exclude some virtual drivers to take advantage of this set of tests.
> > > > > This commit adds a mechanism that checks if a module has VBlank. If the
> > > > > target module has VBlank support, kms_flip will run all the VBlank
> > > > > tests; otherwise, the VBlank tests will be skipped. Additionally, this
> > > > > commit improves the test coverage by checks if the function
> > > > > drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).
> > > > > 
> > > > > V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
> > > > > DRM_VBLANK_NEXTONMISS
> > > > > 
> > > > > V3: Add documentation (Daniel Vetter)
> > > > > 
> > > > > V2: Add new branch coverage to check if VBlank is enabled or not and
> > > > > update commit message
> > > > > 
> > > > > V1: Chris Wilson
> > > > >   - Change function name from igt_there_is_vblank to kms_has_vblank
> > > > >   - Move vblank function check from igt_aux to igt_kms
> > > > >   - Utilizes memset in dummy_vbl variable
> > > > >   - Directly return the result of drmWaitVBlank()
> > > > > 
> > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > > ---
> > > > >  lib/igt_kms.c    | 20 ++++++++++++++++++++
> > > > >  lib/igt_kms.h    |  2 ++
> > > > >  tests/kms_flip.c | 22 ++++++++++++++++++++++
> > > > >  3 files changed, 44 insertions(+)
> > > > > 
> > > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > > > index e1eacc1e..1d2d7188 100644
> > > > > --- a/lib/igt_kms.c
> > > > > +++ b/lib/igt_kms.c
> > > > > @@ -1655,6 +1655,26 @@ void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
> > > > >     igt_assert_eq(visible, visibility);
> > > > >  }
> > > > >  
> > > > > +/**
> > > > > + * kms_has_vblank:
> > > > > + * @fd: DRM fd
> > > > > + *
> > > > > + * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
> > > > > + * function is useful for checking if a driver has support or not for VBlank.
> > > > > + *
> > > > > + * Returns: true if target driver has VBlank support, otherwise return false.
> > > > > + */
> > > > > +bool kms_has_vblank(int fd)
> > > > > +{
> > > > > +   drmVBlank dummy_vbl;
> > > > > +
> > > > > +   memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > > > > +   dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
> > > > 
> > > > Why the NEXTONMISS?
> > > 
> > > I added this flag because I was suspecting that in case of any problem
> > > during the kms_has_vblank() execution the flag NEXTONMISS will wait for
> > > the next VBlank and avoid to generate problems in the subsequent tests.
> > 
> > That is true, and a valid use for using NEXTONMISS if you want to align
> > your code to the start of a vblank to avoid overrunning into the next
> > vblank.
> > 
> > However, that is not the purpose of kms_has_vblank()! Whose only purpose
> > is answer the question of whether the device has vblank support, and so
> > should be as quick and simple as possible, so the trivial query of the
> > current vblank counter.
> 
> Nice! Thanks for your answer.
> 
> So, I suppose that use DRM_VBLANK_RELATIVE is enough for this case,
> right?

Yes, that should return immediately both on success and on failure.
-Chris
Rodrigo Siqueira March 18, 2019, 10:06 p.m. UTC | #6
On 03/18, Chris Wilson wrote:
> Quoting Rodrigo Siqueira (2019-03-18 21:53:42)
> > On 03/18, Chris Wilson wrote:
> > > Quoting Rodrigo Siqueira (2019-03-18 21:35:44)
> > > > On 03/18, Ville Syrjälä wrote:
> > > > > On Sat, Mar 16, 2019 at 11:00:23AM -0300, Rodrigo Siqueira wrote:
> > > > > > The kms_flip test relies on VBlank support, and this situation may
> > > > > > exclude some virtual drivers to take advantage of this set of tests.
> > > > > > This commit adds a mechanism that checks if a module has VBlank. If the
> > > > > > target module has VBlank support, kms_flip will run all the VBlank
> > > > > > tests; otherwise, the VBlank tests will be skipped. Additionally, this
> > > > > > commit improves the test coverage by checks if the function
> > > > > > drmWaitVBlank() returns EOPNOTSUPP (i.e., no VBlank support).
> > > > > > 
> > > > > > V4: Replace DRM_VBLANK_ABSOLUTE by DRM_VBLANK_RELATIVE and
> > > > > > DRM_VBLANK_NEXTONMISS
> > > > > > 
> > > > > > V3: Add documentation (Daniel Vetter)
> > > > > > 
> > > > > > V2: Add new branch coverage to check if VBlank is enabled or not and
> > > > > > update commit message
> > > > > > 
> > > > > > V1: Chris Wilson
> > > > > >   - Change function name from igt_there_is_vblank to kms_has_vblank
> > > > > >   - Move vblank function check from igt_aux to igt_kms
> > > > > >   - Utilizes memset in dummy_vbl variable
> > > > > >   - Directly return the result of drmWaitVBlank()
> > > > > > 
> > > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > > > ---
> > > > > >  lib/igt_kms.c    | 20 ++++++++++++++++++++
> > > > > >  lib/igt_kms.h    |  2 ++
> > > > > >  tests/kms_flip.c | 22 ++++++++++++++++++++++
> > > > > >  3 files changed, 44 insertions(+)
> > > > > > 
> > > > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > > > > index e1eacc1e..1d2d7188 100644
> > > > > > --- a/lib/igt_kms.c
> > > > > > +++ b/lib/igt_kms.c
> > > > > > @@ -1655,6 +1655,26 @@ void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
> > > > > >     igt_assert_eq(visible, visibility);
> > > > > >  }
> > > > > >  
> > > > > > +/**
> > > > > > + * kms_has_vblank:
> > > > > > + * @fd: DRM fd
> > > > > > + *
> > > > > > + * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
> > > > > > + * function is useful for checking if a driver has support or not for VBlank.
> > > > > > + *
> > > > > > + * Returns: true if target driver has VBlank support, otherwise return false.
> > > > > > + */
> > > > > > +bool kms_has_vblank(int fd)
> > > > > > +{
> > > > > > +   drmVBlank dummy_vbl;
> > > > > > +
> > > > > > +   memset(&dummy_vbl, 0, sizeof(drmVBlank));
> > > > > > +   dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
> > > > > 
> > > > > Why the NEXTONMISS?
> > > > 
> > > > I added this flag because I was suspecting that in case of any problem
> > > > during the kms_has_vblank() execution the flag NEXTONMISS will wait for
> > > > the next VBlank and avoid to generate problems in the subsequent tests.
> > > 
> > > That is true, and a valid use for using NEXTONMISS if you want to align
> > > your code to the start of a vblank to avoid overrunning into the next
> > > vblank.
> > > 
> > > However, that is not the purpose of kms_has_vblank()! Whose only purpose
> > > is answer the question of whether the device has vblank support, and so
> > > should be as quick and simple as possible, so the trivial query of the
> > > current vblank counter.
> > 
> > Nice! Thanks for your answer.
> > 
> > So, I suppose that use DRM_VBLANK_RELATIVE is enough for this case,
> > right?
> 
> Yes, that should return immediately both on success and on failure.
> -Chris

Thanks! I will prepare a v6 tomorrow
diff mbox series

Patch

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index e1eacc1e..1d2d7188 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1655,6 +1655,26 @@  void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility)
 	igt_assert_eq(visible, visibility);
 }
 
+/**
+ * kms_has_vblank:
+ * @fd: DRM fd
+ *
+ * Get the VBlank errno after an attempt to call drmWaitVBlank(). This
+ * function is useful for checking if a driver has support or not for VBlank.
+ *
+ * Returns: true if target driver has VBlank support, otherwise return false.
+ */
+bool kms_has_vblank(int fd)
+{
+	drmVBlank dummy_vbl;
+
+	memset(&dummy_vbl, 0, sizeof(drmVBlank));
+	dummy_vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
+
+	drmWaitVBlank(fd, &dummy_vbl);
+	return (errno != EOPNOTSUPP);
+}
+
 /*
  * A small modeset API
  */
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 407f3d64..1cc15eea 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -230,6 +230,8 @@  void kmstest_wait_for_pageflip(int fd);
 unsigned int kmstest_get_vblank(int fd, int pipe, unsigned int flags);
 void igt_assert_plane_visible(int fd, enum pipe pipe, bool visibility);
 
+bool kms_has_vblank(int fd);
+
 /*
  * A small modeset API
  */
diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index dfa5a69e..614efc5b 100755
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -71,6 +71,7 @@ 
 #define TEST_SUSPEND		(1 << 26)
 #define TEST_BO_TOOBIG		(1 << 28)
 
+#define TEST_NO_VBLANK		(1 << 29)
 #define TEST_BASIC		(1 << 30)
 
 #define EVENT_FLIP		(1 << 0)
@@ -126,6 +127,18 @@  struct event_state {
 	int seq_step;
 };
 
+static bool vblank_dependence(int flags)
+{
+	int vblank_flags = TEST_VBLANK | TEST_VBLANK_BLOCK |
+			   TEST_VBLANK_ABSOLUTE | TEST_VBLANK_EXPIRED_SEQ |
+			   TEST_CHECK_TS | TEST_VBLANK_RACE;
+
+	if (flags & vblank_flags)
+		return true;
+
+	return false;
+}
+
 static float timeval_float(const struct timeval *tv)
 {
 	return tv->tv_sec + tv->tv_usec / 1000000.0f;
@@ -1177,6 +1190,7 @@  static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 	unsigned bo_size = 0;
 	uint64_t tiling;
 	int i;
+	int vblank = true;
 
 	switch (crtc_count) {
 	case RUN_TEST:
@@ -1260,6 +1274,14 @@  static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
 	}
 	igt_assert(fb_is_bound(o, o->fb_ids[0]));
 
+	vblank = kms_has_vblank(drm_fd);
+	if (!vblank) {
+		if (vblank_dependence(o->flags))
+			igt_require_f(vblank, "There is no VBlank\n");
+		else
+			o->flags |= TEST_NO_VBLANK;
+	}
+
 	/* quiescent the hw a bit so ensure we don't miss a single frame */
 	if (o->flags & TEST_CHECK_TS)
 		calibrate_ts(o, crtc_idxs[0]);