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

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

On Thu, Apr 23, 2026 at 05:21:02PM +0800, Jing-Ping Jan wrote:
> On Thu, Apr 23, 2026 at 1:23 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > 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
> 
> I don’t believe swapping the pointers is the right approach.
> Currently, old_primary and new_primary point to the first element and
> the original primary plane in crtc->planes, respectively. Swapping the
> pointers merely redirects them without addressing the underlying data
> structure.

We already swap the data. Just need to *also* swap the pointers
so that they track their data correctly. Then the asserts will
make sense.

-- 
Ville Syrjälä
Intel