Re: [PATCH i-g-t v2] tests/kms: correct index validation logic

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 22, 2026 at 07:05:57PM +0200, Kamil Konieczny wrote:
> Hi Jani,
> On 2026-04-21 at 15:02:01 +0300, Jani Nikula wrote:
> > On Tue, 21 Apr 2026, Jing-Ping Jan <jingpingjan@google.com> wrote:
> > > Based on the original implementation and the comment above the function,
> > > it appears the expected behavior is for the plane indices to remain
> > > consistent with those in crtc->planes. Therefore, after swapping the old
> > > and new primary planes, we must update the index in igt_plane_t to
> > > ensure it matches the corresponding index in crtc->planes.
> > >
> > > Therefore, we should expect the indices of the old and new primary
> > > planes to remain unchanged after swapping them.
> > >
> > > Fixes: ac37e1174cc4 ("lib/kms: Pimp the primary plane swapping")
> > 
> > With Fixes: it's customary to Cc the author and reviewer(s).
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > > Signed-off-by: Jing-Ping Jan <jingpingjan@google.com>
> > > ---
> > > v2:
> > >   - Add Fixes tag to the commit message.
> > >  lib/igt_kms.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index bec96f267..405a04640 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -2982,8 +2982,8 @@ void igt_display_reset_outputs(igt_display_t *display)
> > 
> 
> Lets have for simplicity simple struct { char name; int index; }
> and old -> { A, 0 }, new -> { F, 7 }
> 
> > The whole context here is:
> > 
> > 			if (new_primary->index != 0) {
> > 				igt_assert(old_primary != new_primary);
> > 
> > 				igt_assert_eq(old_primary->index, 0);
> > 				igt_assert_neq(new_primary->index, 0);
> > 
> > >  				igt_swap(*old_primary, *new_primary);
> 
> Now old -> { F, 7 }, new -> { A, 0 }
> 
> > >  				igt_swap(old_primary->index, new_primary->index);
> 
> Now old -> { F, 0 }, new -> { A, 7 }
> 
> Or do I miss something?

The problem is the pointers weren't swapped.

[0] = { 0, A } <- old
[1] = { 1, B } <- new

step1: full swap
[0] = { 1, B } <- old
[1] = { 0, A } <- new

step2: index swap
[0] = { 0, B } <- old
[1] = { 1, A } <- new

So after this I think the proper fix is to also swap the pointers.
Then the asserts after the swap start to make actual sense.

step3: pointer swap
[0] = { 0, B } <- new
[1] = { 1, A } <- old

> 
> > >  
> > > -				igt_assert_neq(old_primary->index, 0);
> > > -				igt_assert_eq(new_primary->index, 0);
> > > +				igt_assert_eq(old_primary->index, 0);
> > > +				igt_assert_neq(new_primary->index, 0);
> > 
> > Why do you expect these to remain the same as before the swapping?
> > That's why we are swapping.
> > 
> > >  			} else {
> > >  				igt_assert(old_primary == new_primary);
> > 
> > -- 
> > Jani Nikula, Intel

-- 
Ville Syrjälä
Intel