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

Submitted by Petri Latvala on Feb. 7, 2019, 1:36 p.m.

Details

Message ID 20190207133652.5346-1-petri.latvala@intel.com
State New
Headers show
Series "tests/kms_flip: Fix comparison of unsigned integers" ( rev: 1 ) in IGT

Not browsing as part of any series.

Commit Message

Petri Latvala Feb. 7, 2019, 1:36 p.m.
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(-)

Patch hide | download patch | download mbox

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,

Comments

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