[igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[Petri Latvala]

Difference of unsigned ints is unsigned int, and therefore always
larger than zero.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/kms_flip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index 798fc4e8..96f4a2f7 100755
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -502,7 +502,7 @@ static void check_state(const struct test_output *o, const struct event_state *e
 	if (o->flags & TEST_TS_CONT) {
 		/* Ignore seq_step here since vblank waits time out immediately
 		 * when we kill the crtc. */
-		igt_assert_f(es->current_seq - es->last_seq >= 0,
+		igt_assert_f(es->current_seq >= es->last_seq,
 			     "unexpected %s seq %u, should be >= %u\n",
 			     es->name, es->current_seq, es->last_seq);
 		igt_assert_f(es->current_seq - es->last_seq <= 150,
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[Daniel Vetter]

On Thu, Feb 07, 2019 at 03:36:52PM +0200, Petri Latvala wrote:
> Difference of unsigned ints is unsigned int, and therefore always
> larger than zero.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

I think we have some other tests for this, but this looks correct.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/kms_flip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> index 798fc4e8..96f4a2f7 100755
> --- a/tests/kms_flip.c
> +++ b/tests/kms_flip.c
> @@ -502,7 +502,7 @@ static void check_state(const struct test_output *o, const struct event_state *e
>  	if (o->flags & TEST_TS_CONT) {
>  		/* Ignore seq_step here since vblank waits time out immediately
>  		 * when we kill the crtc. */
> -		igt_assert_f(es->current_seq - es->last_seq >= 0,
> +		igt_assert_f(es->current_seq >= es->last_seq,
>  			     "unexpected %s seq %u, should be >= %u\n",
>  			     es->name, es->current_seq, es->last_seq);
>  		igt_assert_f(es->current_seq - es->last_seq <= 150,
> -- 
> 2.19.1
> 

-- 
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

Re: [igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[Ville Syrjälä]

On Thu, Feb 07, 2019 at 03:36:52PM +0200, Petri Latvala wrote:
> Difference of unsigned ints is unsigned int, and therefore always
> larger than zero.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/kms_flip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> index 798fc4e8..96f4a2f7 100755
> --- a/tests/kms_flip.c
> +++ b/tests/kms_flip.c
> @@ -502,7 +502,7 @@ static void check_state(const struct test_output *o, const struct event_state *e
>  	if (o->flags & TEST_TS_CONT) {
>  		/* Ignore seq_step here since vblank waits time out immediately
>  		 * when we kill the crtc. */
> -		igt_assert_f(es->current_seq - es->last_seq >= 0,
> +		igt_assert_f(es->current_seq >= es->last_seq,

That won't handle wraparound. I guess I should actually push my
vblank_before/after() stuff and extend its use a bit.

Assuming I can still find it...

>  			     "unexpected %s seq %u, should be >= %u\n",
>  			     es->name, es->current_seq, es->last_seq);
>  		igt_assert_f(es->current_seq - es->last_seq <= 150,
> -- 
> 2.19.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[Petri Latvala]

On Thu, Feb 07, 2019 at 04:02:12PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 07, 2019 at 03:36:52PM +0200, Petri Latvala wrote:
> > Difference of unsigned ints is unsigned int, and therefore always
> > larger than zero.
> > 
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  tests/kms_flip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> > index 798fc4e8..96f4a2f7 100755
> > --- a/tests/kms_flip.c
> > +++ b/tests/kms_flip.c
> > @@ -502,7 +502,7 @@ static void check_state(const struct test_output *o, const struct event_state *e
> >  	if (o->flags & TEST_TS_CONT) {
> >  		/* Ignore seq_step here since vblank waits time out immediately
> >  		 * when we kill the crtc. */
> > -		igt_assert_f(es->current_seq - es->last_seq >= 0,
> > +		igt_assert_f(es->current_seq >= es->last_seq,
> 
> That won't handle wraparound.

Are you referring to something other than the checked overflow? Or in
other words, are you objecting to this patch?


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[Chris Wilson]

Quoting Petri Latvala (2019-03-05 12:55:01)
> On Thu, Feb 07, 2019 at 04:02:12PM +0200, Ville Syrjälä wrote:
> > On Thu, Feb 07, 2019 at 03:36:52PM +0200, Petri Latvala wrote:
> > > Difference of unsigned ints is unsigned int, and therefore always
> > > larger than zero.
> > > 
> > > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  tests/kms_flip.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> > > index 798fc4e8..96f4a2f7 100755
> > > --- a/tests/kms_flip.c
> > > +++ b/tests/kms_flip.c
> > > @@ -502,7 +502,7 @@ static void check_state(const struct test_output *o, const struct event_state *e
> > >     if (o->flags & TEST_TS_CONT) {
> > >             /* Ignore seq_step here since vblank waits time out immediately
> > >              * when we kill the crtc. */
> > > -           igt_assert_f(es->current_seq - es->last_seq >= 0,
> > > +           igt_assert_f(es->current_seq >= es->last_seq,
> > 
> > That won't handle wraparound.
> 
> Are you referring to something other than the checked overflow? Or in
> other words, are you objecting to this patch?

/* a is later than b */
static bool seqno_later(u32 a, u32 b) { return (s32)(a - b) >= 0; }
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/kms_flip: Fix comparison of unsigned integers

[Petri Latvala]

On Tue, Mar 05, 2019 at 01:02:58PM +0000, Chris Wilson wrote:
> Quoting Petri Latvala (2019-03-05 12:55:01)
> > On Thu, Feb 07, 2019 at 04:02:12PM +0200, Ville Syrjälä wrote:
> > > On Thu, Feb 07, 2019 at 03:36:52PM +0200, Petri Latvala wrote:
> > > > Difference of unsigned ints is unsigned int, and therefore always
> > > > larger than zero.
> > > > 
> > > > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  tests/kms_flip.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> > > > index 798fc4e8..96f4a2f7 100755
> > > > --- a/tests/kms_flip.c
> > > > +++ b/tests/kms_flip.c
> > > > @@ -502,7 +502,7 @@ static void check_state(const struct test_output *o, const struct event_state *e
> > > >     if (o->flags & TEST_TS_CONT) {
> > > >             /* Ignore seq_step here since vblank waits time out immediately
> > > >              * when we kill the crtc. */
> > > > -           igt_assert_f(es->current_seq - es->last_seq >= 0,
> > > > +           igt_assert_f(es->current_seq >= es->last_seq,
> > > 
> > > That won't handle wraparound.
> > 
> > Are you referring to something other than the checked overflow? Or in
> > other words, are you objecting to this patch?
> 
> /* a is later than b */
> static bool seqno_later(u32 a, u32 b) { return (s32)(a - b) >= 0; }


I see now! My mental gymnastics failed to process the combined
overflows and the type conversion, and how that leads to correct
results but some quick testing made me see the truth.

Although it might be moot anyway if we end up removing this code
altogether.


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for tests/kms_flip: Fix comparison of unsigned integers

[Patchwork]

== Series Details ==

Series: tests/kms_flip: Fix comparison of unsigned integers
URL   : https://patchwork.freedesktop.org/series/56343/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5559 -> IGTPW_2351
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2351 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2351, 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/56343/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2351:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       PASS -> DMESG-FAIL

  
Known issues
------------

  Here are the changes found in IGTPW_2351 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_busy@basic-flip-b:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         PASS -> FAIL [fdo#104008]

  
#### Possible fixes ####

  * igt@pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       {SKIP} [fdo#109271] -> PASS

  * igt@pm_rpm@basic-rte:
    - fi-bsw-kefka:       FAIL [fdo#108800] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109567]: https://bugs.freedesktop.org/show_bug.cgi?id=109567


Participating hosts (50 -> 45)
------------------------------

  Missing    (5): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4813 -> IGTPW_2351

  CI_DRM_5559: 5426efb407f3d2c90c5020f9c87631058362379d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2351: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2351/
  IGT_4813: 09f506726d0e115ee7f4a1604ae71adcf9f12690 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2351/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev